Encoder supported colors

[RunDevelopment]

Resolves #2990

Changes:

• Add ImageEncoder::supported_colors.
• Remove make_compatible_img
• Add make_compatible_img util method based on supported_colors
• Add tests to verify encoders implement ImageEncoder::supported_colors correctly.

I quickly implemented my proposal. Basically, instead of encoders implementing how to convert a dynamic image to something compatible, they now declare what is compatible. This makes it possible to have a unified conversion function for all encoders.

Open questions:

• Should all encoders implement ImageEncoder::supported_colors? Right now, I only implemented it for the encoders that previously implemented make_compatible_img to get equivalent behavior.
• The new make_compatible_img is potentially quite heavy in terms of code size. It needs the full matrix of conversions between color types and is not generic. Now, this might sound really bad, but it's not that bad. The 10x10 color types quickly map to CicpRgb::cast_pixels_by_layout, which is generic over the to and from sub-pixel type. So there are only 3x3 heavy functions being monomorphized. Still, not necessarily super good.

[197g]

It needs the full matrix of conversions between color types and is not generic. Now, this might sound really bad, but it's not that bad.

I would say it is bad considering the use of ExtendedColorType here (which at the same time I consider necessary for a public interface though). So really it would bloom to a much larger table than this. Now there is of course the same sort of sketch that CICP conversion does which runs N+M conversion through a common middle (with an analogy to ICC's profile connection space) plus a constant amount of specialized cases on top. However, I also do not think this should happen before we have the canvas / buffer implemented for those types—i.e. before we can dogfood our own use of these interfaces with library types out of a lack of testing and many more.

That said, my bigger concern is the interface and conversion algorithm being underspecified. Conversions between two of the same depths is not lossless if it requires changing color space in the process for instance. So, is a list of possible color models really enough information to decide on a conversion sequence in the general case? I'm not against changing the internal interface so much as making it public is not motivated strongly enough for me.

[RunDevelopment]

I would say it is bad considering the use of ExtendedColorType here (which at the same time I consider necessary for a public interface though).

I'm not really sure why you say any of this. The current conversion is for DynamicImage, so the fact that ExtendedColorType can represent more color layouts than DynamicImage seems irrelevant to me. This is also why the conversion matrix can't grow anymore unless DynamicImage itself grows.

After all, I'm not suggesting expanding these conversions to cover all save/write methods (e.g. the ones on ImageBuffer) in this PR. I think this would be useful and this PR creates the necessary foundation, but I simply haven't put much thought into it yet.

That said, my bigger concern is the interface and conversion algorithm being underspecified.

It seems to me that you are conflating the interface (ImageEncoder::supported_colors) and one use case of it (conversions).

I see ImageEncoder::supported_colors as its own thing. It's basically just the encoder saying "this is what I can do" and reporting information about itself (similar to ImageDecoder::format_attributes). After all, it is necessary to know what colors an encoder supports to use it.

Conversions between two of the same depths is not lossless if it requires changing color space in the process for instance. So, is a list of possible color models really enough information to decide on a conversion sequence in the general case?

I think lossy conversions and loss of color space information aren't a huge issue here. ImageEncoder doesn't consider color space when encoding anyway. (Although it probably should... Idk enough about color spaces and how formats can represent them to make decisions on that though.)

That said, if they are an issue, we can also just not do them. Conversions are best effort and refusing to perform non-trivial conversions (like RGB -> gray) seems fine to me.

[RunDevelopment]

Alrighty. I redid the color conversion.

Since ColorType is now the product $\set{RGB,Luma} \times \set{Alpha,NoAlpha} \times \set{U8,U16,F32}$, I defined that the conversion only touches alpha and precision. I.e. it is allowed to add/remove alpha and change precision. Importantly, these conversions do not change the underlying color space, only the color format. Conversions Rgb <-> Luma are not allowed. This behavior is consistent with the previous implementations.

(We may want to allow Luma -> Rgb, but I don't know enough about color spaces to judge if this makes sense.)

I avoided code size bloat by separating the conversion into 2 passes: alpha and precision. Having 2 phases also means that an intermediate image will be created if both alpha and precision need to be changed. I believe this is acceptable in terms of performance hit.

I wanted to avoid ImageBuffer::convert for code size reasons, so I added a new method: ImageBuffer::convert_precision. This works the same as convert but restricts the target pixel type to have the same color format as the current pixel type. This makes it possible to outline the conversion loop and share it between all pixel types that convert between the same precisions. (Honestly, this should have been an optimizations within convert itself, but that requires specialization.)

Since precision conversion on its own might be a useful feature for users too, I implement it as (internal) methods on DynamicImage. We can decide whether we want to expose them later.

---

I believe that this should address the main concerns raised by @197g (if I understood them correctly).

I also want to speak a little about motivation.
My main issue with color conversions in DynamicImage::save and co being accessible only via an internal API is that it creates two tiers of formats: First class built-in formats and second-class hook formats. The reason color conversion for writing DynamicImages was added in the first place is that using our decoders in a generic way is pretty annoying. The same image might encode fine with PNG but fail for JPEG. And of course, which color types are supported typically isn't documented. So having color conversions when encoding with generic encoders isn't just convenient, it's necessary. Having this behind an internal API systematically disadvantages third-party implementations and I don't like that.