Fix: Segfault in encoder on platforms that use I420 frame buffer type. #1101
Conversation
…ypes. On MacOS (and any other platform without native texture support) a segfault will happen if the encoder thread times out waiting for the GPU to copy the native Unity frame texture to the CPU to be converted to I420 format. This timeout is currently set to 60ms and can trigger on a lag spike of GPU work. When this happens, a null buffer will be submitted to the encoder, which will then segfault attempting to access it.
|
|
|
It looks good to me. I'm preparing the PR to avoid the same issue by adding the patch for libwebrtc. But your fix is more affordable. I'll test the fix on my device and approve if it works. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical segfault in the video encoder on platforms using I420 frame buffers (macOS, iOS, Android, and software encoding fallbacks on Windows/Linux). The crash occurs when GPU-to-CPU frame conversion times out after 60ms, resulting in a null frame buffer being passed to the encoder.
Key changes:
- Added validation check for I420 frame buffers before encoding
- Added error handling to drop frames when GPU timeout occurs
- Added warning logging for failed frame conversions
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Eagerly check for a valid I420 buffer to handle GPU timeouts. | ||
| rtc::scoped_refptr<VideoFrameBuffer> vfb = frame.video_frame_buffer(); | ||
| if (vfb && vfb->type() == VideoFrameBuffer::Type::kI420) | ||
| { | ||
| if (!vfb->GetI420()) | ||
| { | ||
| // If the buffer is null, the encoder will crash if we submit the frame. The GPU timed out | ||
| // copying the bytes to the CPU (most likely), or the conversion to I420 failed, so drop the frame. | ||
| RTC_LOG(LS_WARNING) << "Converting GPU frame to I420 failed. Dropping frame."; | ||
| return WEBRTC_VIDEO_CODEC_ERROR; | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling GetI420() twice could be inefficient as it may trigger the GPU-to-CPU copy operation twice. Consider storing the result of GetI420() in a variable and reusing it, or check if the frame buffer pointer itself is null before calling GetI420().
Copilot uses AI. Check for mistakes.
This PR fixes a segfault on MacOS, and likely other platforms that utilize the I420 fallback path. The encoder thread will segfault if more than 60ms of work is bottlenecked on the GPU.
In the I420 pipeline, when requesting an I420 frame, a GPU -> CPU copy is queued up, and then waited on in
WaitSync(). There is a default timeout set to 60ms, in which caseWaitSyncsimply returns false and anullptr. This helps avoid stalling the encoder thread, however this case isn't handled properly by theUnityVideoEncoder, causing it to submit awebrtc::VideoFramewith anullptras a data buffer. When the encoding thread receives this, it attempts to read from that null frame buffer and segfaults.Fix: This eagerly checks the result of
GetI420()in order to make sure it is valid before sending it along to the encoder. We only do this on kI420 typed frame buffers, so that paths with GPU based textures don't accidentally enqueue an additional copy.Happy to make any modifications to the approach -- let me know if you have any thoughts.
Reproduceability: 100%
Tested: MacOS Silicon
I believe this segfault will also happen on the following platforms: