Skip to content
New issue

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

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

Already on GitHub? # to your account

Always return little-endian packed value in Rgba8::to_u32 #100

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

tomcur
Copy link
Member

@tomcur tomcur commented Dec 17, 2024

(And PremulRgba8.)

If the intention is for the bytes of R, G, B and A to always be in the same memory order, I believe this patch is required. The docstring on the method suggests that's the intention.

In the order of memory of Rgba8::to_u8_array, the red channel is at the lowest memory address and therefore (in little endian) the least significant byte. On a big endian system, using from_le_bytes would move the red byte to the highest memory address.

If the intention is for the bytes of r, g, b and a to always be in the
same memory order, I believe this patch is required.

In the order of memory of `to_u8_array`, the red channel is at the
lowest memory address and therefore (in little endian) the least
significant byte. On a big endian system, using `from_le_bytes` would
move the red byte to the highest memory addres.
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

This function has gone through all three of these functions in the last couple of weeks.

We really need some miri testing for a big-endian system...

I think... that I agree that this is right.

@tomcur
Copy link
Member Author

tomcur commented Dec 17, 2024

The invocation

$ cargo miri test --target s390x-unknown-linux-gnu --features bytemuck -- rgba8::

fails on main:

running 2 tests
test rgba8::tests::bytemuck_to_u32 ... FAILED
test rgba8::tests::to_u32 ... FAILED

failures:

---- rgba8::tests::bytemuck_to_u32 stdout ----
thread 'rgba8::tests::bytemuck_to_u32' panicked at color/src/rgba8.rs:139:9:
assertion `left == right` failed
  left: 67305985
 right: 16909060
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect

---- rgba8::tests::to_u32 stdout ----
thread 'rgba8::tests::to_u32' panicked at color/src/rgba8.rs:119:9:
assertion `left == right` failed
  left: 16909060
 right: 67305985

It passes with this patch.

@waywardmonkeys
Copy link
Collaborator

I've done a PR with Miri in CI in #101 ...

Copy link
Collaborator

@waywardmonkeys waywardmonkeys left a comment

Choose a reason for hiding this comment

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

Okay ... and we'll use Miri in CI to keep things right going forward!

@waywardmonkeys
Copy link
Collaborator

Going to merge this now so that I can perhaps do the release now. (Breaking the usual policy...)

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Dec 17, 2024
Merged via the queue into linebender:main with commit 5eac8af Dec 17, 2024
15 checks passed
@waywardmonkeys waywardmonkeys added this to the 0.2.0 milestone Dec 17, 2024
@tomcur tomcur deleted the little-endian branch January 7, 2025 10:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants