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 778453 - v4l2videodec: Makes singleton caps ref increase on every access
v4l2videodec: Makes singleton caps ref increase on every access
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal normal
: 1.10.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-02-10 13:48 UTC by Juan Pablo Ugarte
Modified: 2017-02-22 09:18 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
Proposed patch (1.54 KB, patch)
2017-02-10 13:48 UTC, Juan Pablo Ugarte 		none Details | Review
Proposed patch v2 (2.02 KB, patch)
2017-02-10 21:03 UTC, Juan Pablo Ugarte 		none Details | Review
Proposed patch v3 (2.16 KB, patch)
2017-02-10 21:20 UTC, Juan Pablo Ugarte 		committed Details | Review

Description Juan Pablo Ugarte 2017-02-10 13:48:05 UTC 
Created attachment 345458 [details] [review]
Proposed patch

gst_v4l2_video_dec_open() is leaking the filter caps passed to gst_v4l2_object_get_caps()
Comment 1 Nicolas Dufresne (ndufresne) 2017-02-10 14:31:56 UTC 
Review of attachment 345458 [details] [review]:

The comment is miss-leading, since it does not fix a leak, it just prevent the refcount from increasing. I think instead you should drop  gst_caps_ref (caps); in the return statement of those getters. As you should have seen in gstv4l2object.c, those are leaked on purpose to avoid the overhead of re-building the base template caps. Note that I notice those function are not thread safe, might be worth fixing.

On other notes use style "v4l2videodec:" for comment prefix. You should use valgrind or internal leak tracer to validate your patches.
Comment 2 Juan Pablo Ugarte 2017-02-10 21:03:57 UTC 
Created attachment 345484 [details] [review]
Proposed patch v2

I removed the gst_caps_ref() and made sure every call to the api did not unref it.
Made API thread safe by using g_once_init[enter|leave]
Used GST_MINI_OBJECT_FLAG_MAY_BE_LEAKED so that it does not show up in the tracer!
Comment 3 Nicolas Dufresne (ndufresne) 2017-02-10 21:10:35 UTC 
Review of attachment 345484 [details] [review]:

Looks good, now please fix the commit comment. Format shall be

v4l2object: short desc (max 80chars)
<empty line>
Long describption
<empty line>
https://bugzilla.gnome.org/show_bug.cgi?id=778453
Comment 4 Juan Pablo Ugarte 2017-02-10 21:20:33 UTC 
Created attachment 345489 [details] [review]
Proposed patch v3

Updated comments, sorry about that!
Comment 5 Nicolas Dufresne (ndufresne) 2017-02-10 21:26:41 UTC 
Review of attachment 345489 [details] [review]:

Thanks.
Comment 6 Nicolas Dufresne (ndufresne) 2017-02-10 21:46:57 UTC 
Committed as b6723ecd3cea0f6e819e8671582b3638698a3f16
Comment 7 Nicolas Dufresne (ndufresne) 2017-02-22 09:15:39 UTC 
In 1.10 as 3c911b5a6ba8d3e69240ec72a892afc36f40e923
Comment 8 Nicolas Dufresne (ndufresne) 2017-02-22 09:18:18 UTC 
Wrong ref, 1.10 as 7f554fcf739cea4e70a16f57d558a590200bea66