Fix AV1 VA-API encoder crash on AMD/Mesa by initializing DPB for keyframes [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@tmath...@chromium.org - changed to a "only use defaults if 0" approach, could you take a look if that would be more feasable and less limited/magic?
Related details
- Andres Calderon Jaramillo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ted (Chromium) Meyer (Gerrit)
Ted (Chromium) Meyer added 10 comments![]( "Open in Gerrit")
Looks mostly good, just remove some superfluous comments and fix up the signed int types.
The GPU process crashes (exit_code=8) when initializing the AV1 VA-API
encoder on AMD GPUs with Mesa drivers. This occurs because AMD's
stateless AV1 encoder driver is sensitive to the DPB (Decoded Picture
Buffer) state and may access reference frame slots even when encoding
a keyframe.
This CL also changes the handling of 0 framerate and 0 bitrate values:
- 0 is now treated as "use default" rather than enforcing a minimum
- Framerate defaults to VideoEncodeAccelerator::kDefaultFramerate (30fps)
- Bitrate defaults to 100 kbps
- Explicitly requested low values (e.g., 50 kbps) are now respected
This allows callers like WebRTC to signal "encoder's choice" when the
actual values are not yet known, while still supporting explicit low
bitrate/framerate configurations for WebCodecs use cases.I recommend just un-indenting this manually or the auto-formatter will do weird things to it.
constexpr int kDefaultBitrateKbps = 100;unsigned type
constexpr int kAmdWarmupFrameCount = 15; // ~0.5 seconds at 30fps for QP limitunsigned type
int layer_bitrate_kbps = bitrate_sum / 1000;Change this (and bitrate_sum) to uint64_t. `bitrate_allocation.GetBitrateBps` already returns an unsigned 32 bit int, so there should never be a case where this is negative.
Then you can drop the " > 0" component in your expression below.
// Log order_hint for AMD debuggingremove
// Warm-up logging for AMD/Mesa debugging (crbug.com/471780477).remove.
// Log DPB state after update for AMD debuggingremove
// Log frame header parameters for AMD debuggingremove
Related details
- Andres Calderon Jaramillo
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 12 comments![]( "Open in Gerrit")
Helmut JanuschkaI mostly have concerns over the use of default values here. I'm not sure it makes a lot of sense for different codecs' encoder delegates to handle default values differently. If I ask for a 50Kbps video in h264 vs av1, we _should_ probably get the same bitrate out of our encoders, whether or not we decide if there is a minimum.
Ted (Chromium) Meyeryou're right that silently adjusting the bitrate is surprising. The issue is that AV1 on AMD/Mesa produces unusable output at very low bitrates, frames that decoders outright reject, not just low-quality video.
WebRTC sometimes sends zero/very-low bitrate during startup, and failing would break the stream entirely. so the 100 is just a "safe" min. and the log atleast gives a hint.
Helmut JanuschkaIt's my understanding that webrtc isn't the only user of these encoders. I took a cursory look through the bug, but wasn't able to find out what kind of very-low/non-zero framerates we're talking about, or where the threshold is for outputting crap frames.
Also, it might be good to update the commit message to mention that this CL is changing the meaning of 0FPS and 0Kbps in addition to the dpb frame fix.
Done
The GPU process crashes (exit_code=8) when initializing the AV1 VA-API
encoder on AMD GPUs with Mesa drivers. This occurs because AMD's
stateless AV1 encoder driver is sensitive to the DPB (Decoded Picture
Buffer) state and may access reference frame slots even when encoding
a keyframe.
This CL also changes the handling of 0 framerate and 0 bitrate values:
- 0 is now treated as "use default" rather than enforcing a minimum
- Framerate defaults to VideoEncodeAccelerator::kDefaultFramerate (30fps)
- Bitrate defaults to 100 kbps
- Explicitly requested low values (e.g., 50 kbps) are now respected
This allows callers like WebRTC to signal "encoder's choice" when the
actual values are not yet known, while still supporting explicit low
bitrate/framerate configurations for WebCodecs use cases.I recommend just un-indenting this manually or the auto-formatter will do weird things to it.
Done
if (current_params_.framerate == 0) {Helmut JanuschkaI'm not an expert on the encoder pipeline, but `current_params_` seems like something controlled by whatever JS call is attempting to do encoding. Does it not make sense to move default value handling to the VideoEncodeAccelerator::Config structure? Shouldn't all encoders have these same defaults, assuming they're good defaults to have? Maybe a user-supplied zero frame rate encode request _should_ fail?
Ted (Chromium) MeyerVideoEncodeAccelerator already defines `kDefaultFramerate = 30` (https://source.chromium.org/chromium/chromium/src/+/main:media/video/video_encode_accelerator.h;l=231?q=kDefaultFramerate%20%3D%2030%20file:video_encode_accelerator.h&ss=chromium%2Fchromium%2Fsrc), so there's precedent for this default. I could change this to use that constant instead of a magic 30 to make it clearer this is an intentional fallback.
As for whether zero framerate should fail: in WebRTC scenarios, the framerate can legitimately be unknown at initialization time. Failing would break these use cases.
Helmut JanuschkaI suppose I am ok with treating 0 for framerate (and bitrate) as a "do whatever you think is best mr. encoder" message. Maybe it makes sense to ensure that they are both zero to enable this behavior? So if the caller requests 50Kbps and 0FPS, they get a legitimate error, since that makes no sense. If both are zero, then use defaults.
I considered requiring both to be zero, but WebRTC can legitimately have one known and one unknown at init time (e.g.,known target bitrate but unknown framerate). Treating each independently seems more flexible for real-world scenarios.
// 100 kbps is a reasonable minimum for low-resolution video.
constexpr int kMinTargetBitrateKbps = 100;Helmut JanuschkaAs far as I am aware, webcodecs allows these encoders to be used via some JS calls (@eugene, is this correct?). This seems like a very webrtc specific set of minimum values. It's conceivable that someone might truly want video under 100Kbps, what is the reasoning for deciding this is a "reasonable" minimum? Do other encoders have this same minimum?
Ted (Chromium) Meyer100Kbps was the value that made the pipeline stable in testing. The real issue is that WebRTC shows up with 0 bitrate during startup, which breaks the AV1 encoder on AMD/Mesa, it produces frames that decoders reject entirely.
I don't have strong evidence that 100Kbps is the "right" minimum vs something lower like 50Kbps. The goal was just to ensure a non-zero value gets to the encoder.
Happy to lower it to 50Kbps if that seems more reasonable for WebCodecs use cases.
Or alternatively, I could change the check to only kick in when bitrate is literally 0, rather than enforcing a minimum. WDYT?
Helmut JanuschkaMy gut feeling here is that "zero -> 100" is better than "max(value, 100)", since webrtc isn't the only thing using this file, and undocumented codec/platform specific combinations are going to be a debugging nightmare later. It a perfect world, this would be an optional<int>, so webrtc could legitimately say "give me whatever you think it should be", while still allowing anyone to explicitly set a value to whatever they'd like. Treating 0 as that optional is probably ok though, since it does not actually make sense to have a 0Kbps video.
Done
constexpr int kDefaultBitrateKbps = 100;Helmut Januschkaunsigned type
Done
constexpr int kAmdWarmupFrameCount = 15; // ~0.5 seconds at 30fps for QP limitHelmut Januschkaunsigned type
Done
int target_bandwidth_kbps =Helmut Januschkauint32
Done
Change this (and bitrate_sum) to uint64_t. `bitrate_allocation.GetBitrateBps` already returns an unsigned 32 bit int, so there should never be a case where this is negative.
Then you can drop the " > 0" component in your expression below.
Done
// Log order_hint for AMD debuggingHelmut Januschkaremove
Done
// Warm-up logging for AMD/Mesa debugging (crbug.com/471780477).Helmut Januschkaremove.
Done
// Log DPB state after update for AMD debuggingHelmut Januschkaremove
Done
// Log frame header parameters for AMD debuggingHelmut Januschkaremove
Done
Related details
- Andres Calderon Jaramillo
- Ted (Chromium) Meyer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ted (Chromium) Meyer (Gerrit)
Ted (Chromium) Meyer voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ted (Chromium) Meyer
| Code-Review | +1 |
I'll also add Eugene as a reviewer since he does a lot with encoders.
// Log order_hint for AMD debuggingHelmut Januschkaremove
Ted (Chromium) MeyerDone
AH! I hadn't intended this to mean "remove the comment" not "remove the log". Comments should be "why" and not "what". The raison d'etre for logs is self evident.
anyway, you can put the logs back in (though maybe making them DVLOGF(2) or DVLOGF(3) might be a bit better). feel free to leave them out if you don't think it's important though.
Related details
- Andres Calderon Jaramillo
- Eugene Zemtsov
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
// Log order_hint for AMD debuggingHelmut Januschkaremove
Ted (Chromium) MeyerDone
AH! I hadn't intended this to mean "remove the comment" not "remove the log". Comments should be "why" and not "what". The raison d'etre for logs is self evident.
anyway, you can put the logs back in (though maybe making them DVLOGF(2) or DVLOGF(3) might be a bit better). feel free to leave them out if you don't think it's important though.
oh my fault, yes they will be usefull for "me in 3 months" 😂
Related details
- Andres Calderon Jaramillo
- Eugene Zemtsov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hirokazu Honda (Gerrit)
Hirokazu Honda added 1 comment![]( "Open in Gerrit")
Hi, I am a bit swarmed these days but I will review this in a week if that's ok.
Related details
- Andres Calderon Jaramillo
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Hi, I am a bit swarmed these days but I will review this in a week if that's ok.
- Andres Calderon Jaramillo
- Hirokazu Honda
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Hirokazu Honda (Gerrit)
Hirokazu Honda added 1 comment![]( "Open in Gerrit")
Supproting a resolutin change in this class seems to be strange.
VaapiVEA doesn't support the dynamic resolution change.
If that actually happens, the webrtc encoder client (RTCVideoEncoder) doesn't work as expected.
Related details
- Andres Calderon Jaramillo
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka voted and added 1 comment![]( "Open in Gerrit")
Votes added by Helmut Januschka
| Commit-Queue | +1 |
Supproting a resolutin change in this class seems to be strange.
VaapiVEA doesn't support the dynamic resolution change.If that actually happens, the webrtc encoder client (RTCVideoEncoder) doesn't work as expected.
- Andres Calderon Jaramillo
- Hirokazu Honda
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hirokazu Honda (Gerrit)
Hirokazu Honda added 2 comments![]( "Open in Gerrit")
Sorry, would you mind splitting this to multiple small piece CLs?
This CL does multiple things which makes review harder.
current_params_.bitrate_allocation = bitrate_allocation;Could you rather ignroe the request that framerate or bitrate is zero?
Related details
- Andres Calderon Jaramillo
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
current_params_.bitrate_allocation = bitrate_allocation;Could you rather ignroe the request that framerate or bitrate is zero?
- Andres Calderon Jaramillo
- Hirokazu Honda
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hirokazu Honda (Gerrit)
Hirokazu Honda added 1 comment![]( "Open in Gerrit")
Sorry, would you mind splitting this to multiple small piece CLs?
This CL does multiple things which makes review harder.
Hi Helmut, would you mind splitting the CL to multiple small CLs? It would be very helpful for me to review.
Related details
- Andres Calderon Jaramillo
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Hirokazu HondaSorry, would you mind splitting this to multiple small piece CLs?
This CL does multiple things which makes review harder.
Hi Helmut, would you mind splitting the CL to multiple small CLs? It would be very helpful for me to review.
while i usually just follow reviewers request to split in this case,i can try to split out the color config and robustness defaults into separate CLs, but the core AMD/Mesa changes (DPB, OBU structure, encoder params) are tightly coupled and would need to stay together, would that work for you?
Related details
- Andres Calderon Jaramillo
- Hirokazu Honda
Hirokazu Honda (Gerrit)
Hirokazu Honda added 1 comment![]( "Open in Gerrit")
Hirokazu HondaSorry, would you mind splitting this to multiple small piece CLs?
This CL does multiple things which makes review harder.
Helmut JanuschkaHi Helmut, would you mind splitting the CL to multiple small CLs? It would be very helpful for me to review.
while i usually just follow reviewers request to split in this case,i can try to split out the color config and robustness defaults into separate CLs, but the core AMD/Mesa changes (DPB, OBU structure, encoder params) are tightly coupled and would need to stay together, would that work for you?
Yes, it would work for me. The tightly coupled change is good hint for me to review.
I appreciate your understanding. Thanks.
Related details
- Andres Calderon Jaramillo
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@hi...@chromium.org that's the last CL in that bug, the color defaults have been confirmed to be not required, can you continue/finalize review of this CL?
Related details
- Andres Calderon Jaramillo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
- Andres Calderon Jaramillo
- Hirokazu Honda
Hirokazu Honda (Gerrit)
Hirokazu Honda added 1 comment![]( "Open in Gerrit")
pong!
- Andres Calderon Jaramillo
- Helmut Januschka
Hirokazu Honda (Gerrit)
Hirokazu Honda added 2 comments![]( "Open in Gerrit")
Hi Helmut,
If I understand correctly, what this CL is primrary doing is to add code for doing the workaround for the bugs in the AMD VA-API driver.
The bugs seems to be too unreasonable to add the workaround in chromium code base.
I would rather add the AMD driver to the block list for AV1 encoding because they contain bad bugs, rather than adding workarounds (so we can mitigate GPU process crash).
https://source.chromium.org/chromium/chromium/src/+/main:gpu/config/gpu_driver_bug_list.json
The right way is to reach out to mesa community to fix the AMD VA-API av1 encoding.
I hesitate saying above as you took your time to analyze the bug and craft this CL.
This is my insight though.
What do you think?
if (is_mesa_gallium && amd_warmup_frames_encoded_ == 0) {
VLOGF(1) << "AMD: Forcing keyframe for first frame at "
<< coded_size_.ToString();
encode_job.ProduceKeyframe();
}
frame_num_ is set to intra_period in Initialize(), so the keyframe is requested on the first frame.
This is unnecessary in my understanding.
Could you confirm that?
Related details
- Andres Calderon Jaramillo
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Hi Helmut,
If I understand correctly, what this CL is primrary doing is to add code for doing the workaround for the bugs in the AMD VA-API driver.
The bugs seems to be too unreasonable to add the workaround in chromium code base.
I would rather add the AMD driver to the block list for AV1 encoding because they contain bad bugs, rather than adding workarounds (so we can mitigate GPU process crash).
https://source.chromium.org/chromium/chromium/src/+/main:gpu/config/gpu_driver_bug_list.jsonThe right way is to reach out to mesa community to fix the AMD VA-API av1 encoding.
I hesitate saying above as you took your time to analyze the bug and craft this CL.
This is my insight though.
What do you think?
@hi...@chromium.org - no worries, OWNER's right, to keep or kill.
yes in fact its bypassing/fixing a bug in AMD VA-API driver.
adding it to block list would ultimatly disable HW accel, i guess.
do you have any ideas, where to get in contact with mesa community - that honestly feels like a final dead end?!
cheers
Related details
- Andres Calderon Jaramillo
- Hirokazu Honda
Hirokazu Honda (Gerrit)
Hirokazu Honda added 1 comment![]( "Open in Gerrit")
Helmut JanuschkaHi Helmut,
If I understand correctly, what this CL is primrary doing is to add code for doing the workaround for the bugs in the AMD VA-API driver.
The bugs seems to be too unreasonable to add the workaround in chromium code base.
I would rather add the AMD driver to the block list for AV1 encoding because they contain bad bugs, rather than adding workarounds (so we can mitigate GPU process crash).
https://source.chromium.org/chromium/chromium/src/+/main:gpu/config/gpu_driver_bug_list.jsonThe right way is to reach out to mesa community to fix the AMD VA-API av1 encoding.
I hesitate saying above as you took your time to analyze the bug and craft this CL.
This is my insight though.
What do you think?
@hi...@chromium.org - no worries, OWNER's right, to keep or kill.
yes in fact its bypassing/fixing a bug in AMD VA-API driver.
adding it to block list would ultimatly disable HW accel, i guess.
do you have any ideas, where to get in contact with mesa community - that honestly feels like a final dead end?!cheers
- Andres Calderon Jaramillo
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka abandoned this change.
Helmut Januschka abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka restored this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
if (is_mesa_gallium && amd_warmup_frames_encoded_ == 0) {
VLOGF(1) << "AMD: Forcing keyframe for first frame at "
<< coded_size_.ToString();
encode_job.ProduceKeyframe();
}
frame_num_ is set to intra_period in Initialize(), so the keyframe is requested on the first frame.
This is unnecessary in my understanding.
Could you confirm that?
Done
Related details
- Hirokazu Honda
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |