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 764319 - videorate : avoid useless buffer copy un drop-only mode
videorate : avoid useless buffer copy un drop-only mode
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other All
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-03-29 16:17 UTC by Frédéric Bertolus
Modified: 2016-04-03 08:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (1.37 KB, patch)
2016-03-29 16:17 UTC, Frédéric Bertolus
committed Details | Review

Description Frédéric Bertolus 2016-03-29 16:17:08 UTC
Created attachment 324956 [details] [review]
Proposed patch

The videorate element has a reference to the last buffer in videorate->prevbuf.
In gst_video_rate_flush_prev, before push the buffer, videorate has to change the timestamp of the buffer so it make it writable with gst_buffer_make_writable. Before that, videorate take another reference on the buffer because one of these ref will be transfered at the push pad and the other one is for the videorate->prevbuf.

So when gst_buffer_make_writable is call, the ref count of the buffer is at least 2 and the buffer will be copied. If the memory is not sharable, there will be a deep copy.

This is the expected behavior but it's not optimal in the drop-only mode.
In drop only mode, videorate will never duplicate a buffer so it don't have to keep a reference on the previous buffer. In fact, video rate don't keep the previous buffer : just after calling gst_video_rate_flush_prev, it release its ref to the buffer :

/* No need to keep the buffer around for longer */
gst_buffer_replace (&videorate->prevbuf, NULL);

However this is done after taking the ref for the push pad and after the gst_buffer_make_writable, so the copy is already done, and the videorate never use this copy.

The proposed patch release the useless reference to prevbuf before make the buffer writable, in the case we are in drop-only mode.
Comment 1 Sebastian Dröge (slomo) 2016-04-03 08:41:30 UTC
commit 6788003912f5e261530e854e64ebbfe2f9591207
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sun Apr 3 11:40:50 2016 +0300

    videorate: Don't fill up the segment with duplicate buffers if drop_only==TRUE

commit eda44c640e3257fb36a9ed2e48a4b2ad0411f6ec
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sun Apr 3 11:38:28 2016 +0300

    videorate: Remove dead code
    
    We never get into this code path at all if drop_only==TRUE.

commit 2626c02149ab7947cec9890215a76bddb643554c
Author: Frédéric Bertolus <frederic.bertolus@parrot.com>
Date:   Tue Mar 29 17:19:41 2016 +0200

    videorate: avoid useless buffer copy in drop-only mode
    
    Make writable the buffer before pushing it lead to a buffer copy. It's
    because a reference is keep for the previous buffer.
    The previous buffer reference is only need to duplicate the buffer. In
    drop-only mode, the previous buffer is release just after pushing the
    buffer so a copy is done but it's useless.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=764319