fix(bin): lcms2 panic on 16-bit PNGs with an ICC profile#32
Merged
Conversation
Transform<u16, u16> with PixelFormat::RGB_16 trips the lcms2-rs size assertion: RGB_16 is 6 bytes per pixel while size_of::<u16>() is 2, and only u8 slices are special-cased. Scoring any 16-bit PNG that embeds an ICC profile (including the bundled tank_distorted.png) panics. Transform [u16; 3] pixels instead, and add a regression test that runs the image command on the tank pair.
Contributor
|
Good catch! This should not have slipped through, especially considering it fails on our own test data... When reviewing the code, I notice that |
Review feedback: the code always operates on whole pixels, so extract_rgb_alpha_u16 now returns Vec<[u16; 3]> and the ICC transform uses it directly, with no flat buffer or re-chunking in between.
Contributor
Author
|
Done in 7835d2f — extract_rgb_alpha_u16 now returns Vec<[u16; 3]> and the ICC transform consumes it directly, no flat buffer or re-chunking left in the 16-bit path. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scoring any 16-bit PNG that embeds an ICC profile panics since #31:
This includes the bundled test pair:
ssimulacra2_rs image tank_source.png tank_distorted.pngcrashes at current HEAD (tank_distorted.pngis 16-bit RGB with an iCCP chunk).Cause:
apply_icc_transform_u16builds aTransform<u16, u16>withPixelFormat::RGB_16. lcms2-rs asserts that the element type size matches the pixel size (6 bytes for RGB_16); onlyu8slices are special-cased, which is why the 8-bit path works.Fix: transform
[u16; 3]pixels instead of bareu16.Includes a regression test that runs the
imagecommand on the tank pair and checks the score against--no-icc(the embedded profile is sRGB, so the conversion should be nearly neutral; observed delta is ~0.008).