[gstreamer-bugs] [Bug 435120] cairosvgoverlay

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=435120
  GStreamer | gst-plugins-bad | git

Sebastian Dröge <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #166265|none                        |needs-work
             status|                            |

--- Comment #8 from Sebastian Dröge <slomo at circular-chaos.org> 2010-08-31 08:44:32 UTC ---
Review of attachment 166265:
 --> (https://bugzilla.gnome.org/review?bug=435120&attachment=166265)

Looks good in general but some comments below and in general I think it might
be better to not have properties for setting the filename or setting the SVG
data but instead to have another sinkpad, where you push in the SVG data.
That's more generic and allows the element to be used in more scenarios.
See gst-plugins-good/gst/shapewipe for an example of an element, that takes
some kind of "picture" on one sinkpad and uses this to transform the video
stream from the other sinkpad.

::: ext/rsvg/gstrsvg.c
@@ +34,3 @@
+  return (gst_element_register (plugin, "rsvgoverlay",
+          GST_RANK_NONE, GST_TYPE_RSVG_OVERLAY)
+      &

You want an && here... not that it matters much in this case

::: ext/rsvg/gstrsvgoverlay.c
@@ +46,3 @@
+    GST_PAD_SRC,
+    GST_PAD_ALWAYS,
+    GST_STATIC_CAPS (GST_VIDEO_CAPS_RGBA)

This is wrong. cairo doesn't produce RGBA. It produces ARGB or BGRA depending
on the hosts endianness. Use something like

#if G_BYTE_ORDER == G_LITTLE_ENDIAN
#define GST_RSVG_VIDEO_CAPS GST_VIDEO_CAPS_BGRA
#else
#define GST_RSVG_VIDEO_CAPS GST_VIDEO_CAPS_ARGB
#endif

and use that instead then. See gstrsvgdec.c

@@ +56,3 @@
+
+GST_BOILERPLATE (GstRsvgOverlay, gst_rsvg_overlay, GstBaseTransform,
+    GST_TYPE_BASE_TRANSFORM);

It might be better to inherit from GstVideoFilter instead. It's like
GstBaseTransform but additionally implements sane defaults for QoS. You only
need to change this here and in the header

@@ +68,3 @@
+  switch (prop_id) {
+    case ARG_DATA:
+    case ARG_FILENAME:

This is not threadsafe. You need to use some lock here and in the transform
function

@@ +135,3 @@
+
+  if (G_UNLIKELY (!gst_video_format_parse_caps (GST_BUFFER_CAPS (buf),
+              &format, &width, &height)))

Do the caps parsing only once in the GstBaseTransform::setcaps vfunc

@@ +200,3 @@
+
+  g_object_class_install_property (G_OBJECT_CLASS (klass), ARG_DATA,
+      g_param_spec_string ("data", "data", "RSVG data.", "",
G_PARAM_WRITABLE));

Use G_PARAM_WRITABLE | G_PARAM_STATIC_STRINGS here and for the other properties

::: ext/rsvg/gstrsvgoverlay.h
@@ +31,3 @@
+  ARG_FILENAME,
+  ARG_FIT_TO_FRAME
+};

Put this enum into the source file (and rename the elements to PROP_0,
PROP_DATA, etc)

Also, maybe properties for the x/y position, the x/y scale factor and a
property to force keeping of the aspect ratio might be a good thing to have

@@ +50,3 @@
+{
+  GstBaseTransform element;
+

Add a comment like

/* < private > */

here to let gtk-doc ignore the remaining fields of the struct.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.