Simplify Pixel trait

[fintelia]

The trait has lots of methods that aren't especially useful, yet have to be implemented by every pixel struct. This PR attempts to make things better by removing a bunch of methods.

[HeroicKatora]

I appeciate any effort moving us towards separating pure byte-based channel from color space conversion. That said, a few questions remained.

[HeroicKatora]

As long as to_rgba is required on Pixel it seems to still hold that its color space must be interpreted as sRGB. But then the other methods, except invert, could also be default implemented utilizing this one. Why remove them outright instead?

[fintelia]

I was trying to remove as much as I could without breaking too much other code in the crate. Perhaps it would be better to step back and consider what generic functionality we can provide on pixels without baking in assumptions about color spaces and so forth. For instance, all of the map/apply variations are rather questionable without knowing the color space

[HeroicKatora]

That was contrary to my thoughts, map and apply were the most reasonable in my eye. They do not themselves enforce any particular interpretation. Sure, the caller can not currently find out the color space through image but if it is known then the function can correctly interpret the channel value. And no color type has to come up with an interpretation of its own, just copy values into the function argument and back.

[fintelia]

Don't the channels have to be either L or RGB (plus an optional alpha) for that to work? I don't think it could work for things like HSV or xyY since different channels have different meanings but the map and apply functions would run the same closure on each?

[fintelia]

I guess one view is that Pixel should just be a wrapper around a &[Subpixel] plus an associated ColorType and ColorSpace. It would then fall on the user to properly handle each possible pair they cared about. There would be convenience functions to operate on channels but they'd have no higher level understanding of what they meant.

The alternative is to discourage users from interacting with pixels that way and instead provide higher level functionality so that generic code could work while remaining oblivious to the ColorType and ColorSpace of the pixel. It isn't really obvious to me what this would look like, but seems like the preferable option if feasible

[aschampion]

I guess one view is that Pixel should just be a wrapper around a &[Subpixel] plus an associated ColorType and ColorSpace...The alternative is to discourage users from interacting with pixels that way and instead provide higher level functionality

Those may not be mutually exclusive alternatives, but levels of abstraction in the API. Pixels are just buffers with color types and spaces as you say, then higher level pixel traits provide higher level functionality where available. So if a user just needs (strawmen) Blendable, Invertible, HasChannel<Channel::Luminance> or InterpretableAs<Channel::Red, ColorSpace::sRGB>, they can construct fn<A: Invertible + HasChannel<Channel::Luminance>, B: Blendable<A> + InterpretableAs<Channel::Red, ColorSpace::sRGB>> myFancyFilter(a: &A, b: &mut B), etc.

If they need something more format-specific not captured in those higher level traits provided by image or imageproc or other crates, then they can operate on Pixels directly and match/bound the color types and spaces they need to, creating new higher level traits if they wish, e.g., trait Darken: Pixel {...} impl<P: Pixel<ColorType=RGB, ColorSpace=sRGB>> Darken for P { ... }.

So while I agree with @HeroicKatora that it's not an entirely coherent change to remove some of these conversions while leaving others, I can see the point in at least shrinking the exposed API since what's exposed is not really coherent as it is, with color spaces and types a bit abused.

[HeroicKatora]

Was this missed during removal of methods or is keeping this method intentional?

[aschampion]

Could this be let color::Rgba::<u16>([r16, g16, b16, a16]) = pixel.into_color(); instead so as to not duplicate color conversion logic?

[aschampion]

Now that I think about it, all of these could be replaced with into_color, e.g.,

 DynamicImage::ImageBgra8(ref mut p) => p.put_pixel(x, y, pixel.into_color()),

For the 16bpc types you just need an extra,

let pixel16: color::Rgba<u16> = pixel.into_color();

...

DynamicImage::ImageRgb16(ref mut p) => p.put_pixel(x, y, pixel16.into_color()),

This is consistent with the two-level API described above in the other comment, with IntoColor being a high level trait expressing convertibility across color types that lets DynamicImage not have to be responsible for how these conversions are done. This means the FromColor/IntoColor implementation will be responsible for handling color space conversions once we represent them, as they should. The downside for now is still needing to special case the 16bpc types, but I expect we'll clear that up later with the Pixel types themselves.

To note this means that put_pixel could really be (not that we would want to expose this API yet, given there is still design flux):

fn<P: Pixel> put_pixel(&mut self, x: u32, y: u32, pixel: P)
    where P: IntoColor<color::Rgba<u8>> + IntoColor<color::Rgba<u16>> {
    let pixel8: color::Rgba<u8> = pixel.into_color();
    let pixel16: color::Rgba<u16> = pixel.into_color();
    ...
}

[aschampion]

Removing these functions that assume a lot of color interpretation in Pixel is good and I'm in favor of this given the following changes:

• Using into_color in put_pixel/blend_pixel so that pixel/color/channel conversion logic is not duplicated with FromColor.
• By doing the above, we may also be able to remove Pixel::to_rgba/Pixel::to_luma_alpha. This makes pixel a minimal core trait and leaves FromColor/IntoColor as a higher level trait and the sole authority on pixel and color conversion, which is a good step towards minimizing the surface area of Pixel while enshrining higher level traits that express semantic capabilities. In other words, FromColor is the only place making wrong assumptions about color spaces, so as we introduce them we know where to start.

FromColor/IntoColor will need refactoring later, especially to solve the transitive conversion problem (8 bit pixel -> 16 bit RGBA -> 16 bit pixel of other type) and to handle space conversions and their interaction with type conversions. Along these lines we should start thinking about where color spaces are going to be represented, either as associated const/type on Pixel (which would require a macro-generated proliferation of pixel types), or as generic parameters or method arguments to the high level traits, (FromColorSpace<OtherPixel: Pixel, From: ColorSpace, To: ColorSpace>) so that only buffers and images know in which color space their pixels reside. But this doesn't have to be decided to land this PR.

One thing I'm undecided on is if it makes sense to remove the Invert trait. Removing Pixel::invert is good; high level traits should build on top of the low level Pixel trait, not vice versa. But by the high level trait story the way forward would be to make Invert public. On second thought, removing Invert for now is fine until we have more ergonomics around these high level APIs, meaning both color space representation and ways to expose high level traits for whole views/buffers/images built on top of high level traits over pixels. We can start that process with FromColor, no need to complicate it with other traits yet.

It seems like this matches with your goals @fintelia. Let me know if I've misunderstood.

[HeroicKatora]

@fintelia This was auto-closed. Would you do a rebase on top of the changes in #1585 and reopen it? (And wow, automation moved it to 'Done'. Github's PR and project system is such a mess).