GNOME Bugzilla – Bug 794773
buffer: gst_buffer_copy_into should not copy parent buffer metas when doing a deep copy
Last modified: 2018-11-03 12:45:33 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.
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.
GstParentBufferMetaAPI is currently registered with empty tags. Do you have a suggestion for a tag name? "parent"?
I think elsewhere we use "memory" for things that are tied to a specific memory.
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.
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?
-- 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.