Closed e00E closed 1 year ago
In addition to this, the Pixmap types should store [PremultipliedColorU8]
instead of [u8]
for more type safety. I didn't implement it here because it doesn't matter for correctness. It mostly involves changing some plumbing with bytemuck::cast_slice and deciding whether Pixmap should still have functions that return &[u8]
or whether those should change to &[PremultipliedColorU8]
making the user do the conversion with bytemuck.
So to clarify once more, this patch changes ColorU8
and PremultipliedColorU8
storage from u32
to [u8; 4]
. Which resolves u32
byte-order ambiguity? No need to pack and unpack bytes. No need to worry about CPU byte-order. It's always RGBA now. Right?
And I do not think it fixes #70, because this one is about the Pixmap storage, aka Vec<u8>
.
I believe it solves #70 because from what I'm seeing the Vec<u8>
never gets turned into the equivalent of a &[u32]
anymore, only &[[u8; 4]]
which is perfectly fine. (Unless there's some code path somewhere that still needs u32).
[u8]
to [u32]
being UB. The reason this is UB is that the types have different alignment requirements. [u32]
has stricter alignment requirements, which a cast isn't guaranteed to uphold. Now that Color is also [u8]
the alignment requirement is the same so the cast can't be UB.Which resolves
u32
byte-order ambiguity?
I was wrong about this part. The byte order was always correct. I misunderstood how the packing worked. When I tested I realized it was fine. Let me close that issue immediately. This PR is still useful with regard to that, because the new code is simpler. There is no reason to pack into u32.
@CryZe @e00E Wait, &[u32]
is UB, but &[[u8; 4]]
isn't?! I guess I misunderstood #70. I thought it was about casting to a "wider" type.
@e00E I like the new code more, since it's simpler. The fact it was confusing to you initially is worth the change.
It's because [u8; 4]
has an alignment of 1, whereas u32
has an alignment of 4 (on most platforms). So &[u8]
and &[[u8; 4]]
both have the same alignment of 1.
Yes, this what confused me. I thought that &[[u8; 4]]
has alignment of 4 as well...
Not my area of expertise.
This representation is more natural and leads to simpler code. Also fixes alignment issues when casting Pixmap::data into PremultipliedColorU8 which used to have alignment requirement 4 due to the u32 but now has alignment requirement 1.
I removed the
get
functions because I see no reason to make the internal representation part of the public api. The only guarantee we need to make is that bytemuck casting is in RGBA. Whether that comes fromu32
or[u8; 4]
is irrelevant.I've collected the following benchmarks on my machine (linux, x86-64, Amd Ryzen 7 3700x): master
my branch
There is no change in performance based on eyeballing it.
Fixes https://github.com/RazrFalcon/tiny-skia/issues/85 . Fixes https://github.com/RazrFalcon/tiny-skia/issues/70 . I assume this issue is fixed because of the mentioned alignment change. I couldn't run miri locally to confirm.