Bug 78095

Summary: [GStreamer] HRTFDatabaseLoader conflicts with AudioFileReader
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: crogers, dynastpsh, eric.carlson, feature-media-reviews, gns, menard, mrobinson, pnormand, rakuco, s.choi, sh919.park, slomo, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 67187    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
webkit-ews: commit-queue-
Patch
pnormand: commit-queue-
Patch mrobinson: review+, pnormand: commit-queue-

Description Philippe Normand 2012-02-08 02:45:34 PST
While running the webaudio tests I noticed a racy behavior in the GStreamer implementation of WebAudio.

Sometimes while the HRTF DatabaseLoader thread is still running (and underneath calling gst_init_check() which updates the gst registry) another thread is trying to create an AudioBus and fails to create a giostreamsrc gst element. One example of the 2 threads:


Thread 10 (Thread 0xae620b70 (LWP 9464)):
#0  0xb60d96ca in __i686.get_pc_thunk.bx () from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Dependencies/Root/lib/libglib-2.0.so.0
#1  0xb6149dac in g_mutex_get_impl (mutex=0x84c9d58) at gthread-posix.c:118
#2  0xb614a0a8 in g_mutex_unlock (mutex=0x84c9d58) at gthread-posix.c:227
#3  0xb438ab11 in gst_object_set_name (object=0x84d4360, name=0x0) at gstobject.c:708
#4  0xb438ad20 in gst_object_set_property (value=0xae61fadc, object=0x84d4360, prop_id=<optimized out>, pspec=<optimized out>) at gstobject.c:1085
#5  gst_object_set_property (object=0x84d4360, prop_id=1, value=0xae61fadc, pspec=0x846a6c8) at gstobject.c:1076
#6  0xb61f66c6 in object_set_property (nqueue=0x84c9e50, value=0x84d7b08, pspec=0x846a6c8, object=0x84d4360) at gobject.c:1342
#7  g_object_constructor (type=138861384, n_construct_properties=<optimized out>, construct_params=0x84d7cb0) at gobject.c:1853
#8  0xb61f7d89 in g_object_newv (object_type=138861384, n_parameters=0, parameters=0x0) at gobject.c:1703
#9  0xb43e2b83 in gst_registry_chunks_load_feature (plugin=0x84cc6a0, end=0xadcff7ca "", in=0xae61fc90, registry=0x8476410)
    at gstregistrychunks.c:580
#10 _priv_gst_registry_chunks_load_plugin (registry=0x8476410, in=0xae61fc90, end=0xadcff7ca "", out_plugin=0x0) at gstregistrychunks.c:861
#11 0xb440c4c4 in gst_registry_binary_read_cache (registry=0x8476410, location=0x8476338 "/home/phil/.gstreamer-0.10/registry.i686.bin")
    at gstregistrybinary.c:600
#12 0xb43df9a2 in ensure_current_registry (error=0xae61fd0c) at gstregistry.c:1649
#13 gst_update_registry () at gstregistry.c:1759
#14 0xb43886c8 in init_post (context=<optimized out>, group=<optimized out>, data=<optimized out>, error=<optimized out>) at gst.c:793
#15 init_post (context=0x84696d0, group=0x8467fa0, data=0x0, error=0x0) at gst.c:658
#16 0xb61166a0 in g_option_context_parse (context=0x84696d0, argc=0x0, argv=0x0, error=0x0) at goption.c:2015
#17 0xb4389151 in gst_init_check (argc=0x0, argv=0x0, err=0x0) at gst.c:445
#18 0xb73c26d4 in WebCore::AudioFileReader::createBus(float, bool) ()
   from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#19 0xb73c27e8 in WebCore::createBusFromAudioFile(char const*, bool, float) ()
   from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#20 0xb73c445c in WebCore::AudioBus::loadPlatformResource(char const*, float) ()
   from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#21 0xb73baeaf in WebCore::HRTFElevation::calculateKernelsForAzimuthElevation(int, int, float, WTF::String const&, WTF::RefPtr<WebCore::HRTFKernel>&, WTF::RefPtr<WebCore::HRTFKernel>&) () from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#22 0xb73bb0fe in WebCore::HRTFElevation::createForSubject(WTF::String const&, int, float) ()
   from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#23 0xb73b958b in WebCore::HRTFDatabase::HRTFDatabase(float) ()
   from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#24 0xb73b9789 in WebCore::HRTFDatabase::create(float) () from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#25 0xb73b98f8 in WebCore::HRTFDatabaseLoader::load() () from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#26 0xb73b9d0a in WebCore::databaseLoaderEntry(void*) () from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#27 0xb7f2583a in WTF::threadEntryPoint(void*) () from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libjavascriptcoregtk-3.0.so.0
#28 0xb5c6bc39 in start_thread (arg=0xae620b70) at pthread_create.c:304
#29 0xb5bd912e in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:130
Thread 1 (Thread 0xb3e71ad0 (LWP 9452)):
#0  g_logv (log_domain=0xb621d7a4 "GLib-GObject", log_level=<optimized out>, format=0xb61745b4 "%s: assertion `%s' failed", 
    args1=0xbfffda9c "\004\032\"\266)\340!\266@g\377\267\200\255E\b\320#\021\266\244\327!\266p\254@\266@\002#\266\217\223\037\266\244\327!\266\004\032\"\266)\340!\266") at gmessages.c:765
#1  0xb61123c3 in g_log (log_domain=0xb621d7a4 "GLib-GObject", log_level=G_LOG_LEVEL_CRITICAL, format=0xb61745b4 "%s: assertion `%s' failed")
    at gmessages.c:792
#2  0xb611240d in g_return_if_fail_warning (log_domain=0xb621d7a4 "GLib-GObject", pretty_function=0xb6221a04 "g_object_set", 
    expression=0xb621e029 "G_IS_OBJECT (object)") at gmessages.c:801
#3  0xb61f938f in g_object_set (_object=0x0, first_property_name=0xb78228e6 "stream") at gobject.c:2040
#4  0xb73c2463 in WebCore::AudioFileReader::createBus(float, bool) ()
   from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#5  0xb73c2856 in WebCore::createBusFromInMemoryAudioFile(void const*, unsigned int, bool, float) ()
   from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#6  0xb739f2c5 in WebCore::AudioBuffer::createFromAudioFileData(void const*, unsigned int, bool, float) ()
   from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#7  0xb73a28d2 in WebCore::AudioContext::createBuffer(WTF::ArrayBuffer*, bool, int&) ()
   from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
#8  0xb746c678 in WebCore::jsAudioContextPrototypeFunctionCreateBuffer(JSC::ExecState*) ()
   from /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0
...

I wonder if in the GTK case we should simply run the HRTFDatabaseLoader in the main loop instead of creating a new thread.
Comment 1 Sebastian Dröge (slomo) 2012-02-08 03:05:13 PST
Yes, gst_init() should be called from the main thread before any other GStreamer function and ideally before anything else. Same for gobject_init() and gtk_init() btw, I hope these are not called from different threads? :)
Comment 2 Philippe Normand 2012-02-08 03:09:02 PST
(In reply to comment #1)
> Yes, gst_init() should be called from the main thread before any other GStreamer function and ideally before anything else. Same for gobject_init() and gtk_init() btw, I hope these are not called from different threads? :)

We're safe on the gobject/gtk side I think :)
I started already looking at make the HRTF DatabaseLoader run in the main loop, some good results already!
Comment 3 Martin Robinson 2012-02-08 07:41:11 PST
(In reply to comment #2)

> We're safe on the gobject/gtk side I think :)
> I started already looking at make the HRTF DatabaseLoader run in the main loop, some good results already!

Why not just run gst_init() in the main loop and keep the HRTF DatabaseLoader in it's own thread? I believe it's best to keep expensive operations out of the main thread.
Comment 4 Philippe Normand 2012-03-05 09:51:05 PST
Created attachment 130157 [details]
Patch
Comment 5 Martin Robinson 2012-03-05 09:53:52 PST
Comment on attachment 130157 [details]
Patch

Maybe it'd be better to move this to a more global location? Is there some function that initializes the WebAudio backend as a whole?
Comment 6 Philippe Normand 2012-03-05 10:27:39 PST
(In reply to comment #5)
> (From update of attachment 130157 [details])
> Maybe it'd be better to move this to a more global location? Is there some function that initializes the WebAudio backend as a whole?

I haven't found such function yet. I'll keep looking!
The reason why I moved this in FFTFrame::initialize is because it's called by AudioContext::constructCommon() which is called in the AudioContext ctors.
Comment 7 Philippe Normand 2012-03-07 09:41:48 PST
That should be landed after bug 61355.
Comment 8 Philippe Normand 2012-03-07 09:42:35 PST
I mean, bug 67187
Comment 9 Philippe Normand 2012-03-12 01:27:21 PDT
Created attachment 131290 [details]
Patch
Comment 10 Philippe Normand 2012-03-16 10:21:43 PDT
Chris, it's indeed a bit odd to use FFTFrame::initialize() to initialize various backend-specific bits. Would it be Ok to add a more general function for this? Any suggestion welcome! :)
Comment 11 Philippe Normand 2012-04-03 02:36:07 PDT
Chris, can you please provide some insight on this matter? :)
Comment 12 Chris Rogers 2012-04-03 13:34:55 PDT
(In reply to comment #10)
> Chris, it's indeed a bit odd to use FFTFrame::initialize() to initialize various backend-specific bits. Would it be Ok to add a more general function for this? Any suggestion welcome! :)

Hi Philippe, I think it's ok to create your GTK-specific initialize function which will call gst_init_check() instead of putting it in FFTFrame::initialize() (where it doesn't really belong).

Then in AudioContext::contructCommon() you could conditionally compile in the call to this function.
Comment 13 Philippe Normand 2012-04-04 08:17:23 PDT
(In reply to comment #12)
> (In reply to comment #10)
> > Chris, it's indeed a bit odd to use FFTFrame::initialize() to initialize various backend-specific bits. Would it be Ok to add a more general function for this? Any suggestion welcome! :)
> 
> Hi Philippe, I think it's ok to create your GTK-specific initialize function which will call gst_init_check() instead of putting it in FFTFrame::initialize() (where it doesn't really belong).
> 
> Then in AudioContext::contructCommon() you could conditionally compile in the call to this function.

Thanks Chris!
I'm preparing a patch adding a new GStreamerUtilities.cpp/h which for now only contains the init function, but later we could add more functions if needed.
Comment 14 Philippe Normand 2012-04-04 08:30:45 PDT
Created attachment 135598 [details]
Patch
Comment 15 Martin Robinson 2012-04-04 08:39:59 PDT
Comment on attachment 135598 [details]
Patch

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

Looks good, though I've pointed out a few small nits below.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:186
> +    WebCore::initializeGStreamer();

You are already in the WebCore namespace here, right?

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:22
> +
> +#include "GStreamerUtilities.h"

Extra newline here.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:27
> +#include <wtf/gobject/GOwnPtr.h>
> +
> +#if USE(GSTREAMER)
> +#include <gst/gst.h>

I think I'd prefer the includes to all be on one side of the guard.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:196
> +    GRefPtr<GstElementFactory> factory = gst_element_factory_find(gPlaybinName);
> +    if (factory)

I think you could actually do:

GRefPtr<GstElementFactory> factory = gst_element_factory_find(gPlaybinName);
return factory;

> Source/autotools/symbols.filter:138
> +_ZN7WebCore19initializeGStreamerEv;

Why is this necessary? It should only be required if yo uare using this function outside of libwebkitgtk, I think.
Comment 16 Early Warning System Bot 2012-04-04 08:59:59 PDT
Comment on attachment 135598 [details]
Patch

Attachment 135598 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12323816
Comment 17 Early Warning System Bot 2012-04-04 09:09:57 PDT
Comment on attachment 135598 [details]
Patch

Attachment 135598 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12327010
Comment 18 Philippe Normand 2012-04-04 09:22:53 PDT
(In reply to comment #15)
> (From update of attachment 135598 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135598&action=review
> > Source/autotools/symbols.filter:138
> > +_ZN7WebCore19initializeGStreamerEv;
> 
> Why is this necessary? It should only be required if yo uare using this function outside of libwebkitgtk, I think.

Hum I had a runtime failure when running the tests about that missing symbol. I'll double check and upload a new patch which hopefully should build on Qt and EFL too.
Comment 19 Gustavo Noronha (kov) 2012-04-04 09:30:37 PDT
Comment on attachment 135598 [details]
Patch

Attachment 135598 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12319950
Comment 20 Gyuyoung Kim 2012-04-04 09:57:46 PDT
Comment on attachment 135598 [details]
Patch

Attachment 135598 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12321929
Comment 21 Philippe Normand 2012-04-04 10:06:16 PDT
Created attachment 135615 [details]
Patch
Comment 22 Philippe Normand 2012-04-04 11:51:25 PDT
Comment on attachment 135615 [details]
Patch

Attachment 135615 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12325905
Comment 23 Philippe Normand 2012-04-10 08:11:29 PDT
Created attachment 136460 [details]
Patch

Same patch but just to make sure GTK EWS builds it.
Comment 24 Philippe Normand 2012-04-10 08:20:14 PDT
Comment on attachment 136460 [details]
Patch

Attachment 136460 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12384233
Comment 25 Philippe Normand 2012-04-10 09:20:54 PDT
(In reply to comment #24)
> (From update of attachment 136460 [details])
> Attachment 136460 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/12384233

A clean build is needed.
Comment 26 Martin Robinson 2012-04-16 08:53:23 PDT
Comment on attachment 136460 [details]
Patch

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

Looks good, but take a look at my comment below before landing.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:179
> +bool doGstInit()

Is this used outside the file now? It's no longer static. Now that you have initializeGStreamer, perhaps it makese sense to rename this method to differeniate it from initializeGStreamer. How about initializeGStreamerAndRegisterWebKitWebSrc?
Comment 27 Philippe Normand 2012-04-16 09:05:48 PDT
Comment on attachment 136460 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:179
>> +bool doGstInit()
> 
> Is this used outside the file now? It's no longer static. Now that you have initializeGStreamer, perhaps it makese sense to rename this method to differeniate it from initializeGStreamer. How about initializeGStreamerAndRegisterWebKitWebSrc?

I agree about renaming but I saw some branches of this code doing additional elements registration. So maybe we should have a less specific name, like initializeGStreamerAndAndRegisterWebKitElements ? long names ftw :(
Comment 28 Philippe Normand 2012-04-16 10:14:53 PDT
Committed r114269: <http://trac.webkit.org/changeset/114269>