Bug 166886

Summary: [GStreamer] Use smart pointers and modernize code in WebKitWebAudioSourceGStreamer
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, zan
Priority: P2 Keywords: Gtk
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
calvaris: review-
Updated patch calvaris: review+

Description Carlos Garcia Campos 2017-01-10 03:35:17 PST 
While debugging bug #166884 I ended up cleaning up the code a bit to be able to follow it, and also made it use smart pointers to ensure we were not leaking refs/memory.
Comment 1 Carlos Garcia Campos 2017-01-10 03:39:33 PST 
Created attachment 298454 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2017-01-10 06:19:10 PST 
Comment on attachment 298454 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        smart pointers, uses WTF::Vactor instead of GSList and simplifies the code to map/unmap GstBuffers.

WTF:Vector

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:353
> +        auto& buffer = channelBufferList[i];

Specify the type, please

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:362
> +        auto& appsrc = priv->sources[i];

Specify the type, please

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:403
> +        src->priv->pool = adoptGRef(gst_buffer_pool_new());

gst_buffer_pool_new returns a floating reference. According to the defined adoptGRef for this time, this should trigger the ASSERT.

Not adopting is ok here.

Actually in line 221 a similar thing is done correctly.
Comment 3 Carlos Garcia Campos 2017-01-10 06:23:57 PST 
Comment on attachment 298454 [details]
Patch

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

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:353
>> +        auto& buffer = channelBufferList[i];
> 
> Specify the type, please

why?

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:362
>> +        auto& appsrc = priv->sources[i];
> 
> Specify the type, please

why?

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:403
>> +        src->priv->pool = adoptGRef(gst_buffer_pool_new());
> 
> gst_buffer_pool_new returns a floating reference. According to the defined adoptGRef for this time, this should trigger the ASSERT.
> 
> Not adopting is ok here.
> 
> Actually in line 221 a similar thing is done correctly.

Right!
Comment 4 Carlos Garcia Campos 2017-01-11 02:01:49 PST 
Created attachment 298561 [details]
Updated patch
Comment 5 Xabier Rodríguez Calvar 2017-01-11 02:19:54 PST 
(In reply to comment #3)
> >> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:353
> >> +        auto& buffer = channelBufferList[i];
> > 
> > Specify the type, please
> 
> why?
> 
> >> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:362
> >> +        auto& appsrc = priv->sources[i];
> > 
> > Specify the type, please
> 
> why?

Cause I thought it was better for readability, but given the latest discussions on -dev and -reviewers it does not matter anymore.
Comment 6 Carlos Garcia Campos 2017-01-11 03:28:53 PST 
Committed r210584: <http://trac.webkit.org/changeset/210584>