Skip to content
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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

strasdat
Copy link
Contributor

@strasdat strasdat commented Jan 25, 2023

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.

Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why not. It seems all conversion to make this work already exists and agree with the color type as indicated in the impl.

@fintelia
Copy link
Contributor

fintelia commented Jan 25, 2023

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

@HeroicKatora
Copy link
Member

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.

@fintelia
Copy link
Contributor

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

@strasdat
Copy link
Contributor Author

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

@@ -26,6 +26,10 @@ pub enum ColorType {
/// Pixel is 16-bit RGBA
Rgba16,

/// Pixel is 8-bit luminance
Copy link

@LDeakin LDeakin Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8-bit -> 32-bit. And below as well

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.

None yet

4 participants