GNOME Bugzilla – Bug 708914
Add openni2 plugin and openni2src element
Last modified: 2013-12-03 14:17:35 UTC
Created attachment 255918 [details] [review] Full patch with openni2 plugin and openni2src element OpenNI2 is the (a?) open source library to access 3D sensors such as those based on PrimeSense depth sensor. Examples of such sensors are the Kinect used in Microsoft Xbox consoles and Asus WAVI Xtion. Notably recordings of 3D sessions can also be replayed as the original devices. This bug tracks the introduction in gst-plugins-bad of support for OpenNI2 devices, in the form of a plugin containing one source element capable of streaming 16-bit depth maps (sent as GRAY16_LE), RGB color frames and RGBA frames.
Ping?
Review of attachment 255918 [details] [review]: ::: 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).
(In reply to comment #2) > Review of attachment 255918 [details] [review]: > > ::: 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 :)
Created attachment 256923 [details] [review] First round of comments addressed Addressed most comments of Sebastian's previous review. I had to do a hack @ gst_openni2_src_stop() _not_ to free the |video_frame| , the function was called twice and called a SIGSEGV... :(
Created attachment 256925 [details] [review] Ooops First round of comments addressed Incremental patch addressing most of Sebastian's comments. [hack] I had to comment out free'ing of the |video_frame| at gst_openni2_src_stop() because it was called several times, then SIGSEGV'ing... :(
ping?
bugzilla is back! :) ping?
ping :'(
Review of attachment 256925 [details] [review]: ::: ext/openni2/gstopenni2src.cpp @@ +283,3 @@ } } + src->video_frame = (GstVideoFrame*)malloc(sizeof(GstVideoFrame)); Maybe just store the GstVideoFrame on the stack in the functions where you need it? I see no reason for storing it inside the instance struct @@ +297,3 @@ src->color.stop(); + //free(src->video_frame); Maybe g_free(src->video_frame); src->video_frame=NULL; here? @@ +312,1 @@ return gst_caps_new_empty(); You're leaking the caps created at the top here @@ +588,3 @@ /* Is this safe or is a hack? */ + gst_video_frame_unmap (src->video_frame); + //gst_buffer_resize (buf, 0, map.size); Why?
Created attachment 259053 [details] [review] 2nd full patch openni2 plugin and openni2src element Comments addressed TTBOMK :) More concretely there were two points: - the creation of a GstVideoFrame in the read_gstbuffer did not make much sense to me on second reads. Perhaps I copied it blindly? So what I do now in openni2_read_gstbuffer() is to create a buffer with g_malloc() and associate it with a GstMemory that on free, calls g_free. The buffer is created and filled in by copying the openni2 data, and sent on. (I could have associated the openni2 buffer but I'm not sure about its lifetime...). - the other review was on leaking the caps in gst_openni2_src_get_caps(). What I do now is to keep a pointer-ref to the caps in the element structure, get a reference to the caps the first time and send it on subsequent times. This was strongly inspired by gstv4l2src.cpp :)
Review of attachment 259053 [details] [review]: Remove the common change from the patch ::: ext/openni2/gstopenni2src.cpp @@ +70,3 @@ +#define DEFAULT_SOURCETYPE SOURCETYPE_DEPTH + +#define SAMPLE_READ_WAIT_TIMEOUT 2000 //2000ms No // comments, use /* */ comments @@ +307,3 @@ + GstCaps *ret; + ni2src = GST_OPENNI2_SRC (src); + if (ni2src->gst_caps) This is racy btw, you need to lock access to ni2src->gst_caps @@ +553,3 @@ + data = (guint8*) g_malloc(framesize); + ni2src_buffer_as_gst_memory = gst_memory_new_wrapped(GST_MEMORY_FLAG_READONLY, + data, framesize, 0, framesize, data, g_free); You can also just use memory = gst_allocator_alloc (NULL, framesize, NULL); instead of that. Or just use gst_buffer_new_and_alloc() But why readonly? Also you need to be careful with the strides, maybe they're not the same for openni and GStreamer. @@ +559,3 @@ + /* Add depth as 8bit alpha channel, depth is 16bit samples */ + guint16* pDepth = (guint16*) src->depthFrame.getData(); + for( int i=0; i < src->depthFrame.getDataSize()/2; ++i) Why divided by 2?
(In reply to comment #11) > Review of attachment 259053 [details] [review]: > > Remove the common change from the patch > > ::: ext/openni2/gstopenni2src.cpp > @@ +70,3 @@ > +#define DEFAULT_SOURCETYPE SOURCETYPE_DEPTH > + > +#define SAMPLE_READ_WAIT_TIMEOUT 2000 //2000ms > > No // comments, use /* */ comments > Done. Here and elsewhere. > @@ +307,3 @@ > + GstCaps *ret; > + ni2src = GST_OPENNI2_SRC (src); > + if (ni2src->gst_caps) > > This is racy btw, you need to lock access to ni2src->gst_caps > Mutex accessed. > @@ +553,3 @@ > + data = (guint8*) g_malloc(framesize); > + ni2src_buffer_as_gst_memory = > gst_memory_new_wrapped(GST_MEMORY_FLAG_READONLY, > + data, framesize, 0, framesize, data, g_free); > > You can also just use memory = gst_allocator_alloc (NULL, framesize, NULL); > instead of that. Or just use gst_buffer_new_and_alloc() > Did so: gst_buffer_new_and_alloc and used it to dump the Openni2 data. > But why readonly? Also you need to be careful with the strides, maybe they're > not the same for openni and GStreamer. > I meant to be wrapping up the openni2 memory as read only, because I wouldn't know how it'd react to writing onto it. Anyway this doesn't apply anymore :) > @@ +559,3 @@ > + /* Add depth as 8bit alpha channel, depth is 16bit samples */ > + guint16* pDepth = (guint16*) src->depthFrame.getData(); > + for( int i=0; i < src->depthFrame.getDataSize()/2; ++i) > > Why divided by 2? Multiplexing RGB and depth as RGBA is done by hand in this part of the code; with the depth map having 16bpp and alpha just 8bpp. The key here is that getDataSize() gives the size of the depth in bytes, so we divide it by two and then each sample is divided by 2^8, to make it 8bpp - losing accuracy in the process of course. Patch comming!
Created attachment 259096 [details] [review] Delta over 2nd full patch openni2 src. Addressing comments.
Comment on attachment 259096 [details] [review] Delta over 2nd full patch openni2 src. Bugzilla's review functionality breaks on that patch for some reason :( In get_caps() it's not enough to just return the cached caps. For every call you must intersect the result with the filter caps before returning, see other get_caps functions everywhere. You can also just use the GST_OBJECT_LOCK() instead of adding your own mutex (which btw needs a g_mutex_clear() call in the finalize or dispose function). gst_buffer_map() is enough to be called with GST_MAP_WRITE, you do no reading. Not that it matters much here but just for cleaner code :) You should make sure that the strides for openni are the same as for GStreamer, and otherwise convert between them.
Created attachment 259449 [details] [review] Second delta over second patch. On the caps management, I followed your suggestion, and I took inspiration from gstupdsrc.c on the caps management. Interestingly, v4l2src and videotestsrc do not seem to filter the caps at all, and that's possibly where I took first understanding that src's generally impose their caps and let the overall pipeline glue decide if there's an intersection, as opposed to doing it by hand. I still cache the capabilities btw. Also I changed to GST_BUFFER_LOCK, GST_BUFFER_UNLOCK. On the minus side, now it takes a while longer to negotiate the caps :) During buffer filling-in, I check that stride indeed coincides with expected width; I haven't seen any other case, so I wouldn't write a lot of code at this point. If and when this gets committed and used, if there are any troubles we can come back to it. Those are the only changes, the rest is style, sorry about that... :S Please have a look!
Created attachment 259450 [details] [review] 3rd full patch containing 2nd patch and both deltas. In the interest of Bugzilla and reviewing process, here's the whole file. Anyway we seem to be centered in caps and buffer management :)
Ping? Come on, I feel we're close by :) :)
I've merged it locally and cleaned up some things and fixed some more bugs, will push that later after some more things. Sorry for taking so long
No problem, thanks for the time spent! :)
Pushed :) How did that even work for you, the buffer handling was completely wrong. Please take a look at my changes :) I think for the future we should also consider marking the depth channel in a specific way. Maybe by outputting it as a separate view, or putting it into a GstMeta. commit 26bf14c9fd5eaefefcd1d4ffa7d65b793e1aabdd Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Dec 3 14:53:24 2013 +0100 openni2src: Check color format commit 4b3d2a1b09f4b09593d8c273ab3c7710ce2e52a5 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Dec 3 14:47:32 2013 +0100 openni2src: Add support for the video meta commit dbb64a69f4138dc37c75d0c8eab02946c312e84a Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Dec 3 14:46:25 2013 +0100 openni2src: Use GstVideoFrame API for better handling of strides And do proper stride conversion. commit 39c82d0dec8bed2adc067a027d011484d66d49f1 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Dec 3 14:35:57 2013 +0100 openni2src: Fix buffer handling to actually work and properly timestamp buffers commit 7a3eaa8f8a6ba7e091453f36302406318acab91f Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Dec 3 14:35:42 2013 +0100 openni2src: Don't shut down library when finalizing an element instance commit 04e8d63597f7017509f0f3ef0b50d9925bc2df8e Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Dec 3 14:35:21 2013 +0100 openni2src: Fix negotiation and creation of a video buffer pool commit 034757e936c2150ca706029f9b4fb58ca9040b90 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Dec 3 14:34:56 2013 +0100 openni2src: Use debug category properly commit 6450d87c63daae107f2f3c7a1d954b6616f17a15 Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Dec 2 17:03:46 2013 +0100 openni2src: Classification of the element should be Source/Video commit 4b7bead2d41fdf3ed4786dd3ea865541ce219f51 Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Dec 2 17:01:36 2013 +0100 openni2src: Use GstVideoInfo to create caps and don't leak them commit 1ba3edf682c9c7a40833e60600baf6f82bdb0f96 Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Dec 2 16:59:14 2013 +0100 openni2src: Some random cleanup and minor fixes commit 64675f07127016f77a1414456f58065f8b4a8bf3 Author: Miguel Casas-Sanchez <miguelecasassanchez@gmail.com> Date: Mon Dec 2 11:17:02 2013 +0100 openni: Add OpenNNI2 plugin https://bugzilla.gnome.org/show_bug.cgi?id=708914
Holy c*w! So many changes! I'm sorry I gave you that ton of work... it worked for me... :D but don't ask me how, perhaps you were using another device?? RGB and Depth at once work only so-so, and yet another trouble in this case is reducing 16b to 8b when packing it into the RGBA frame. I guess in that case it makes sense to open two openni2src, one for RGB and another for depth-16b and do the synchronisation/processing in other Gstreamer block? FWIW I find two things particularly confusing in new elements: - The caps negotiation process, and its associated query, etc, methods. - Forgot the other one :) A tutorial or so and best with a git example would be terrific.