GNOME Bugzilla – Bug 695655
audiovisualizer: crash while clearing video frame
Last modified: 2013-03-13 01:06:02 UTC
Created attachment 238621 [details] [review] Makes Basic Tutorial 7 from the GStreamer SDK work with 1.0 In the process of clearing the video frame, the current chain function performs an illegal memset. I found this while trying to run through the GStreamer SDK Basic Tutorials using gstreamer-1.0. Steps to reproduce: 1) Start with the code from Basic Tutorial 7 and save it as bt7.c (http://docs.gstreamer.com/display/GstSDK/Basic+tutorial+7%3A+Multithreading+and+Pad+Availability) 2) Apply the attached patch so that the code will compile/run against gstreamer-1.0 3) Run to see the crash
Created attachment 238622 [details] [review] Proposed fix
Comment on attachment 238622 [details] [review] Proposed fix Thanks for the patch! > /* gst_video_frame_clear() or is output frame already cleared */ >- memset (outframe.data, 0, scope->vinfo.size); >+ for (i = 0; i < scope->vinfo.finfo->n_planes; i++) { >+ memset (outframe.data[i], 0, scope->vinfo.size); >+ } I'm not sure if this (scope->vinfo.size) is entirely right here for more than 1 plane, though of course all visualizers in this plugin only support BGRx for now, so there's only one plane. There doesn't appear to be a convenient way to get the plane size as far as I can see (maybe frame.map[i].size?).
Created attachment 238661 [details] [review] Updated patch based on reviewer comments I think you are exactly right. Each plane's data is the result of a memory map operation, so the GstMapInfo structure at the given plane index contains the appropriate size for that plane.
Created attachment 238662 [details] [review] Fix incorrect patch file from previous submission Accidentally committed the wrong patch file in my last comment. This file contains my actual proposed fix
Thanks, pushed: commit c66fd54e78fbc777e928ad82d6cb5064676ab7ea Author: Greg Rutz <greg@gsr-tek.com> Date: Mon Mar 11 21:55:28 2013 -0600 audiovisualizer: fix improper video frame clear operation The current code is memsetting the GstVideoFrame.data address to 0s (which causes a segfault). This member is actually an array of data buffers (one for each plane). This fix iterates over each data plane to clear them all. https://bugzilla.gnome.org/show_bug.cgi?id=695655 It actually occurred to me that it's still not 100% correct, but it will work fine for all cases we care about here (in case you wonder, it's e.g. possible that we are operating on a buffer that represents part of a larger picture, so has a large stride, and we shouldn't overwrite the 'padding' data then, because that's other parts of the image not exposed here; so really we need to clear line-by-line to accommodate that. And then memsetting 0s only works for RGB, not for YUV, say. But that can all be handled in a generic _clear_frame() function some day). Committed same fix to copy in libvisual in -base as well: commit c480bac5b7c9628f97d2459bbde534abd219e5c6 Author: Greg Rutz <greg@gsr-tek.com> Date: Mon Mar 11 21:55:28 2013 -0600 libvisual: fix improper video frame clear operation The current code is memsetting the GstVideoFrame.data address to 0s (which causes a segfault). This member is actually an array of data buffers (one for each plane). This fix iterates over each data plane to clear them all. https://bugzilla.gnome.org/show_bug.cgi?id=695655