GNOME Bugzilla – Bug 747911
glcolorconvertelement: GstGLColorConvert is leaked
Last modified: 2015-04-17 12:01:51 UTC
.
Created attachment 301623 [details] [review] glcolorconvertelement: fix GstGLColorConvert leak convert->convert was never unreffed. https://bugzilla.gnome.org/show_bug.cgi?id=747911 Signed-off-by: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Review of attachment 301623 [details] [review]: ::: ext/gl/gstglcolorconvertelement.c @@ +110,3 @@ + if (convert->convert) + gst_object_unref (convert->convert); I think this should be done in READY->NULL, and not just in finalize. It might contain system resources. Probably the same bug in other elements
Created attachment 301648 [details] [review] glcolorconvertelement: fix GstGLColorConvert leak convert->convert was never unreffed. This can be reproduce with the validate.file.glvideomixer.simple.play_15s.synchronized scenario. https://bugzilla.gnome.org/show_bug.cgi?id=747911 Signed-off-by: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Created attachment 301797 [details] [review] gluploadelement: unref GstGLUpload when going from READY to NULL gl elements can own system ressources and so should be freed when going back to NULL. https://bugzilla.gnome.org/show_bug.cgi?id=747911 Signed-off-by: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Shouldn't the gstglcolorconvert object be freed when going from PAUSED to READY as the GstGLContext is freed during this same state change in gstglimagesink. Otherwise the patch looks good to me and fix the issues in 746251.
Let's use your patch on bug#748033 for the upload element.
Created attachment 301804 [details] [review] glcolorconvertelement: fix GstGLColorConvert leak convert->convert was never unreffed. This can be reproduce with the validate.file.glvideomixer.simple.play_15s.synchronized scenario. https://bugzilla.gnome.org/show_bug.cgi?id=747911 Signed-off-by: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Review of attachment 301804 [details] [review]: ::: ext/gl/gstglcolorconvertelement.c @@ +105,3 @@ + element_class->change_state = + GST_DEBUG_FUNCPTR (gst_gl_color_convert_element_change_state); Same comment as for the other patch, better do it in basetransform_class->stop. Less code for you to write and cleaner
Created attachment 301820 [details] [review] glcolorconvertelement: fix GstGLColorConvert leak convert->convert was never unreffed. This can be reproduce with the validate.file.glvideomixer.simple.play_15s.synchronized scenario. https://bugzilla.gnome.org/show_bug.cgi?id=747911 Signed-off-by: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Comment on attachment 301820 [details] [review] glcolorconvertelement: fix GstGLColorConvert leak commit 53ed701974a8fd2c5085135c14b81a08c6df7d43 Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Apr 17 14:03:21 2015 +0200 glcolorconvertelement: Also unref caps in ::stop() already They are not useful anymore afterwards, so keeping them until ::finalize() might only cause someone to use them later and then fail. commit 17446a420cb1ca4b2646af69c5bdc69e08796ff1 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Wed Apr 15 14:49:02 2015 +0200 glcolorconvertelement: fix GstGLColorConvert leak convert->convert was never unreffed. This can be reproduce with the validate.file.glvideomixer.simple.play_15s.synchronized scenario. https://bugzilla.gnome.org/show_bug.cgi?id=747911