Created attachment 188305 [details] [review]
Proposed patch for minimal level of integration

Please prepare a patch against gst-plugins-bad for this. New plugins are first added to gst-plugins-bad. I'll review this later nonetheless :)

Review of attachment 188305 [details] [review]:

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

Also, please port it to the new GStremer 0.11/1.0 API (git master) :)

Hi Jon, do you plan on updating this element based on the feedback so we can get it included?

Hi Christian,
yes, I would like to get it in. And to write up a bit how GEGL and GStreamer could be used together for more advanced video processing/compositing scenarios.
Hope to find time for it perhaps the coming 2 weeks.

Are you planning to work on this any time soon? This also has to be updated to GStreamer 1.0

Hi Sebastian.
Realistically speaking it is not very likely. The usecase I had for it has been solved in a different way, and I've reduced my involvement in GEGL a bit.

That said, if someone wants to hack on it, I'll happily help with questions that may come up.

Ok, let's just close it for now then until someone else is interested in working on this. No point in keeping the bug open forever without any action :)