Bug 80823 – [GStreamer] run AudioFileReader in a nested loop

• Component: Web Audio
• Status: RESOLVED FIXED

[Philippe Normand]

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!

[Philippe Normand]

Created attachment 131292 [details]
Patch

[Martin Robinson]

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?

[Philippe Normand]

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

[Philippe Normand]

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.

[Martin Robinson]

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

[Philippe Normand]

Created attachment 132229 [details]
Patch

This version doesn't use RunLoop, maybe it could be done later when
RunLoop supports non-default GMainContext?

[Philippe Normand]

Created attachment 132230 [details]
Patch

[Martin Robinson]

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?

[Philippe Normand]

Committed r111119: <http://trac.webkit.org/changeset/111119>

[Philippe Normand]

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