-
Notifications
You must be signed in to change notification settings - Fork 667
tga: decode 5-bit-per-primary images #2609
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
base: main
Are you sure you want to change the base?
Conversation
4d10dff
to
d8185b8
Compare
src/color.rs
Outdated
/// Pixel is 4-bit RGB with an alpha channel | ||
Rgba4, | ||
/// Pixel contains 5-bit R, G and B channels | ||
Rgb5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be better as Rgb5x1
or similar to indicate that each pixel is actually 16 bits rather than 15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but I should warn you that I think ExtendedColorType is starting to get stuck between two concepts: the in-memory layout of uncompressed arrays of pixel data (needed to extend ColorType), and the logical content of each numeric pixel value (which is just how many bits of float/uint each channel has, without stating an order or binary representation). The former is useful for direct access to uncompressed images; the latter for lossless conversions between image types (assuming compatible color representation information). I'd recommend splitting off the latter into a new type eventually.
src/codecs/tga/decoder.rs
Outdated
rawbuf.extend_from_slice(&buf[..num_raw_bytes]); | ||
|
||
self.expand_color_map(&rawbuf, buf, color_map)?; | ||
} else if self.original_color_type == Some(ExtendedColorType::Rgb5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth adding a comment here about needing this only if there's no color map, because of the pre-expand in the constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done.
d8185b8
to
d8a2100
Compare
This is very loosely based on a patch by red-001 <[email protected]>.
This is very loosely based on another PR to implement decoding of 5-bit-per-primary TGA images, #2303. That PR (also licensed
MIT OR Apache-2.0
) has not been updated in a bit more than a year, in which time there have been refactorings and other changes to the TGA decoder which would require additional work to resolve merge conflicts for. As a result, I expect it is unlikely that it will be updated this year, and am making this PR to try to have the feature implemented more quickly.Notes:
ExtendedColorType::Rgb5
enum entry to indicate the "type" of image data being provided. A precise description of the pixel format used by uncompressed true-color 5 bit-per-primary TGA images is DRM_FORMAT_XRGB1555, but I don't think going into such precision is appropriate for how ExtendedColorType is used.image
crate, specifically asMIT OR Apache-2.0
. You may also use them under CC0.Let me know if you'd like me to make any changes, split off the addition of ExtendedColor::Rgb5 into another commit, etc.