Bug 113875 – [Gstreamer] Avoid calls to g_slist_nth_data in webKitWebAudioSrcLoop()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

In webKitWebAudioSrcLoop(), we iterate over 2 GSLists by using a counter and then calling g_slist_nth_data() to get the element of each GSList.
This is inefficient because calling g_slist_nth_data() will iterate the GSList up until index 'n'.

[Chris Dumez]

Created attachment 196347 [details]
Patch

[Philippe Normand]

Comment on attachment 196347 [details]
Patch

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

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:377
> +    for (padsIt = priv->pads, buffersIt = channelBufferList; padsIt && buffersIt; padsIt = padsIt->next, buffersIt = buffersIt->next) {

Use g_slist_next() perhaps?

[Chris Dumez]

Created attachment 196350 [details]
Patch

Use g_slist_next().

[Philippe Normand]

Comment on attachment 196350 [details]
Patch

Ok

[WebKit Review Bot]

Comment on attachment 196350 [details]
Patch

Clearing flags on attachment: 196350

Committed r147555: <http://trac.webkit.org/changeset/147555>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Praveen Jadhav]

This patch causes build error with gstreamer-0.10. Maybe including these changes under GST_API_VERSION_1 should be good in my opinion.

[Philippe Normand]

That patch doesn't use any GStreamer 1.x-only API, I think. Can you please post the error message?

[Praveen Jadhav]

Its because of the variable 'i' in line no. 384. which was removed in this patch.

/home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp: In function ‘void webKitWebAudioSrcLoop(WebKitWebAudioSrc*)’:
/home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: error: name lookup of ‘i’ changed for ISO ‘for’ scoping [-fpermissive]
/home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: note: (if you use ‘-fpermissive’ G++ will accept your code)

[Philippe Normand]

So this is unrelated with GStreamer 1.x. But feel free to file a patch in a separate bug.

[Chris Dumez]

(In reply to comment #9)
> Its because of the variable 'i' in line no. 384. which was removed in this patch.
> 
> /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp: In function ‘void webKitWebAudioSrcLoop(WebKitWebAudioSrc*)’:
> /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: error: name lookup of ‘i’ changed for ISO ‘for’ scoping [-fpermissive]
> /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: note: (if you use ‘-fpermissive’ G++ will accept your code)

Right, then the proper fix it to add the i counter under #ifndef GST_API_VERSION_1. Do you want to do it or should I?

[Philippe Normand]

Perhaps use g_slist_index()

[Praveen Jadhav]

How about moving setcaps related code to the first "for" loop in the function a(In reply to comment #11)
> (In reply to comment #9)
> > Its because of the variable 'i' in line no. 384. which was removed in this patch.
> > 
> > /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp: In function ‘void webKitWebAudioSrcLoop(WebKitWebAudioSrc*)’:
> > /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: error: name lookup of ‘i’ changed for ISO ‘for’ scoping [-fpermissive]
> > /home/praveen.j/Git_WebKit/WebKit/Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:384:90: note: (if you use ‘-fpermissive’ G++ will accept your code)
> 
> Right, then the proper fix it to add the i counter under #ifndef GST_API_VERSION_1. Do you want to do it or should I?

How about moving setcaps related code to first "for" loop and retain your changes as it is? You may please continue with the checkin. :-)

[Chris Dumez]

gstreamer 0.10 build fix landed in <http://trac.webkit.org/changeset/149021>.