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 745287 - basesink: drain query doesn't fully work to release v4l2 buffers
basesink: drain query doesn't fully work to release v4l2 buffers
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-02-27 14:35 UTC by Thiago Sousa Santos
Modified: 2015-03-14 11:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test application (3.07 KB, text/x-csrc)
2015-02-27 14:35 UTC, Thiago Sousa Santos
  Details
gstbuffer: add gst_buffer_copy_deep (5.54 KB, patch)
2015-02-27 19:08 UTC, Thiago Sousa Santos
committed Details | Review
basesink: when draining, deep copy the last buffer to unref old memory (1.09 KB, patch)
2015-02-27 19:08 UTC, Thiago Sousa Santos
committed Details | Review

Description Thiago Sousa Santos 2015-02-27 14:35:00 UTC
Created attachment 298090 [details]
test application

When receiving a drain query basesink does a gst_buffer_copy of its last sample. By default the gst_buffer_copy copies all metadata and also 'copies' the memory. 'Copies' because it might only ref the memory.

This makes the drain query not be effective in returning all memory to the v4l2 buffer pool. We have 3 options:

1) Make v4l2 mark its memory as 'no-share'
2) Make gst_buffer_copy always do 'deep' memory copies
3) Replace basesink's gst_buffer_copy with gst_buffer_copy_into and pass the 'deep' flag to really copy the memory

Can be easily tested with the attached application
Comment 1 Thiago Sousa Santos 2015-02-27 14:36:28 UTC
I think option 1 is more appropriate here, while we allow the gst buffers with v4l2 memory to be reffed, we need a 1:1 association from v4l2 buffers to v4l2 memory to have the bufferpool and allocation working nicely.
Comment 2 Nicolas Dufresne (ndufresne) 2015-02-27 17:45:16 UTC
(In reply to Thiago Sousa Santos from comment #0)
> Created attachment 298090 [details]
> test application
> 
> When receiving a drain query basesink does a gst_buffer_copy of its last
> sample. By default the gst_buffer_copy copies all metadata and also 'copies'
> the memory. 'Copies' because it might only ref the memory.
> 
> This makes the drain query not be effective in returning all memory to the
> v4l2 buffer pool. We have 3 options:
> 
> 1) Make v4l2 mark its memory as 'no-share'
Strong NO ;-P

> 2) Make gst_buffer_copy always do 'deep' memory copies
> 3) Replace basesink's gst_buffer_copy with gst_buffer_copy_into and pass the
> 'deep' flag to really copy the memory

I'm not sure I see the difference between these two, but seem like a better approach. Though, arguably, buffers might not be mappable, in which case the copy will fail. We can make this non-fatal, or simply always drop the sample and be consistent.

(note that DRAIN query it's largely overkill for v4l2src renegotiation. We also have better argument on why dropping the last sample when proposing a new buffer pool)
Comment 3 Nicolas Dufresne (ndufresne) 2015-02-27 17:49:19 UTC
no-share cause pay-loaders to copy the encoded buffers, leading to disastrous performance. The point of tracking memories with an allocator is exactly to avoid that.
Comment 4 Thiago Sousa Santos 2015-02-27 19:08:18 UTC
Created attachment 298119 [details] [review]
gstbuffer: add gst_buffer_copy_deep

A variant of gst_buffer_copy that forces the underlying memory
to be copied.

This is added to avoid adding an extra reference to a GstMemory
that might belong to a bufferpool that is trying to be drained.
The use case is when the buffer copying is done to release the
old buffer and all its resources.
Comment 5 Thiago Sousa Santos 2015-02-27 19:08:23 UTC
Created attachment 298120 [details] [review]
basesink: when draining, deep copy the last buffer to unref old memory

Use gst_buffer_copy_deep() to force the copy of the underlying
memory instead of possibly doing a shallow copy of the buffer
and just referencing the memory
Comment 6 Thiago Sousa Santos 2015-03-14 11:14:17 UTC
commit 901fea5985b82fcdf3310fa1d34920d231113613
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Fri Mar 13 18:35:14 2015 +0000

    basesink: when draining, deep copy the last buffer to unref old memory
    
    Use gst_buffer_copy_deep() to force the copy of the underlying
    memory instead of possibly doing a shallow copy of the buffer
    and just referencing the memory
    
    https://bugzilla.gnome.org/show_bug.cgi?id=745287

commit 96eaeadc0f3cbad3cf35d26f542b8359d27e316d
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Fri Mar 13 18:35:01 2015 +0000

    gstbuffer: add gst_buffer_copy_deep
    
    A variant of gst_buffer_copy that forces the underlying memory
    to be copied.
    
    This is added to avoid adding an extra reference to a GstMemory
    that might belong to a bufferpool that is trying to be drained.
    The use case is when the buffer copying is done to release the
    old buffer and all its resources.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=745287