[Bug 650750] Allow one to use GEGL to manipulate video

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #3 from Sebastian Dröge <slomo at circular-chaos.org> 2011-05-23 10:39:48 UTC ---
Review of attachment 188305:
 --> (https://bugzilla.gnome.org/review?bug=650750&attachment=188305)

::: ext/Makefile.am
@@ +161,3 @@
     $(TAGLIB_DIR) \
+    $(WAVPACK_DIR) \
+    $(GEGL_DIR)

Wrong indention

@@ +184,3 @@
     taglib \
+    wavpack \
+    gegl

Same here

::: ext/gegl/Makefile.am
@@ +3,3 @@
+
+libgstgegl_la_SOURCES = gstgegl.c gstgeglfilter.c
+libgstgegl_la_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS)$(GST_BASE_CFLAGS)
$(GST_CFLAGS) $(GEGL_CFLAGS)

There's a space missing between the gst-plugins-base and base CFLAGS

@@ +4,3 @@
+libgstgegl_la_SOURCES = gstgegl.c gstgeglfilter.c
+libgstgegl_la_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS)$(GST_BASE_CFLAGS)
$(GST_CFLAGS) $(GEGL_CFLAGS)
+libgstgegl_la_LIBADD = $(GST_LIBS) $(GST_PLUGINS_BASE_LIBS)
-lgstvideo-$(GST_MAJORMINOR) $(GST_LIBS) $(GEGL_LIBS)

Remove the GST_LIBS from the beginning and only keep the second

::: ext/gegl/gstgeglfilter.c
@@ +52,3 @@
+Set pads dynamically based on the formats that Babl supports
+Support a more GStreamer native type, like YUV420 in Gegl/Babl
+*/

Which other formats are supported by Gegl/Babl? But yes, support for them would
be great too but that can happen later :)

@@ +111,3 @@
+  Babl *format = babl_format ("RGBA u8");
+
+  // Push buffer to GEGL

Don't use C++/C99 comments

@@ +113,3 @@
+  // Push buffer to GEGL
+  gegl_buffer_set (self->buffer, &roi, format,
+      GST_BUFFER_DATA (buf), GEGL_AUTO_ROWSTRIDE);

You should use the GStreamer rowstride here, look at libgstvideo (especially
video.h) for how to get the rowstride and other information from caps

@@ +116,3 @@
+
+  // Get result back from GEGL
+  if (self->input_node) {

Shouldn't there always be an input node?

@@ +134,3 @@
+      if (self->input_node)
+        g_object_unref (self->input_node);
+      self->input_node = g_value_dup_object (value);

Shouldn't this be protected with the object lock too? And then take the object
lock in transform_ip and set_caps when changing something in one of the gegl
nodes.

@@ +193,3 @@
+
+  trans_class->set_caps = gst_gegl_filter_set_caps;
+  trans_class->transform_ip = gst_gegl_filter_transform_ip;

Use = GST_DEBUG_FUNCPTR (gst_gegl_filter_transform_ip) here and for setcaps

@@ +206,3 @@
+  pspec = g_param_spec_object ("input-node",
+      "Input node",
+      "Gegl node to take input from.", GEGL_TYPE_NODE, G_PARAM_READWRITE);

Use G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS here

@@ +217,3 @@
+  pspec = g_param_spec_object ("output-node",
+      "Output node", "Gegl node for the output.", GEGL_TYPE_NODE,
+      G_PARAM_READABLE);

And | G_PARAM_STATIC_STRINGS here too

@@ +225,3 @@
+gst_gegl_filter_init (GstGeglFilter * self, GstGeglFilterClass * klass)
+{
+  self->output_node = gegl_node ("gegl:buffer-source", NULL);

You should unref this in GObject::dispose

-- 
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.