Bug 95833 – [GStreamer] GstBuffer ref race in WebKitWebAudioSrcLoop

• Component: Platform
• Status: RESOLVED FIXED

[kdj]

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
[e-mail]

[Philippe Normand]

What do you mean by glitch? Something similar to bug 82347 ?

[Philippe Normand]

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

[Philippe Normand]

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.

[kdj]

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.

[kdj]

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.

[Philippe Normand]

So you're sure doing a buffer copy is the only viable solution?

[kdj]

With the constraints, as of now it seems the solution.

[Philippe Normand]

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?

[Philippe Normand]

< slomo> philn: re bug #95833, yes sounds like a good idea

[Philippe Normand]

*** Bug 82347 has been marked as a duplicate of this bug. ***

[Philippe Normand]

kaustubh, would you mind trying the buffer list approach?

[kdj]

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.

[Philippe Normand]

Zan, would you be interested to work on this patch?

[Zan Dobersek]

(In reply to comment #13)
> Zan, would you be interested to work on this patch?

OK, I'll have a look and produce something.

[kdj]

Thanks Mr.Zen and Mr.Philippe, for taking it. I am patched up with other issues. Definitely will come back after getting over it.

[Zan Dobersek]

Created attachment 167826 [details]
Patch

[Zan Dobersek]

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

[Philippe Normand]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

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

[Philippe Normand]

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

[Zan Dobersek]

(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