GNOME Bugzilla – Bug 764902
Explicitly initialize GstVideoCropMeta fields to 0 on init.
Last modified: 2016-04-13 09:59:49 UTC
Created attachment 325733 [details] [review] implicitly initialize gstvideocropmeta on init When creating a GstVideoCropMeta metadata on a GstBuffer, its fields (x, y, width, height) are not set to 0 creating the risk that e.g. xvimagesink tries to access uninitialized values which could ultimately result in a segfaults. This patch adds an explicit meta_init function for GstVideoCropMeta.
As suggested by Tim, I also add a patch which adds a setter and getter for GstVideoCropMeta.
Created attachment 325736 [details] [review] Add setter and getter for GstVideoCropMeta
Usage in python: from gi.repository import Gst, GstVideo # Add metadata to GstBuffer gstMeta = buffer.add_meta(GstVideo.VideoCropMeta.get_info(), 0) # Set metadata values to GstMeta GstVideo.buffer_set_video_crop_meta(gstMeta, 10, 20, 100, 200) # To verify whether we really wrote the metadata, we retrieve a fresh reference to the metadata on the GstBuffer gstVideoVideoCropMeta = GstVideo.buffer_get_video_crop_meta(buffer) # Should print 10 print gstVideoVideoCropMeta.x
Created attachment 325764 [details] [review] buffer: Allocate all metas initialized to all zeroes There's code out there that assumes this is already the case.
Or this, which brings behaviour in line with miniobject/gobject allocation. For the getter/setter, I think we're also not having that for all the other metas? Should we change them all while we're at it?
Review of attachment 325764 [details] [review]: ::: gst/gstbuffer.c @@ +2092,3 @@ /* create a new slice */ size = ITEM_SIZE (info); + item = g_slice_alloc0 (size); Didn't someone changed this last week for "performance" reason ?
Review of attachment 325764 [details] [review]: ::: gst/gstbuffer.c @@ +2092,3 @@ /* create a new slice */ size = ITEM_SIZE (info); + item = g_slice_alloc0 (size); Nevermind, can't find that patch anywhere, might have been something else.
Wim said we should always initialize all fields in the init function. So let's merge the original patch and before that check if all init functions are doing that.
gst_video_gl_texture_upload_meta_get_info [1] and gst_video_meta_get_info [2] have no explicit init functions yet. [1] https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/video/gstvideometa.c#n526 [2] https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/video/gstvideometa.c#n109
commit a82ef8983ea59b27fde716d72c0c747526bc4b22 Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Apr 13 10:07:33 2016 +0300 videometa: Initialize all fields of all metas with default values The metas are not allocated with all fields initialized to zeroes. https://bugzilla.gnome.org/show_bug.cgi?id=764902 commit c5ed98a35b6b7acd18a3d82775b7ee7fb70f0727 Author: Arjen Veenhuizen <arjen.veenhuizen@tno.nl> Date: Mon Apr 11 15:28:00 2016 +0000 videometa: Explicitly initialize GstVideoCropMeta on init It is not allocated with all fields initialized to 0. https://bugzilla.gnome.org/show_bug.cgi?id=764902 commit 4103f3d7f39ecfe50b547fec2b7593defeab944b Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Apr 13 09:57:16 2016 +0300 ximage: Initialize all fields in the meta explicitly The meta is not allocated with all fields initialized to zeroes. https://bugzilla.gnome.org/show_bug.cgi?id=764902
And in bad commit ccc068576a96935dddbf453820beca0d6d5112b6 Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Apr 13 10:17:24 2016 +0300 meta: Initialize all GstMeta fields During allocation they are not initialized to all zeroes. https://bugzilla.gnome.org/show_bug.cgi?id=764902
And in core commit 3f3af84a8fdb0a56612f60f3aae099715fe36d44 Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Apr 13 10:21:15 2016 +0300 meta: Warn if a meta implementation is registered without init function This previously caused uninitialized memory unless something else was initializing all the fields explicitly to something. To be on the safe side, we also allocate metas without init function to all zeroes now as it was relatively common. https://bugzilla.gnome.org/show_bug.cgi?id=764902
And more in bad commit 6c020b7f3c617814b6d36beef103492f814086b2 Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Apr 13 10:25:32 2016 +0300 meta: Initialize all remaining metas in their init function https://bugzilla.gnome.org/show_bug.cgi?id=764902
Arjen, can you create another bug for tracking the patch with the getter/setter? That's a separate problem
Sebastian: great work! I have created a separate bug for GstVideoCropMeta setter and getter here: https://bugzilla.gnome.org/show_bug.cgi?id=764987
Comment on attachment 325736 [details] [review] Add setter and getter for GstVideoCropMeta Tracked in the other bug