Created attachment 363877 [details] [review]
gstbuffer: Need check GstParentBufferMeta when get buffer meta.

Comment on attachment 363877 [details] [review]
gstbuffer: Need check GstParentBufferMeta when get buffer meta.

Please remove the .swp file from the patch


Also what's the use case for this? The parent buffer meta only exists to keep the parent buffer around, not to make the child buffer "mirror" the parent buffer with regard to metas.

If that is needed, the metas would've had to be copied when creating the child buffer already.

I met the issue in gst_kms_sink_import_dmabuf(). gst_buffer_add_parent_buffer_meta() be called in the function, but not copy crop meta. gst_kms_sink_show_frame() will get crop meta from output buffer from gst_buffer_add_parent_buffer_meta(). So loss the crop meta.

Another way is gst_kms_sink_show_frame() get meta from original buffer or copy meta from original buffer to output buffer. What is your option?

It should be copied

I'd prefer we don't waste time copying to a local and meaningless wrapper, and make sure to always access the original buffer. To me, it's a bug to check meta's on the internal wrapper. If it was a filter, I would have a different talk.

Created attachment 364092 [details] [review]
Copy metas from parent buffer when importing DMA buffer.

I prefer copy those metas to avoid other mistake when need other information.

If I understand correctly, the bug is crop meta is lost when importing a dmabuf, am I right?

Review of attachment 364092 [details] [review]:

Still, it seems error prone, an example is the GstVideoMeta, which is not copied.

Created attachment 364248 [details] [review]
Get metas from original buffer.

If so, need get all metas from original buffer.

Created attachment 364544 [details] [review]
kmssink: need copy meta data from orginal buffer

Created attachment 364547 [details] [review]
kmssink: need copy meta data from orginal buffer

Review of attachment 364248 [details] [review]:

TO me this one is the right fix. Is there anything not covered ?

(In reply to Nicolas Dufresne (stormer) from comment #12)
> Review of attachment 364248 [details] [review] [review]:
> 
> TO me this one is the right fix. Is there anything not covered ?

if support overlay interface, then show_frame() call will input NULL buffer for refresh, as a result, get meta will fail.

(In reply to Haihua Hu from comment #13)
> (In reply to Nicolas Dufresne (stormer) from comment #12)
> > Review of attachment 364248 [details] [review] [review] [review]:
> > 
> > TO me this one is the right fix. Is there anything not covered ?
> 
> if support overlay interface, then show_frame() call will input NULL buffer
> for refresh, as a result, get meta will fail.

That cannot happen because

  if (buf)
    buffer = gst_kms_sink_get_input_buffer (self, buf);
  else if (self->last_buffer)
    buffer = gst_buffer_ref (self->last_buffer);

  if (!buffer)
    return GST_FLOW_ERROR;

If we don't hold on the original buffer, then we have a serious importation bug.

Created attachment 364917 [details] [review]
kmssink: Refer to the original buffer to get meta

This fixes support for the GstVideoCrop meta which was no longer
working.

Please test, I don't have anything setting crop meta on buffers at the moment.

Ok, that being said, I forgot that for the copy_to_dumb case, we don't keep a ref on the parent (because it's not needed). And for this case, the gst_video_frame_copy() won't copy over the CropMeta.

Created attachment 364935 [details] [review]
kmssink: Fix CropMeta support

We copy the meta's from the original buffer to the wrapper or copied
buffer.

This takes the originally proposed solution, but place it properly so it won't only fix it for DMABuf.

Attachment 364935 [details] pushed as 44dabe2 - kmssink: Fix CropMeta support

(In reply to Nicolas Dufresne (stormer) from comment #21)
> Attachment 364935 [details] pushed as 44dabe2 - kmssink: Fix CropMeta support

Looks good to me