Skip to content
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

Support YUY2-encoded images #4877

Merged
merged 17 commits into from
Jan 25, 2024
Merged

Support YUY2-encoded images #4877

merged 17 commits into from
Jan 25, 2024

Conversation

oxkitsune
Copy link
Contributor

@oxkitsune oxkitsune commented Jan 20, 2024

What

This PR Introduces support for YUV422/YUY2 encoded images based on #3541.

Image showcasing RGB, NV12 and YUV422 encoded images

The raw (encoded) image data is stored in an R8Uint texture. The contents of the texture get decoded in the rectangle_fs.wgsl via the decoder written in decodings.wgsl.

Other YUV image formats could be added easily, following the NV12 implementation with slight modifications to the decoder.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@oxkitsune
Copy link
Contributor Author

I want to improve the rust api for logging images with a different encoding than the traditional rgb and resolve #4820 in the process, but I'm not sure what the best approach would be for this.

One thought is to implement some helper methods on TensorData e.g. TensorData::from_yuv422. WDYT?

@oxkitsune oxkitsune force-pushed the gdjeong/yuv422 branch 2 times, most recently from 166a90c to abbb72a Compare January 20, 2024 14:22
@abey79 abey79 added 🔺 re_renderer affects re_renderer itself include in changelog labels Jan 20, 2024
@oxkitsune oxkitsune marked this pull request as ready for review January 23, 2024 01:12
@Wumpf Wumpf self-requested a review January 23, 2024 19:39
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Can go in as-is once we figured out the CI kinks, if you want I can take care of it.

The only wrinkle here is that we nowhere specify that this is the YUY2 variant (YuYv instead of uYvY) for the format. I don't know how common the other one is, but https://en.wikipedia.org/wiki/YCbCr#4:2:2 mentions both.

Also, just like with NV12 before we don't have these documented anywhere. I tried to find a nice spot to put doctext in the fbs files, but codegen isn't picking that up yet, so we'll have to do this as a follow-up.
It's also a bit sad how many places needs to be touched to get this going, something to re-conciliate for us in the future.
That said, as an extension to status quo this looks perfect 👌

crates/re_renderer/shader/decodings.wgsl Show resolved Hide resolved
@oxkitsune
Copy link
Contributor Author

Nice work! Can go in as-is once we figured out the CI kinks, if you want I can take care of it.

I'll sort it asap, I want to get my local environment set up so I can easily fix CI stuff in the future, seems like my python formatter is producing some different output.

The only wrinkle here is that we nowhere specify that this is the YUY2 variant (YuYv instead of uYvY) for the format. I don't know how common the other one is, but en.wikipedia.org/wiki/YCbCr#4:2:2 mentions both.

Yea, @YukumoHunter also mentioned this. The gist linked in #3541 specifically used YUV422, so I based it on that. I think renaming it to YUY2 over YUV422 might be better? WDYT?

@Wumpf
Copy link
Member

Wumpf commented Jan 24, 2024

Hmm yeah let's rename it to YUY2 everywhere to proof against adding the other variant in the future. Thank you!
Might be good to document that in the fbs file where the format is "first" mentioned already despite that not showing up in generated code just yet

@oxkitsune oxkitsune changed the title Support YUV422-encoded images Support YUY2-encoded images Jan 24, 2024
@oxkitsune
Copy link
Contributor Author

Alright, I've renamed it to YUY2 and @YukumoHunter cleaned up the colour space conversions. I think this is ready now 👍

@Wumpf Wumpf self-requested a review January 25, 2024 08:54
} else {
// we're on an odd pixel, so we need to sample the second y value
// we add 2 to the column to get the second y value
y = f32(textureLoad(texture, vec2u(uv_col + 2u, uv_row), 0).r);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't notice either at first, but this offset is wrong actually. We already muliplied the uv_col with 2, so it points at the Y value right now. I found out when adding yuy2 to the (former) nv12 python test script which also highlighted that there was still some stuff missing in ImageEncoded

To illustrate:
With that offset:
image

Without that offset:
image

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work and extra thanks for further cleaning up the conversion code both in wgsl and Rust!

When trying to test this I found that extension for ImageFormat etc. was still missing and once I added that it looked a bit off, highlighting that there was a small mistake in the offset calculation for Y.
I think it's all good now and ready to go, but can you please confirm first with your own dataset that this checks out?

@oxkitsune
Copy link
Contributor Author

image Just tested it out, and it seems to be working nicely! Thanks!

@oxkitsune
Copy link
Contributor Author

On closer look, seems like there is a small issue though. Might be an issue with our dataset though, I'll quickly test this with live data when I get to the lab in a few hours.

image

@Wumpf
Copy link
Member

Wumpf commented Jan 25, 2024

Hmm looks like the row is offset every second pixel or so, strange!
btw. worth pointing out that the test image I used was a quasi-manual conversion since opencv didn't offer it, see here https://github.com/rerun-io/rerun/pull/4877/files#diff-3d8a8e06114b0f435bfd964a239e55fa6d7f8ef6308276e632a36d5f62af03e5R28

@Wumpf
Copy link
Member

Wumpf commented Jan 25, 2024

not entirely sure what's up with the python wheel builds, but it's obviously not this pr's fault. Things work for me locally so what could possibly go wrong. Time to merge!

@Wumpf Wumpf merged commit 50671b2 into rerun-io:main Jan 25, 2024
32 of 35 checks passed
jleibs added a commit that referenced this pull request Feb 7, 2024
… and avoid crash on chrome (#3931) (#5074)

### What
When we added support for YUY2 format in
(#4877) we we re-triggered the
issue originally originally reported in
#3931 and now
#5073
- This is a more extreme version of:
#3948 which was the workaround the
first time we hit this.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5074/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5074/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5074/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/5074)
- [Docs
preview](https://rerun.io/preview/e35e3a58e1f2093140d4d63479098124f6556463/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/e35e3a58e1f2093140d4d63479098124f6556463/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants