[Webkit-unassigned] [Bug 79795] Unreleased gst_object_reference to audio sink in MediaPlayerPrivateGStreamer

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from David Corvoysier <david.corvoysier at orange-ftgroup.com>  2012-02-29 07:09:58 PST ---
> Well Martin is right indeed. We have implemented exactly what you describe in GRefPtrGStreamer.cpp. So if you can update the patch it would be awesome :)

You're right, sorry I missed that: I am mainly working on the qtwebkit22 branch where they have not been introduced yet, although GOwnPtr was already there ...

Anyway, I am still a bit puzzled by the way these RefPtr are used in MediaPlayerPrivateGStreamer. 

See for example how the handle on the audio-sink is retrieved in updateAudioSink:

Previously, using GOwnPtr:

    GOwnPtr<GstElement> element;

    g_object_get(m_playBin, "audio-sink", &element.outPtr(), NULL);
    gst_object_replace(reinterpret_cast<GstObject**>(&m_webkitAudioSink),
                       reinterpret_cast<GstObject*>(element.get()));

Let's say that the audio sink element initially has a gst refcount of n.
- The call to g_object_get will increase it to n+1 (from what I see in playbin2),
- The call to gst_object_replace passing GOwnPtr.get() will increase the gst refcount to n+2
- the audio sink refcount will go back to n+1 when the element GOwnPtr goes out of scope

The consequence is that in the destructor, an explicit call to gst_object_unref on the audio sink is required (my patch).

Now, using GRefPtr:

    GRefPtr<GstElement> element;
    GstElement* sinkPtr = 0;

    g_object_get(m_playBin, "audio-sink", &sinkPtr, NULL);
    element = adoptGRef(sinkPtr);

    gst_object_replace(reinterpret_cast<GstObject**>(&m_webkitAudioSink),
                       reinterpret_cast<GstObject*>(element.get()));

Let's say that the audio sink element initially has a gst refcount of n.
- The call to g_object_get will increase it to n+1 (from what I see in playbin2),
- The transfer to the GstRefPtr (through adoptGRef) will further increase it to n+2,
- The call to gst_object_replace will further increase the gst refcount to n+3 (no refcount decrease as passing GRefPtr.get() is used here)
- When element loses visibility, the gst refcount will go back to n+2.

Now, when returning fom updateAudioSink, the m_webkitAudioSink member points to an element whose gst refcount has been increased to n+2, instead of n+1.

So, correct me if I am wrong, but the situation is even worse than before, and two gst_unref would now be required to actually finalize the audio-sink (the same applies to the webkit source element)...

If the code was purely gstreamer, it would look like:

    GstElement* sinkPtr = 0;

    g_object_get(m_playBin, "audio-sink", &sinkPtr, NULL);

    gst_object_replace(reinterpret_cast<GstObject**>(&m_webkitAudioSink),
                       reinterpret_cast<GstObject*>(sinkPtr));

    gst_object_unref(sinkPtr);

Now, if we wanted to store the audio sink handle as a GRefPtr, I assume it would look like:

    GRefPtr<GstElement> m_webkitAudioSink;
...

    GstElement* sinkPtr = 0;

    g_object_get(m_playBin, "audio-sink", &sinkPtr, NULL);

    m_webkitAudioSink= adoptGRef(sinkPtr);

    gst_object_unref(sinkPtr);

Is it correct or did I get it all wrong ?

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