[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

--- Comment #3 from Miguel (elmiguelao) Casas-Sanchez <miguelecasassanchez at gmail.com> 2013-10-10 15:43:57 UTC ---
(In reply to comment #2)
> Review of attachment 255918 [details]:
> 
> ::: 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

Will change.

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

Done adding it to docs/plugins/Makefile.am but not the other, do you mean
docs/plugins/html/index.sgml?

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

I have no clue, Pixel format reference ([1]) won't say and couldn't find it
either :(

[1]
http://www.openni.org/wp-content/doxygen/html/namespaceopenni.html#a6d1ecf2502394d600cb8d3709092d5a5

> 
> @@ +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?

Yes exactly.

> 
> @@ +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?).
> 

Moved to class_init since indeed should be done once per lifetime.

> Maybe make this a recounted initialization or something?

Not sure. Do you think a process could shutdown the whole library for another
-otherwise independent- process?

> 
> @@ +337,3 @@
> +                   "height", G_TYPE_INT, ni2src->height,
> +                   NULL);
> +  }
> 
> and otherwise? Should return empty then I guess

Initialised on declaration to gst_caps_new_empty();

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

Done.

> 
> @@ +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?

Indeed. I corrected it so that here there's a call gst_openni2_src_stop to stop
the individual stream(s); this function should not include a call to
finalise().

Openni finalise should only be called from gst_openni2_src_finalize(), killing
the whole library etc, and needing a call to openni2_initialise() function etc.

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

Done.

> 
> @@ +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
> 

Done but I don't much see the point, I use all the values as calculated in
openni2_initialise_devices(), what's the advantage to use GstVideoFrame to wrap
& manipulate the buffer to be sent? 

> @@ +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).

Actually after seeing this code again, I don't remember what is this added
resize for :)

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