WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
95833
[GStreamer] GstBuffer ref race in WebKitWebAudioSrcLoop
https://bugs.webkit.org/show_bug.cgi?id=95833
Summary
[GStreamer] GstBuffer ref race in WebKitWebAudioSrcLoop
kdj
Reported
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
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Philippe Normand
Comment 1
2012-09-05 02:23:43 PDT
What do you mean by glitch? Something similar to
bug 82347
?
Philippe Normand
Comment 2
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
Philippe Normand
Comment 3
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.
kdj
Comment 4
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.
kdj
Comment 5
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.
Philippe Normand
Comment 6
2012-09-11 23:20:25 PDT
So you're sure doing a buffer copy is the only viable solution?
kdj
Comment 7
2012-09-12 01:09:31 PDT
With the constraints, as of now it seems the solution.
Philippe Normand
Comment 8
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?
Philippe Normand
Comment 9
2012-09-13 04:10:25 PDT
< slomo> philn: re
bug #95833
, yes sounds like a good idea
Philippe Normand
Comment 10
2012-09-13 07:36:44 PDT
***
Bug 82347
has been marked as a duplicate of this bug. ***
Philippe Normand
Comment 11
2012-09-13 07:38:09 PDT
kaustubh, would you mind trying the buffer list approach?
kdj
Comment 12
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.
Philippe Normand
Comment 13
2012-09-18 00:08:45 PDT
Zan, would you be interested to work on this patch?
Zan Dobersek
Comment 14
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.
kdj
Comment 15
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.
Zan Dobersek
Comment 16
2012-10-09 12:54:11 PDT
Created
attachment 167826
[details]
Patch
Zan Dobersek
Comment 17
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.
Philippe Normand
Comment 18
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.
Zan Dobersek
Comment 19
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.
Zan Dobersek
Comment 20
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.
Philippe Normand
Comment 21
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
Zan Dobersek
Comment 22
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug