Robust check for arrived color frame.

[hovren]

Previous implementation assumed a full frame was reached if the current transfer is
smaller than 0x4000.
This implementation instead sees if the data footer has arrived by checking the expected
length against the total buffer size, and that the sequence number agrees.
This should be more robust.

[hovren]

As noted in the comments, you could probably make this even more robust.
The footer contains some data which seems to be static, and could be used as some kind of magic marker.

[larshg]

Hi Hovren,

I tried your approach, but when I run it, it just fills up the buffer and hits [RgbPacketStreamParser::handleNewData] buffer overflow!

For some reasons it doesn't hit the "magic" position to match the footer and the header.

I'm running windows 7 x64 with VS2013 express

Regards, Lars

[hovren]

Unfortunately I don't have a Windows installation available to me, so I can't reproduce it.
It works consistently on my machine (Fedora 20, x64, GCC 4.8.3).

[hovren]

Looking more closely at the code, I can see that buffer overflows are not handled correctly.
When an overflow occurs you can never escape it. The old code could, because a non-full USB-transfer triggered a front buffer reset.

[larshg]

Hey Hovren,

Could you try out my branch here :
https://github.com/larshg/libfreenect2/tree/rgbPacketHandling

I tried to identify the start and end of the Jpeg image - and some other magic bytes.

[hovren]

I see some potential issues:

• Having to iterate over every received byte and doing single byte memcpy feels like it could be slower than necessary. I have no idea whether this is actually a problem.
• I am not sure only checking for "FF D9" is a good idea. Are we sure this sequence can never occur in the (JPEG) data stream without being a marker for EOI? I just skimmed the Wikipedia article, and although that seems to suggest that it shouldn't happen, I don't feel very confident about it.

I still feel that the most robust way should be to terminate on hasHeader && hasFooter. And for extra safety the sequence number in the header could be compared with the one in the footer.

[larshg]

Yeah, I see your points - but did it work at your end?

I know iterating through each byte seems cumbersome and hopefully we can change it, when we manage to get the full package description and perhaps can make use of the packageSize value to received atleast that, until making inspection from both ends to find both start and end sequences.

I noticed that the sequence "BBBB" and "9999" is repeating itself and might be magic numbers for start and end of subparts. Atleast I think that your RgbPacketFooter starts right after the "9999" and this might be the header? since it includes a package length of some sort?

I'm trying to make more "secure" markers than just "FF D9" but I haven't found it yet :)

And in the end we should be able to compare header and footer values to be able to safely return a complete package.

[hovren]

Yes, it worked for me as well.
The "9999" and "BBBB" strings seems to be constant for the (admittedly quite small amount of) RGB images I captured.

Between the "9999" marker and the JPEG "FF D9" marker I could only find bytes valued A5 or 5A.

So I guess the following to find the footer could work

1. Find 9999 in data. Simple (?) since this is always located at an exact offset from the end of the total data.
2. Iterate backwards through the data while the value is "5A" or "A5", and find "FF D9".

If we can assume that a single USB transfer never contains two different images, it should be safe to just accumulate data in the buffer and then apply the footer struct to the end of the currently accumulated data.
Since the previous code worked on that assumption this should probably be fine. I mean, the current code does work pretty much all the time. The only time it should fail is if the submitted frame happens to end on a USB transfer border, which shouldn't happen very often.

[larshg]

Ahh, nice.

I have maybe found some more information about the footer if you would try it out and see if its the same for you. I have made a pull request with the changes.

I don't like the if you talk about, as for my experience you cannot be sure how data is received with respect to the number of packages and a USB transfer won't have data from two consecutive images.

Just out of curiosity, what is your package size when receiving a single usb transfer? Mine is 16384 - is that equivalent on both windows and linux? Perhaps a USB specification for bulk transfers?

The amount of "5A" and "A5" varies from frame to frame at my end - I don't know if it is some padding?

I find it odd that the footer contains the package length, because if that was the header, you could identify the "9999" and find the length and then accumlate data until you have more or exact that value - to avoid inspecting each byte you mentioned earlier :)

[hovren]

Yes, it is 16384 for me as well. Although I am a bit confused, because if I read my lsusb output the maximum size for that endpoint should have been 1024 bytes and not 16384.

And yes I agree that making assumption that two frames won't mix in the same USB transfer seems dangerous. But, empirically this seems to be the case, and it also is not really surprising.

Why the package size is in the footer and not the header is beyond me as well. That's where I would have expected it.

[christiankerl]

Hi guys,

nice work. Could you please unify PR #71, #72 and #73 into a single PR?

The Kinect seems to use for RGB transfer the same approach as for the depth transfer. For the depth transfer there is also a footer containing a magic, packet length and other data. The DepthPacketStreamParser also searches through the incoming data for the magic and copies byte by byte. This was never a performance problem and should work for the RGB stream as well... Maybe you can use the DepthPacketStreamParser as "inspiration" for an enhanced RgbPacketStreamParser.

The packet size is not 1024 but computed according to some formula also involving the max burst value. You could check the USB3 spec ;-)

[larshg]

Maybe this shouldn't be posted here, but I get a awful lot of "errors" when running protonect as of now, from the depth parsing code:

And I'm trying to make a better parsing to avoid these problems. I don't know if there is a different way windows handle the USB transfers, but its not running optimally on my windows 7 x64 machine :)

[christiankerl]

did you build Protonect in Debug mode? Maybe switching to Release helps to improve performance?

[larshg]

I'm testing both debug and release - I think its more how windows works compared to linux... but its just a feeling... Also, on my machine the ISO packet size is 33792 bytes what is it on linux? I saw some update setting it to about 49000 or is this obsolete?

[christiankerl]

I'd close this in favor of #73 and #74, ok?

[xlz]

This PR has been subsumed by 221.