Created attachment 373821 [details] [review]
overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream

Created attachment 373836 [details] [review]
overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream

Created attachment 373848 [details] [review]
overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream

Comment on attachment 373848 [details] [review]
overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream

What's the reason this is not a GstVideoFilter or GstBaseTransform subclass? To avoid mapping the buffer if we don't need to blend?

I'm fine with adding it to -base directly. It's very useful and straight forward enough, but I think it needs some documentation then (what is it for? when to use it rather than cairooverlay for example?) and a minimal unit test that ideally exercises all the common code paths.

Disclaimer: I didn't look at caps nego, I'll just assume it'll work at least as well as textoverlay if it's a direct copy from there.

>+  /**
>+   * GstOverlayComposition::draw:
>+   * @overlay: Overlay element emitting the signal.
>+   * @timestamp: Timestamp (see #GstClockTime) of the current buffer.
>+   * @duration: Duration (see #GstClockTime) of the current buffer.
>+   *
>+   * This signal is emitted when the overlay should be drawn.
>+   *
>+   * Returns: #GstVideoOverlayComposition or %NULL
>+   */

Should it be timestamp or running time, or both? I wonder if we should
actually pass the buffer here too, there might be metas on the buffer that
are relevant for what should be drawn (e.g. extra time metas or other kind
of metas).


>+  overlay_composition_signals[SIGNAL_DRAW] =
>+      g_signal_new ("draw",
>+      G_TYPE_FROM_CLASS (klass),
>+      0,
>+      0,
>+      NULL,
>+      NULL,
>+      g_cclosure_marshal_generic,
>+      GST_TYPE_VIDEO_OVERLAY_COMPOSITION, 2, G_TYPE_UINT64, G_TYPE_UINT64);

We have GST_TYPE_CLOCK_TIME defines fwiw, though they map to the same of course.


>+  /**
>+   * GstOverlayComposition::caps-changed:
>+   * @overlay: Overlay element emitting the signal.
>+   * @caps: The #GstCaps of the element.
>+   * @window_width: The window render width of downstream, or 0.
>+   * @window_height: The window render height of downstream, or 0.
>+   *
>+   * This signal is emitted when the caps of the element has changed.
>+   */

I think the "window render width" might need explaining? What does it mean here?


>+static GstFlowReturn
>+gst_overlay_composition_sink_chain (GstPad * pad, GstObject * parent,
>+    GstBuffer * buffer)
>+{
>+  ...
>+
>+  if (gst_pad_check_reconfigure (self->srcpad)) {
>+    if (!gst_overlay_composition_negotiate (self, NULL)) {
>+      gst_pad_mark_reconfigure (self->srcpad);
>+      gst_buffer_unref (buffer);
>+      if (GST_PAD_IS_FLUSHING (self->srcpad))
>+        return GST_FLOW_FLUSHING;
>+      else
>+        return GST_FLOW_NOT_NEGOTIATED;
>+    }
>+  }

Not sure the GST_PAD_IS_FLUSHING is thread-safe here, since we don't hold the srcpad object lock? Maybe we want a gst_pad_is_flushing() too?


>+  g_signal_emit (self, overlay_composition_signals[SIGNAL_DRAW], 0,
>+      GST_BUFFER_PTS (buffer), GST_BUFFER_DURATION (buffer), &compo);

As mentioned above, think we should maybe just pass the entire buffer here?


>+  /* If upstream attached a meta, we can safely add our own things
>+   * in it. Upstream must've checked that downstream supports it */
>+  upstream_compo_meta = gst_buffer_get_video_overlay_composition_meta (buffer);
>+  if (upstream_compo_meta) {
>+    GstVideoOverlayComposition *merged_compo =
>+        gst_video_overlay_composition_copy (upstream_compo_meta->overlay);
>+    guint i, n;
>+
>+    GST_DEBUG_OBJECT (self->sinkpad,
>+        "Appending to upstream overlay composition");
>+
>+    n = gst_video_overlay_composition_n_rectangles (compo);
>+    for (i = 0; i < n; i++) {
>+      GstVideoOverlayRectangle *rect =
>+          gst_video_overlay_composition_get_rectangle (compo, i);
>+      gst_video_overlay_composition_add_rectangle (merged_compo, rect);
>+    }
>+
>+    gst_video_overlay_composition_unref (compo);
>+    gst_video_overlay_composition_unref (upstream_compo_meta->overlay);

Maybe we should make a convenience function for this? I think we have similar code elsewhere?

>+map_failed:
>+  {
>+    GST_ERROR_OBJECT (self->sinkpad, "Failed to map buffer");

Nitpick: we can just use 'pad' here, but it's relaly not a pad-related error is it, so maybe just use 'self' instead?

I also think we should make something like this into a public base class at some point, but that can be done independently/later.

(In reply to Tim-Philipp Müller from comment #4)
> Comment on attachment 373848 [details] [review] [review]
> overlaycomposition: New element that allows applications to draw
> GstVideoOverlayComposition on a stream
> 
> What's the reason this is not a GstVideoFilter or GstBaseTransform subclass?
> To avoid mapping the buffer if we don't need to blend?

You can't do caps negotiation and allocation query together in basetransform properly due to how the base class works. But it also prevents us from requiring a writable buffer (basetransform) or mapping it writable (videofilter).

> I'm fine with adding it to -base directly. It's very useful and straight
> forward enough, but I think it needs some documentation then (what is it
> for? when to use it rather than cairooverlay for example?) and a minimal
> unit test that ideally exercises all the common code paths.

Agreed, 

> Disclaimer: I didn't look at caps nego, I'll just assume it'll work at least
> as well as textoverlay if it's a direct copy from there.

It's a direct copy from there.

> >+  /**
> >+   * GstOverlayComposition::draw:
> >+   * @overlay: Overlay element emitting the signal.
> >+   * @timestamp: Timestamp (see #GstClockTime) of the current buffer.
> >+   * @duration: Duration (see #GstClockTime) of the current buffer.
> >+   *
> >+   * This signal is emitted when the overlay should be drawn.
> >+   *
> >+   * Returns: #GstVideoOverlayComposition or %NULL
> >+   */
> 
> Should it be timestamp or running time, or both? I wonder if we should
> actually pass the buffer here too, there might be metas on the buffer that
> are relevant for what should be drawn (e.g. extra time metas or other kind
> of metas).

Maybe we should just pass a GstSample here? Then we have the buffer and also the segment.

What do you think?

> >+  overlay_composition_signals[SIGNAL_DRAW] =
> >+      g_signal_new ("draw",
> >+      G_TYPE_FROM_CLASS (klass),
> >+      0,
> >+      0,
> >+      NULL,
> >+      NULL,
> >+      g_cclosure_marshal_generic,
> >+      GST_TYPE_VIDEO_OVERLAY_COMPOSITION, 2, G_TYPE_UINT64, G_TYPE_UINT64);
> 
> We have GST_TYPE_CLOCK_TIME defines fwiw, though they map to the same of
> course.

Will clean that up even if it doesn't make any practical difference

> >+  /**
> >+   * GstOverlayComposition::caps-changed:
> >+   * @overlay: Overlay element emitting the signal.
> >+   * @caps: The #GstCaps of the element.
> >+   * @window_width: The window render width of downstream, or 0.
> >+   * @window_height: The window render height of downstream, or 0.
> >+   *
> >+   * This signal is emitted when the caps of the element has changed.
> >+   */
> 
> I think the "window render width" might need explaining? What does it mean
> here?

If it's not 0 then this is the width/height that is used for the final rendering of the video (after rescaling and everything). E.g. the video sink's window size. I don't know why this API does not consider a pixel-aspect-ratio, but that's how it's implemented.

> >+static GstFlowReturn
> >+gst_overlay_composition_sink_chain (GstPad * pad, GstObject * parent,
> >+    GstBuffer * buffer)
> >+{
> >+  ...
> >+
> >+  if (gst_pad_check_reconfigure (self->srcpad)) {
> >+    if (!gst_overlay_composition_negotiate (self, NULL)) {
> >+      gst_pad_mark_reconfigure (self->srcpad);
> >+      gst_buffer_unref (buffer);
> >+      if (GST_PAD_IS_FLUSHING (self->srcpad))
> >+        return GST_FLOW_FLUSHING;
> >+      else
> >+        return GST_FLOW_NOT_NEGOTIATED;
> >+    }
> >+  }
> 
> Not sure the GST_PAD_IS_FLUSHING is thread-safe here, since we don't hold
> the srcpad object lock? Maybe we want a gst_pad_is_flushing() too?

If the negotiation failed due to flushing, then the relevant locks were all taken already before. But using gst_pad_is_flushing() would be cleaner, yes.

(In reply to Sebastian Dröge (slomo) from comment #6)

> 
> > I'm fine with adding it to -base directly. It's very useful and straight
> > forward enough, but I think it needs some documentation then (what is it
> > for? when to use it rather than cairooverlay for example?) and a minimal
> > unit test that ideally exercises all the common code paths.
> 
> Agreed, 

Forgot to continue writing. Agreed, I'll add those once we agree on an API and that this is a good idea in general :)

> > I wonder if we should actually pass the buffer here too, 
> > there might be metas on the buffer that are relevant for
> > what should be drawn (e.g. extra time metas or other kind
> > of metas).
> 
> Maybe we should just pass a GstSample here? Then we have the
> buffer and also the segment.

Yes, sounds like a good idea, if we can implement it in a way that reuses a GstSample in the normal case and doesn't create a GstSample for each buffer.

> > >+  if (gst_pad_check_reconfigure (self->srcpad)) {
> > >+    if (!gst_overlay_composition_negotiate (self, NULL)) {
> > >+      gst_pad_mark_reconfigure (self->srcpad);
> > >+      gst_buffer_unref (buffer);
> > >+      if (GST_PAD_IS_FLUSHING (self->srcpad))
> > >+        return GST_FLOW_FLUSHING;
> > >+      ..
> > >+    }
> > >+  }
> > 
> > Not sure the GST_PAD_IS_FLUSHING is thread-safe here, since we don't hold
> > the srcpad object lock? Maybe we want a gst_pad_is_flushing() too?
> 
> If the negotiation failed due to flushing, then the relevant locks were all
> taken already before. But using gst_pad_is_flushing() would be cleaner, yes.

Negotiation could have failed for another reason, and we could have a flush-start event being processed at the same time in another thread, no? (unlikely as it may be, but still)

Ack, going for that then :)

Created attachment 373892 [details] [review]
overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream

Review of attachment 373892 [details] [review]:

What do you think? Docs are also added with a simple example

::: gst/overlaycomposition/gstoverlaycomposition.c
@@ +24,3 @@
+ *
+ * The full example can be found in
+ * tests/examples/overlaycomposition/overlaycomposition.c

Need to write this example, and tests

Looks good.

Maybe turn the reference to the example in the docs into a link to cgit instead.

For the window size description, it would be good to provide an example of what that could be used for, e.g. "In cases where the output window resolution is higher than the resolution of the current video, for example, it could make sense to render the overlay in the output window resolution in order to avoid scaling artefacts later." or somesuch.

In the sample update part, we could probably optimise the bit where it sets the caps and/or segment so that those are only updated when they actually changed? Not sure if it's worth it, not so important I guess.

Comment on attachment 373892 [details] [review]
overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream

Also needs an entry in meson_options.txt

Created attachment 373915 [details] [review]
overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream

Created attachment 374031 [details] [review]
overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream

Created attachment 374089 [details] [review]
overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream

Attachment 374089 [details] pushed as 088b4c0 - overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream