The current implementation blocks and iterates on the GLib default main context. This potentially triggers ASSERTs, the file reader not running in the main thread and some timer being fired from the resource loader (running in the main thread) in the file reader thread. We should use a nested GLib main loop!
Created attachment 131292 [details] Patch
Comment on attachment 131292 [details] Patch Is it possible to use Mario's new nested main loop support in WebCore::RunLoop to simplify this code?
(In reply to comment #2) > (From update of attachment 131292 [details]) > Is it possible to use Mario's new nested main loop support in WebCore::RunLoop to simplify this code? I'll look into it!
Created attachment 131574 [details] Patch I can't use RunLoop here because I need a new GMainContext, the RunLoop code afaik uses the default context. Compared to previous patch I made the m_loop and m_context variables smart pointers.
Comment on attachment 131574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131574&action=review I think that reusing RunLoop is something that we want to support. Why not add an optional parameter to RunLoop::pushNestedMainLoop that takes a GMainContext. If we do that we can reuse this for synchronous XMLHttpRequests as well. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:276 > +void AudioFileReader::start() This could probably use a more descriptive name. I'm not certain, but perhaps something like "decodeAudioForBusCreation" > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:289 > + GstElement* decodebin = gst_element_factory_make("decodebin2", "decodebin"); Perhaps a member variable for the decodebin? > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:315 > + m_context = g_main_context_new(); m_context is only used locally so it doesn't need to be a member variable. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:322 > GstBus* bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline)); > gst_bus_add_signal_watch(bus); > gulong busSignalHandlerId = g_signal_connect(bus, "message", G_CALLBACK(messageCallback), this); > Can't this and the cleanup go into AudioFileReader::start now? > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:326 > + g_source_set_callback(timeoutSource, (GSourceFunc) enteredMainLoopCallback, this, 0); reinterpret_cast
Created attachment 132229 [details] Patch This version doesn't use RunLoop, maybe it could be done later when RunLoop supports non-default GMainContext?
Created attachment 132230 [details] Patch
Comment on attachment 132230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132230&action=review > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:332 > + GstBus* bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline)); > + g_signal_handlers_disconnect_by_func(bus, reinterpret_cast<gpointer>(messageCallback), this); > + gst_object_unref(bus); You could use a RefPtr here with adoptRef. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:334 > + g_signal_handlers_disconnect_by_func(m_decodebin.get(), reinterpret_cast<gpointer>(onGStreamerDecodebinPadAddedCallback), this); Do you need to release m_decodeBin or does it happen later automatically?
Committed r111119: <http://trac.webkit.org/changeset/111119>
(In reply to comment #8) > (From update of attachment 132230 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132230&action=review > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:332 > > + GstBus* bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline)); > > + g_signal_handlers_disconnect_by_func(bus, reinterpret_cast<gpointer>(messageCallback), this); > > + gst_object_unref(bus); > > You could use a RefPtr here with adoptRef. > Right, I added GstBus support in GRefPtrGStreamer! > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:334 > > + g_signal_handlers_disconnect_by_func(m_decodebin.get(), reinterpret_cast<gpointer>(onGStreamerDecodebinPadAddedCallback), this); > > Do you need to release m_decodeBin or does it happen later automatically? Hum when the pipeline is unreffed all its elements are cleared as well AFAIK. But I just explicitely clear out the decodebin and deinterleave smart pointers too.