Skip to content

Encoder supported colors#2992

Open
RunDevelopment wants to merge 4 commits into
image-rs:mainfrom
RunDevelopment:encoder-supported-colors
Open

Encoder supported colors#2992
RunDevelopment wants to merge 4 commits into
image-rs:mainfrom
RunDevelopment:encoder-supported-colors

Conversation

@RunDevelopment
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member

197g commented May 27, 2026

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
Copy link
Copy Markdown
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add encoder API to get support color formats

2 participants