Bug 166886 – [GStreamer] Use smart pointers and modernize code in WebKitWebAudioSourceGStreamer

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 298454 [details]
Patch

[Xabier Rodríguez Calvar]

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.

[Carlos Garcia Campos]

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!

[Carlos Garcia Campos]

Created attachment 298561 [details]
Updated patch

[Xabier Rodríguez Calvar]

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

[Carlos Garcia Campos]

Committed r210584: <http://trac.webkit.org/changeset/210584>