OpenNI2 driver

[hanyazou]

My todo list for this driver is empty now.

please refer #243

[xlz]

Too many commits to review practically. If possible please organize your changes in this way: If new files are created you can group them into commits of logical units on files basis. If existing files are modified, create commits incrementally so it is clear why there should such changes. Also, avoid intermediate commits (renaming, reverting, back and forth). It is fine to reset --hard the history and recreate a "patch series".

[hanyazou]

Thank you for your comment. I can understand what you mean and I had concatenated some of my changes before the pull request using rebase -i and many cherry picking.

How many changes in order do you suppose? 10 or so?
(Please note that all of these files are newly added.)

[christiankerl]

some comments:

• could you change the extern folder to a git submodule referencing the OpenNI2 repository?
• are the D2S.h and S2D.h files still used? otherwise remove them
• the license headers have to be fixed
• why don't you use OpenNI2 logging functions/classes
• Driver should not be a subclass of libfreenect2::Freenect2, but have a member of this type
• why is there commit @001cf03, waiting for all frames seems to add additional delay and OpenNI2 provides a synchronizing frame listener (afaik) EDIT: I checked the discussion leading to this change, which build type was used to compile the driver for measuring the time? (you don't set it in CMakeLists.txt, and default is Debug?!)
• what's the problem with the libfreenect2 frame sequence numbers?

these are my major concerns, I have more minor comments I'll add once the major once are addressed ;)

[HenningJ]

why is there commit @001cf03, waiting for all frames seems to add additional delay and OpenNI2 provides a synchronizing frame listener (afaik) EDIT: I checked the discussion leading to this change, which build type was used to compile the driver for measuring the time? (you don't set it in CMakeLists.txt, and default is Debug?!)

Yes, well...at least for the registered RGB image you need depth and RGB anyways.
But primarily it was done to separate further processing from the libfreenect2::onNewFrame() callback. Because if you spend to much time in there, you stall libfreenect2's processing of new frames.
But you're right, for the IR image and when the registration is turned off, it would make more sense to have separate unsynchronized frame listeners.

[hanyazou]

could you change the extern folder to a git submodule referencing the OpenNI2 repository?

It might be better to remove the extern folder and use installed OpenNI2 instead because OpenNI2 is not actively updated.

the license headers have to be fixed

Which headers ? I've updated libfreenect2/OpenNI2-FreenectDriver/src/DeviceDriver.cpp in 5a30f2d .

Driver should not be a subclass of libfreenect2::Freenect2, but have a member of this type

I think current implementation is no so bad while it might be better to have a member of the type. Could you tell me why the driver should not be a subclass of Freenect2?

what's the problem with the libfreenect2 frame sequence numbers?

The frame sequence numbers (FSN) of depth image and the FSN of color image have a large offset. If you use those FSN to create timestamp of the frames, depth and color timestamps have a large time lag. It might be dozen seconds, several minutes or a few hours. It looks that the FNS of depth image is reset to zero when you start streaming while the FNS of color image is free-run and not reset to zero.

[christiankerl]

extern folder:
I guess any local installation will be at least as outdated as code from https://github.com/occipital/OpenNI2. so using this as git submodule should be fine

license header:
I mean the comment at the beginning of every source file in libfreenect2, if you want the OpenNI2 driver in this repository you need to add this to every of your source files (not those in extern/):

/*
 * This file is part of the OpenKinect Project. http://www.openkinect.org
 *
 * Copyright (c) 2015 individual OpenKinect contributors. See the CONTRIB file
 * for details.
 *
 * This code is licensed to you under the terms of the Apache License, version
 * 2.0, or, at your option, the terms of the GNU General Public License,
 * version 2.0. See the APACHE20 and GPL2 files for the text of the licenses,
 * or the following URLs:
 * http://www.apache.org/licenses/LICENSE-2.0
 * http://www.gnu.org/licenses/gpl-2.0.txt
 *
 * If you redistribute this file in source form, modified or unmodified, you
 * may:
 *   1) Leave this header intact and distribute it under the same terms,
 *      accompanying it with the APACHE20 and GPL20 files, or
 *   2) Delete the Apache 2.0 clause and accompany it with the GPL2 file, or
 *   3) Delete the GPL v2 clause and accompany it with the APACHE20 file
 * In all cases you must keep the copyright notice intact and include a copy
 * of the CONTRIB file.
 *
 * Binary distributions must follow the binary distribution requirements of
 * either License.
 */

you seem to have copied the DeviceDriver.cpp from some other project. I don't know how large the overlap still is and if the original license allows simply changing the license header.

sub classing Freenect2:
what's the reason for sub classing? do you need access to any internals of Freenect2? if there is no need for inheritance, I think composition should be preferred.

[christiankerl]

@HenningJ it's just strange that copying the color frame to a different buffer leads to such a large performance drop (even with registration enabled which needs just a few ms)

[hanyazou]

• could you change the extern folder to a git submodule referencing the OpenNI2 repository?
• Driver should not be a subclass of libfreenect2::Freenect2, but have a member of this type

done.

• are the D2S.h and S2D.h files still used? otherwise remove them

I tried to remove them to find that they are required by the NiTE. You can't remove them.

you seem to have copied the DeviceDriver.cpp from some other project. I don't know how large the overlap still is and if the original license allows simply changing the license header.

Yes... original driver came from "the OpenKinect Project",
https://github.com/OpenKinect/libfreenect/tree/master/OpenNI2-FreenectDriver. I wrote a mail to the author piedar to invite him to this thread. I hope that he will agree to add the license headers in the source files.

• why don't you use OpenNI2 logging functions/classes

OpenNI2 logging function XnLogXxxx() seems not to be a part of OpenNI2 API. You can't use the function without OpenNI2 full source code since OpenNI binary distribution does not contain XnLog.h. It's better that the driver doesn't depend OpenNI2 internal functions.

[christiankerl]

The driver services provide logging functions. For those you don't need full OpenNI2 source ?!

OK too bad nite needs s2d and d2s.

Can you add the license comment to every source file?

[piedar]

Please feel free to add APACHE20 / GPL2 license headers to code taken from OpenNI2-FreenectDriver. Thank you for your efforts to port it!

In the original project, the driver had to subclass Freenect::FreenectDevice because the frame callbacks were implemented as virtual methods. It is probably now not necessary thanks to libfreenect2::FrameListener.

And I never did get OpenNI2 logging to work...

[hanyazou]

I've pushed two changes. It seems that all issues except an issue about 001cf03 are resolved or answered.

[HenningJ]

it's just strange that copying the color frame to a different buffer leads to such a large performance drop (even with registration enabled which needs just a few ms)

Yeah, it does seem excessive. Part of it was the slower registration code that was used at the time, which I changed in a later commit (a312555).
And I did never track it down completely. I was just happy that I found an easy solution which worked fast enough for me. :-)

[hanyazou]

I made new pull request #518.