image icon indicating copy to clipboard operation
image copied to clipboard

feat: add PixelWithColorType for Luma<f32> and LumaA<f32>

Open strasdat opened this issue 2 years ago • 4 comments

This PR merely includes these two types:

  • Luma<f32>
  • LumaA<f32>

Motivating use-case:

image::flat::FlatSamples { ... }.as_view::<image::Luma<f32>>()

Single channel floating point images are pretty common, e.g. in computer vision. LumaA is mainly added for symmetry - but there are some more niche use-cases too (e.g. flow field representation).

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to choose either at their option.

strasdat avatar Jan 25 '23 07:01 strasdat

This adds ColorType::L32F and ColorType::La32F which were intentionally left out when we added 32-bit float support.

My specific concern is that every additional variant we add to ColorType adds complexity in a bunch of places: every user of ImageDecoder has to handle every possible variant, all implementations of ImageEncoder should support image data with any of them, we want a DynamicImage variant for each one, and we support pairwise conversions between every combination of ColorTypes

fintelia avatar Jan 25 '23 23:01 fintelia

Generally, yes, but I don't think this is one of the cases that adds complexity.

  • every user of ImageDecoder has to handle every possible variant: of extended color types, and this is something that the library should support with, i.e. provide functionality to decode into a reduced set and then do the automatic conversion.
  • all implementations of ImageEncoder should support image data with any of them: this is already not true, png does not support any of the floating point types and other non-alpha formats will error.
  • we want a DynamicImage variant for each one,
  • and we support pairwise conversions between every combination of ColorTypes: as by my comment above this is 'trivial' / already exists. It's not added complexity (in particular unlike the f64 other PR).

I think inaction here is worse than the additional implementation. In general, real world image formats diverge quite a bit from the neat rga-pixel-matrix thinking that we try to model for the Buffer use. And the difference is only going to get larger with HDR support where (non-destructive) mixing of different color spaces is a hard requirement. We don't cope with that at all. To be frank but slightly hyperbole, if we can't support this case in IO, we may just as well deprecate the entire library now and speed up the process of phasing it out.

HeroicKatora avatar Jan 26 '23 09:01 HeroicKatora

My comment above was perhaps too harsh. Thanks @strasdat for making the PR, the code itself looks good, and now we just have to think through API implications.

Right now, this library has major gaps when working with images that don't have a ColorType. I had thought we supported using ExtendedColorType when saving images, and converting between image formats, but apparently that was never implemented.

This evening I'll try writing up a proposal I think could make the situation much better

fintelia avatar Jan 26 '23 17:01 fintelia

@fintelia, @HeroicKatora - thanks for insightful discussion and feedback. My current use-case is unblocked by #1847, so no urgency from my end.

strasdat avatar Jan 28 '23 03:01 strasdat