GNOME Bugzilla – Bug 780767
videorate: buffers are copied in drop-only mode
Last modified: 2017-05-20 14:36:47 UTC
The 'drop-only' mode is supposed to not perform any copy (needed in zero-copy pipelines). Bug #764319 meant to avoid copying in this mode but I still get copies with this simple pipeline: "videotestsrc ! videorate drop-only=true ! fakesink". - gst_video_rate_transform_ip() is called with a buffer (refcount == 1) - it calls gst_video_rate_swap_prev(), setting videorate->prevbuf to the current buffer and so increasing its refcount to 2 - gst_video_rate_flush_prev() is called, videorate->prevbuf is reffed and then videorate->prevbuf is unset, so the refcount is still 2 - gst_buffer_make_writable() copies the buffer as the refcount is > 1
Created attachment 349226 [details] [review] videorate: factor out gst_video_rate_push_buffer() No semantic change, just factor out this function from gst_video_rate_flush_prev(). I'm about to use it to change the 'drop-only' code path.
Created attachment 349227 [details] [review] videorate: stop copying buffers in drop-only mode gst_video_rate_flush_prev() ensures that the pushed buffer is writable by calling gst_buffer_make_writable() on videorate->prevbuf. In drop-only mode we always push buffers directly when they are received from GstBaseTransform (gst_video_rate_transform_ip()) and do not keep them around. GstBaseTransform already ensures that those buffers are writable so there is no need to do it twice. This change saves us from copying buffers in drop-only mode as we no longer calls gst_buffer_make_writable() with a buffer having a refcount of 2 (one ref owned by GstBaseTransform and one in videorate->prevbuf).
I split these two patches to ease reviewing but then can probably be squashed together.
Review of attachment 349226 [details] [review]: The gst_video_rate_flush_prev () is never called in the ::: gst/videorate/gstvideorate.c @@ +719,3 @@ + outbuf = gst_buffer_make_writable (outbuf); + + return gst_video_rate_push_buffer (videorate, outbuf, duplicate, next_intime); This is not struct refactoring as you add a push here? and the
Review of attachment 349226 [details] [review]: ohh, I had misread the patch! forget what I said
Review of attachment 349227 [details] [review]: Looks good
Any reason this wasn't merged yet ?
(In reply to Nicolas Dufresne (stormer) from comment #7) > Any reason this wasn't merged yet ? AFAIK no, just need someone with git access to push it.
Attachment 349226 [details] pushed as 2c2d2a4 - videorate: factor out gst_video_rate_push_buffer() Attachment 349227 [details] pushed as 36b7e58 - videorate: stop copying buffers in drop-only mode