[Bug 708914] Add openni2 plugin and openni2src element

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge (slomo) <slomo> changed:

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

--- Comment #2 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-10-02 13:18:33 UTC ---
Review of attachment 255918:
 --> (https://bugzilla.gnome.org/review?bug=708914&attachment=255918)

::: configure.ac
@@ +2500,3 @@
 ext/opencv/Makefile
 ext/openjpeg/Makefile
+ext/openni2/Makefile

You will also need to add something to ext/Makefile.am to get it into SUBDIRS
if the plugin is enabled

::: ext/openni2/gstopenni2.cpp
@@ +21,3 @@
+
+/**
+ * SECTION:plugin-openni2src

If you add docs, also add them to docs/gst/Makefile.am and gst-plugins-bad.sgml
and all the other files

::: ext/openni2/gstopenni2src.cpp
@@ +48,3 @@
+    GST_PAD_ALWAYS,
+    GST_STATIC_CAPS ("video/x-raw, "
+             "format = (string) {RGBA, RGB, GRAY16_LE} "

Is this maybe endianness dependant? LE on little endian systems, BE on big
endian ones (and ABGR/BGR there)?

@@ +81,3 @@
+      {SOURCETYPE_DEPTH, "Get depth readings", "depth"},
+      {SOURCETYPE_COLOR, "Get color readings", "color"},
+      {SOURCETYPE_BOTH, "Get color and depth (as alpha) readings", "both"},

Depending on this it will output one of the three video formats?

@@ +178,3 @@
+
+  /* OpenNI2 initialisation inside this function */
+  openni2_initialise_library (ni2src);

Shouldn't this be in class_init? It's only required once per process, right?
Otherwise there might be race conditions if multiple elements are created at
once, and also with openni2_finalise() below (::shutdown() should only be
called if all elements are gone, right?).

Maybe make this a recounted initialization or something?

@@ +337,3 @@
+                   "height", G_TYPE_INT, ni2src->height,
+                   NULL);
+  }

and otherwise? Should return empty then I guess

@@ +339,3 @@
+  }
+  //ni2src->probed_caps = gst_caps_ref (ret);
+  GST_WARNING_OBJECT (ni2src, "probed caps: %" GST_PTR_FORMAT, ret);

Not a warning

@@ +371,3 @@
+  switch (transition) {
+    case GST_STATE_CHANGE_READY_TO_NULL:
+      openni2_finalise (src);

Doesn't this shut down the library and thus make going to PLAYING again later
fail?

@@ +484,3 @@
+      return GST_FLOW_ERROR;
+    }
+    GST_WARNING_OBJECT (src, "DEPTH&COLOR resolution: %dx%d",

No warning, and below

@@ +527,3 @@
+  /* Now we plug the data from ni2src->frame into buf */
+  GstMapInfo map;
+  gst_buffer_map (buf, &map, GST_MAP_WRITE);

Ideally use gst_video_frame_map() here and then the stride and everything from
the GstVideoFrame

@@ +586,3 @@
+  /* Is this safe or is a hack? */
+  gst_buffer_unmap (buf, &map);
+  gst_buffer_resize (buf, 0, map.size);

This is a hack :) You should instead make sure that proper sized buffers are
allocated. E.g. by using GstVideoBufferPool (take a look at the pool code in
videotestsrc for an example).

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