Bug 234134 - [GStreamer][WebRTC] Huge memory leak
Summary: [GStreamer][WebRTC] Huge memory leak
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-10 02:07 PST by Philippe Normand
Modified: 2021-12-17 04:50 PST (History)
14 users (show)

See Also:


Attachments
Patch (21.62 KB, patch)
2021-12-11 08:54 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (21.73 KB, patch)
2021-12-16 05:26 PST, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2021-12-10 02:07:06 PST
Initially reported in https://github.com/Igalia/meta-webkit/issues/335

Also happens with mock capture sources.
Comment 1 Philippe Normand 2021-12-10 11:00:34 PST
There's at least one leak here, https://github.com/WebKit/WebKit/blob/main/Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.cpp#L114

I don't have a fix yet though...
Comment 2 Philippe Normand 2021-12-11 08:41:11 PST
(In reply to Philippe Normand from comment #1)
> There's at least one leak here,
> https://github.com/WebKit/WebKit/blob/main/Source/WebCore/platform/graphics/
> gstreamer/MediaSampleGStreamer.cpp#L114
> 
> I don't have a fix yet though...

That's not the culprit actually... Patch incoming
Comment 3 Philippe Normand 2021-12-11 08:54:10 PST
Created attachment 446889 [details]
Patch
Comment 4 Xabier Rodríguez Calvar 2021-12-15 05:02:14 PST
Comment on attachment 446889 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.cpp:140
> +    const GstClockTime minimumDuration = 1000; // 1 us

period at the end.

> Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:65
> +rtc::scoped_refptr<webrtc::VideoFrameBuffer> GStreamerVideoFrameLibWebRTC::create(GRefPtr<GstSample> sample)

Why did you remove the &&? We should either have const & or &&

> Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.h:40
> +    GStreamerVideoFrameLibWebRTC(GRefPtr<GstSample> sample, GstVideoInfo info)

Ditto

> Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.h:44
> +    static rtc::scoped_refptr<webrtc::VideoFrameBuffer> create(GRefPtr<GstSample>);

Ditto.
Comment 5 Philippe Normand 2021-12-15 05:30:55 PST
Comment on attachment 446889 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.cpp:140
>> +    const GstClockTime minimumDuration = 1000; // 1 us
> 
> period at the end.

Why? This is not a sentence.

>> Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:65
>> +rtc::scoped_refptr<webrtc::VideoFrameBuffer> GStreamerVideoFrameLibWebRTC::create(GRefPtr<GstSample> sample)
> 
> Why did you remove the &&? We should either have const & or &&

&& would transfer ownership iiuc? As mentioned in the changelog we can't do that. Maybe a const& though.

>> Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.h:40
>> +    GStreamerVideoFrameLibWebRTC(GRefPtr<GstSample> sample, GstVideoInfo info)
> 
> Ditto

Ditto

>> Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.h:44
>> +    static rtc::scoped_refptr<webrtc::VideoFrameBuffer> create(GRefPtr<GstSample>);
> 
> Ditto.

Ditto!.
Comment 6 Xabier Rodríguez Calvar 2021-12-15 08:13:16 PST
Comment on attachment 446889 [details]
Patch

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

>>> Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:65
>>> +rtc::scoped_refptr<webrtc::VideoFrameBuffer> GStreamerVideoFrameLibWebRTC::create(GRefPtr<GstSample> sample)
>> 
>> Why did you remove the &&? We should either have const & or &&
> 
> && would transfer ownership iiuc? As mentioned in the changelog we can't do that. Maybe a const& though.

const & sounds good.
Comment 7 Philippe Normand 2021-12-16 05:26:23 PST
Created attachment 447347 [details]
Patch
Comment 8 EWS 2021-12-17 04:49:08 PST
Committed r287180 (245349@main): <https://commits.webkit.org/245349@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447347 [details].
Comment 9 Radar WebKit Bug Importer 2021-12-17 04:50:19 PST
<rdar://problem/86627091>