After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 595520 - Implement a generic cairo overlay
Implement a generic cairo overlay
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal enhancement
: 0.10.29
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 617905
 
 
Reported: 2009-09-17 23:35 UTC by Jon Nordby
Modified: 2011-11-23 15:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Initial patch (11.28 KB, patch)
2011-01-28 23:58 UTC, Jon Nordby
needs-work Details | Review
Example application showing the element in use (3.61 KB, text/x-csrc)
2011-01-29 00:00 UTC, Jon Nordby
  Details
Patch v2 (24.10 KB, patch)
2011-02-27 16:36 UTC, Jon Nordby
needs-work Details | Review
Patch v3 (24.05 KB, patch)
2011-03-01 15:39 UTC, Jon Nordby
committed Details | Review

Description Jon Nordby 2009-09-17 23:35:34 UTC
Provide a plug-in that allows one to draw arbitrary things as an overlay using Cairo.

A proposed way of doing this is to pull out everything text render specific in the existing cairotextoverlay plugin, and instead provide a callback that gets called every time the plug-in wants you to draw a frame. 
Ref: discussion with David Schleef on #gstreamer, 18 Sept 2009
Comment 1 Sebastian Dröge (slomo) 2009-09-18 07:08:00 UTC
This would be really useful and comes together with bug #523397 ;)

Should probably be part of gst-plugins-cairo ( http://cgit.freedesktop.org/~company/gst-plugins-cairo )


For the generic cairooverlay you would then want a signal, to which applications can connect and that is called everytime a new buffer should be processed?
Comment 2 Benjamin Otte (Company) 2009-09-18 07:23:35 UTC
That should be pretty much trivial with the intended API of my branch, as it'd just be a custom video filter with an in-place transform function using gst_cairo_create_surface ().
Comment 3 Jon Nordby 2011-01-28 09:36:50 UTC
I've started working on this, and currently I have a working element which emits a signal and allows you to draw in the signal handler. Missing is passing the size of the buffer that can be drawn to the signal handler, and I need to check if I can avoid depending on cairo-gobject.

Hope to have a patch ready in a couple of days.
Comment 4 Sebastian Dröge (slomo) 2011-01-28 09:50:28 UTC
Without a dependency on cairo-gobject you would have to register your own types for cairo_surface_t or cairo_t... which would then cause problems with bindings. IMO there's no reason to not use cairo-gobject here
Comment 5 Jon Nordby 2011-01-28 23:58:16 UTC
Created attachment 179562 [details] [review]
Initial patch
Comment 6 Jon Nordby 2011-01-29 00:00:35 UTC
Created attachment 179563 [details]
Example application showing the element in use
Comment 7 Sebastian Dröge (slomo) 2011-02-26 13:20:35 UTC
Ok, bugzilla's review function is broken unfortunately... I'll write the comments here then.

One general comments first: You should conditionally enable or disable the element if cairo-gobject >= 1.10.0 is available or not. Always depending on cairo-gobject or cairo >= 1.10.0 is not yet possible because it's a very new version that isn't packaged in the latest releases of all major distributions

Other comments below


ext/cairo/gstcairooverlay.c
46	#define GST_CAIRO_OVERLAY_VIDEO_CAPS GST_VIDEO_CAPS_BGRA
47	#else
48	#define GST_CAIRO_OVERLAY_VIDEO_CAPS GST_VIDEO_CAPS_ARGB
You could also add support for BGRx and xRGB here


103	  }
104	 
105	  g_signal_emit_by_name ((gpointer) overlay, "draw",
No need to cast to gpointer here
Also you should use g_signal_emit with gst_cairo_overlay_signals[SIGNAL_DRAW] here, it's much faster and you have the signal ID anyway


140	  gst_cairo_overlay_signals[SIGNAL_DRAW] =
141	      g_signal_new ("draw",
142	      G_TYPE_FROM_CLASS (parent_class),
Not G_TYPE_FROM_CLASS(parent_class) but G_TYPE_FROM_CLASS(klass). You want to make this signal for the cairo overlay class, not the GstVideoFilter class ;)


141	      g_signal_new ("draw",
142	      G_TYPE_FROM_CLASS (parent_class),
143	      G_SIGNAL_RUN_LAST | G_SIGNAL_NO_RECURSE | G_SIGNAL_NO_HOOKS,
Any special reason why you use RUN_LAST here if you don't specify a default signal handler aka object method handler anyway?
The other flags don't make much sense here either I guess, the signal is only meant to be emitted from transform_ip and emitting it again from the signal handlers is not a good idea in any case and I don't think there's a reason why you should forbid signal hooks (except them being not very useful for this signal). IMHO you can just use 0 for the signal flags here and it doesn't make any difference. Having all the flags here will only confuse :)


146	      NULL,
147	      gst_cairo_marshal_VOID__POINTER_INT_INT,
148	      G_TYPE_NONE, 3, CAIRO_GOBJECT_TYPE_CONTEXT, G_TYPE_INT, G_TYPE_INT);
The cairo context is no pointer but a boxed type. Using it as a boxed type will make use of correct reference handling here
 
Also I think more information than just the width and height are interesting here. Especially the pixel aspect ratio will be of interest for the signal handlers and for animations the timestamp, the framerate or duration might be interesting too. Maybe use void draw(cairo_t *ct, guint64 timestamp, guint64 duration) here and additionally add a setcaps signal that is emitted from setcaps and passes the GstCaps* to the signal handlers? For some overlays they also might need to create an expensive context whenever the width/height or other things of the caps are changing so a setcaps signal might be really useful anyway
 
And please add gtk-doc documentation to the signal :)


155	 
156	GType
157	gst_cairo_overlay_get_type (void)
You could use GST_BOILERPLATE here instead of all the boilerplate get_type() code and the parent_class = g_type_blablabla code in class_init


172	    };
173	 
174	    cairo_overlay_type = g_type_register_static (GST_TYPE_BASE_TRANSFORM,
Use GST_TYPE_VIDEO_FILTER here, not basetransform


ext/cairo/gstcairooverlay.h
42	 
43	typedef struct _GstCairoOverlay {
44	  GstBaseTransform basetransform;
Use GstVideoFilter here, not basetransform


50	typedef struct _GstCairoOverlayClass {
51	  GstVideoFilterClass parent_class;
52	  void (*draw) (GstCairoOverlay *overlay, cairo_t *cr, gint width, gint height);
You can remove this, you're not using a default signal handler during signal creation anyway and you don't need one for this signal
Comment 8 Sebastian Dröge (slomo) 2011-02-26 13:23:15 UTC
Oh and if you want you could add the sample application to tests/examples/cairo. I think it might be interesting for others too. And maybe add it to the element's documentation instead of the useless example pipeline. See the documentation of the n-bands equalizer for an example of how to include example source code in the documentation
Comment 9 Bastien Nocera 2011-02-26 15:40:53 UTC
I'm interested in this to draw some status items in the video in Totem, for example, when buffering, or paused.

The main thing for me is that the performance impact should be close to none when not drawing anything (eg. passthrough). The drawing would only be done when either paused, or buffering so the impact on normal playback should be minimal.
Comment 10 Sebastian Dröge (slomo) 2011-02-26 18:20:36 UTC
The performance impact if no drawing is done should be minimal, only a cairo surface around existing data and a cairo context is created. These are just some memory allocations. The bigger problem is, that cairo only supports ARGB/xRGB currently and you always need colorspace conversion to this, no matter if you're actually drawing something or not. This is going to hurt performance and can only be fixed by adding YUV support to cairo
Comment 11 Jon Nordby 2011-02-26 18:48:48 UTC
Thanks for review, working on the issues now.

The color conversions are the heavy operations here, so unless you happen to have RGB data in your pipeline at the appropriate place you will take a significant performance hit.
A possible work-around is to move YUV<->RGB conversion into the element (but also accept RGB), having a boolean property "enabled", and only doing the conversions+drawing if it is true. I will not do this, but I'm not opposed to someone else adding it.
Comment 12 Jon Nordby 2011-02-27 16:36:34 UTC
Created attachment 182044 [details] [review]
Patch v2

Updated patch. I have incorporated all the suggested changes, including:
- Conditional compiling, only build if cairo-gobject 1.10 is available
- A signal emitted at setcaps time, passing the GstCaps* to signal handlers
- Pass timestamp and duration in draw signal
- Updated example and added to tests/examples/cairo
- Documentation for element and its signals

Right now I'm only passing the input caps to signal handlers.
Do we want to pass the output caps as well? Should signal handlers be allowed to change the output caps?
Comment 13 Bastien Nocera 2011-02-27 18:01:05 UTC
(In reply to comment #11)
> Thanks for review, working on the issues now.
> 
> The color conversions are the heavy operations here, so unless you happen to
> have RGB data in your pipeline at the appropriate place you will take a
> significant performance hit.
> A possible work-around is to move YUV<->RGB conversion into the element (but
> also accept RGB), having a boolean property "enabled", and only doing the
> conversions+drawing if it is true. I will not do this, but I'm not opposed to
> someone else adding it.

I believe that the deinterlace plugin had similar problems when used within playbin2.
Comment 14 Sebastian Dröge (slomo) 2011-02-27 19:23:59 UTC
(In reply to comment #12)

> Right now I'm only passing the input caps to signal handlers.
> Do we want to pass the output caps as well? Should signal handlers be allowed
> to change the output caps?

No, you always want the same caps for input and output in this element. Thanks for updating the patch, I'll review it tomorrow or later :)

(In reply to comment #13)

> I believe that the deinterlace plugin had similar problems when used within
> playbin2.

Yes, it has a passthrough mode that is used for RGB and other unsupported formats. It would be possible to add a enabled/disabled property to this element, that then reconfigures basetransform for the next buffer and switches between allowing YUV/RGB and only RGB.
Comment 15 Sebastian Dröge (slomo) 2011-02-28 21:01:51 UTC
Looks good to me, just two small comments :) After that this can be committed IMHO unless we really want the enable/disable behaviour from the very beginning


ext/cairo/Makefile.am
10	built_headers = gstcairo-marshal.h
11	 
12	BUILT_SOURCES = $(built_sources) $(built_headers)
You're including the marshaller even if cairo-gobject is not found
ext/cairo/gstcairooverlay.c


163	      cairo_image_surface_create_for_data (GST_BUFFER_DATA (buf), format,
164	      overlay->caps_width, overlay->caps_height,
165	      cairo_format_stride_for_width (format, overlay->caps_width));
Use width*4 in any case, the cairo preffered format stride could be different and you must use the GStreamer stride here
Comment 16 Jon Nordby 2011-03-01 15:39:05 UTC
Created attachment 182184 [details] [review]
Patch v3

Addressed the two above comments.
I don't need enable/disable, and I don't see how we could not add it later if anyone wants to do it, so I'm hoping this can go in now. :)
Comment 17 Sebastian Dröge (slomo) 2011-03-02 22:22:55 UTC
commit 32dff9df75942c51b3ecbd7ffa394ef755881d50
Author: Jon Nordby <jononor@gmail.com>
Date:   Fri Jan 28 02:14:04 2011 +0200

    cairooverlay: Add generic Cairo overlay video element.
    
    Allows applications to connect to the "draw" signal of
    the element and do their custom drawing there.
    Includes an example application demonstrating usage.
    
    Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=595520
Comment 18 Jon Nordby 2011-03-03 18:57:21 UTC
Great! 
The element is not shown on http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-good-plugins/html/ yet, is that expected?
Comment 19 Sebastian Dröge (slomo) 2011-03-04 07:12:19 UTC
Yes, the docs on the website are updated manually and nobody did it yet
Comment 20 Jon Nordby 2011-11-23 15:53:36 UTC
Removed wrong bug dependency.