GNOME Bugzilla – Bug 745287
basesink: drain query doesn't fully work to release v4l2 buffers
Last modified: 2015-03-14 11:14:54 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
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.
(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)
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.
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.
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
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