Bug 78095 – [GStreamer] HRTFDatabaseLoader conflicts with AudioFileReader

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

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.

[Sebastian Dröge (slomo)]

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? :)

[Philippe Normand]

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

[Martin Robinson]

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

[Philippe Normand]

Created attachment 130157 [details]
Patch

[Martin Robinson]

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?

[Philippe Normand]

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

[Philippe Normand]

That should be landed after bug 61355.

[Philippe Normand]

I mean, bug 67187

[Philippe Normand]

Created attachment 131290 [details]
Patch

[Philippe Normand]

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! :)

[Philippe Normand]

Chris, can you please provide some insight on this matter? :)

[Chris Rogers]

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

[Philippe Normand]

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

[Philippe Normand]

Created attachment 135598 [details]
Patch

[Martin Robinson]

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.

[Early Warning System Bot]

Comment on attachment 135598 [details]
Patch

Attachment 135598 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12323816

[Early Warning System Bot]

Comment on attachment 135598 [details]
Patch

Attachment 135598 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12327010

[Philippe Normand]

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

[Gustavo Noronha (kov)]

Comment on attachment 135598 [details]
Patch

Attachment 135598 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12319950

[Gyuyoung Kim]

Comment on attachment 135598 [details]
Patch

Attachment 135598 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12321929

[Philippe Normand]

Created attachment 135615 [details]
Patch

[Philippe Normand]

Comment on attachment 135615 [details]
Patch

Attachment 135615 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12325905

[Philippe Normand]

Created attachment 136460 [details]
Patch

Same patch but just to make sure GTK EWS builds it.

[Philippe Normand]

Comment on attachment 136460 [details]
Patch

Attachment 136460 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12384233

[Philippe Normand]

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

[Martin Robinson]

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?

[Philippe Normand]

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

[Philippe Normand]

Committed r114269: <http://trac.webkit.org/changeset/114269>