| Summary: | [GTK] Leak: GstElement* leaking in MediaPlayerPrivateGStreamer::createAudioSink() | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brian Holt <brian.holt> | ||||
| Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED INVALID | ||||||
| Severity: | Normal | CC: | cdumez, cgarcia, commit-queue, eric.carlson, glenn, gustavo, jer.noble, kbalazs, menard, mrobinson, pnormand | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 116317 | ||||||
| Attachments: |
|
||||||
Created attachment 206249 [details]
Patch
I don't know much about gst, but I thought elements had a floating reference that is consumed when added to a container. (In reply to comment #2) > I don't know much about gst, but I thought elements had a floating reference that is consumed when added to a container. Indeed, and the proof of that is that you don't actually "adopt". So which one(s) were actually leaking? Comment on attachment 206249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206249&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578 > + GRefPtr<GstElement> audioSink = adoptGRef(gst_bin_new("audio-sink")); The doc says that gst_bin_new() returns a floating reference: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-new How come adopting doesn't assert here? template <> GRefPtr<GstElement> adoptGRef(GstElement* ptr) { ASSERT(!ptr || !gstObjectIsFloating(GST_OBJECT(ptr))); return GRefPtr<GstElement>(ptr, GRefPtrAdopt); } Comment on attachment 206249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206249&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578 >> + GRefPtr<GstElement> audioSink = adoptGRef(gst_bin_new("audio-sink")); > > The doc says that gst_bin_new() returns a floating reference: > http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-new > > How come adopting doesn't assert here? > template <> GRefPtr<GstElement> adoptGRef(GstElement* ptr) > { > ASSERT(!ptr || !gstObjectIsFloating(GST_OBJECT(ptr))); > return GRefPtr<GstElement>(ptr, GRefPtrAdopt); > } Thanks very much for the link - I should have checked first whether gst_bin_new returns a floating reference. Regarding the ASSERT, I'm working with a release build so it's ifdef'ed out. Comment on attachment 206249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206249&action=review >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578 >>> + GRefPtr<GstElement> audioSink = adoptGRef(gst_bin_new("audio-sink")); >> >> The doc says that gst_bin_new() returns a floating reference: >> http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-new >> >> How come adopting doesn't assert here? >> template <> GRefPtr<GstElement> adoptGRef(GstElement* ptr) >> { >> ASSERT(!ptr || !gstObjectIsFloating(GST_OBJECT(ptr))); >> return GRefPtr<GstElement>(ptr, GRefPtrAdopt); >> } > > Thanks very much for the link - I should have checked first whether gst_bin_new returns a floating reference. > > Regarding the ASSERT, I'm working with a release build so it's ifdef'ed out. Sometimes to streamer doc is wrong about these things so it should be double checked. Also note that the behaviour is sometimes different between streamer 0.10 and gstreamer 1.0. It is important that you test with a debug build. (In reply to comment #3) > (In reply to comment #2) > > I don't know much about gst, but I thought elements had a floating reference that is consumed when added to a container. > > Indeed, and the proof of that is that you don't actually "adopt". So which one(s) were actually leaking? I think all of them were leaking: gst_element_factory_make () returns a new GstElement or NULL if unable to create element. [transfer floating] (In reply to comment #6) > (From update of attachment 206249 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=206249&action=review > > >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578 > >>> + GRefPtr<GstElement> audioSink = adoptGRef(gst_bin_new("audio-sink")); > >> > >> The doc says that gst_bin_new() returns a floating reference: > >> http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-new > >> > >> How come adopting doesn't assert here? > >> template <> GRefPtr<GstElement> adoptGRef(GstElement* ptr) > >> { > >> ASSERT(!ptr || !gstObjectIsFloating(GST_OBJECT(ptr))); > >> return GRefPtr<GstElement>(ptr, GRefPtrAdopt); > >> } > > > > Thanks very much for the link - I should have checked first whether gst_bin_new returns a floating reference. > > > > Regarding the ASSERT, I'm working with a release build so it's ifdef'ed out. > > Sometimes to streamer doc is wrong about these things so it should be double checked. Also note that the behaviour is sometimes different between streamer 0.10 and gstreamer 1.0. > It is important that you test with a debug build. I definitely will in the future. Thanks for the helpful comments. (In reply to comment #7) > (In reply to comment #3) > > (In reply to comment #2) > > > I don't know much about gst, but I thought elements had a floating reference that is consumed when added to a container. > > > > Indeed, and the proof of that is that you don't actually "adopt". So which one(s) were actually leaking? > > I think all of them were leaking: gst_element_factory_make () returns a new GstElement or NULL if unable to create element. [transfer floating] gst_element_factory() returns a floating reference and get_bin_add_many() should take ownership of the returned values later on: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-add Therefore, I don't understand why it leaks. (In reply to comment #7) > (In reply to comment #3) > > (In reply to comment #2) > > > I don't know much about gst, but I thought elements had a floating reference that is consumed when added to a container. > > > > Indeed, and the proof of that is that you don't actually "adopt". So which one(s) were actually leaking? > > I think all of them were leaking: gst_element_factory_make () returns a new GstElement or NULL if unable to create element. [transfer floating] a GstElement is a GInitiallyUnowned, so it's returning a new element with a floating reference. You only need to convert it into a normal ref (which is what GRefPtr is does) if it's not added to a container that consumes the floating ref. Some more detail about the origins of the leaks from the debug build:
Leak_StillReachable
40 bytes in 1 blocks are still reachable in loss record 335 of 1,176
malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
...
gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446)
WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1564)
GstElement* scale = gst_element_factory_make("scaletempo", 0);
Leak_StillReachable
554 bytes in 41 blocks are still reachable in loss record 855 of 1,176
malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
...
gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446)
WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1570)
GstElement* convert = gst_element_factory_make("audioconvert", 0);
Leak_StillReachable
576 bytes in 4 blocks are still reachable in loss record 836 of 1,139
realloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
...
gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446)
WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1571)
GstElement* resample = gst_element_factory_make("audioresample", 0);
Leak_StillReachable
304 bytes in 10 blocks are still reachable in loss record 750 of 1,176
malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
...
gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446)
WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1572)
GstElement* sink = gst_element_factory_make("autoaudiosink", 0);
Leak_StillReachable
120 bytes in 3 blocks are still reachable in loss record 525 of 1,139
malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
...
gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446)
WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578)
GstElement* audioSink = gst_bin_new("audio-sink");
Additionally there are leaks at
Leak_StillReachable
3,928 bytes in 49 blocks are still reachable in loss record 1,016 of 1,138
calloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
...
gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446)
WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1599)
m_playBin = gst_element_factory_make(gPlaybinName, "play");
Leak_StillReachable
3,104 bytes in 28 blocks are still reachable in loss record 1,008 of 1,139
calloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
...
gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446)
WebCore::MediaPlayerPrivateGStreamerBase::createVideoSink(_GstElement*) (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:563)
m_fpsSink = gst_element_factory_make("fpsdisplaysink", "sink");
(In reply to comment #11) > Some more detail about the origins of the leaks from the debug build: > > Leak_StillReachable > 40 bytes in 1 blocks are still reachable in loss record 335 of 1,176 > malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ... > gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) > WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1564) > GstElement* scale = gst_element_factory_make("scaletempo", 0); > still-reachebles are not leaks, at least in WebKit sense. It's normal that there are unfreed heap objects at termination because WebKit is intentionally does not care about that (the idea is to terminate faster, the OS frees up the memory anyway). It means that for example singletons are usually created on the heap and never freed. We should be sure that the objects holding these pointers are actually destroyed. Only in that case can we say that these are real leaks. Given that in this case valgrind would rather report definitely-lost failures, I assume these are not leaks. (In reply to comment #12) > (In reply to comment #11) > > Some more detail about the origins of the leaks from the debug build: > > > > Leak_StillReachable > > 40 bytes in 1 blocks are still reachable in loss record 335 of 1,176 > > malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > > ... > > gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) > > WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1564) > > GstElement* scale = gst_element_factory_make("scaletempo", 0); > > > > still-reachebles are not leaks, at least in WebKit sense. > It's normal that there are unfreed heap objects at termination because WebKit is intentionally does not care about that (the idea is to terminate faster, the OS frees up the memory anyway). It means that for example singletons are usually created on the heap and never freed. > We should be sure that the objects holding these pointers are actually destroyed. Only in that case can we say that these are real leaks. Given that in this case valgrind would rather report definitely-lost failures, I assume these are not leaks. That's a fair point. I will restrict my efforts to Definitely-Lost in the meantime. Should I mark this as wontfix? (In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > Some more detail about the origins of the leaks from the debug build: > > > > > > Leak_StillReachable > > > 40 bytes in 1 blocks are still reachable in loss record 335 of 1,176 > > > malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > > > ... > > > gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) > > > WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1564) > > > GstElement* scale = gst_element_factory_make("scaletempo", 0); > > > > > > > still-reachebles are not leaks, at least in WebKit sense. > > It's normal that there are unfreed heap objects at termination because WebKit is intentionally does not care about that (the idea is to terminate faster, the OS frees up the memory anyway). It means that for example singletons are usually created on the heap and never freed. > > We should be sure that the objects holding these pointers are actually destroyed. Only in that case can we say that these are real leaks. Given that in this case valgrind would rather report definitely-lost failures, I assume these are not leaks. > > That's a fair point. I will restrict my efforts to Definitely-Lost in the meantime. > > Should I mark this as wontfix? I would rather mark it invalid. |
In Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp: Leaks found using the "--leak" option in the Gtk port: Command: /home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/Programs/DumpRenderTree - Leak_StillReachable 3,328 bytes in 13 blocks are still reachable in loss record 939 of 1,061 realloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) g_realloc (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/gmem.c:224) g_array_maybe_expand (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/garray.c:785) g_array_append_vals (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/garray.c:423) gst_structure_id_set_value (/WebKitBuild/Dependencies/Source/gstreamer/gst/gststructure.c:476) gst_element_class_set_static_metadata (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelement.c:1345) gst_audio_convert_class_intern_init (/WebKitBuild/Dependencies/Source/gst-plugins-base/gst/audioconvert/gstaudioconvert.c:204) g_type_class_ref (/WebKitBuild/Dependencies/Source/glib-2.36.0/gobject/gtype.c:2239) g_object_newv (/WebKitBuild/Dependencies/Source/glib-2.36.0/gobject/gobject.c:1624) gst_element_factory_create (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:377) gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin() (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::MediaPlayerPrivateGStreamer::load(WTF::String const&) [clone .part.24] (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::MediaPlayer::loadWithNextMediaEngine(WebCore::MediaPlayerFactory*) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::MediaPlayer::load(WebCore::KURL const&, WebCore::ContentType const&, WTF::String const&) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::HTMLMediaElement::loadResource(WebCore::KURL const&, WebCore::ContentType&, WTF::String const&) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::HTMLMediaElement::selectMediaResource() (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::HTMLMediaElement::loadTimerFired(WebCore::Timer<WebCore::HTMLMediaElement>*) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::ThreadTimers::sharedTimerFiredInternal() (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::timeout_cb(void*) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) g_timeout_dispatch (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/gmain.c:4413) g_main_context_dispatch (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/gmain.c:3054) g_main_context_iterate.isra.22 (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/gmain.c:3701) g_main_loop_run (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/gmain.c:3895) gtk_main (/WebKitBuild/Dependencies/Source/gtk+-3.6.0/gtk/gtkmain.c:1163) runTest(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/Programs/DumpRenderTree) runTestingServerLoop() (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/Programs/DumpRenderTree) Suppression (error hash=#FDF24213CEA9E828#): For more info on using suppressions see http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/memory-sheriff#TOC-Suppressing-memory-reports { <insert_a_suppression_name_here> Memcheck:Leak fun:realloc fun:g_realloc fun:g_array_maybe_expand fun:g_array_append_vals fun:gst_structure_id_set_value fun:gst_element_class_set_static_metadata fun:gst_audio_convert_class_intern_init fun:g_type_class_ref fun:g_object_newv fun:gst_element_factory_create fun:gst_element_factory_make fun:_ZN7WebCore27MediaPlayerPrivateGStreamer15createAudioSinkEv fun:_ZN7WebCore27MediaPlayerPrivateGStreamer16createGSTPlayBinEv fun:_ZN7WebCore27MediaPlayerPrivateGStreamer4loadERKN3WTF6StringE.part.24 fun:_ZN7WebCore11MediaPlayer23loadWithNextMediaEngineEPNS_18MediaPlayerFactoryE fun:_ZN7WebCore11MediaPlayer4loadERKNS_4KURLERKNS_11ContentTypeERKN3WTF6StringE fun:_ZN7WebCore16HTMLMediaElement12loadResourceERKNS_4KURLERNS_11ContentTypeERKN3WTF6StringE fun:_ZN7WebCore16HTMLMediaElement19selectMediaResourceEv fun:_ZN7WebCore16HTMLMediaElement14loadTimerFiredEPNS_5TimerIS0_EE fun:_ZN7WebCore12ThreadTimers24sharedTimerFiredInternalEv fun:_ZN7WebCoreL10timeout_cbEPv fun:g_timeout_dispatch }