Rgb packet handling

[larshg]

A try at getting a better rgb packet handling with inspiration from hovren's pull request: #72

Its still a work in progress, but I wouldn't mind any feedback on the way 👍

[larshg]

Hi @christiankerl

I have tried to unify the pull request #71 #72 and #73 within this one.

The timestamp is only implemented for the rgb parser in this one, but I have made a similar updated request for the depth_packet_parser in #75 .

Feedback would be appreciated.

[christiankerl]

rename to magic_header

[xlz]

First, can you please squash these into a single commit? The second one is cosmetic.

As a matter of fact, a custom hardware accelerated JPEG decoder I'm testing cannot handle garbage after JPEG. So this extra check is actually required for compatibility.

My experiments show your unknown1 is the length of a filler before the footer. Some new specifications:

// starting from JPEG EOI: 0xff 0xd9
// char _pad_0xa5[]; //0-3 bytes alignment of 0xa5
// char _filler[_filler_length] = "ZZZZ...";
LIBFREENECT2_PACK(struct RgbPacketFooter {
    uint32_t magic_header; // is '9999' equal 0x39393939
    uint32_t sequence;
    int32_t unknown1; // _filler_length
[...]

Also, version is shown to be always 0x3f800000 (1.0f). It's likely a float number of certain meaning, but it is probably not representing a version number. The rest of the unknown* fields are also shown to be zeros in all my experiments. Maybe you can rename the unknown* fields to be reserved* because they are not exactly unknown.

I wrote and tested some code to find out the exact size of the JPEG image by searching for its EOI. For your reference. Following the new specifications the code will finish in finite steps and if EOI is not found, returning failure is justified. In my testing it always worked.

[christiankerl]

@xlz would it suffice to compute the correct jpeg_buffer_length using the filler_length?

@larshg can you rename unknown1 accordingly?

[christiankerl]

This copies all remaining data to the fb. I guess it would be more correct to just copy i+sizeof(footer) and copy any remaining data to the new fb after the swap?

[xlz]

@christiankerl Yes.

Here is my take (edit: updated commit) to compute the exact value of jpeg_buffer_length using filler_length.

Several changes:

• Return immediate after buffer overflow because the next JPEG packet will be incomplete anyway.
• Do not scan through every single byte checking for magic bytes. This is much more costly than a memcpy. The current implementation and the lack of reports about buffer overflow and image corruption show it is safe to assume the footer arrives at the very end of a data transfer, and there is no more data after the footer within a data transfer.

[xlz]

~~Changes from v1:~~
- ~~Corrected size calculation about bytes before JPEG~~
- ~~Tested on Linux~~

~~This is ready for cherry picking.~~

[larshg]

Nice suggestions for improvements @xlz. I have tried to incorporate most of them - Still lacking to calculate the exact jpeg buffer size with the 0-3 padding bytes, but I'll try look at it tomorrow.

I have removed the for loop, but this requires the assumption that there will be no concatenated images in the USB transfers - which I'm not qualified to determine if it can or will happen? If it does, we will just hit bufferoverflow and reset buffers.

Over 1000 runs in the onDataReceived method, I tried with and without a for loop and I measured difference from 160 ms to 1.6 ms in processing time - so indeed it is more costly, but difference compared to the overall processing is minimal.
But if there is no reason to do it (more secure, removes doubt of the assumption of concatenated images), we might as well remove it?

[xlz]

In fact you can totally detect the magic bytes in the middle of an image. There is no guarantee these 8 bytes won't appear. If our assumption does not hold, report of buffer overflows will immediately show up.

(It is similar for depth processing which is also doing scanning currently. I'll put on my patch shortly.)

On the side of performance, on ARM platform the CPU is significantly weaker for these kind of memory scanning.

[xlz]

What's the status of these two stream processor PR #73 #75 ?

[larshg]

Closing in favor of #221 .