Bug 95833 - [GStreamer] GstBuffer ref race in WebKitWebAudioSrcLoop
Summary: [GStreamer] GstBuffer ref race in WebKitWebAudioSrcLoop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P1 Blocker
Assignee: Zan Dobersek
URL:
Keywords:
: 82347 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-09-05 02:12 PDT by kdj
Modified: 2012-10-16 11:23 PDT (History)
11 users (show)

See Also:


Attachments
The webaudio glitch issue went-off with gst-buffer-allocation and data copy from bus to new buffer. Using this new buffer to connect to queue-pad (3.04 KB, patch)
2012-09-11 23:09 PDT, kdj
no flags Details | Formatted Diff | Diff
Patch (6.64 KB, patch)
2012-10-09 12:54 PDT, Zan Dobersek
pnormand: review+
pnormand: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description kdj 2012-09-05 02:12:40 PDT
Hi Mr.Phillipe Normand,

I have found the Gst implementation issue with WebAudio usage.

In WebKitWebAudioSourceGStreamer:WebKitWebAudioSrcLoop(), the data from Audio-Bus is being assigned to Gst local buffer, and respective offset, size is being set.
But on my linux system, I observed the glitch is coming over the usage of simple webaudio app. This app works fine with other browsers.

After taking data dumps at various places, I found that before going to queue-pad, one local gst-buffer pointer is used for getting data from bus. And directly the bus->data() pointer is assigned to gst-buffer.

So here after doing following my glitch went-off

1. gst-allocation done for the one more gst8 buffer of buffer size.
2. data copied from bus to new buffer
3. then assigned pointer of this new buffer to existing gst-buffer

Please let me know your views over this fix.

Thanks and Regards,
kdj
kdj.tikka@gmail.com
Comment 1 Philippe Normand 2012-09-05 02:23:43 PDT
What do you mean by glitch? Something similar to bug 82347 ?
Comment 2 Philippe Normand 2012-09-05 02:28:57 PDT
Thanks for investigating this issue btw, I'd be happy to review your patch, feel free to attach it and following our contributor guidelines, http://www.webkit.org/coding/contributing.html
Comment 3 Philippe Normand 2012-09-05 02:55:06 PDT
I'm not sure doing a copy is the most efficient approach though, maybe the loop is spinning too fast and we'd need to wait for each buffer to be properly processed. Not sure.
Comment 4 kdj 2012-09-05 03:29:28 PDT
Thanks Mr.Normand for replying,
Yes, thinking on same line; trying one optimized solution; 'll update as get it working.
 
(In reply to comment #3)
> I'm not sure doing a copy is the most efficient approach though, maybe the loop is spinning too fast and we'd need to wait for each buffer to be properly processed. Not sure.
Comment 5 kdj 2012-09-11 23:09:57 PDT
Created attachment 163522 [details]
The webaudio glitch issue went-off with gst-buffer-allocation and data copy from bus to new buffer. Using this new buffer to connect to queue-pad 

To Webkit.org: Mr. Phillipe Normand.
Please review the patch.
Comment 6 Philippe Normand 2012-09-11 23:20:25 PDT
So you're sure doing a buffer copy is the only viable solution?
Comment 7 kdj 2012-09-12 01:09:31 PDT
With the constraints, as of now it seems the solution.
Comment 8 Philippe Normand 2012-09-13 03:56:41 PDT
the call to AudioIOCallback::render should correctly fill-in the AudioBus channels during all the time needed. Maybe we should keep a buffer list around instead of re-creating the GstBuffer at each iteration of the loop.

Sebastian, wdyt?
Comment 9 Philippe Normand 2012-09-13 04:10:25 PDT
< slomo> philn: re bug #95833, yes sounds like a good idea
Comment 10 Philippe Normand 2012-09-13 07:36:44 PDT
*** Bug 82347 has been marked as a duplicate of this bug. ***
Comment 11 Philippe Normand 2012-09-13 07:38:09 PDT
kaustubh, would you mind trying the buffer list approach?
Comment 12 kdj 2012-09-13 19:44:59 PDT
Actually I am busy with Convolver Node issue, want to get first working Convolver Node, Bug 96644. Bit difficult to get time for 'efficient approach' implementation.
Comment 13 Philippe Normand 2012-09-18 00:08:45 PDT
Zan, would you be interested to work on this patch?
Comment 14 Zan Dobersek 2012-09-18 11:12:03 PDT
(In reply to comment #13)
> Zan, would you be interested to work on this patch?

OK, I'll have a look and produce something.
Comment 15 kdj 2012-09-21 00:34:51 PDT
Thanks Mr.Zen and Mr.Philippe, for taking it. I am patched up with other issues. Definitely will come back after getting over it.
Comment 16 Zan Dobersek 2012-10-09 12:54:11 PDT
Created attachment 167826 [details]
Patch
Comment 17 Zan Dobersek 2012-10-09 13:12:39 PDT
(In reply to comment #16)
> Created an attachment (id=167826) [details]
> Patch

This patch takes a bit different approach. In each src loop GstBuffer is created for each channel, with the channel data being set to point to the allocated data of the corresponding buffer.

After rendering each buffer is chained to the appropriate GstPad. It works as well, the popping noise is gone.

I'll look further into possibly just creating a permanent GstBuffer for each channel before the looping even begins, zeroing it out before each rendering. It would spare a lot of malloc calls, but the main problem is how exactly gst_pad_chain behaves with the same buffer being passed to it each time.
Comment 18 Philippe Normand 2012-10-15 00:08:40 PDT
Comment on attachment 167826 [details]
Patch

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

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:322
> +    GSList* channelBufferList = 0;

What about using a GstBufferList? Seems like it would fit here instead of a GSList.
Comment 19 Zan Dobersek 2012-10-16 10:39:40 PDT
Comment on attachment 167826 [details]
Patch

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

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:322
>> +    GSList* channelBufferList = 0;
> 
> What about using a GstBufferList? Seems like it would fit here instead of a GSList.

I think it's not worth the trouble. The current approach pushes just a buffer to each pad while the buffer list is most helpful when pushing a bunch of buffers to the pad.
Comment 20 Zan Dobersek 2012-10-16 10:45:28 PDT
(In reply to comment #17)
> I'll look further into possibly just creating a permanent GstBuffer for each channel before the looping even begins, zeroing it out before each rendering. It would spare a lot of malloc calls, but the main problem is how exactly gst_pad_chain behaves with the same buffer being passed to it each time.

Using only one specific GstBuffer for each channel doesn't seem to work because the sound is again distorted.

In discussion on IRC Philippe recommended using GstBufferPool which seems ideal. Unfortunately it's a new addition to GStreamer, available from 1.0 onwards.

I'm recommending the approach in the latest patch as the one to use for now, until migration to GStreamer 1.0 is done.
Comment 21 Philippe Normand 2012-10-16 11:04:00 PDT
Comment on attachment 167826 [details]
Patch

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

Alright, thanks a lot Zan :) We should indeed port this code to 1.0 soon. I started a branch but haven't got far yet... too busy :(

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:325
> +        GstBuffer* channelBuffer = gst_buffer_new_and_alloc(bufferSize);

It'd be good to have an ASSERT(channelBuffer) here I think
Comment 22 Zan Dobersek 2012-10-16 11:23:16 PDT
(In reply to comment #21)
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:325
> > +        GstBuffer* channelBuffer = gst_buffer_new_and_alloc(bufferSize);
> 
> It'd be good to have an ASSERT(channelBuffer) here I think

Done. Landed in r131478.
http://trac.webkit.org/changeset/131478