Add Rgb(a)32F DynamicImage Variant

[johannesvollmer]

Discussion wanted

Adds F32 support to the DynamicImage.

Points to be discussed:

• grayscale(image) returns Luma<T>, but Luma<f32> isn't a supported color type, so... f32 images will panic?
• impl GenericImage for DynamicImage { type SubPixel = u8; } is difficult for float images, as it is unclear how f32 colors should be converted to u8 colors. Maybe the default subpixel type should be f32 for best quality, instead of u8?
• color.invert() result is unclear when the color contains f32 values, because the max_value of a float color component may not be known
• implement Eq & Hash for dynamic image by declaring all NaN values to be equal? I don't think so.
• implementing f32 support for trait Pixel:
  - for conversions like Rgb<u8>::from(Rgb<f32>), we must assume the float range to be 0..1. Tonemapping is definitely too much for this.

Todo:

1. Merge basic f32 support from OpenEXR pull request into next?
2. impl Pixel for Rgba<f32>
3. Add version field to the new deprecation attributes

(couldn't resist to add other changes as well, they are of course to be discussed)

[johannesvollmer]

@fintelia @HeroicKatora

Here's a first draft. Would you like to discuss?

These are the most important limitations of this approach concerning DynamicImage:

• DynamicImage cannot implement Eq and Hash anymore because f32 doesn't want to implement that
• Luma<f32> and LumaA<f32> implement Pixel, but do not have a ColorType and there is no Luma32FImage. [Edit:] Therefore, in fn grayscale, we cannot return Luma, but we return a desaturated Rgb32FImage in that case.

These are the most important limitations of this approach concerning Pixel:

• Major Breaking Change: Pixel::Subpixel now requires Sample instead of Primitive
• Assumes white means 1.0 for f32, otherwise all the color traits cannot be implemented for f32
• [Edit:] ColorTypenow isResult<ColorType, &str>` to represent unsupported color types
• Does not scale very well in case we want to add more colors, because each color needs a const ColorType in trait Sample
• Why is downcast_bitdepth_early important?

Also, I could not resist doing some deprecation progress and code deduplication (boyscout rule).

Furthermore,

• Some algorithms need more testing to make sure that f32 works with those (for example colorops). I don't know the inner workings of these algorithms though
• I need help on how to make that failing test pass ("public private dependencies"). Seems like the dependency can not be found, even though it works on my local machine [Edit: I made it pass, it was a missing dependency].

[johannesvollmer]

So, can you do another review?
@fintelia @HeroicKatora

Something blocking this?

[HeroicKatora]

Something blocking this?

Yeah, finding time for careful deliberation on the maintainability of the interface which I finally did. I'm pretty content with most of these changes. There's maybe two things that I find slightly annoying from an user perspective:

• Is the Primitive trait useful any more? We do control both of these traits after all. Due to the changes to Pixel everyone needs to switch to Sample anyways. The churn could be far less if we'd merge those two traits and just change Primitive.

[johannesvollmer]

Glad that you took the time :)

Is the Primitive trait useful any more? We do control both of these traits after all. Due to the changes to Pixel everyone needs to switch to Sample anyways. The churn could be far less if we'd merge those two traits and just change Primitive

Yes, why not. However, I think Sample would be the best name for the merged trait. It could be confusing for downstream users to find out that the generic sounding Primitive trait contains highly color-related content. I'd favor merging the traits, given that the name of the merged trait is not confusing.

[johannesvollmer]

Considering all the discussions about renaming and splitting, what do you think of the following PixelComponent definition?

/// The type of each channel in a pixel. For example, this can be `u8`, `u16`, `f32`.
// TODO rename to `PixelComponent`
pub trait Primitive: Copy + NumCast + Num + PartialOrd<Self> + Clone + Bounded {
    const DEFAULT_MAX_COMPONENT_VALUE: Self = usize::MAX;
}

/* ... */

/// The type of each channel in a pixel, with an associated color type.
pub trait PixelComponentWithColorType: Primitive + 'static + private::Sealed {
    const RGB_COLOR_TYPE: ColorTypeOrErr;
    const RGBA_COLOR_TYPE: ColorTypeOrErr;
    const L_COLOR_TYPE: ColorTypeOrErr;
    const LA_COLOR_TYPE: ColorTypeOrErr;
}

We can rename Primitive to PixelComponent in a follow-up pull request

[HeroicKatora]

The main problem seems to be the associated ColorType constant, in combination with the impls on Luma*. The color type just is not a property of the component, what's stopping us from getting rid of it? I would move that to an extension trait that we selectively implement in Luma*, Rgb* etc.

[HeroicKatora]

So more concretely I would suppose something like this:

/// The type of each channel in a pixel. For example, this can be `u8`, `u16`, `f32`.
// TODO rename to `PixelComponent`
pub trait Primitive: Copy + NumCast + Num + PartialOrd<Self> + Clone + Bounded {
    const DEFAULT_MAX_COMPONENT_VALUE: Self = usize::MAX;
}

pub trait Pixel {
}

pub trait ColoredPixel: Pixel {
    const COLOR_TYPE: ColorType;
}

[johannesvollmer]

I proposed such a solution once. It's one of the cleanest solutions I had come up with. I don't recall why we decided against it at that time, but I think it's a great solution. I'll go on and push some changes with this strategy next

[fintelia]

I think the PR is in pretty good shape at this point

[HeroicKatora]

+1 from as well, with my one outstanding nit and @fintelia 's review resolved. The CI should be fixed if you merge or rebase to the current master .

[johannesvollmer]

The tests pass on my machine. Nearly all the checks in the workflow fail immediately though. For example, here is one of the errors:

error: failed to select a version for the requirement `ravif = "^0.7.0"`
candidate versions found which didn't match: 0.8.7, 0.8.4, 0.8.3, ...
location searched: crates.io index
required by package `image v0.24.0-alpha (/home/runner/work/image/image)`
Error: The operation was canceled.

Now doing a clean compile on my machine....

[HeroicKatora]

There was an upgrade to ravif = 0.8 in the main repository, since every 0.7.* version is yanked. Should be fixed by a merge (or a rebase) with the current master branch.

[johannesvollmer]

This pr is against the next branch. Wouldn't it be cleaner to merge that change from master into next, and then from next into this pr?

Merging from master to this branch may break the pr, I fear, as it might undo changes made in the next branch, and I don't feel qualified to perform that merge.

[HeroicKatora]

We had merged next into master a short time ago, actually.

[johannesvollmer]

Then why do I have to solve merge conflict in files that this pr didn't touch, when merging upstream/master into this pr?

I have no idea how to resolve this (and many other) conflicts:

[johannesvollmer]

I merged your master into this pr by hand and to my surprise it still compiles and the tests still run. However, I can't push it, as merging master into this pr modifies .github/workflows/rust.yml, which I don't have access to, so the push is rejected.

I don't think there is any way for this pr to merge, other than you merging master into next. The difference between master and next is more than 40 files, next is 43 commits ahead and 112 commits behind master. I can't see how it would be a clean solution to merge master into this pr with such a big difference between next and master.

[fintelia]

I think we're at the stage were we should probably be merging next into master, potentially with one final 0.23.x release first. @HeroicKatora do you have thoughts?

[HeroicKatora]

LGTM then. I'll have a look resolving the merge madness here if it requires maintainer priviledges. @fintelia Unfortunately the master branch has already diverged from the msrv/stability promises so no easy final release. We could make a backport branch with the most prominent bugfixes like in previous versions.

[fintelia]

👍 In that case, seems to just be a matter of getting all the merges to go through so everything ends up on master

[johannesvollmer]

final 0.23.x release first

I can imagine it could be rather confusing for users to publish partial float support - exr supports it, but there's not much else you can do with floats. Don't we want to wait for this pr before releasing?

[HeroicKatora]

It seems github auto-closed this as a consequence of the base branch being removed. Good thing it warned me about this. Not. Anyways, I wanted to rebase it as #1585. However now I need to find all other PRs that were auto-closed..