ObjectiveC WebRTC frame buffers are autoreleased late, especially on Debug builds

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

ObjectiveC WebRTC frame buffers are autoreleased late, especially on Debug builds.
On debug builds, running https://webrtc.github.io/samples/src/content/peerconnection/change-codecs/ with VP8 and mock capture source (click start, then call) will see memory increases a lot, up to more than 1 GB.
When clicking hang up, all this memory got autoreleased but we would hope it would be released sooner.

On debug builds, running https://webrtc.github.io/samples/src/content/peerconnection/pc1/ with the real camera fails after a few frames being encoded (click start, then call).
The camera CVPixelBuffers are only released after clicking hang up, which makes the camera stopping to emit frames.

[youenn fablet]

Created attachment 440047 [details]
Explicit close approach

[youenn fablet]

For the real camera case, RTCCVPixelBuffer are sometimes deallocated too late, and deallocation triggers releasing the wrapped CVPixelBuffer.
For the VP8 case, CVPixelBuffers are converted in I420 webrtc internal buffers, which are kept alive until RTCI420Buffer are deallocated.

I feel like we might be able to do better (@autoreleasepool maybe?) but I am not familiar with how to do it. Advices most welcome.

[youenn fablet]

I wonder whether this might explain rdar://83435926.

[youenn fablet]

Also not sure why the issue occurs now.

[David Kilzer (:ddkilzer)]

Comment on attachment 440047 [details]
Explicit close approach

View in context: https://bugs.webkit.org/attachment.cgi?id=440047&action=review

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitEncoder.mm:374
> -    auto *videoFrame = [[RTCVideoFrame alloc] initWithBuffer:ToObjCVideoFrameBuffer(pixelBufferToFrame(pixelBuffer)) rotation:RTCVideoRotation(rotation) timeStampNs:timeStampNs];
> +    auto videoFrameBuffer = pixelBufferToFrame(pixelBuffer);
> +    auto *videoFrame = [[RTCVideoFrame alloc] initWithBuffer:ToObjCVideoFrameBuffer(videoFrameBuffer) rotation:RTCVideoRotation(rotation) timeStampNs:timeStampNs];

This change is likely what fixes the autoreleased objects.

I'd have to look at generated code to be sure (which I'm not very good at), but this change probably allows the compiler to avoid autoreleasing the result of ToObjCVideoFrameBuffer() somehow, which is where these objects come from.

Under ARC, any method that returns an Objective-C object (that's not marked NS_RETURNS_RETAINED) will return a +1 autoreleased object unless the caller is also compiled with ARC and the result is stored in some kind of a (temp) variable, which allows the +1 autoreleased object to become a +1 retained object using a special optimization trick.

I would have said that this change would be a "safer" bet to eliminate the autoreleased object because it provides a local variable for the Objective-C object that was formerly autoreleased:

    auto videoFrameBuffer = ToObjCVideoFrameBuffer(pixelBufferToFrame(pixelBuffer));  // Obj-C object is now +1 retained instead of +1 autoreleased
    auto *videoFrame = [[RTCVideoFrame alloc] initWithBuffer:videoFrameBuffer rotation:RTCVideoRotation(rotation) timeStampNs:timeStampNs];

Having said that, changing ToObjCVideoFrameBuffer() into an inline method (moving it from objc_frame_buffer.mm to objc_frame_buffer.h) might also allow the optimization to occur (without changes to this caller), but I'm not 100% sure about that.  (If ToObjCVideoFrameBuffer() is used in more than one place, this might eliminate other places where videoFrameBuffer objects are autoreleased as well.)

(I used to have a blog article that I linked to where I learned about the optimization, but I can't find it now.)

[David Kilzer (:ddkilzer)]

Ah, here's the article where I read about this:
<https://mikeash.com/pyblog/friday-qa-2011-09-30-automatic-reference-counting.html>

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #6)
> Ah, here's the article where I read about this:
> <https://mikeash.com/pyblog/friday-qa-2011-09-30-automatic-reference-
> counting.html>

See the sections about objc_retainAutoreleaseReturnValue().

[Radar WebKit Bug Importer]

<rdar://problem/83876310>

[youenn fablet]

Comment on attachment 440047 [details]
Explicit close approach

View in context: https://bugs.webkit.org/attachment.cgi?id=440047&action=review

>> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitEncoder.mm:374
>> +    auto *videoFrame = [[RTCVideoFrame alloc] initWithBuffer:ToObjCVideoFrameBuffer(videoFrameBuffer) rotation:RTCVideoRotation(rotation) timeStampNs:timeStampNs];
> 
> This change is likely what fixes the autoreleased objects.
> 
> I'd have to look at generated code to be sure (which I'm not very good at), but this change probably allows the compiler to avoid autoreleasing the result of ToObjCVideoFrameBuffer() somehow, which is where these objects come from.
> 
> Under ARC, any method that returns an Objective-C object (that's not marked NS_RETURNS_RETAINED) will return a +1 autoreleased object unless the caller is also compiled with ARC and the result is stored in some kind of a (temp) variable, which allows the +1 autoreleased object to become a +1 retained object using a special optimization trick.
> 
> I would have said that this change would be a "safer" bet to eliminate the autoreleased object because it provides a local variable for the Objective-C object that was formerly autoreleased:
> 
>     auto videoFrameBuffer = ToObjCVideoFrameBuffer(pixelBufferToFrame(pixelBuffer));  // Obj-C object is now +1 retained instead of +1 autoreleased
>     auto *videoFrame = [[RTCVideoFrame alloc] initWithBuffer:videoFrameBuffer rotation:RTCVideoRotation(rotation) timeStampNs:timeStampNs];
> 
> Having said that, changing ToObjCVideoFrameBuffer() into an inline method (moving it from objc_frame_buffer.mm to objc_frame_buffer.h) might also allow the optimization to occur (without changes to this caller), but I'm not 100% sure about that.  (If ToObjCVideoFrameBuffer() is used in more than one place, this might eliminate other places where videoFrameBuffer objects are autoreleased as well.)
> 
> (I used to have a blog article that I linked to where I learned about the optimization, but I can't find it now.)

I tried that but it does not seem to work for me.

[youenn fablet]

Thanks for the r+, I will add some WEBRTC_WEBKIT_BUILD to keep track of the changes.

[youenn fablet]

Created attachment 440340 [details]
Patch for landing

[EWS]

Committed r283610 (242562@main): <https://commits.webkit.org/242562@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440340 [details].

[youenn fablet]

<rdar://83435926>

[youenn fablet]

<rdar://problem/83876310>