Bug 79795 – Unreleased gst_object_reference to audio sink in MediaPlayerPrivateGStreamer

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[David Corvoysier]

The MediaPlayerPrivateGStreamer object holds a reference to the underlying pipeline audio sink stored in m_webkitAudioSink (see MediaPlayerPrivateGStreamer::updateAudioSink()).
The trouble is that, unlike other references to source element or video sink, this reference is not released upon the object destruction (ie gst_object_unref is not called).

[Philippe Normand]

Good catch indeed! I'll be happy to review the patch :)

[David Corvoysier]

Created attachment 129267 [details]
Proposed patch

[Philippe Normand]

Comment on attachment 129267 [details]
Proposed patch

Thanks, David! Do you need the commit-queue to land this?

[WebKit Review Bot]

Comment on attachment 129267 [details]
Proposed patch

Attachment 129267 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11662224

New failing tests:
css3/filters/effect-invert-hw.html

[Martin Robinson]

Looks like a good candidate for a RefPtr!

[David Corvoysier]

(In reply to comment #5)
> Looks like a good candidate for a RefPtr!

Not really, I think, as this is purely related to the specific gstreamer refcounting (it could be if a specific type of RfPtr was added for gst_objects, to explicitly call gst_object_unref upon object destruction).
What happens here is that the audio-sink is neither "disposed" nor "finalized" (in the gstreamer terminology), because its gstreamer refcount stays > 0.

[David Corvoysier]

(In reply to comment #4)
> (From update of attachment 129267 [details])
> Attachment 129267 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11662224
> 
> New failing tests:
> css3/filters/effect-invert-hw.html

I don't see the relationship between the patch and this failing test: is Chrome even using gstreamer ? What am I supposed to do ?

[Philippe Normand]

(In reply to comment #7)
> (In reply to comment #4)
> > (From update of attachment 129267 [details] [details])
> > Attachment 129267 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11662224
> > 
> > New failing tests:
> > css3/filters/effect-invert-hw.html
> 
> I don't see the relationship between the patch and this failing test: is Chrome even using gstreamer ? What am I supposed to do ?

You can ignore this warning. Chromium indeed doesn't use the gstreamer backend.

(In reply to comment #6)
> (In reply to comment #5)
> > Looks like a good candidate for a RefPtr!
> 
> Not really, I think, as this is purely related to the specific gstreamer refcounting (it could be if a specific type of RfPtr was added for gst_objects, to explicitly call gst_object_unref upon object destruction).
> What happens here is that the audio-sink is neither "disposed" nor "finalized" (in the gstreamer terminology), because its gstreamer refcount stays > 0.

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 :)

[David Corvoysier]

> 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 ?

[Martin Robinson]

(In reply to comment #9)

> - The transfer to the GstRefPtr (through adoptGRef) will further increase it to n+2,

When creating a GRefPtr with adoptGRef or a RefPtr with adopRef, the reference count does not increase. Indeed this is the purpose of adoptRef. When doing a naked assignment, the reference count will increase. For more information, check out this document: http://www.webkit.org/coding/RefPtr.html

[David Corvoysier]

OK, got it: thanks for the link. I had missed the different constructor used when calling adoptGRef ("Adopting constructor").

How about that, then:

    GRefPtr<GstElement> m_webkitAudioSink;
...

    GstElement* sinkPtr = 0;

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

    m_webkitAudioSink= adoptGRef(sinkPtr);

And that's it: no need for explicit call to gst_unref in the destructor.

[Martin Robinson]

(In reply to comment #11)
> OK, got it: thanks for the link. I had missed the different constructor used when calling adoptGRef ("Adopting constructor").
> 
> How about that, then:
> 
>     GRefPtr<GstElement> m_webkitAudioSink;
> ...
> 
>     GstElement* sinkPtr = 0;
> 
>     g_object_get(m_playBin, "audio-sink", &sinkPtr, NULL);
> 
>     m_webkitAudioSink= adoptGRef(sinkPtr);
> 
> And that's it: no need for explicit call to gst_unref in the destructor.

Looks great.

[Philippe Normand]

Can you please update the patch David? It would be a good clean-up, the same approach can be used for the ::sourceChanged() method too :)

[David Corvoysier]

Yes, but since the patch against the trunk is now a bit different than the patch in my build environment, I need to set up a new desktop build environment to test against the trunk. It is taking me some time, especially with the current responsiveness of webkit.org ...

[David Corvoysier]

Created attachment 130357 [details]
New patch using GRefPtr<GstElement>

Bug fix: Used a GRefPtr to hold the reference to the audio sink instead of a GstElement*.
Code cleanup: Used the same pattern for webkit web source and removed explicit gst_unref in destructor.

Tested it on a typical video page, with GST_DEBUG=GST_REFCOUNTING:5, and verified both audio sink and webkit web source are properly released.

[WebKit Review Bot]

Attachment 130357 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 130357 [details]
New patch using GRefPtr<GstElement>

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-33
> -#include "GRefPtrGStreamer.h"

Why did you remove this include?

[Philippe Normand]

(In reply to comment #17)
> (From update of attachment 130357 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130357&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-33
> > -#include "GRefPtrGStreamer.h"
> 
> Why did you remove this include?

Ignore this please :)

[Philippe Normand]

All good, would you mind fixing that little coding style error?

[David Corvoysier]

Well, I think it is a false positive, as even the existing code triggers that error, and I don't see how I can obey both the include_order and include_system rules ...

[Philippe Normand]

Hum it seems to be an issue even without the patch. Here's a combination that makes the style checker happy here:


#if ENABLE(VIDEO) && USE(GSTREAMER)

#include "MediaPlayerPrivate.h"

#include "Timer.h"

#include <glib.h>
#include <gst/gst.h>
#include <wtf/Forward.h>

[Philippe Normand]

I mean:

#if ENABLE(VIDEO) && USE(GSTREAMER)

#include "GRefPtrGStreamer.h"
#include "MediaPlayerPrivate.h"
#include "Timer.h"
#include <glib.h>
#include <gst/gst.h>
#include <wtf/Forward.h>

[David Corvoysier]

Created attachment 130390 [details]
New patch with coding style fix

[Philippe Normand]

Comment on attachment 130390 [details]
New patch with coding style fix

Thank you David!

[WebKit Review Bot]

Comment on attachment 130390 [details]
New patch with coding style fix

Clearing flags on attachment: 130390

Committed r109933: <http://trac.webkit.org/changeset/109933>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.