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 739754 - v4l2bufferpool: Should validate that all memories are writeable before queueing back
v4l2bufferpool: Should validate that all memories are writeable before queuei...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.4.3
Other Linux
: Normal normal
: 1.4.5
Assigned To: Nicolas Dufresne (ndufresne)
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-11-07 02:05 UTC by Nicolas Dufresne (ndufresne)
Modified: 2014-11-07 15:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] v4l2bufferpool: Improve buffer validation (1.81 KB, patch)
2014-11-07 02:22 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2014-11-07 02:05:00 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.
Comment 1 Nicolas Dufresne (ndufresne) 2014-11-07 02:22:14 UTC
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(-)
Comment 2 Nicolas Dufresne (ndufresne) 2014-11-07 02:28:58 UTC
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.
Comment 3 Aurélien Zanelli 2014-11-07 09:00:44 UTC
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.
Comment 4 Nicolas Dufresne (ndufresne) 2014-11-07 15:42:25 UTC
Thanks for having a look, I've been doing more tests and I do think it's safe merging.
Comment 5 Nicolas Dufresne (ndufresne) 2014-11-07 15:45:39 UTC
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
Comment 6 Nicolas Dufresne (ndufresne) 2014-11-07 15:46:39 UTC
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