Bug 113880

Summary: [Gstreamer] Use gst_buffer_extract() in copyGstreamerBuffersToAudioChannel()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, feature-media-reviews, jer.noble, laszlo.gombos, mrobinson, pnormand, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2013-04-03 08:58:05 PDT 
copyGstreamerBuffersToAudioChannel() currently maps the GstBuffer content to memcpy it the the AudioChannel buffer. We could leverage gst_buffer_extract() API function to simplify the code and avoid issues with error handling.
Comment 1 Chris Dumez 2013-04-03 09:08:38 PDT 
Created attachment 196362 [details]
Patch
Comment 2 Philippe Normand 2013-04-03 09:33:33 PDT 
Comment on attachment 196362 [details]
Patch

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

Looks cool indeed!

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:105
> +    guint bufferCount = gst_buffer_list_length(buffers);
> +    for (guint i = 0; i < bufferCount; ++i) {

bufferCount is not needed I think. Also please use unsigned instead of guint if possible, we try to give preference to non-glib types when possible.
Comment 3 Chris Dumez 2013-04-03 10:29:59 PDT 
Comment on attachment 196362 [details]
Patch

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

>> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:105
>> +    for (guint i = 0; i < bufferCount; ++i) {
> 
> bufferCount is not needed I think. Also please use unsigned instead of guint if possible, we try to give preference to non-glib types when possible.

Caching the container size before the loop is commonly done in WebKit and it avoids calling gst_buffer_list_length() for each iteration. It's not a big deal here because gst_buffer_list_length() is inexpensive but I still think it is good practice.

I'll replace guint by unsigned, I wasn't sure what the policy was about this.
Comment 4 Chris Dumez 2013-04-03 10:34:14 PDT 
Created attachment 196374 [details]
Patch

Using unsigned instead of guint. I kept the container size caching for now.
Comment 5 WebKit Review Bot 2013-04-03 11:11:07 PDT 
Comment on attachment 196374 [details]
Patch

Clearing flags on attachment: 196374

Committed r147567: <http://trac.webkit.org/changeset/147567>
Comment 6 WebKit Review Bot 2013-04-03 11:11:11 PDT 
All reviewed patches have been landed.  Closing bug.