Bug 113880 - [Gstreamer] Use gst_buffer_extract() in copyGstreamerBuffersToAudioChannel()
Summary: [Gstreamer] Use gst_buffer_extract() in copyGstreamerBuffersToAudioChannel()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-03 08:58 PDT by Chris Dumez
Modified: 2013-04-03 11:11 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.66 KB, patch)
2013-04-03 09:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (2.66 KB, patch)
2013-04-03 10:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.