GNOME Bugzilla – Bug 739754
v4l2bufferpool: Should validate that all memories are writeable before queueing back
Last modified: 2014-11-07 15:46:39 UTC
Currently, if a buffer memories get copied, and original buffer unreffed, the pool will accept the released buffer memory as valid and queue it back. This causes tearing in pipeline that uses gst_buffer_make_writable(): gst-launch-1.0 v4l2src ! "video/x-raw,framerate=30/1" ! intervideosink intervideosrc ! xvimagesink More generally, the gst_v4l2_is_buffer_valid() is too light and should check that all memories are of right type, same group, and writeable. Failing validation will simply cause the buffer to be discarded, and eventually the memory will be released and the Allocator will notify the pool. The pool will finally create a new GstBuffer to wrap the lost group. Considering there is no large allocation involved, keeping the original GstBuffer and fixing it would not give much gain. Fix coming soon.
Created attachment 290130 [details] [review] [PATCH] v4l2bufferpool: Improve buffer validation Improve buffer validation by making sure each memory are the right one and that each memory is writable. This fixes tearing issues in case downstream uses gst_buffer_make_writable() or other type of GstBuffer copy where memory are only reffed. https://bugzilla.gnome.org/show_bug.cgi?id=73975 --- sys/v4l2/gstv4l2bufferpool.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
Note that it reveals contention when running the test pipeline proposed here. This is due to sub-optimal amount of buffers being allocated. The issues are because the video sink don't account for their last-sample, something that could be fixed in base class most likely. intervideosink internal also need an extra buffer, though it does not really need 2 extra, as both last sample and the internal buffer have same life time. These should be filed and fixed in separate bugs imho.
Review of attachment 290130 [details] [review]: Recently, I wondered how it's work if some element reference v4l2 memory and I didn't understand. Now I know why :) Otherwise, patch looks good to me, but maybe, someone else shall approve.
Thanks for having a look, I've been doing more tests and I do think it's safe merging.
Comment on attachment 290130 [details] [review] [PATCH] v4l2bufferpool: Improve buffer validation commit 3282df51a4c942e806e424a6fb9660387befee21 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Thu Nov 6 21:21:40 2014 -0500 v4l2bufferpool: Improve buffer validation Improve buffer validation by making sure each memory are the right one and that each memory is writable. This fixes tearing issues in case downstream uses gst_buffer_make_writable() or other type of GstBuffer copy where memory are only reffed. https://bugzilla.gnome.org/show_bug.cgi?id=739754
And for 1.4.5 commit 4588123170148f17477e1e3475c1304e32f2ffae Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Thu Nov 6 21:21:40 2014 -0500 v4l2bufferpool: Improve buffer validation