[Bug 797234] overlaycomposition: New element that allows applications to draw GstVideoOverlayComposition on a stream

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797234

Tim-Philipp Müller <t.i.m at zen.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #373848|none                        |reviewed
             status|                            |

--- Comment #4 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
Comment on attachment 373848
  --> https://bugzilla.gnome.org/attachment.cgi?id=373848
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?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.