Fakenect: support applications needing _DEPTH_MM and _VIDEO_YUV_RAW

[rib]

We have a test application that's configuring a Kinect to capture _DEPTH_MM and _VIDEO_YUV_RAW video data. So that we can work with pre-recorded data this updates fakenect so that it can convert recorded _DEPTH_11BIT data to _MM and convert _RGB data into _YUV_RAW.

One notable issue with being able to map _DEPTH_11BIT data to _DEPTH_MM was that we need to save and load the registration parameters, which this does by introducing a device.json file in the output directory.

Another somewhat orthogonal change in this branch is make fakenect loop over the recorded data instead of stopping at the end since that was more practical behaviour for our use case, and it seems like a reasonable default behaviour considering that the Kinect camera doesn't normally stop spontaneously while in use. I could pull this patch out to a separate pull request if preferred.

Yet another orthgonal change was to add support for the freenect_get/set_ir_brightness apis that our application also uses but they weren't interposed by fakenect (again don't mind factoring this out to a separate pull request if preferred).

[piedar]

This is a very nicely formatted PR! I look forward to reviewing it more closely.

[piedar]

Does this one need to be freed as well?

[rib]

I'm fairly sure parson will recursively free the objects given the top-level one to free at the end, but can double check that

[rib]

ok, just to confirm; json_value_free() is recursive, so since that object got linked into the hierarchy of the top-level js value then it will also be freed.

[piedar]

Should it skip all the rest if device.json doesn't exist? Ideally old recordings will still work.

[rib]

Yeah makes sense. In that case it probably also makes sense to move the follow-up call to freenect_init_registration to the bottom of read_device_info at a point where we know we have read the required reg_info state.

I've tested an updated version with this change and will update my pull request.

Since I decided to return silently if we fail to load a device.json (for compatibility) I also added a check in freenect_set_depth_mode whereby requesting _DEPTH_MM will result in a "Warning:..." if registration state wasn't previously loaded.

[piedar]

Just a couple more comments.

I'd like the endless loop to be a separate PR (maybe with a runtime switch), just in case anyone depends on the current behavior.

Also I think parson should be a submodule instead of being embedded in this repo to make it easier to stay current with upstream. I just did

cd fakenect
git submodule add https://github.com/kgabis/parson

and then updated CMakeLists.txt to include_directories(parson/) and add_library(... parson/parson.c). I've already got that staged locally, but you could change it here too if you'd like a cleaner merge without cherry-pick.

[rib]

I can only ask you to possibly reconsider using a submodule to make it easier to update a single .c file :) If you've used git submodules before and know how it goes that's fine and I won't loose sleep over it, but git submodules adds quite a faff to the experience imho. In particular it will affect the basic instructions for any user/developer cloning and building libfreenect.

If the file is copied then updating goes something like:

cd parson-repo
git pull
cp parson.c /to/libfreenect-repo/fakenect/
cd /to/libfreenect-repo
git commit -a -m "update parson.c"

(and you / maintainers are the only ones who have to do these steps)

With submodules, now everyone needs to learn to pass --recursive when cloning the libfreenect repo, or for older versions of git have to run git submodules init + git submodules update after cloning.

To update parson it goes something like:

cd fakenect/parson
git pull
cd ..
git commit -a -m "update parson.c"

(seems like a very similar effort, just one step fewer than above, to update from a maintainer pov, but at the cost of complicating basic operations like cloning the repo)

As you might be able to tell, it's just one of those features that makes me sigh very heavily whenever I see it used :) (I even went overboard once and attempted writing a replacement: https://github.com/rib/git-subdir)

Anyway, I've pushed a https://github.com/rib/libfreenect/tree/fakenect-depth-mm-video-yuv-submodule branch that squashes in the addition of a submodule as you suggest, but will just wait for a confirmation to update the pull request with that branch.

[piedar]

Ok, you convinced me. There's one bugfix upstream that needs to be included, but other than that it's good without the submodule.

--- parson.c    2017-11-22 09:04:18.365023490 -0500
+++ parson/parson.c     2017-11-19 22:02:26.385795991 -0500
@@ -1718,7 +1718,7 @@
     char *current_name = NULL;
     JSON_Object *temp_obj = NULL;
     JSON_Value *new_value = NULL;
-    if (value == NULL || name == NULL || value == NULL) {
+    if (object == NULL || name == NULL || value == NULL) {
         return JSONFailure;
     }
     dot_pos = strchr(name, '.');