[webkit-reviews] review denied: [Bug 80823] [GStreamer] run AudioFileReader in a nested loop : [Attachment 131574] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 80823: [GStreamer] run AudioFileReader in a nested loop
https://bugs.webkit.org/show_bug.cgi?id=80823

Attachment 131574: Patch
https://bugs.webkit.org/attachment.cgi?id=131574&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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