Bug 79795

Summary: Unreleased gst_object_reference to audio sink in MediaPlayerPrivateGStreamer
Product: WebKit Reporter: David Corvoysier <david.corvoysier>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, gns, menard, mrobinson, pnormand, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
webkit.review.bot: commit-queue-
New patch using GRefPtr<GstElement>
none
New patch with coding style fix none

Description David Corvoysier 2012-02-28 08:07:51 PST
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).
Comment 1 Philippe Normand 2012-02-28 08:20:00 PST
Good catch indeed! I'll be happy to review the patch :)
Comment 2 David Corvoysier 2012-02-28 08:47:11 PST
Created attachment 129267 [details]
Proposed patch
Comment 3 Philippe Normand 2012-02-28 09:10:11 PST
Comment on attachment 129267 [details]
Proposed patch

Thanks, David! Do you need the commit-queue to land this?
Comment 4 WebKit Review Bot 2012-02-28 09:35:09 PST
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
Comment 5 Martin Robinson 2012-02-28 09:47:29 PST
Looks like a good candidate for a RefPtr!
Comment 6 David Corvoysier 2012-02-28 23:55:49 PST
(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.
Comment 7 David Corvoysier 2012-02-28 23:58:20 PST
(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 ?
Comment 8 Philippe Normand 2012-02-29 01:11:36 PST
(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 :)
Comment 9 David Corvoysier 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 ?
Comment 10 Martin Robinson 2012-02-29 08:01:22 PST
(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
Comment 11 David Corvoysier 2012-02-29 08:35:28 PST
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.
Comment 12 Martin Robinson 2012-02-29 09:11:54 PST
(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.
Comment 13 Philippe Normand 2012-03-02 00:56:02 PST
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 :)
Comment 14 David Corvoysier 2012-03-02 02:35:19 PST
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 ...
Comment 15 David Corvoysier 2012-03-06 04:49:31 PST
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.
Comment 16 WebKit Review Bot 2012-03-06 04:52:55 PST
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.
Comment 17 Philippe Normand 2012-03-06 05:00:15 PST
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?
Comment 18 Philippe Normand 2012-03-06 05:01:25 PST
(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 :)
Comment 19 Philippe Normand 2012-03-06 05:10:21 PST
All good, would you mind fixing that little coding style error?
Comment 20 David Corvoysier 2012-03-06 05:41:39 PST
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 ...
Comment 21 Philippe Normand 2012-03-06 05:47:13 PST
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>
Comment 22 Philippe Normand 2012-03-06 05:48:54 PST
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>
Comment 23 David Corvoysier 2012-03-06 08:29:35 PST
Created attachment 130390 [details]
New patch with coding style fix
Comment 24 Philippe Normand 2012-03-06 08:53:50 PST
Comment on attachment 130390 [details]
New patch with coding style fix

Thank you David!
Comment 25 WebKit Review Bot 2012-03-06 10:15:16 PST
Comment on attachment 130390 [details]
New patch with coding style fix

Clearing flags on attachment: 130390

Committed r109933: <http://trac.webkit.org/changeset/109933>
Comment 26 WebKit Review Bot 2012-03-06 10:15:22 PST
All reviewed patches have been landed.  Closing bug.