| Summary: | [GStreamer] HRTFDatabaseLoader conflicts with AudioFileReader | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||||
| Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | crogers, dynastpsh, eric.carlson, feature-media-reviews, gustavo, 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: |
|
||||||||||||||
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? :) (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! (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. Created attachment 130157 [details]
Patch
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?
(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. Created attachment 131290 [details]
Patch
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! :) Chris, can you please provide some insight on this matter? :) (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. (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. Created attachment 135598 [details]
Patch
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 on attachment 135598 [details] Patch Attachment 135598 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12323816 Comment on attachment 135598 [details] Patch Attachment 135598 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12327010 (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 on attachment 135598 [details] Patch Attachment 135598 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12319950 Comment on attachment 135598 [details] Patch Attachment 135598 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12321929 Created attachment 135615 [details]
Patch
Comment on attachment 135615 [details] Patch Attachment 135615 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12325905 Created attachment 136460 [details]
Patch
Same patch but just to make sure GTK EWS builds it.
Comment on attachment 136460 [details] Patch Attachment 136460 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12384233 (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 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 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 :( Committed r114269: <http://trac.webkit.org/changeset/114269> |
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.