Add a dump depth processor. Reactivate the RGB dump processor. #551

brendandburns · 2016-02-01T22:42:23Z

Add a dump pipeline.

xlz · 2016-02-02T00:27:32Z

include/internal/libfreenect2/depth_packet_processor.h

 #endif // LIBFREENECT2_WITH_OPENCL_SUPPORT
+
+class DumpDepthPacketProcessor : public DepthPacketProcessor


Can you explain why you want to dump depth packets? The raw depth packets will require P0, X, and Z tables to decode. The tables are pulled from the Kinect at run time. So far my impression is that the raw depth packets are too big to be practical (85MB/s).

We'd like to do offline processing of the depth files in order to facilitate faster/easier algorithm refining and testing.

When you mention the tables, can I pull them once and keep them around or are they re-calculated whenever the kinect restarts?

And yes, I'm seeing ~3Mb / frame, so yes, I think your size estimate is correct. However I'm not offput by those sizes, and I can successfully capture to disk at those rates w/o frame dropping.

OK, I see now storage is not a problem for Google. :)

P0 table contains the initial phase shift of the device and can only be obtained at run time. X and Z tables are algorithmic helper and are derived from the intrinsic parameters of the IR camera. The intrinsic parameters are factory built-in and obtained at run time but you can provide your own calibration parameters.

More details about the tables: https://github.com/OpenKinect/libfreenect2/blob/master/src/libfreenect2.cpp#L63

If you don't plan to do your own calibration, you can save the tables in DumpDepthPacketProcessor::load*Table***().

or are they re-calculated whenever the kinect restarts?

Yes, but saving the tables at the start would be sufficient for the following continuous session.

brendandburns · 2016-02-02T06:34:08Z

Someone on my team suggested that an alternative to this approach would be to extend libfreenect2::Frame to include a Type of Raw and then pass the raw data along via Frame objects of type Raw to the corresponding Listener and have them handle it. This would probably mean adding a buffer_length field to the Frame object as well, since the bytes_per_pixel isn't applicable for the raw data.

Let me know if you prefer that approach to the one I've implemented here and I can change the implementation.

(and thanks for the fast review! I will try to get the portable timing function in tomorrow)

xlz · 2016-02-02T06:55:59Z

That was what I planned. I planned to add a JPEG format which is produced by a dummy JPEG processor doing nothing. Maybe you don't want to save it to disk, instead you want to stream it over network. We have some demand on a data recording tool #438, and specifying a recording format is not really a job of the library proper.

I don't want to add a new field if that breaks the ABI. Maybe you can abuse bytes_per_pixel for a Raw format by using proper documentation to redefine its meaning. There is quite some cruft in the library unfortunately, but no ABI breakage is accepted.

My preference:

The dump pipeline is acceptable for debug purposes, because the code is there and code matters.

If you don't break the ABI, the suggestion from your teammate is preferred, for further extension of a data recording tool. The actual code for writing the data to disk could be added to Protonect.cpp or you could directly create the data recording tool under tools/ to suit your needs (we can extend it later).

brendandburns · 2016-02-02T17:22:11Z

ok, SGTM.

I don't think adding a Type Raw would break the ABI, since Type is listed as TBD in the docs.

So how about the following:

We will add Type RAW to the type enum, and if Type is RAW bytes_per_pixel will actually be buffer_length which is kind of hacky but will work.

If that works for you, I will re-arrange this PR to do that.

Thanks
--brendan

xlz · 2016-02-02T19:26:45Z

I meant adding buffer_length to Frame would break the ABI.

The current version of your plan is good for me.

floe · 2016-02-02T19:41:54Z

Just curious: what's Google doing with the Kinect v2? Anything related to Project Tango? (Feel free to silently ignore me if I'm being too nosy ;-)

brendandburns · 2016-02-02T22:35:12Z

@xlz I've updated this PR to reflect our discussions. please take another look.

@floe sorry...

--brendan

xlz · 2016-02-02T22:44:55Z

include/internal/libfreenect2/depth_packet_processor.h

+  virtual void loadLookupTable(const short *lut);
+
+  // Get the P0 Tables
+  unsigned char* getP0Tables();


const unsigned char*? I think you would not need to modify the values.

brendandburns · 2016-02-02T23:03:23Z

comments addressed, please take another look.

Thanks!
--brendan

xlz · 2016-02-02T23:13:29Z

include/internal/libfreenect2/depth_packet_processor.h

+
+  const unsigned char* getP0Tables();
+
+  float* getXTable();


All these float* -> const float* too.

short* -> const short *

xlz · 2016-02-02T23:50:04Z

My review is complete.

brendandburns · 2016-02-03T00:28:53Z

Thanks for the extensive review. Comments addressed, please re-check.

--brendan

xlz · 2016-02-03T00:53:25Z

src/rgb_packet_processor.cpp

 {
 }

 DumpRgbPacketProcessor::~DumpRgbPacketProcessor()
 {
+  delete frame;


~~Double free here. As in DumpDepthPacketProcessor::~DumpDepthPacketProcessor(), just remove this line.~~

I see you have frame = NULL;, but this delete is useless anyway.

yeah, I figured that out ;) I deleted the member variable entirely, its not needed.

xlz · 2016-02-03T00:55:03Z

src/rgb_packet_processor.cpp

-  //std::ofstream file(name.str().c_str());
-  //file.write(reinterpret_cast<char *>(packet->jpeg_buffer), jpeg_buffer_length);
-  //file.close();
+  frame = new Frame(1, 1, 1920*1080*4);


Still need this:

frame->bytes_per_pixel = packet.jpeg_buffer_length;

1920*1080*4 is the size for the allocation, but not for the actual data.

brendandburns · 2016-02-03T00:57:19Z

thanks, sorry for all the little errors...

--brendan

Add a dump pipeline.

xlz · 2016-02-03T01:10:28Z

Build test OK.

ABI report looks OK.

Functions changes summary: 0 Removed, 0 Changed (5 filtered out), 3 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
Function symbols changes summary: 0 Removed, 0 Added function symbol not referenced by debug info
Variable symbols changes summary: 0 Removed, 3 Added variable symbols not referenced by debug info

3 Added functions:

  'method libfreenect2::DumpPacketPipeline::DumpPacketPipeline()'    {_ZN12libfreenect218DumpPacketPipelineC1Ev, aliases _ZN12libfreenect218DumpPacketPipelineC2Ev}
  'method virtual libfreenect2::DumpPacketPipeline::~DumpPacketPipeline(int)'    {_ZN12libfreenect218DumpPacketPipelineD0Ev}
    note that this adds a new entry to the vtable of class libfreenect2::DumpPacketPipeline
  'method virtual libfreenect2::DumpPacketPipeline::~DumpPacketPipeline(int)'    {_ZN12libfreenect218DumpPacketPipelineD1Ev, aliases _ZN12libfreenect218DumpPacketPipelineD2Ev}
    note that this adds a new entry to the vtable of class libfreenect2::DumpPacketPipeline

3 Added variable symbols not referenced by debug info:

  _ZTIN12libfreenect218DumpPacketPipelineE
  _ZTSN12libfreenect218DumpPacketPipelineE
  _ZTVN12libfreenect218DumpPacketPipelineE

Add a dump depth processor. Reactivate the RGB dump processor.

brendandburns · 2016-02-03T01:17:44Z

Awesome, thanks!

On Tue, Feb 2, 2016, 5:10 PM Lingzhu Xiang [email protected] wrote:

Merged #551 #551.

—
Reply to this email directly or view it on GitHub
#551 (comment).

xlz reviewed Feb 2, 2016
View reviewed changes

brendandburns force-pushed the master branch 2 times, most recently from 17ee696 to cf9ba02 Compare February 2, 2016 22:34

xlz reviewed Feb 2, 2016
View reviewed changes

brendandburns force-pushed the master branch from cf9ba02 to bc8c8d5 Compare February 2, 2016 23:02

xlz reviewed Feb 2, 2016
View reviewed changes

brendandburns force-pushed the master branch from bc8c8d5 to 1510925 Compare February 3, 2016 00:27

brendandburns force-pushed the master branch 3 times, most recently from 68fa3e3 to e0fd47c Compare February 3, 2016 00:49

xlz reviewed Feb 3, 2016
View reviewed changes

brendandburns force-pushed the master branch from e0fd47c to 35b94a6 Compare February 3, 2016 00:54

xlz reviewed Feb 3, 2016
View reviewed changes

brendandburns force-pushed the master branch from 35b94a6 to b063783 Compare February 3, 2016 00:57

Add a dump depth processor. Reactivate the RGB dump processor.

464a813

Add a dump pipeline.

brendandburns force-pushed the master branch from b063783 to 464a813 Compare February 3, 2016 01:06

xlz added a commit that referenced this pull request Feb 3, 2016

Merge pull request #551 from brendandburns/master

163b575

Add a dump depth processor. Reactivate the RGB dump processor.

xlz merged commit 163b575 into OpenKinect:master Feb 3, 2016

xlz mentioned this pull request Feb 3, 2016

test_opengl input data dump #169

Closed

brendandburns mentioned this pull request Feb 3, 2016

Add accessors for the various depth tables. #554

Merged

robozo mentioned this pull request Jul 23, 2016

Record the RGB and IR image from the libfreenect2 #691

Closed

		#endif // LIBFREENECT2_WITH_OPENCL_SUPPORT

		class DumpDepthPacketProcessor : public DepthPacketProcessor

Add a dump depth processor. Reactivate the RGB dump processor. #551

Add a dump depth processor. Reactivate the RGB dump processor. #551

Uh oh!

Conversation

brendandburns commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Feb 2, 2016

Uh oh!

xlz commented Feb 2, 2016

Uh oh!

brendandburns commented Feb 2, 2016

Uh oh!

xlz commented Feb 2, 2016

Uh oh!

floe commented Feb 2, 2016

Uh oh!

brendandburns commented Feb 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Feb 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xlz commented Feb 2, 2016

Uh oh!

brendandburns commented Feb 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Feb 3, 2016

Uh oh!

xlz commented Feb 3, 2016

Uh oh!

brendandburns commented Feb 3, 2016

Uh oh!

Uh oh!