Bug 80823

Summary: [GStreamer] run AudioFileReader in a nested loop
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: Web AudioAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: crogers, donggwan.kim, pnormand, s.choi, sh919.park
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61355    
Attachments:
Description Flags
Patch
none
Patch
mrobinson: review-
Patch
none
Patch mrobinson: review+

Description Philippe Normand 2012-03-12 01:31:28 PDT
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!
Comment 1 Philippe Normand 2012-03-12 01:35:41 PDT
Created attachment 131292 [details]
Patch
Comment 2 Martin Robinson 2012-03-12 10:20:40 PDT
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?
Comment 3 Philippe Normand 2012-03-12 10:26:26 PDT
(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!
Comment 4 Philippe Normand 2012-03-13 02:23:05 PDT
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 5 Martin Robinson 2012-03-13 12:35:32 PDT
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
Comment 6 Philippe Normand 2012-03-16 02:05:44 PDT
Created attachment 132229 [details]
Patch

This version doesn't use RunLoop, maybe it could be done later when
RunLoop supports non-default GMainContext?
Comment 7 Philippe Normand 2012-03-16 02:08:43 PDT
Created attachment 132230 [details]
Patch
Comment 8 Martin Robinson 2012-03-16 10:01:00 PDT
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?
Comment 9 Philippe Normand 2012-03-17 04:30:46 PDT
Committed r111119: <http://trac.webkit.org/changeset/111119>
Comment 10 Philippe Normand 2012-03-17 04:35:20 PDT
(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.