Bug 229661 – [WebRTC] Leak or over-release of CFPixelBufferRef returned from webrtc::createPixelBufferFromFrame()

• Component: Media
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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.

[Radar WebKit Bug Importer]

<rdar://problem/82507827>

[David Kilzer (:ddkilzer)]

Created attachment 436754 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Created attachment 436771 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Created attachment 436778 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Created attachment 436790 [details]
Patch v4

[Eric Carlson]

Comment on attachment 436790 [details]
Patch v4

r=me once the bots are happy

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 436852 [details]
Alternative approach

[youenn fablet]

I uploaded the alternative approach.
I am fine with your approach since that might be easier to maintain.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 436986 [details]
Patch v5

[EWS]

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].

[Darin Adler]

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());

[David Kilzer (:ddkilzer)]

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.

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

(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