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

Make rgb565 conversion algorithms a bit better #95

Merged
merged 9 commits into from Jan 25, 2022
Merged

Make rgb565 conversion algorithms a bit better #95

merged 9 commits into from Jan 25, 2022

Conversation

LoganDark
Copy link
Contributor

@LoganDark LoganDark commented Jan 24, 2022

The old algorithm has some unpleasant tinting. Native components [0xFF, 0xFF] don't result in rgb components [0xFF, 0xFF, 0xFF] but rather [0xF8, 0xFC, 0xF8]. This affects screenshots for example.

Obviously some fudging is needed in order to come up with the "extra resolution" in rgb8 to make the mins and maxes match up. Roundtripping from 565 -> 888 -> 565 is still a thing. And now it's a part of the test suite, which verifies that every rgb565 value can be roundtripped to rgb8 and back. And that rgb565 -> rgb8 conversions behave properly.

The new conversion routines still only use integer arithmetic. I haven't benchmarked them but they should actually be slightly faster than the old routines.

Note that I tested these on my desktop machine which required #92 in order to get libremarkable to build. Just a preview of how useful that can be.

@LoganDark
Copy link
Contributor Author

LoganDark commented Jan 24, 2022

This actually fixes a bug in color::to_rgb8 that results in r and b being swapped (I think?)

In any case everything works now and everything interacts perfectly with the simulator I've set up...

  1. builtin colors look correct
  2. round tripping works
  3. color::WHITE shows as #FFFFFF white
  4. color blending on the buffer is correct

Copy link
Collaborator

@bkirwi bkirwi left a comment

Choose a reason for hiding this comment

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

Solid improvement, and thanks for the tests!

Haven't heard of any issues building on desktop before... which dep failed to build / any idea why?

src/framebuffer/common.rs Show resolved Hide resolved
@bkirwi bkirwi closed this Jan 24, 2022
@bkirwi bkirwi reopened this Jan 24, 2022
@LoganDark
Copy link
Contributor Author

LoganDark commented Jan 24, 2022

I had issues building evdev and epoll since I was cross-compiling for Windows (WSL 1 - I don't like VMs - no graphics support). #92 adds the ability to exclude those from the build. Although it looks like nobody's wanted to touch it for a while - now that I depend on it, I need it merged at some point but I fear that it's too big/complicated of a change now.

I've been testing this by merging it into #92 on my local checkout and then running cargo test --target x86_64-unknown-linux-gnu --package libremarkable --lib framebuffer::common::rgb565_conversions --no-default-features --features framebuffer-types -- --exact. It's a mouthful but it works!

There's also the visual test of running my app on #92 with and without this PR. Without, color blending is totally scuffed, but with, it works fine. It's literally a case where color::RGB(255, 0, 0).to_rgb8() will return 0, 0, 255 on the old version

@bkirwi
Copy link
Collaborator

bkirwi commented Jan 24, 2022

I had issues building evdev and epoll since I was cross-compiling for Windows (WSL 1 - I don't like VMs - no graphics support).

Make sense - thanks!

@LoganDark
Copy link
Contributor Author

I added tests for the specific colors, and I'm 100% sure that red/green were swapped. I'm also 100% sure that the endianness is CORRECT this time - both in terms of the byte order of the u16 and the order of the components in that u16. Test passes. Phew........

@LoganDark
Copy link
Contributor Author

You should probably squash all these commits when you merge.

@LoganDark

This comment has been minimized.

@LoganDark LoganDark marked this pull request as draft January 25, 2022 01:25
@LoganDark LoganDark marked this pull request as ready for review January 25, 2022 01:58
@LoganDark
Copy link
Contributor Author

Just for the record the endianness is now correct and proven to work correctly on-device. Since the encoding and decoding algorithms match, I was able to run screenshot with #96 to get this beauty:

image

to prove beyond a shadow of a doubt that YES, we understand the rgb565_le format correctly. ([gggbbbbb, rrrrrggg])

For those following along at home, xochitl only writes color information to the framebuffer if screenshare is turned on.

@bkirwi bkirwi merged commit b146c39 into canselcik:master Jan 25, 2022
@fenollp
Copy link
Collaborator

fenollp commented Jan 25, 2022

@bkirwi Next time maybe select the "Squash and merge" option?

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.

None yet

3 participants