Revamp the Pixel trait via the pixeli crate

[ripytide]

This is an overhaul of the crate to extract out the Pixel types and traits and improve them simultaneously into the pixeli crate. This would fix #1523 and possibly many of its linked issues.

Summary of Changes:

• Pixel types and traits move to the pixeli crate which becomes a dependency
• Pixel types switch representation from Rgb([T; 3]) to #[repr(C)] struct Rgb<T> {r: T, g: T, b: T} which is nicer to work with (pixel.g vs pixel.0[1])
• Re-export the pixeli dependecy since users will often need to use those types/traits for doing pixel related stuff
• Luma is renamed to Gray as that is more understandable
• Pixel::Subpixel is renamed to Pixel::Component since subpixels are used with physical monitors for the rgb components and so the alpha component doesn't fit
• Two new DynamicImage variants needed to be added to pass the tests Gray32F and GrayAlpha32F which removes the edge condition for the grayscale() function not actually converting Rgb32F image variants to grayscale.
• Refactored away from deprecated channels4() legacy methods.
• Deprecated some less generic color conversion functions over a new more flexible version convert_generic_image()

I understand this is huge PR, but for this to be an atomic PR there wasn't really a way of splitting it any smaller. I also understand that this probably is a breaking change in many different areas, but I think that's acceptable given the scale of improvement this PR offers.

Btw the pixeli crate is pretty cool in my biased opinion, the new Pixel trait is wayyy more powerful than the previous one (credit to @johannesvollmer for some of the inspiration (#1523) and @kornelski for the related discussions in the rgb crate) and solves lots of previous issues, (like now map_components() can map to a different component type!).

[ripytide]

The only remaining failing CI check is the deny(public_private_dependencies) lints failing for all the pixeli trait bounds which seems fine to me since its a public dependency? Can we add an exception for it somehow?

[fintelia]

Thanks for your interest in this crate. This change is far too large and has way too many breaking changes to review/iterate on. Breaking changes and big API additions need to be discussed and agreed on individually before they can be added. And in any case, we've generally been aiming to go 1-2 years between breaking releases

[ripytide]

Just to be clear here, this PR would solve:

• Let's talk about Pixel #1523
• Simplify Pixel trait #1099
• Add alpha count associated constant to Pixel trait #956
• Pixel::map methods should map to arbitrary subpixel type #1464
• Why is the FromColor trait not public? #1212
• implement std::convert::From<T: Pixel> for Pixel #1013
• A compatible option for decoding to various pixel types #1499

But if you think not making breaking changes is more important to users, then that is your decision to make I suppose.

I'd be interested to your opinion @HeroicKatora on this topic if you have time.

[HeroicKatora]

I don't think this addresses enough of major points raised by #1523 to warrant the breakage either. This PR changes the representation which the least problem here. Declaring a new pre-0.* crate as a compatible solution is a non-starter, there already is rgb with several advantages such as its bytemuck integration allowing many opaque (not mentioned in type-bounds) uses.

Designing around the easy goals won't work, as it is the harder goals that break the current design, too. The two key points missing are awareness of color spaces and single responsibility. Instead, this design encodes even more information into the type system and it will run into the same troubles. The Pixel, if indeed one even supposes such a trait is needed, needs foremost a clear design boundary, but this offers even less of one.

There are a few interesting pieces here and there:

• Overall, renaming of the variants and types from *Luma* to *Gray* might be good for consistency.
• Component is a better wording than Subpixel.
• On the other hand, ContiguousPixel suggests a NonContiguousPixel which is a pretty obvious non-goal for the simple kinds of buffers that DynamicImage tries to offer as variants. There's definitely a conflict in design here. You're not going to be able to semantically encode all existing texel layouts in types (at least not in Rust).
• FromPixelCommon raises an interesting point. If we explicitly close the DynamicImage design against new variants, we do have a static 'top' element on our pixel conversion lattice and it's not Rgba<u8> but rather Rgba<f64>. Then DynamicImage should probably use that one. But also, if we close the conversion trait anyways we could do type-id dispatch instead of relying on associated methods in traits, which would probably be extremely more performant.

If you take one key point, then let it be the one summary also raised during gfx's lessons learned to wgpu: It's not user-friendly to encode all of this in the type system, and algorithms built onto the representation can be (much) more optimal if they get to decide the mode of access (such as: bringing lookup-tables for color conversion instead of From<_> traits not having any such parameters). image's own design still pre-dates this by quite a bit and if there be breakage then for the sake of fixing this. (But also, DynamicImage tries to represent a simplified view that shouldn't be so affected by the underlying reasons, so no breakage-for-breakage sake).

[kornelski]

BTW we've had a similar conversation about the rgb crate. I'm interested in adding such improved Pixel trait to it, but also want to preserve backwards compatibility with the current structs.

[kornelski]

gfx's lessons learned to wgpu: It's not user-friendly to encode all of this in the type system, and algorithms built onto the representation can be (much) more optimal if they get to decide the mode of access

Do you have more info on this?

[HeroicKatora]

See this whole talk, but particularly starting from here: https://youtu.be/m0JgF5Wb-dA?t=395&si=XoK-TroLp4idLt2g

[fintelia]

Repeating some feedback given offline:

The breaking changes aspect of the PR was a concern, but there were actually a few bigger factors...

Multi-thousand line PRs are too big: The larger a PR is the more rounds of feedback it takes to review and the longer each of those reviews takes. There's a good chance the reviews needed for a PR of this scale would need 10+ hours of maintainer time. And even if some parts are ready after a couple rounds of back-and-forth, there's still a very high chance of all the work being wasted because the PR stalled out before everyone reached agreement on the remaining details.

API changes need to be discussed individually: A single GitHub PR is a bad place to try to discuss a lot of different changes. Every API change has tradeoffs and questions to address, even when it isn't a breaking change.

[ripytide]

The two key points missing are awareness of color spaces and single responsibility. Instead, this design encodes even more information into the type system and it will run into the same troubles.

Good point, I didn't tackle the color space issue at the same time, in that regard the four choices I can think of for attaching color-space to pixels are:

1. Enum in Pixel types:

enum ColorSpace {
    Unknown,
    Linear,
...
}
struct Rgba<T> {
    r: T,
    g: T,
    b: T,
    a: T,
    color_space: ColorSpace,
}

2. Enum in Image Type

struct ImageBuffer<P> {
    pixels: Vec<P>,
    color_space: ColorSpace,
}

3. Same as 1, but using a PhantomData<U> with a generic tag-type like eudlid Point types.
4. Same as 2, but using PhantomData tags as well.

As you say the type-system heavy approaches in 3. and 4. are harder to reason with and learn, (is that similar to how gfx did things?). 1. Seems terrible for storage space which leaves options 2. as probably the best option given all the tradeoffs I think but I don't know if you had something else in mind?

With option 2. I think FromPixelCommon would change to something like:

trait FromPixelCommon<P> {
    fn from_pixel_common(pixel: P, source_color_space: ColorSpace, destination_color_space: ColorSpace) -> Self;
}

Am I understanding correctly?

On the single responsibility aspect, which bit of this design goes did you see going against that principle? Are you suggesting moving the helper functions such as component_array() color_component_array() and the map_*() functions to an extension trait? I think I'd probably be fine with that but I'm not sure as to the advantages/disadvantages of separating them either.