After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 735795 - imagefreeze: Don't call gst_caps_unref() on NULL caps
imagefreeze: Don't call gst_caps_unref() on NULL caps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal minor
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-01 08:17 UTC by Vineeth
Modified: 2014-09-01 11:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
imagefreeze: missing gst_caps_unref (1.63 KB, patch)
2014-09-01 08:21 UTC, Vineeth
needs-work Details | Review
Update the patch as per review comments (1.33 KB, patch)
2014-09-01 11:28 UTC, Vineeth
committed Details | Review

Description Vineeth 2014-09-01 08:17:39 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.
Comment 1 Vineeth 2014-09-01 08:21:40 UTC
Created attachment 284982 [details] [review]
imagefreeze: missing gst_caps_unref
Comment 2 Sebastian Dröge (slomo) 2014-09-01 09:02:46 UTC
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
Comment 3 Sebastian Dröge (slomo) 2014-09-01 09:03:12 UTC
The other parts are correct though
Comment 4 Vineeth 2014-09-01 09:14:18 UTC
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
Comment 5 Sebastian Dröge (slomo) 2014-09-01 10:58:31 UTC
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
Comment 6 Vineeth 2014-09-01 11:02:39 UTC
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.
Comment 7 Sebastian Dröge (slomo) 2014-09-01 11:20:17 UTC
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.
Comment 8 Vineeth 2014-09-01 11:28:59 UTC
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.
Comment 9 Sebastian Dröge (slomo) 2014-09-01 11:35:24 UTC
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