https://bugzilla.gnome.org/show_bug.cgi?id=595520
GStreamer | gst-plugins-good | git
--- Comment #7 from Sebastian Dröge <slomo at circular-chaos.org> 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
--
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.