Add L32F and La32F to ColorType and DynamicImage#2934
Conversation
RunDevelopment
left a comment
There was a problem hiding this comment.
Great stuff!
I really like the symmetry between color types this creates. I'm also surprised by how little code you needed to change to make this happen. I expected it to be much more.
This fills out the ColorType enum to handle a complete array of color and channel type combinations, and implements the matching updates to DynamicImage and all operations on it. Now operations changing the color type, like DynamicImage::grayscale, will always produce grayscale data, and it will be easier for TIFF, EXR, and other image formats' decoders to produce grayscale float data.
fintelia
left a comment
There was a problem hiding this comment.
If you're going to relitigate intentional design decisions that we previously made (even citing the previous thread where I stated that these variants intentionally left off!) then please start with a issue thread explaining what has changed/why we should reconsider, rather than jumping straight to a PR.
|
@fintelia Could you please state the intentions/reasons behind this design decision? The previous comment you referred to only states that this decision exists, not why. |
|
What definitely did surface since then is that, while implementing color conversion, it was noted that |
|
Expanding on the above, concretely it was impossible to add The design decision I remember is to use a small subset of variants we can get away with as informed by combinations occurring real world image files. We also have those here now or in the forseeable future: unassociated alpha, uncolored image data, 3d-depth data, and more scientific files (tiff but also JPEGXL) support 24-bit or 32-bit colors in floating point. We additional made another design choice of performing color conversion when loading into |
This adds new L32F and La32F ColorType entries.
My motivation here is mainly to make it easier to decode image formats that can contain L32F pixels (TIFF, EXR, and perhaps PFM), but this change also the nice benefit of making DynamicImage's subformats form a grid of pixel dimensions and subpixel type, so that operations like DynamicImage::grayscale reliably produce a Luma or LumaA image, and we could later add methods like
to_32fthat affect the subpixel type but not the pixel component count.See also a previous attempt at this in #1846, and the recent change adding L32F and La32F ExtendedColorTypes in #2793.
There is a lot of casework involved in this MR; I think I've updated or added every function that needs adding, but it's not impossible that I missed something.