Skip to content

Improve RGB and depth stream parsers #221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 20, 2015
Merged

Improve RGB and depth stream parsers #221

merged 3 commits into from
May 20, 2015

Conversation

xlz
Copy link
Member

@xlz xlz commented May 4, 2015

In the RGB stream parser, add an extra check using information the RGB packet footer revealed by new protocol analysis to detect JPEG EOI marker. Then use the marker's position to derive a correct JPEG length for some hardware JPEG decoders that can't handle garbage after the end of JPEG image.

In the depth stream parser, remove the scanning of a 8-byte magic footer which is very unreliable (the particular 8 bytes may appear in the middle) and inefficient. Based on analysis of the isochronous transfer protocol, assume packets have fixed size and the footer always resides at the end of a fixed sized packet. The changes made are minimized, and the execution order in the original parser is preserved, including the order of memcpy's and subsequence/sequence assembling order.

The code has been tested on Linux and Windows.

@christiankerl
Copy link
Contributor

Nice +1 i guess this subsumes the other packet parsing related PRS? However, timestamps are added but not passed through parsers/processors or did I miss something? Could you fix this otherwise?

@xlz
Copy link
Member Author

xlz commented May 4, 2015

@christiankerl
I have reorganized the commits so that there is a standalone commit to fully deal with timestamps and sequence numbers.

The RGB commit subsumes #72 and #73. The depth commit subsumes #75. The timestamp/sequence commit subsumes #71 and #148.

This was referenced May 5, 2015
@larshg
Copy link
Contributor

larshg commented May 5, 2015

This PR works fine at my end 👍

Only cosmetic is to remove the old comment?

@larshg larshg mentioned this pull request May 5, 2015
@hovren
Copy link

hovren commented May 8, 2015

I did a quick check, and it seems to be working fine on my end as well (Linux). 👍

@floe
Copy link
Contributor

floe commented May 12, 2015

Also working fine for me (Ubuntu 14.04). @christiankerl since this mostly touches your code, could you do the merge?

@xlz
Copy link
Member Author

xlz commented May 13, 2015

Merge conflicts from #227 which changes cosmetic log strings. To fix later.

@floe
Copy link
Contributor

floe commented May 13, 2015

Sorry, didn't realize that - should I revert #227?

xlz added 3 commits May 13, 2015 09:37
Inspect the magic markers at the end of a JPEG frame
and match the sequence number and length.
Find out the exact size of the JPEG image for decoders
that can't handle garbage after JPEG EOI.
Remove magic footer scanning: may appear in the middle.
Assume fixed packet size.
Pass timestamps and sequence numbers from {rgb,depth} stream
processors to turbojpeg rgb processor and {cpu,opengl,opencl}
depth processors, then to rgb and depth frames.

This commit subsumes PR #71 by @hovren and #148 by @MasWag.
@xlz
Copy link
Member Author

xlz commented May 13, 2015

No. Fixed conflicts.

@kohrt
Copy link
Contributor

kohrt commented May 20, 2015

I just tested this pull request and it also solves the issue of restarting the device.

@floe
Copy link
Contributor

floe commented May 20, 2015

OK, sounds like we should integrate this now. I'll give it until today in the evening for any last-minute objections, then I'll merge it.

floe added a commit that referenced this pull request May 20, 2015
Improve RGB and depth stream parsers
@floe floe merged commit ef34cfc into OpenKinect:master May 20, 2015
@xlz xlz deleted the stream-parsers branch August 22, 2015 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants