[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

--- Comment #6 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
(In reply to Tim-Philipp Müller from comment #4)
> 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?

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.

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