[Webkit-unassigned] [Bug 69835] [GStreamer] WebAudio AudioDestination

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=69835





--- Comment #28 from Martin Robinson <mrobinson at webkit.org>  2011-11-21 09:11:50 PST ---
(From update of attachment 115118)
View in context: https://bugs.webkit.org/attachment.cgi?id=115118&action=review

>>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:95
>>> +    GstElement* audiosink = gst_element_factory_make("autoaudiosink", 0);
>> 
>> RefPtr here?
> 
> Doesn't work :( I need to unref only if the element is not added to the bin.

You can use GRefPtr::leakRef in that case with a note that gst_add_many is transfer full.

>>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:98
>>> +    if (audiosink) {
>> 
>> Just move the early return below before here.
> 
> Hum I can't because m_audioSinkAvailable might change just below, line 106.

If m_audioSinkAvailable changes you return early right away, so it shouldn't matter. What I'm suggesting is:

ASSERT_WITH_MESSAGE(audioSink, "Failed to create GStreamer autoaudiosink element");
if (!audioSink)
    return;

GstStateChangeReturn stateChangeReturn = gst_element_set_state(audiosink, GST_STATE_READY);
ASSERT_WITH_MESSAGE(m_audioSinkAvailable, "Failed to change autoaudiosink element state");
if (stateChangeReturn == GST_STATE_CHANGE_FAILURE) {
    gst_element_set_state(audiosink, GST_STATE_NULL);
    m_audioSinkAvailable = false;
    gst_object_unref(audiosink);
            return;

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:112
> +    ASSERT_WITH_MESSAGE(m_audioSinkAvailable, "Failed to create GStreamer autoaudiosink element");

ASSERT actually crashes the application in debug mode. Can happen as the result of a missing installed plugin or some other non-fatal gstreamer error? If that's the case, I think you sould just print a message and not assert. The main signal that this shouldn't be an assert is that you are handling the failure case really thoroughly.

>>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:207
>>> +    priv->wavEncoder = gst_element_factory_make("wavenc", 0);
>> 
>> Do these have floating references or full references? If full references you need to call adoptGRef here.
> 
> Well, full references afaik, so trying to use adoptGRef here, the destructor is called immediately :(
> 
> #0  WTF::derefGPtr<_GstElement> (ptr=0x8077260) at ../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:42
> #1  0xb6af957d in WTF::GRefPtr<_GstElement>::~GRefPtr (this=0xbfffd75c, __in_chrg=<optimized out>)
>     at ../../Source/JavaScriptCore/wtf/gobject/GRefPtr.h:72
> #2  0xb6af851d in webKitWebAudioSrcConstructed (object=0x84c7150)
>     at ../../Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:206
> 
> Not sure what's going on here :( So the GstElement is unreffed and destructed even before we get a chance to add it to the bin...

That would suggest they are floating references. The documentation seems to confirm that: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstObject.html

I think that this means the GRefPtr needs to do:
gst_object_ref(GST_OBJECT(m_ptr));
gst_object_sink(GST_OBJECT(m_ptr));

This is because the gst_object_sink function works differently than the g_object_ref_sink function. gst_object_sink does nothing if the GstObject does not have a floating reference, while in this case g_object_ref_sink increases the reference count by one. I think that should fix the crash.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.