GNOME Bugzilla – Bug 732535
openni2src: Various fixes
Last modified: 2016-12-31 10:09:40 UTC
I'm attaching a bunch of fixes that I found necessary while playing with this plugin. They're not in any particular order (just what was easiest to split and commit).
Created attachment 279646 [details] [review] openni2src: Mark element as a live source
Created attachment 279647 [details] [review] openni2src: Fix deadlock when _get_caps() is called before READY The object lock was not being dropped in the empty case. Restructured the code a bit to make this sort of error less likely.
Created attachment 279648 [details] [review] openni2src: Open device on NULL->READY
Created attachment 279649 [details] [review] openni2src: Make the location property not be mandatory Our calls to device open already handle the unset location case (by opening any available device).
Created attachment 279650 [details] [review] openni2src: Fix timestamping OpenNI2 makes no guarantees of timestamp starting from zero, just that it will be a millisecond timestamp. Make timestamps start from zero manually so things work correctly.
Created attachment 279651 [details] [review] openni2src: Close device when stopping the stream
Created attachment 279652 [details] [review] openni2src: Don't embed C++ objects in our GObject Since C++ objects shoudl be properly constructed, we keep only pointers to them and manually construct them on the heap.
Created attachment 279653 [details] [review] openni2src: Add proper clean up of OpenNI2 objects
Comment on attachment 279646 [details] [review] openni2src: Mark element as a live source Is it also live when playing from a file? I think it should be conditional :)
Review of attachment 279647 [details] [review]: ::: ext/openni2/gstopenni2src.cpp @@ +352,3 @@ + + if (!ni2src->gst_caps) + return gst_pad_get_pad_template_caps (GST_BASE_SRC_PAD (ni2src)); Previously it returned empty caps here, which arguably makes more sense if it couldn't probe any possible caps
Review of attachment 279652 [details] [review]: ::: ext/openni2/gstopenni2src.cpp @@ -505,3 @@ - /** OpenNI2 open device or file **/ - rc = src->device.open (deviceURI); Why do you remove the comment? ;)
(In reply to comment #10) > Review of attachment 279647 [details] [review]: > > ::: ext/openni2/gstopenni2src.cpp > @@ +352,3 @@ > + > + if (!ni2src->gst_caps) > + return gst_pad_get_pad_template_caps (GST_BASE_SRC_PAD (ni2src)); > > Previously it returned empty caps here, which arguably makes more sense if it > couldn't probe any possible caps Hum, we could distinguish between device being open and not finding caps vs. device not being open, I suppose.
(In reply to comment #11) > Review of attachment 279652 [details] [review]: > > ::: ext/openni2/gstopenni2src.cpp > @@ -505,3 @@ > > - /** OpenNI2 open device or file **/ > - rc = src->device.open (deviceURI); > > Why do you remove the comment? ;) Felt like it was a bit redundant. :)
(In reply to comment #9) > (From update of attachment 279646 [details] [review]) > Is it also live when playing from a file? I think it should be conditional :) Probably. Is this usually done via file:// device, and is checking that enough?
*** Bug 726586 has been marked as a duplicate of this bug. ***
There were patches for some of these things in bugzilla already btw ;)