Summary: | Unreleased gst_object_reference to audio sink in MediaPlayerPrivateGStreamer | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Corvoysier <david.corvoysier> | ||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, gustavo, menard, mrobinson, pnormand, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
David Corvoysier
2012-02-28 08:07:51 PST
Good catch indeed! I'll be happy to review the patch :) Created attachment 129267 [details]
Proposed patch
Comment on attachment 129267 [details]
Proposed patch
Thanks, David! Do you need the commit-queue to land this?
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 Looks like a good candidate for a RefPtr! (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. (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 ? (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 :) > 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 ?
(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 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. (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. 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 :) 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 ... 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.
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 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? (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 :) All good, would you mind fixing that little coding style error? 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 ... 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> 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> Created attachment 130390 [details]
New patch with coding style fix
Comment on attachment 130390 [details]
New patch with coding style fix
Thank you David!
Comment on attachment 130390 [details] New patch with coding style fix Clearing flags on attachment: 130390 Committed r109933: <http://trac.webkit.org/changeset/109933> All reviewed patches have been landed. Closing bug. |