Created attachment 337553 [details] [review]
Need unref composition when remove video overlay composition meta

Created attachment 337555 [details] [review]
Need unref composition when remove video overlay composition meta

Review of attachment 337555 [details] [review]:

Interesing, that means when we add a video meta, unless we cache the composition, we need to drop the extra ref. Though, this new public function is not useful, since this should already be cleared by gst_buffer_remove_meta(). The remove meta is completely generic, if not, fix it ;-P.

> Interesting, that means when we add a video meta, unless we
> cache the composition, we need to drop the extra ref.

In the usual case you attach the same composition to many video buffers, so that seems to make sense, no?

Kevin, please provide a minimal test case that leaks, or let us know if gst_buffer_remove_meta() is enough for your purposes.

(In reply to Tim-Philipp Müller from comment #4)
> > Interesting, that means when we add a video meta, unless we
> > cache the composition, we need to drop the extra ref.
> 
> In the usual case you attach the same composition to many video buffers, so
> that seems to make sense, no?

I don't remember exactly, I thought we could re-use the GstMeta, which makes caching of the composition less useful. I need to re-read some code, I didn't notice, and may have introduced leaks all over there.

(In reply to Tim-Philipp Müller from comment #4)
> > Interesting, that means when we add a video meta, unless we
> > cache the composition, we need to drop the extra ref.
> 
> In the usual case you attach the same composition to many video buffers, so
> that seems to make sense, no?
> 
> Kevin, please provide a minimal test case that leaks, or let us know if
> gst_buffer_remove_meta() is enough for your purposes.

Yes, our case is copy gstbuffer and attach the composition to new gstbuffer. it will cause leaks evenif gst_buffer_remove_meta().

Below is our code.

  while ((meta = gst_buffer_iterate_meta (src, &state))) {
    if (meta->info->api == GST_VIDEO_OVERLAY_COMPOSITION_META_API_TYPE) {
      compmeta = (GstVideoOverlayCompositionMeta*)meta;
      if (GST_IS_VIDEO_OVERLAY_COMPOSITION (compmeta->overlay)) {
        comp_copy = gst_video_overlay_composition_copy(compmeta->overlay);
        if (comp_copy) {
          gst_buffer_add_video_overlay_composition_meta(dst, comp_copy);
        }
      }
    }
  }

gst_video_overlay_composition_copy() will new comp_copy and gst_buffer_add_video_overlay_composition_meta() will add one ref. Do you have any suggestion.

(In reply to kevin from comment #7)
> Below is our code.
> 
>   while ((meta = gst_buffer_iterate_meta (src, &state))) {
>     if (meta->info->api == GST_VIDEO_OVERLAY_COMPOSITION_META_API_TYPE) {
>       compmeta = (GstVideoOverlayCompositionMeta*)meta;
>       if (GST_IS_VIDEO_OVERLAY_COMPOSITION (compmeta->overlay)) {
>         comp_copy = gst_video_overlay_composition_copy(compmeta->overlay);
>         if (comp_copy) {
>           gst_buffer_add_video_overlay_composition_meta(dst, comp_copy);
>         }
>       }
>     }
>   }
> 
> gst_video_overlay_composition_copy() will new comp_copy and
> gst_buffer_add_video_overlay_composition_meta() will add one ref. Do you
> have any suggestion.

According to the API, you must unref if you don't want to keep the composition after gst_buffer_add_video_overlay_composition_meta(). So basically, it is your fault if it leaks. The API makes sense, since the the GstVideoOverlayComposition meta is a copy on write object. That means if it's reference count is bigger then 1, it will be considered immutable and gst_video_overlay_composition_make_writable() will copy. The appropriate usage here is the following:

> compmeta = gst_buffer_get_video_overlay_composition_meta(src);
> if (compmeta)
>   gst_buffer_add_video_overlay_composition_meta (dst, compmeta->overlay);

Is there function to iterate composition? I remember video add timeoverlay and clockoverlay have two compositions. Is it right?

gst_buffer_foreach_meta()