GNOME Bugzilla – Bug 735795
imagefreeze: Don't call gst_caps_unref() on NULL caps
Last modified: 2014-09-01 11:35:54 UTC
On reviewing imagefreeze element, found some possible inconsistencies i have made three changes in this patch, 1) gst_caps_unref (filter) is not being called when if (gst_pad_has_current_caps (pad)) satisfies in gst_image_freeze_sink_getcaps() 2) replacing gst_caps_make_writable (gst_caps_ref (caps)) with gst_caps_copy (caps) in line 177, since the functionality is same. 3) in function, gst_image_freeze_sink_getcaps(), when the below else case is executed, ... } else { GST_LOG_OBJECT (self, "going to copy"); ret = gst_caps_copy (templ); } ... templ will already be unreferenced by gst_caps_copy, hence adding a check as below if (templ) gst_caps_unref (templ); Please review and check if the changes are fine.
Created attachment 284982 [details] [review] imagefreeze: missing gst_caps_unref
Review of attachment 284982 [details] [review]: ::: gst/imagefreeze/gstimagefreeze.c @@ +270,3 @@ ret = gst_pad_get_current_caps (pad); + if (filter) + gst_caps_unref (filter); This is wrong, we don't own a reference to filter at this point. Only after the gst_caps_copy() call. Here it's still only owned by the caller
The other parts are correct though
Hi Sebastian, Thanks for the review. The reason i added the first case was, when the function was being called, GstCaps *caps; gst_query_parse_caps (query, &caps); caps = gst_image_freeze_sink_getcaps (self, caps); gst_query_set_caps_result (query, caps); gst_caps_unref (caps); i am guessing the function, gst_image_freeze_sink_getcaps() was written such that, the caps argument being passed is supposed to be unref and a new ref needs to be created for the result caps, similar to how gst_caps_copy() functions. this can be confirmed from the below call in gst_image_freeze_sink_getcaps() if (filter) { filter = gst_caps_copy (filter); gst_image_freeze_remove_fps (self, filter); } where using the call, gst_caps_copy (filter), we are unref'ing the filter owned by caller and creating a new reference to be used. Can you confirm once if my understanding is correct. Regards, Vineeth
It's not, no. The caps returned by gst_query_parse_caps() are not owned by us, we only get the reference that is owned by the query, so must not unref them at any point
In that case, we should replace if (filter) { filter = gst_caps_copy (filter); gst_image_freeze_remove_fps (self, filter); } with if (filter) { filter = gst_caps_ref (filter); gst_image_freeze_remove_fps (self, filter); } ?? since as per my understanding gst_caps_copy will unreference the argument being passed to it.
No, you mix gst_caps_copy() with gst_caps_make_writable(). gst_caps_copy() just creates a copy, gst_caps_make_writable() either returns the parameter directly if it's writable already or calls copy & unref.
Created attachment 284992 [details] [review] Update the patch as per review comments hmmm. I have updated the patch by removing 1st case.. Actually my confusion arises from the below definition given in reference manual. It states, gst_caps_copy() is equivalent to gst_caps_make_writable(gst_caps_ref()), so i thought the behavior of gst_caps_copy() will be similar to gst_caps_make_writable() gst_caps_copy () GstCaps * gst_caps_copy (const GstCaps *caps); Creates a new GstCaps as a copy of the old caps. The new caps will have a refcount of 1, owned by the caller. The structures are copied as well. Note that this function is the semantic equivalent of a gst_caps_ref() followed by a gst_caps_make_writable(). If you only want to hold on to a reference to the data, you should use gst_caps_ref(). When you are finished with the caps, call gst_caps_unref() on it.
commit 3a1e01022151c3262140d5d864a71a1bab2938c7 Author: Vineeth T M <vineeth.tm@samsung.com> Date: Mon Sep 1 16:39:23 2014 +0530 imagefreeze: Don't call gst_caps_unref() on template caps when already unreferenced Adding an extra condition while calling gst_caps_unref (templ) and replacing gst_caps_make_writable (gst_caps_ref (caps)) with gst_caps_copy (caps) in line 177, since the functionality is same. https://bugzilla.gnome.org/show_bug.cgi?id=735795