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 794773 - buffer: gst_buffer_copy_into should not copy parent buffer metas when doing a deep copy
buffer: gst_buffer_copy_into should not copy parent buffer metas when doing a...
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-03-28 16:44 UTC by Philipp Zabel
Modified: 2018-11-03 12:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
buffer: drop parent meta in deep copy (5.34 KB, patch)
2018-03-28 16:44 UTC, Philipp Zabel
needs-work Details | Review

Description Philipp Zabel 2018-03-28 16:44:47 UTC
Created attachment 370250 [details] [review]
buffer: drop parent meta in deep copy

The purpose of a deep buffer copy is to be able to release the source
buffer and all its dependencies. Attaching the parent buffer meta to
the newly created deep copy needlessly keeps holding a reference to the
parent buffer.

Without the attached patch, a v4l2h264dec ! glimagesink pipeline fails to
decode h.264 streams that change format mid-stream because the glimagesink
fails to release all DMABuf buffers to the v4l2videodec when asked to,
even though it tries by making a deep copy of its last buffer and then
releasing it:

 LOG               GST_BUFFER gstbuffer.c:799:gst_buffer_new: new 0x73b19408
 LOG               GST_BUFFER gstbuffer.c:515:gst_buffer_copy_into: copy 0x73b18d28 to 0x73b19408, offset 0-114688/114688
 LOG            glbasetexture gstglmemory.c:159:_calculate_unpack_length: Found alignment of 8 based on width (with plane width:224, plane stride:896 and pixel stride:4. RU8(224*4) = 896)
 DEBUG           glbasememory gstglbasememory.c:178:gst_gl_base_memory_init: new GL buffer memory:0x7250bc40 size:114688
 DEBUG          glbasetexture gstglmemory.c:340:gst_gl_memory_init: new GL texture context:<glcontextegl0> memory:0x7250bc40 target:2D format:6408 dimensions:224x128 stride:896 size:114688
 LOG             glbasememory gstglbasememory.c:259:_map_data_gl: mapping mem 0x7250bc40 flags 20002
 LOG             glbasememory gstglbasememory.c:259:_map_data_gl: mapping mem 0x728e3e78 flags 20001
 LOG            glbasetexture gstglmemory.c:676:gst_gl_memory_copy_teximage: copying memory 0x728e3e78, tex 2 into texture 146
 LOG             glbasememory gstglbasememory.c:338:_unmap_data_gl: unmapping mem 0x728e3e78 flags 20001
 LOG             glbasememory gstglbasememory.c:338:_unmap_data_gl: unmapping mem 0x7250bc40 flags 20002
 LOG               GST_BUFFER gstbuffer.c:414:_memory_add: buffer 0x73b19408, idx -1, mem 0x7250bc40
 DEBUG             GST_BUFFER gstbuffer.c:2201:gst_buffer_add_meta: alloc metadata 0x73b1747c (GstParentBufferMeta) of size 12
 DEBUG             GST_BUFFER gstbuffer.c:2201:gst_buffer_add_meta: alloc metadata 0x73b1b044 (GstGLSyncMeta) of size 52
 LOG               glsyncmeta gstglsyncmeta.c:300:_gst_gl_sync_meta_transform: copying sync meta 0x72506484 into 0x73b1b044
 LOG               glsyncmeta gstglsyncmeta.c:108:_default_copy: copy sync object (nil) from meta 0x72506484 to 0x73b1b044
 LOG               glsyncmeta gstglsyncmeta.c:188:_set_sync_point: setting sync point 0x72506484
 DEBUG             GST_BUFFER gstbuffer.c:2201:gst_buffer_add_meta: alloc metadata 0x73b1b084 (GstGLSyncMeta) of size 52
 LOG               glsyncmeta gstglsyncmeta.c:300:_gst_gl_sync_meta_transform: copying sync meta 0x73b16d4c into 0x73b1b084
 LOG               glsyncmeta gstglsyncmeta.c:108:_default_copy: copy sync object (nil) from meta 0x73b16d4c to 0x73b1b084
 LOG               glsyncmeta gstglsyncmeta.c:188:_set_sync_point: setting sync point 0x73b16d4c
 DEBUG             GST_BUFFER gstbuffer.c:2201:gst_buffer_add_meta: alloc metadata 0x73b0190c (GstVideoMeta) of size 76
 LOG               GST_BUFFER gstbuffer.c:710:_gst_buffer_dispose: release 0x73b18d28 to pool 0x73b230e0
 DEBUG             GST_BUFFER gstbuffer.c:2383:gst_buffer_foreach_meta: remove metadata 0x73206484 (GstParentBufferMeta)
 DEBUG             GST_BUFFER gstbuffer.c:2383:gst_buffer_foreach_meta: remove metadata 0x72506484 (GstGLSyncMeta)
 LOG               glsyncmeta gstglsyncmeta.c:316:_free_gl_sync_meta: free sync meta 0x72506484
 LOG               bufferpool gstbufferpool.c:1284:default_release_buffer:<glbufferpool0> released buffer 0x73b18d28 0

Note that the GL buffer is released, but not the parent DMABuf, which
is added to the new copy via GstParentBufferMeta. It is outstanding from
the v4l2videodec src bufferpool and keeps it from stopping properly:

 DEBUG             GST_BUFFER gstbuffer.c:1375:gst_buffer_is_memory_range_writable: idx 0, length -1
 ERROR           v4l2videodec gstv4l2videodec.c:271:gst_v4l2_video_dec_set_format:<v4l2h264dec0> Stop capture

 DEBUG                   v4l2 gstv4l2object.c:4326:gst_v4l2_object_stop:<v4l2h264dec0:src> stopping
 DEBUG                   v4l2 gstv4l2object.c:4334:gst_v4l2_object_stop:<v4l2h264dec0:src> deactivating pool
 LOG               bufferpool gstbufferpool.c:506:gst_buffer_pool_set_active:<v4l2h264dec0:pool:src> active 0
 LOG               bufferpool gstbufferpool.c:538:gst_buffer_pool_set_active:<v4l2h264dec0:pool:src> outstanding buffers 1

After this, the format change and stream restart fails in the
v4l2videodec.
Comment 1 Nicolas Dufresne (ndufresne) 2018-03-28 18:05:48 UTC
Review of attachment 370250 [details] [review]:

::: gst/gstbuffer.c
@@ -629,2 +629,4 @@
             "don't copy memory meta %p of API type %s", meta,
             g_type_name (info->api));
+      } else if (flags & GST_BUFFER_COPY_DEEP
+          && meta->info->api == GST_PARENT_BUFFER_META_API_TYPE) {

We should not check for one specific meta, but base our decision gst_meta_api_type_has_tag(... , something) like we do elsewhere. This way, if third party adds meta that also should not be copied in deep_copy, it can.
Comment 2 Philipp Zabel 2018-03-29 09:59:24 UTC
GstParentBufferMetaAPI is currently registered with empty tags. Do you have a suggestion for a tag name? "parent"?
Comment 3 Sebastian Dröge (slomo) 2018-03-29 10:04:00 UTC
I think elsewhere we use "memory" for things that are tied to a specific memory.
Comment 4 Philipp Zabel 2018-03-29 11:37:50 UTC
And "memory" tagged metas are already dropped from the deep copy in the lines above, so just tagging GstParentBufferMetaAPI as "memory" would also have the desired effect. Would that be the correct solution? The DMABuf parent buffer certainly is memory specific. Are there other users of parent buffer metas where this might not be the case?
Just grepping for gst_meta_api_type_has_tag (..., _gst_meta_tag_memory), it seems to have a few side effects, like not calling filter/transform_meta on it in the transform, encoder, and decoder elements.
Comment 5 Philipp Zabel 2018-04-04 12:35:02 UTC
I was mistaken, "memory" tagged metas are dropped if

      ((region || !(flags & GST_BUFFER_COPY_MEMORY)
              || (flags & GST_BUFFER_COPY_MERGE))
          && gst_meta_api_type_has_tag (info->api, _gst_meta_tag_memory))

but not on deep copies, so it seems we want something different here.

If I understand correctly, "memory" is for metas that describe or reference memory layout ("This metadata stays relevant as long as memory layout is unchanged"), but here we have a meta that describes a reference to a parent buffer that our buffer is derived from ("This metadata stays relevant until a deep copy is made?").

Would just adding a "parent" meta tag be appropriate?
Comment 6 GStreamer system administrator 2018-11-03 12:45:33 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/283.