GNOME Bugzilla – Bug 595520
Implement a generic cairo overlay
Last modified: 2011-11-23 15:53:36 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
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?
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 ().
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.
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
Created attachment 179562 [details] [review] Initial patch
Created attachment 179563 [details] Example application showing the element in use
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
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
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.
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
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.
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?
(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.
(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.
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
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. :)
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
Great! The element is not shown on http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-good-plugins/html/ yet, is that expected?
Yes, the docs on the website are updated manually and nobody did it yet
Removed wrong bug dependency.