Leak or over-release of CFPixelBufferRef returned from webrtc::createPixelBufferFromFrame(). Found by the clang static analyzer: OpenSource/Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:177:9: warning: Incorrect decrement of the reference count of an object that is not owned at this point by the caller CFRelease(pixelBuffer); ^~~~~~~~~~~~~~~~~~~~~~ OpenSource/Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:177:9: warning: Incorrect decrement of the reference count of an object that is not owned at this point by the caller CFRelease(pixelBuffer); ^~~~~~~~~~~~~~~~~~~~~~ OpenSource/Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:177:9: warning: Incorrect decrement of the reference count of an object that is not owned at this point by the caller CFRelease(pixelBuffer); ^~~~~~~~~~~~~~~~~~~~~~ 3 warnings generated.
<rdar://problem/82507827>
Created attachment 436754 [details] Patch v1
Created attachment 436771 [details] Patch v2
Created attachment 436778 [details] Patch v3
Created attachment 436790 [details] Patch v4
Comment on attachment 436790 [details] Patch v4 r=me once the bots are happy
(In reply to Eric Carlson from comment #6) > Comment on attachment 436790 [details] > Patch v4 > > r=me once the bots are happy Failures reproduce on macOS 12 Monterey as well.
I am probably going to need help figuring out why these two tests are failing, and only on macOS. There are a lot of moving parts I'm not familiar with here.
Comment on attachment 436790 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=436790&action=review > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:434 > - RetainPtr<CVPixelBufferRef> newPixelBuffer; > - auto pixelBuffer = webrtc::pixelBufferFromFrame(frame, [&newPixelBuffer](size_t width, size_t height, webrtc::BufferType bufferType) -> CVPixelBufferRef { > + auto pixelBuffer = adoptCF(webrtc::createPixelBufferFromFrame(frame, [](size_t width, size_t height, webrtc::BufferType bufferType) -> CVPixelBufferRef { I think I was assuming that `newPixelBuffer` and `pixelBuffer` would be the same here, but they may not be. This may be the cause of the regression. Need to check into this.
Comment on attachment 436790 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=436790&action=review >> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:434 >> + auto pixelBuffer = adoptCF(webrtc::createPixelBufferFromFrame(frame, [](size_t width, size_t height, webrtc::BufferType bufferType) -> CVPixelBufferRef { > > I think I was assuming that `newPixelBuffer` and `pixelBuffer` would be the same here, but they may not be. > > This may be the cause of the regression. Need to check into this. The idea of pixelBufferFromFrame is the following: - In most cases, there is no need to do any conversion/allocation, pixelBuffer is directly retrieved from frame which keeps ownership of the underlying pixel buffer. - In some cases, in particular for muted frames, the pixel buffer is not I420, in which case we do need a conversion. The idea is that pixelBuffer will not own ownership on the new pixel buffer but newPixelBuffer will actually own it since it is a RetainPtr. The easiest way to fix this would be to apply the same strategy in WebKitDecoderReceiver::Decoded as in LibWebRTCCodecs::encodeFrame or RealtimeIncomingVideoSourceCocoa.
Created attachment 436852 [details] Alternative approach
I uploaded the alternative approach. I am fine with your approach since that might be easier to maintain.
(In reply to youenn fablet from comment #12) > I uploaded the alternative approach. > I am fine with your approach since that might be easier to maintain. Thanks! The tests clearly say that I made a logic error (logic change) in my patch, but I can't see what that is! I may need to step away from the patch for a few days if I can't find it.
(In reply to David Kilzer (:ddkilzer) from comment #13) > (In reply to youenn fablet from comment #12) > > I uploaded the alternative approach. > > I am fine with your approach since that might be easier to maintain. > > Thanks! The tests clearly say that I made a logic error (logic change) in > my patch, but I can't see what that is! I may need to step away from the > patch for a few days if I can't find it. I think I see the inadvertent logic change that I made in LibWebRTCCodecs.cpp. Testing incremental changes locally now to verify.
Comment on attachment 436790 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=436790&action=review > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:460 > - auto sample = RemoteVideoSample::create(pixelBuffer, MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation())); > + auto sample = RemoteVideoSample::create(pixelBuffer.get(), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation())); > if (!sample) { > // FIXME: Optimize this code path, currently we have non BGRA for muted frames at least. > - newPixelBuffer = convertToBGRA(pixelBuffer); > - sample = RemoteVideoSample::create(newPixelBuffer.get(), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation())); > + auto convertedPixelBuffer = convertToBGRA(pixelBuffer.get()); > + sample = RemoteVideoSample::create(convertedPixelBuffer.get(), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation())); > } > > encoder.connection->send(Messages::LibWebRTCCodecsProxy::EncodeFrame { encoder.identifier, *sample, frame.timestamp(), shouldEncodeAsKeyFrame }, 0); The logic bug I was missing was that the CVPixelBufferRef needed to be retained until after send() is called at the end of the method. By creating a local `convertedPixelBuffer` variable, the CVPixelBufferRef is released before send() is called, causing the bug. The fix is to reuse `pixelBuffer` to store the converted CVPixelBufferRef since its scope is the same as the send() method.
Created attachment 436986 [details] Patch v5
Committed r281868 (241198@main): <https://commits.webkit.org/241198@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436986 [details].
Comment on attachment 436986 [details] Patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=436986&action=review > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:457 > + pixelBuffer = convertToBGRA(pixelBuffer.get()); > + sample = RemoteVideoSample::create(pixelBuffer.get(), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation())); For future reference, don’t need a local variable for this. Intermediates are kept alive until the full expression they are part of is done, in this case until the function returns, so this would be correct: sample = RemoteVideoSample::create(convertToBGRA(pixelBuffer.get()).get(), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation())); But also, is it important to reuse the variable? Could just use a new one: auto convertedBuffer = convertToBGRA(pixelBuffer.get());
Comment on attachment 436986 [details] Patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=436986&action=review >> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:457 >> + sample = RemoteVideoSample::create(pixelBuffer.get(), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation())); > > For future reference, don’t need a local variable for this. Intermediates are kept alive until the full expression they are part of is done, in this case until the function returns, so this would be correct: > > sample = RemoteVideoSample::create(convertToBGRA(pixelBuffer.get()).get(), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation())); > > But also, is it important to reuse the variable? Could just use a new one: > > auto convertedBuffer = convertToBGRA(pixelBuffer.get()); It is important to have a variable outside the scope of the `if` block to keep the CVPixelBufferRef alive until `send()` is called later in the method. If this isn't done, it causes two test failures. See Comment #15 for details.
Comment on attachment 436986 [details] Patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=436986&action=review >>> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:457 >>> + sample = RemoteVideoSample::create(pixelBuffer.get(), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation())); >> >> For future reference, don’t need a local variable for this. Intermediates are kept alive until the full expression they are part of is done, in this case until the function returns, so this would be correct: >> >> sample = RemoteVideoSample::create(convertToBGRA(pixelBuffer.get()).get(), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation())); >> >> But also, is it important to reuse the variable? Could just use a new one: >> >> auto convertedBuffer = convertToBGRA(pixelBuffer.get()); > > It is important to have a variable outside the scope of the `if` block to keep the CVPixelBufferRef alive until `send()` is called later in the method. If this isn't done, it causes two test failures. > > See Comment #15 for details. Makes sense. Seems like we have a small design problem that we should return and fix. RemoteVideoSample has a dangerous interface if it takes a raw pointer and the caller needs to keep it alive.
(In reply to Darin Adler from comment #20) > Makes sense. Seems like we have a small design problem that we should return > and fix. RemoteVideoSample has a dangerous interface if it takes a raw > pointer and the caller needs to keep it alive. Bug 229806: RemoteVideoSample needs CVPixelBufferRef to be kept alive, but relies on caller to retain it