Bug 105292 – [GStreamer] GstMessage handler in AudioDestination

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

Useful to report gst warnings and errors, like in the AudioFileReader implementation.

[Philippe Normand]

Created attachment 179941 [details]
AudioDestination: GstMessage handler

[Martin Robinson]

Comment on attachment 179941 [details]
AudioDestination: GstMessage handler

View in context: https://bugs.webkit.org/attachment.cgi?id=179941&action=review

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:68
> +    GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline));
> +    gst_bus_add_signal_watch(bus.get());
> +    g_signal_connect(bus.get(), "message", G_CALLBACK(messageCallback), this);

See below.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:91
> +    GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline));
> +    g_signal_handlers_disconnect_by_func(bus.get(), reinterpret_cast<gpointer>(messageCallback), this);

Why not just use a raw pointer? By using GRefPtr you are doing a ref and unref.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:140
> +        g_warning("Warning: %d, %s. Debug output: %s", error->code,  error->message, debug.get());

Wouldn't it make sense to use gst_debug_log here?

[Philippe Normand]

(In reply to comment #2)
> (From update of attachment 179941 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179941&action=review
> 
> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:68
> > +    GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline));
> > +    gst_bus_add_signal_watch(bus.get());
> > +    g_signal_connect(bus.get(), "message", G_CALLBACK(messageCallback), this);
> 
> See below.
> 
> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:91
> > +    GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline));
> > +    g_signal_handlers_disconnect_by_func(bus.get(), reinterpret_cast<gpointer>(messageCallback), this);
> 
> Why not just use a raw pointer? By using GRefPtr you are doing a ref and unref.

The bus returned by gst_pipeline_get_bus() needs to be unreffed after usage.

> 
> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:140
> > +        g_warning("Warning: %d, %s. Debug output: %s", error->code,  error->message, debug.get());
> 
> Wouldn't it make sense to use gst_debug_log here?

Well I wanted to use g_warning here so the user sees the message in the console, this is rather minimal error reporting but less hidden than GST_DEBUG :)

I used the same approach in the AudioFileReader BTW, so any change needed in this patch would need to be done there as well.

[Martin Robinson]

Comment on attachment 179941 [details]
AudioDestination: GstMessage handler

View in context: https://bugs.webkit.org/attachment.cgi?id=179941&action=review

>>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:68
>>> +    g_signal_connect(bus.get(), "message", G_CALLBACK(messageCallback), this);
>> 
>> See below.
> 
> The bus returned by gst_pipeline_get_bus() needs to be unreffed after usage.

Ah, then you are actually leaking it because you aren't using adoptGRef.

>>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:140
>>> +        g_warning("Warning: %d, %s. Debug output: %s", error->code,  error->message, debug.get());
>> 
>> Wouldn't it make sense to use gst_debug_log here?
> 
> Well I wanted to use g_warning here so the user sees the message in the console, this is rather minimal error reporting but less hidden than GST_DEBUG :)
> 
> I used the same approach in the AudioFileReader BTW, so any change needed in this patch would need to be done there as well.

I guess it's okay as long as there is a way to disable the output. The issue with libraries that print to stderr and stdout is that they cannot be used easily for command-line tools that produce output.

[Philippe Normand]

Created attachment 180858 [details]
AudioDestination: GstMessage handler

[Philippe Normand]

So I removed all the g_warning stuff. The gst warnings and errors appear in the GST_DEBUG output anyway.

[Martin Robinson]

Comment on attachment 180858 [details]
AudioDestination: GstMessage handler

View in context: https://bugs.webkit.org/attachment.cgi?id=180858&action=review

Oops! I think I miscommunicated. g_warning and g_debug are probably okay since you can control them with the G_DEBUG environment variable. On the other hand I think that the GstBus that you get from gst_pipeline_get_bus are still leaking in this patch.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:68
> +    GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline));
> +    gst_bus_add_signal_watch(bus.get());
> +    g_signal_connect(bus.get(), "message", G_CALLBACK(messageCallback), this);

If gst_pipeline_get_bus doesn't return a new reference you can use a raw pointer. If it does, you should use adoptRef. The documentation says "transfer full" so I think this needs to be adopted.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:91
> +    GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline));
> +    g_signal_handlers_disconnect_by_func(bus.get(), reinterpret_cast<gpointer>(messageCallback), this);

Ditto.

[Philippe Normand]

Created attachment 180893 [details]
AudioDestination: GstMessage handler

[Philippe Normand]

Committed r138545: <http://trac.webkit.org/changeset/138545>

[Chris Dumez]

This patch is causing a lot of crashes on the WK2 EFL Debug build bot:

crash log for WebProcess (pid <unknown>):
STDOUT: <empty>
STDERR: ASSERTION FAILED: !gstObjectIsFloating(GST_OBJECT(ptr))
STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp(130) : WTF::GRefPtr<T> WTF::adoptGRef(T*) [with T = _GstBus]
STDERR: 1   0x7fd7d388adde WTF::GRefPtr<_GstBus> WTF::adoptGRef<_GstBus>(_GstBus*)
STDERR: 2   0x7fd7d38bc9b1 WebCore::AudioFileReader::decodeAudioForBusCreation()
STDERR: 3   0x7fd7d38bbd63 WebCore::enteredMainLoopCallback(void*)
STDERR: 4   0x7fd7ce0c7a1b
STDERR: 5   0x7fd7ce0c6e53 g_main_context_dispatch
STDERR: 6   0x7fd7ce0c71a0
STDERR: 7   0x7fd7ce0c759a g_main_loop_run
STDERR: 8   0x7fd7d38bcd88 WebCore::AudioFileReader::createBus(float, bool)
STDERR: 9   0x7fd7d38bcf5f WebCore::createBusFromAudioFile(char const*, bool, float)
STDERR: 10  0x7fd7d38baad7 WebCore::AudioBus::loadPlatformResource(char const*, float)
STDERR: 11  0x7fd7d2d92589
STDERR: 12  0x7fd7d2d92a6e WebCore::HRTFElevation::calculateKernelsForAzimuthElevation(int, int, float, WTF::String const&, WTF::RefPtr<WebCore::HRTFKernel>&, WTF::RefPtr<WebCore::HRTFKernel>&)
STDERR: 13  0x7fd7d2d92f3c WebCore::HRTFElevation::createForSubject(WTF::String const&, int, float)
STDERR: 14  0x7fd7d2d8fd54 WebCore::HRTFDatabase::HRTFDatabase(float)
STDERR: 15  0x7fd7d2d8fc8e WebCore::HRTFDatabase::create(float)
STDERR: 16  0x7fd7d2d91b31 WebCore::HRTFDatabaseLoader::load()
STDERR: 17  0x7fd7d2d91ab0
STDERR: 18  0x7fd7d39682f5
STDERR: 19  0x7fd7d398fdea
STDERR: 20  0x7fd7ccfffe9a
STDERR: 21  0x7fd7d643dcbd clone

[Philippe Normand]

Seems like adoptGRef was indeed not needed then?

[Chris Dumez]

Created attachment 180954 [details]
Test patch using manual unref'ing

FYI, switching to raw pointers and using gst_object_unref (see attached patch) fixes the crashes for me (I don't see any warnings on stderr either). I'm not quite sure why this is different from using a smart pointer and adopting though.

[Philippe Normand]

(In reply to comment #12)
> Created an attachment (id=180954) [details]
> Test patch using manual unref'ing
> 
> FYI, switching to raw pointers and using gst_object_unref (see attached patch) fixes the crashes for me (I don't see any warnings on stderr either). I'm not quite sure why this is different from using a smart pointer and adopting though.

Because adoptGref() checks if the object has a floating ref or not.
So yeah either we use raw pointers or revert the parts of the landed patch that added the adoptGRef calls.

I'd be for using raw pointers, I never really liked this smart pointer stuff :P

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #12)
> > Created an attachment (id=180954) [details] [details]
> > Test patch using manual unref'ing
> > 
> > FYI, switching to raw pointers and using gst_object_unref (see attached patch) fixes the crashes for me (I don't see any warnings on stderr either). I'm not quite sure why this is different from using a smart pointer and adopting though.
> 
> Because adoptGref() checks if the object has a floating ref or not.
> So yeah either we use raw pointers or revert the parts of the landed patch that added the adoptGRef calls.
> 
> I'd be for using raw pointers, I never really liked this smart pointer stuff :P

Ok, I uploaded a patch at Bug 105869. I also prefer using raw pointers in this case.