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

Redesign the pixel encoding procedure #58

Closed
Ogeon opened this issue Mar 5, 2016 · 31 comments · Fixed by #60
Closed

Redesign the pixel encoding procedure #58

Ogeon opened this issue Mar 5, 2016 · 31 comments · Fixed by #60
Milestone

Comments

@Ogeon
Copy link
Owner

Ogeon commented Mar 5, 2016

Something that has been growing in my mind for a few days is the idea of changing the way colors are encoded into pixel friendly formats. The current procedure may look like this:

let linear = Rgb::new(1.0, 0.3, 0.6);
let srgb = Srgb::from_linear(linear);
let pixel: [u8; 3] = srgb.to_pixel();

or, using a shortcut:

let linear = Rgb::new(1.0, 0.3, 0.6);
let pixel: [u8; 3] = Srgb::linear_to_pixel(linear);

There's also the case where a simple custom gamma exponent can be used:

let linear = Rgb::new(1.0, 0.3, 0.6);
let pixel: [u8; 3] = GammaRgb::linear_to_pixel(linear, 2.2);

This is fine and all, but it's not really easy to use in a general way:

  1. There's no common trait or type for encoded colors.
  2. The alpha component is baked into the color type.
  3. GammaRgb has a variable gamma exponent, which makes it very different from other encodings, and impossible to fully express as a simple type.

This problem is tightly connected to #31, where a general RGB type is proposed. The way GammaRgb is structures interferes with that proposal, since it has an additional gamma component, besides the usual red, green and blue. I'm therefore proposing that we redesign the way pixel encoding works to make it more friendly towards the idea of a more generalized RGB type. I would also like to extend this to other RGB related color spaces, such as HSV, HSL, Luma and HWB. These can all be seen as input types and they are, often enough, defined in terms of already encoded RGB.

The initial proposal is to keep linear and encoded types separate. The purpose of this library is to strongly encourage a linear work flow and this can easily be achieved by only implementing operations on the linear variant. There was a proposal, by @sidred, to have marker type parameters, such as Rgb<Linear, Srgb, ...> and Rgb<Gamma, Srgb, ...>, but recent problems with such marker parameters, as well as the additional complexity of extra type parameters, makes that method less attractive.

The separation of linear and encoded colors, using two different structs, will come with a bit of duplication, but the duplication will mainly be restricted to construction and fields. The linear variants will implement the operation traits, while the encoded variants will be focused on packing and converting data. Here is an example of how it may look when combined with #31:

pub trait RgbEncoding {
    ///Encode a color component.
    fn encode<T: Float>(value: T) -> T;

    ///Decode a color component.
    fn decode<T: Float>(value: T) -> T;
}

pub trait RgbPrimaries {
    //get red, green, blue primaries, etc.
}

///Marker type, which both defines a set of primaries and an encoding
pub struct Srgb;

impl  RgbEncoding for Srgb {
    //...
}

impl  RgbPrimaries for Srgb {
    //...
}

struct LinearRgb<Prim: RgbPrimaries = Srgb, ...> {
    //..
    primaries: PhantomType<Prim>,
}

struct Rgb<Enc: RgbEncoding = Srgb, ...> {
    //..
    primaries: PhantomType<Enc>,
}

Encodings and primaries are often strongly connected to each other, which is why the same type can be used for both. The problem with this is that it's hard to decouple them, but that's hopefully rare enough to make this more good than bad. The only problem left is the GammaRgb. Its variable gamma component makes it incompatible with the above pattern, and having floating point values as type parameters is nothing but a (bad?) dream at this stage. We can, however, make more assumptions and add more escape hatches. I'm therefore proposing the addition of the probably most common gamma exponent, 2.2, as an encoding type, and the extension of the above RgbEncoding that makes it possible for an encoding to map to any set of primaries:

pub trait RgbEncoding {
    type Primaries: RgbPrimaries;

    ///Encode a color component.
    fn encode<T: Float>(value: T) -> T;

    ///Decode a color component.
    fn decode<T: Float>(value: T) -> T;
}

///Gamma exponent 2.2, with any primaries.
struct Gamma22<P: RgbPrimaries>;

impl<P: RgbPrimaries> RgbEncoding for Primaries<P>
    type Primaries = P;

    //...
}

impl  RgbEncoding for Srgb {
    type Primaries = Srgb;
    //...
}

fn main() {
    let encoded = Rgb::<Gamma22<Srgb>>::new(...);
    let linear = encoded.to_linear(); //Gives LinearRgb<Srgb>
}

The preferred primaries can then be fetched from the associated Primaries type when converting from encoded to linear. Now, what about those other gamma values? That's where we need escape hatches. One such escape hatch could be to keep the old GammaRgb type around. It may be a bit unwieldy, but it works. An other is to just have a straight-to-pixel conversion, like the *::linear_to_pixel function in the current design. More suggestions are welcome here.

The last question is what encoded types are allowed to do. My proposal is "basically nothing", since I'm still strongly in favor of keeping them strictly separated from linear types. This is to minimize any risk of mistaking non-linear values for linear and forcing the user to make an explicit conversion between them. The minimal set of functions I can think of at the moment is:

impl<E: RgbEncoding, ...> Rgb<E, ...> {
    fn new(red: T, green: T, blue: T) -> Rgb<E, ...> {...}
    fn new_u8(red: u8, green: u8, blue: u8) -> Rgb<E, ...> {...}
    fn from_pixel<P: RgbPixel<T>>(pixel: P) -> Rgb<E, ...> {...}
    fn to_pixel<P: RgbPixel<T>>(&self) -> P {...}
    fn from_linear<C: Into<LinearRgb<E::Primaries, ...>>(color: C) -> Rgb<E, ...> {...}
    fn to_linear<C: From<LinearRgb<E::Primaries, ...>>(&self) -> C {...}

    //Useful addition
    fn linear_to_pixel<C: Into<LinearRgb<E::Primaries, ...>, P: RgbPixel<T>>(color: C) -> P {
        Rgb<E, ...>::from_linear(color).to_pixel()
    }
}

These can, of course, be tweaked and changed. Some of these functions would be nice as a general encoding trait, but that's quite complex and may be better discussed at a later stage.

Something this proposal hasn't touched, so far, is what to do with the alpha component. I think it would be best to make use of the Alpha type, but I haven't figured out exactly how, so that question remains open.

An other open question is type aliases for common RGB types, such as AdobeRgb. One idea is to not alias sRGB, and only implement new for Rgb<Srgb, D65, T>, similar to how it's done with only D65 today. An other is to alias all of them and not pick a default.

@Ogeon Ogeon mentioned this issue Mar 5, 2016
@Ogeon Ogeon added this to the 0.3.0 milestone Mar 5, 2016
@sidred
Copy link
Contributor

sidred commented Mar 5, 2016

A few things

Is it necessary to separate RgbEncoding and Rgb Primaries? They both are properties of the specific Rgb Color space. Example

///Represents the tristimulus values for the Rgb primaries
pub trait RgbVariant<Wp, T>
    where T: Float,
        Wp: WhitePoint<T>,
{

    fn red() -> Yxy<Wp, T>;

    fn green() -> Yxy<Wp, T>;

    fn blue() -> Yxy<Wp, T>;

    ///The default gamma value for the rgb color space
    fn gamma() -> T;

    fn into_gamma(inp: Rgb<Wp, T>) -> Rgb<Wp, T>{
        let gamma = Self::gamma();
        let inv_gamma = if gamma.is_normal() { T::one() / gamma } else { T::one() };
        Rgb::with_wp(inp.red.powf(inv_gamma), inp.green.powf(inv_gamma), inp.blue.powf(inv_gamma) )
    }


    fn into_linear(inp: Rgb<Wp, T>) -> Rgb<Wp, T> {
        let gamma = Self::gamma();
        Rgb::with_wp(inp.red.powf(gamma), inp.green.powf(gamma), inp.blue.powf(gamma) )
    }


    fn mat3_from_primaries() -> Mat3<T> {
       ....
    }

    ///Geneartes to Rgb to Xyz transformation matrix for the given white point
    fn rgb_to_xyz_matrix() -> Mat3<T> {
      ...
    }

    ///Geneartes the Xyz to Rgb transformation matrix for the given white point
    fn xyz_to_rgb_matrix() -> Mat3<T> {
       ...
    }

}

In this case Srgb can override the default gamma conversion with a custom conversion and for all other colors we just need to provide the default gamma value.

The preferred primaries can then be fetched from the associated Primaries type when converting from encoded to linear.

What do primaries have to do with gamma encoding?

Now, what about those other gamma values? That's where we need escape hatches. One such escape hatch could be to keep the old GammaRgb type around. It may be a bit unwieldy, but it works.

If we have the encoding and decoding on the specifc rgb color space, then there will be no need to have the GammaRgb type. The Rgb type above will hold the gamma encoded values and LinearRgb will hold the linear values. Am I missing some use case that you have in mind?

I need to think a bit more about the pixel type.

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 5, 2016

Am I missing some use case that you have in mind?

I'm trying to keep things open for customization, and dynamically chosen gamma exponent can't be represented as a type. That was the whole point with the GammaRgb type, and it's also the point of separating primaries from encoding. Why assume that primaries and encoding are always tightly coupled? Some custom RGB variant may use the same primaries as sRGB, but a different encoding. Others may just want to encode with some gamma exponent and be done with it. Some primaries may not even have an associated encoding. Separating the definition of primaries and encoding makes these situations easier.

What do primaries have to do with gamma encoding?

Very good question. Answer: Nothing, really. They are just paired together in an RGB standard, together with a white point, and may be anything. The RgbEncoding trait could also be called RgbStandard. That's also what your RgbVariant seem to be, but the difference is that the RgbEncoding doesn't assume one single set of primaries for an encoding.

@sidred
Copy link
Contributor

sidred commented Mar 5, 2016

One issue with having Gamma and Linear types separate is you cannot prevent mistakes like this

let rgb = RgbLinear::new();
let val = rgb.into_linear();// should error or return the same value

This can be avoided if the gamma or linear is part of the trait.

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 5, 2016

into_linear shouldn't even be implemented for LinearRgb. It could be for a hypothetical Rgb<Linear<SomePrimaries>, ...>, and then just return the same values, but as a LinearRgb<SomePrimaries, ...>.

@sidred
Copy link
Contributor

sidred commented Mar 5, 2016

Into_linear shouldn't even be implemented for LinearRgb.

I was confusing the trait implementation with the struct implementation

So we can just have 2 traits. Having fn gamma() on the Gamma trait will make GammaRgb redundant

pub trait Gamma {
    fn gamma() -> T;

    ///Encode a color component.
    fn to_gamma<T: Float>(value: T) -> T {
      let gamma = Self::gamma();
      let inv_gamma = if gamma.is_normal() { T::one() / gamma } else { T::one()}
      value.powf(inv_gamma)
   }

    ///Decode a color component.
    fn to_linear<T: Float>(value: T) -> T {
      let gamma = Self::gamma();
      value.powf(inv_gamma)
   }
}

pub trait RgbPrimaries {
    //get red, green, blue primaries, etc.
}

Having an impl like this will take care of the standard converison linear gamma conversions.

impl<R: Gamma + RgbPrimaries> LinearRgb<R> {
    to_linear() {
     R::to_linear() // will take care of the default Srgb, Adbode Rgb conversions from the traits
  }
}

@sidred
Copy link
Contributor

sidred commented Mar 5, 2016

Why not implement Blend etc on the gamma encoded value by converting to Linear, atleast for the known types.

What about from and into color? Do we add from_rgb_linear and assume existing from_rgb converts to the gamma value?

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 5, 2016

So we can just have 2 traits. Having fn gamma() on the Gamma trait will make GammaRgb redundant

That's basically what this proposal is all about, but without assuming exponential encoding, or tight coupling between encoding and primaries. It's common, but we don't have to assume it.

Having an impl like this will take care of the standard converison linear gamma conversions.

Does that convert from linear to linear?

Why not implement Blend etc on the gamma encoded value by converting to Linear, atleast for the known types.

Well, hmm... I suppose they could. They were mainly meant to be temporary states, but that doesn't stop them from jumping to and from being linear, just like with premultiplied alpha.

What about from and into color? Do we add from_rgb_linear and assume existing from_rgb converts to the gamma value?

I'm not sure if they should be concerned by the encoded variants at all, but then I don't know what happens if we add things like non-linear HSV...

@sidred
Copy link
Contributor

sidred commented Mar 6, 2016

I'm not sure if they should be concerned by the encoded variants at all, but then I don't know what happens if we add things like non-linear HSV...

Lets take an example of converting D50 to D65

let c: Rgb<Srgb, D50, _> = Rgb::new(0.5,0.5,0.5)

let o: Rgb<Srgb, D65, _> = c.adapt_into();

That would be a lot easier than

// Might rrquire more type annotations / intermediate variables
let o: Rgb<Srgb, D65, _> = c.into_linear().adapt_into().to_gamma();

Also for IntoColor and AdaptInto to work for both Rgb and LinearRgb, you would need a trait bound of Rc: RgbPrimaries + Gamma.

So having a custom Gamma Rgb<CustomGamma, Wp, T> will not mean that custom_rgb.into_xyz() will work. You would need to convert it into a known linear or gamma encoded Rgb for it works. So from a usability point of view I am not sure separating RgbVariants into Gamma and Primaries, gets us anything.

If the user has just a separate gamma, wouldn't it be easier to let users convert it into a known color directly or implement the entire RgbVariant trait? That way all other operations will work easily and will be a more explicit error.

Do we need Hsl, LinearHsl, Hwb, LinearHwb, Hsv, LinearHsv as well?

@sidred
Copy link
Contributor

sidred commented Mar 6, 2016

Having an impl like this will take care of the standard converison linear gamma conversions.
Does that convert from linear to linear?

LinearRgb<Srgb, D50,_> to LinearRgb<adobeRgb, D50,_> would be implemented via chromatic adpatation, not from and into color.

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 6, 2016

Would it have to? The white point doesn't change.

@sidred
Copy link
Contributor

sidred commented Mar 6, 2016

Yeah, I guess these will need to be taken care of in the from_rgb and into_rgb by conversion via xyz .

@sidred
Copy link
Contributor

sidred commented Mar 6, 2016

We need to rethink the conversions. Right now From and Into only handle different colors (same white point and rgb variant). Chromatic adaptation handles different whitepoints and uses From and Into for conversions.

We need to handle Rgb<Srgb,> to Hsl<AdobeRgb> and Rgb<AdobeRgb>. May ne another trait or within the from and Into.

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 6, 2016

Sorry, I somehow missed the other post.

I'm not sure I see what the problem is. IntoColor can't be infinite. We have to draw a line somewhere, so I'm thinking that maybe it should just be limited to the linear types. Anything can use it anyway, and the reverse conversion can be done with FromColor. They don't have to take the encoding into account at all. It's not interesting when defining the linear RGB, since it's not encoded.

Separating primaries and encoding will simply separate two independent, but often coupled, concepts. One is part of the definition of the color space, and the other tells us how to store the values. The only thing they really have in common is that standardized encodings comes with an RGB space definition, including primaries, but any primaries can really be used with any encoding.

I want to model this independence in a way that is as usable as possible, which is why I think that the concepts should be separated. The encoding doesn't have to have a preferred set of primaries. It could just as easily be replaced with yet another type parameter: Rgb<Encoding, Primaries, Wp, T>, but I'm not that fond of it. The thing is that I don't want to blur the lines between linear and encoded colors too much. The encoded variants are only really interesting as input/output.

If the user has just a separate gamma, wouldn't it be easier to let users convert it into a known color directly or implement the entire RgbVariant trait? That way all other operations will work easily and will be a more explicit error.

Separating encoding and primaries makes this even easier. Just implement the encoding trait and don't care about primaries.

Do we need Hsl, LinearHsl, Hwb, LinearHwb, Hsv, LinearHsv as well?

Yes, eventually. They are tightly connected to RGB. Didn't the colormine tests assume that they converted to encoded RGB?

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 6, 2016

We need to rethink the conversions. Right now From and Into only handle different colors (same white point and rgb variant). Chromatic adaptation handles different whitepoints and uses From and Into for conversions.

I'm not sure this is entirely related to this particular topic, but yes, we need to decide the limits of those conversions.

@sidred
Copy link
Contributor

sidred commented Mar 6, 2016

  • Gamma + Primaries vs single RgbVariant - It's just an implementations detail. I prefer a single trait, I have only seen rgb color spaces with defined gama's used (like Srgb, adobe rgb etc)), but I don't have a strong opinion on it. We can start with the 2 separate traits.
  • Pixel Type - Having a separate Gamma Trait should take care of your concerns regarding the pixel types. Maybe add more methods on the pixels conversion, but those are implementation details. There is nothing else needed for the pixel type. Please correct me if I am wrong.
  • Hsl/Hwb/Hsv - Do you agree that these need a linear and gamma variant as well?
  • Luma - Does luma need to be color space aware too?
  • Gamma Encoded color - If I understand you correctly, you want to make gamma rgb a sort of secondary color, all operations are only in linear space. But the CSS spec assumes gamma Srgb and all the formula's for blend (from the w3.org link) I believe also assume gamma encoded values. I know its not recommeded but, it will be useful to follow the standard. So I am proposing that we implement those for gamma types as well.
  • From / Into - I am proposing that we add into_linear_rgb, into_linear_hsl etc.. so that GammaRgb -> GammaHsl, GammaHwb -> Xyz etc work seamlessly. It will be easier this way than color.to_linear().to_xyz().
  • ColorSpace conversions - I think this be taken care of in the implementation of the from and into color traits for the linear and gamma types.
  • Color Enum - I am not sure how to handle this. Do we add all linear and gamma variant (Rgb, Hsl, Hwb etc)?

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 6, 2016

Again, there are more encoding methods than a gamma exponent. That's why I don't call it gamma, and that's also why I don't think adding a gamma() method to the encoding trait is such a great idea. sRGB is a common example of a non-exponential encoding, but there are more.

I prefer a single trait, I have only seen rgb color spaces with defined gama's used (like Srgb, adobe rgb etc)), but I don't have a strong opinion on it.

Here are some more: http://www.color.org/chardata/rgb/rgb_registry.xalter Some of them are linear and some of them are a bit more exotic.

Having a separate Gamma Trait should take care of your concerns regarding the pixel types. Maybe add more methods on the pixels conversion, but those are implementation details. There is nothing else needed for the pixel type. Please correct me if I am wrong.

I'm not 100% sure I follow. I would gladly remove the pixel types, but I have seen them used as input types to functions, so I think they may still be useful in those situations. They are otherwise not particularly useful and their conversion functionality can be replaced by functions, as demonstrated by the existence of Srgb::linear_to_pixel.

Hsl/Hwb/Hsv - Do you agree that these need a linear and gamma variant as well?
Luma - Does luma need to be color space aware too?

Yes, but they will just have to follow the same pattern as for RGB. We can ignore them for now. The idea is to convert like this: Hsl -> EncodedRgb -> LinearRgb -> whatever. Luma is gray, which is why it's also affected.

If I understand you correctly, you want to make gamma rgb a sort of secondary color, all operations are only in linear space. But the CSS spec assumes gamma Srgb and all the formula's for blend (from the w3.org link) I believe also assume gamma encoded values. I know its not recommeded but, it will be useful to follow the standard. So I am proposing that we implement those for gamma types as well.

Working with encoded colors will give the wrong result in most situations, except where the opposite is defined. The very description of Palette is "A Rust library for linear color calculations and conversion", and I would like to keep that focus. It's, by the way, very easy to ignore the the linearity restriction, by interpreting pixel values as if they were linear.

I am proposing that we add into_linear_rgb, into_linear_hsl etc.. so that GammaRgb -> GammaHsl, GammaHwb -> Xyz etc work seamlessly. It will be easier this way than color.to_linear().to_xyz()

Conversion to and from linear should only be necessary during input/output. A color doesn't have to be listed in the trait to implement it, meaning that color.to_xyz() is still possible.

Color Enum - I am not sure how to handle this. Do we add all linear and gamma variant (Rgb, Hsl, Hwb etc)?

I think only the linear versions are necessary, given that we only restrict the implementations of various operations to them. Color is lazy mode and could work just as well with only a few variants, but more are included to make it a bit less lossy.

@sidred
Copy link
Contributor

sidred commented Mar 6, 2016

Again, there are more encoding methods than a gamma exponent. That's why I don't call it gamma, and that's also why I don't think adding a gamma() method to the encoding trait is such a great idea. sRGB is a common example of a non-exponential encoding, but there are more.

I am not disagreeing with this. I expect the Rgb encoding to have 3 methods

trait RgbEncoding {
     ///The default gamma value for the rgb color space
    fn gamma() -> T;

    ///Converts linear rgb into the gamma encoded rgb
    fn encode(inp: Rgb<Wp, T>) -> Rgb<Wp, T>{
        let inv_gamma = 1.0 / Self::gamma();
        Rgb::with_wp(inp.red.powf(inv_gamma), inp.green.powf(inv_gamma), inp.blue.powf(inv_gamma) )
    }

    ///Converts gamma enocded rgb into the linear rgb
    fn decode(inp: Rgb<Wp, T>) -> Rgb<Wp, T> {
        let gamma = Self::gamma();
        Rgb::with_wp(inp.red.powf(gamma), inp.green.powf(gamma), inp.blue.powf(gamma) )
    }

}

So this is just a basic implementation which avoids repetition for common use cases like AdobeRgb and AppleRgb. Just set the gamma and you're done. For Srgb etc. gamma() is not used and encode and decode method need to be overridden. This is just to simplify the implementation for a common use case.

I'm not 100% sure I follow. I would gladly remove the pixel types,

Pixel type needs to be there. I was trying to understand the point you were making in the first comment regarding the pixel type. I wanted to know if you expect any major changes to the pixel type or will having the RgbEncoding trait cover most of the use case?

Conversion to and from linear should only be necessary during input/output. A color doesn't have to be listed in the trait to implement it, meaning that color.to_xyz() is still possible.

Yes we can implement from and into color for gamma rgb but it will be different from the rest of the colors.

This works

let xyz = lin_rgb.into_xyz()
let xyz = gamma_rgb.into_xyz()

This won't work

let linear_rgb = xyz.into_rgb()
// will not work
let gamma_rgb = xyz.into_rgb()

//Needs to be
let gamma_rgb = xyz.into_rgb().encode()
//or if From is impelmented
let gamma_rgb = FromColor::from_xyz(xyz)

//This will be a lot easier to use 
let gamma_rgb = xyz.into_gamma_rgb()

Adding into and from for the encoded colors makes sense from a usage point of view.

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 6, 2016

So this is just a basic implementation which avoid repetition for common use cases like AdobeRgb and AppleRgb. Just set the gamma and you're done. For Srgb etc. gamma() is not used and encode and decode method need to be overridden. This is just to simplify the implementation for a common use case.

But gamma must still return a value. There are more ways to achieve the same goal. Like this, for example:

pub trait ExponentialEncoding {
    pub fn gamma<T: Float>() -> T;
}

impl<E: ExponentialEncoding> RgbEnchoding for E {
    //...
}

pub struct Gamma18;

impl ExponentialEnchoding for Gamma18 {
    fn gamma<T: Float>() -> T {
        flt(1.8)
    }
}

type AppleRgb = Gamma18;

...modified for however the primaries are to be connected to the encoding.

Adding into and from for the encoded colors makes sense from a usage point of view.

I'm concerned about them becoming too monolithic, but I don't know... Maybe an Encode trait should be a thing. It may even be possible to blanket implement it for each output.

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 7, 2016

I was playing around with the idea of having separate traits for RgbStandard, Primaries and Encoding. Here's a small and simplified proof of concept: https://play.rust-lang.org/?gist=d6ce765ba1ee1b735ba5&version=stable

This allows RGB standards that functions as both primary sets and encodings, loose encodings and primary sets, and on-the-fly custom standards using a (Primaries, Encoding) tuple. A color can easily be decoded, using the associated primaries from the standard, but encoding it will still require an encoding to somehow be specified. That's hard to avoid but it should hopefully not be a huge deal.

Oh, and standards can also be derived from each other. That's useful for easy customization.

@sidred
Copy link
Contributor

sidred commented Mar 7, 2016

I was just thinking something similar. Is it possible to take it one step further and include white point in it as well


impl<P: Primaries, E: Encoding, W: WhitePoint> ColorProfile for (P, E, W) {
    type Primaries = P;
    type Encoding = E;
   type WhitePoint= W;
}

We can then use this for all colors including the cie colors. For cie colors when the primaries/encodings are not needed, you can just use dummy values ( like SrgbEncidng, SrgbPrimaries etc) which will not be used any way.

We can even define a WhitePoint<Dummy, Dummy, Wp> if it makes it any simpler for the cie colors.

I think it will make the library a lot simpler.

What's not clear to me is how it will play with the encoded colors.

Do we include the float trait in it as well?

One downside I see is that right now it is easier to see which trait is being used where. For example From and Into color use the Primaries Trait for Xyz <-> Rgb, but with the single trait it will be a bit harder to track the effects of a particular trait.

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 7, 2016

I was just thinking something similar. Is it possible to take it one step further and include white point in it as well

Absolutely!

We can then use this for all colors including the cie colors.

I'm not so sure about that, on the other hand... Any RGB specific info is totally irrelevant for the CIE types, unless they are converted to RGB, in which case the RGB type can provide them. I fear that they may become type checking noise, and behave badly because of Dummy != Srgb.

Do we include the float trait in it as well?

Yes, but not in the RgbStandard. I just didn't use it in this example, for simplicity. It shouldn't cause any trouble.

One downside I see is that right now it is easier to see which trait is being used where. For example From and Into color use the Primaries Trait for Xyz <-> Rgb, but with the single trait it will be a bit harder to track the effects of a particular trait.

Downside with my proof of concept or with the ubiquitous ColorProfile trait? I think my proof of concept, where only the necessary traits and types are used, is fine in this case.

@sidred
Copy link
Contributor

sidred commented Mar 7, 2016

Downside with my proof of concept or with the ubiquitous ColorProfile trait? I think my proof of concept, where only the necessary traits and types are used, is fine in this case.

I was talking about the ColorProfile trait.

The more I think about, the more I like the ColorProfile trait. I think for most users of the library this will be a lot simpler. You just choose a color profile and it keeps track through all conversions etc and will need fewer type annotations.

For advanced as you have said it is very customizable. Let's think on it a bit more.

For now I will change the Rgb colors to use the ColorProfile trait and lets see if it makes sense.

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 7, 2016

I'm still not convinced. Forcing the user to pick a particular set of primaries and encoding, even if it's irrelevant, doesn't feel right. It's also like trading one problem for another. What if the user uses more than one "profile"? This extra information will only be used when converting to and from RGB, so it's otherwise just baggage that will interfere with type checking. Xyz<Srgb> to Xyz<AdobeRgb>, for example, will be considered different types, even if they will look and behave exactly the same. The only difference is that they convert to different RGB standards, by default.

For now I will change the Rgb colors to use the ColorProfile trait and lets see if it makes sense.

Can we at least call it something else, and save ColorProfile for actual ICC color profiles?

@sidred
Copy link
Contributor

sidred commented Mar 8, 2016

So here is the plan for the Rgb colors

trait RgbColorSpace<T: Float> {
    type TransferFunction: TransferFunction<T>;
    type Primaries: Primaries<T>;
    type WhitePoint: WhitePoint<T>;
}

///Represents the tristimulus values for the Rgb primaries
pub trait TransferFunction<T: Float>
{
    fn to_gamma(T) -> T;
    fn to_linear(T) -> T;
}

pub trait Primaries<T: Float> {
    type W: WhitePoint<T>;

    fn red() -> Yxy<Self::W, T>;
    fn green() -> Yxy<Self::W, T>;
    fn blue() -> Yxy<Self::W, T>;
}

impl RgbColorSpace for SrgbColorSpace {
 type WhitePoint = D65;
type Primaries = SrgbPrimaries;
type TransferFunciton  =SrgbTransferFunciton
}

pub struct RgbLinear<R = SrgbColorSpace, T = f32>
    where T: Float,
        R: RgbSpace<T>,
{
    pub red: T,
    pub green: T,
    pub blue: T,
    pub rgb_apce: RgbColorSpace<T>
}

I was planning to make the the RgbColorSpace whitepoint same as the primaries white point, but then I realized that you might need something like RgbColorSpace<SrgbTransFn, SrgbPrimaries, D50> where the Srgb primaries are in D65 and need to be converted into D50 space for any conversion implementations.

Also, in the color profile, encoding means something else and the gamma correction function is called color component transfer function. So I changed it. Let me know if you prefer TransferFunction or Gamma or GammaCorrection. I think Gamma is the more common term.

Any other suggestion including naming are welcome :)

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 8, 2016

First, as I have said multiple times now, "gamma" is the exponent in the f(x, gamma) = xgamma function. It's very specific, while a transfer function can be non-exponential. "Transfer function" is an ok term, but I would prefer something else than to_gamma for the previously mentioned reason. Maybe from_linear. Good enough for me. I'm also not 100% convinced regarding the RgbColorSpace name, but the only alternative I have at the moment is RgbStandard (not too bad IMO).

The traits can, in general, be simplified by moving the type parameters and associated types to their functions. That will avoid some conflict risks. Something like this (I'm also playing with other ideas):

trait RgbStandard {
    type TransferFunction: TransferFunction;
    type Primaries: Primaries;
    type WhitePoint: WhitePoint; //The WhitePoint trait shouldn't really need its Float parameter
}

pub trait TransferFunction {
    fn from_linear<T: Float>(T) -> T;
    fn to_linear<T: Float>(T) -> T;
}

///Represents the tristimulus values for the Rgb primaries
pub trait Primaries {
    //The primaries are the same, no matter the white point.
    fn red<W: WhitePoint, T: Float>() -> Yxy<W, T>;
    fn green<W: WhitePoint, T: Float>() -> Yxy<W, T>;
    fn blue<W: WhitePoint, T: Float>() -> Yxy<W, T>;
}

//We can make use of the fact that one type can be many things here,
//like in my proof of concept. Just put it in a module and there won't be
//any significant naming collisions.
impl RgbStandard for Srgb {
    type WhitePoint = D65;
    type Primaries = Srgb;
    type TransferFunciton = Srgb;
}

//I'm still not convinced that we should take the
//transfer function into account in linear RGB.
//I'm therefore checking how it looks without them,
//but a third alternative could be a trait for only primaries
//and white point.
pub struct RgbLinear<P = Srgb, W = D65, T = f32>
    where T: Float,
        P: Primaries,
        W: WhitePoint
{
    pub red: T,
    pub green: T,
    pub blue: T,
    pub rgb_space: PhantomData<(P, W)>,
}

I didn't think about it before, but the WhitePoint trait should not really be parametric over the float type. It should actually be just as generic as Primaries above, since its purpose is to represent a perception-independent and type-independent value.

@sidred
Copy link
Contributor

sidred commented Mar 8, 2016

First, as I have said multiple times now, "gamma" is the exponent in the f(x, gamma) = xgamma function

True. But gamma in color is typically associated with any non linear transformation, not necessarily just an exponent. Srgb is called gamma corrected even though it is strictly not gamma (has a linear transform for low values). While not semantically correct, gamma is a more familiar term and it will be easier for understand. I am ok with using TransferFunction.

I'll change RgbColorSpace to RgbStandard.

I don't see how this will work,

impl RgbStandard for Srgb {
    type WhitePoint = D65;
    type Primaries = Srgb;
    type TransferFunciton = Srgb;
}

Won't I have to do

import primaries::Srgb as SrgbPrimaries;
import transfer_fn::Srgb as SrgbFn;
impl RgbStandard for Srgb {
    type WhitePoint = D65;
    type Primaries = SrgbPrimaries;
    type TransferFunciton = SrgbFn;
}

If we have to rename them, I'd rather name them separately. I think it will avoid confusion.

I don't fully understand why

pub trait TransferFunction {
    fn from_linear<T: Float>(T) -> T;
    fn to_linear<T: Float>(T) -> T;
}

is simpler than having a bound on the trait (for whitepoit or primaries or any other trait). There will never be a case when from_linear will return f32 and into linear will return f64. Is that something that needs to be enforced (that they return the same type of value?)? Do we do this for FromColor and IntoColor as well.

I'm still not convinced that we should take the

Look at this use case. I have some color in Rgb, I convert it into hsl and do some operations on it. convert it back to rgb and return it. I am not using the whitepoint here even though I am forced to select a white point here. I think we should optimize for the most common case case here rather than choosing which characteristics are the important and which are not. That is part of the reason I was suggesting we make cie colors also use color space trait.

Thinking about this, can we make it so that only the traits being used need to be provided. Ex

  • Only using Rgb <-> Hsl no traits
  • Using Rgb <-> Xyz conversion etc. Need to specify white point
  • Converting D50 to D65 within cie space (xyz , lab), White point needed
  • Converting D50 to D65 for rgb, White point and primaries needed.
  • Rgb to Linear - Only TransferFunction needed.

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 8, 2016

Note that I'm not angry, annoyed or anything like that, even if this turned into a bit of a rant. It's just that we seem to have slightly different views of what these type parameters are meant to do, and these views are currently colliding.

If we have to rename them, I'd rather name them separately. I think it will avoid confusion.

I thought they could be the same type. I don't see a reason why the type Srgb can't represent an an RGB standars, as set of primaries and a transfer function, at the same time.

I don't fully understand why [...] is simpler than having a bound on the trait (for whitepoit or primaries or any other trait).

The float types are not part of the transfer function type, so I would say that making the trait parametric over the float types would be restricting in the wrong way. A transfer function is what it is, no matter if it's used for f32 or f64. It's not like in colors, where they actually stores the values.

There will never be a case when from_linear will return f32 and into linear will return f64.

We don't know that. It may be improbable, but not impossible. The user is free to do whatever they want with them, including using them for multiple float types. fn do_something<T: TransferFunction>() -> ...{ can't easily be expressed if it has to have a type parameter. You would need multiple transfer functions, or maybe be able to do something like T: for<F: Float> TransferFunction<F>, but I don't know if that's possible. It's at least very obscure. You would otherwise have to enumerate the floats you need: T: TransferFunction<f32> + TransferFunction<f64>.

Do we do this for FromColor and IntoColor as well.

I don't think so. That's also a different case, since it's more connected to the types that implements them.

I think we should optimize for the most common case case here

Let's take a look at what's the most common case. These are the projects here on GitHub where "palette" occurs in a .toml file. They are only 13, some of them are false positives and some of them are no longer active. Not much for statistics, but still.

  • One includes the library, but doesn't use it. Let's ignore that one, since we don't know the intent.
  • One uses it as a pixel type.
  • Four uses it for just HSL/HSV <-> RGB conversion. Two of them it for gradients as well, and one of them does some more extensive shading and mixing as well.
  • One uses it for Lab <-> Lch <-> Rgb conversion.

That's six useful samples, one without clear intent, and six false positives. I guess we should really optimize for HSL/HSV <-> RGB (66.66% of the cases) and maybe gradients (33.33% of the cases), based on those. None of that needs any information about white point and primaries. Only the transfer function, for "proper" (linear) gradients and shading. 33.33% of all of them makes use of the built in sRGB encoding. It could be 50% if one of them didn't use its own encoding system.

I don't think that's a helpful source for optimization decisions, at the moment.

rather than choosing which characteristics are the important and which are not.

I'm trying to model the definitions of the colors and the only reason why the white point is everywhere is that it's a part of every definition, except two. Filling them in seemed to be more good than bad.

  • RGB is defined by a set of parameters and a white point.
  • HSL, HSV, HWB, etc. are all different ways to express RGB, so they have the same basic definition.
  • XYZ is more or less atomic in the sense that the physical meaning of its parameters are fixed.
  • xyY is an alternative way of expressing XYZ, although a bit warped, but its physical meaning is fixed.
  • Luma is defined by the white point, since it's a scale from black to white.
  • Lab is defined by the white point, since it's normalized by it.
  • Lch is a different way to express Lab.

This is the minimal knowledge you need to express their physical properties. Extending XYZ and xyY, the only non-preceptive spaces, with the white point makes sure they are also preceptive at the same at the type level, saying that they are meant to be observed under the same conditions.

We could express a limited set of the same properties by removing all of the type parameters and using names such as RgbSrgbD65F32, since that's how the type checker interprets it. This is also why I'm so much against using every parameter everywhere, because we would end up with the equivalence of XyzSrgbD65Gamma22F32 and XyzAdobeRgbD65LinearF32. They are both physically and visually similar, but still completely different types, because even more intent is baked into them. They are different because they appear to originate from or appear be intended to be converted to different RGB models.

So, the question isn't exactly what's important or not, but how much intent we want to reflect with the types. We must also remember that this isn't optional. There is no way to opt out of expressing some intent if there is a type parameter for it, so it's always or never.

The primaries are only really relevant when converting to and from the RGB group, and the transfer function is only really relevant when converting between the linear and non-linear RGB groups.

Converting to and from linear RGB should normally only happen on two occasions in a program; during input and output, and it will probably not be much worse than Srgb::from_linear(color).to_pixel(). Sometimes even less, since the output function may take Srgb as input, which makes type inference kick in and allows things like blit(x, y, color.encode()).

Converting to and from something outside the RGB group will only happen during more advanced color computations and that's what I would expect happens most, in a more advanced application. It will hopefully still not be too bad, since such an application is hopefully divided into functions, where the input and output types should define the RGB space well enough for type inference.

The way I see it is that there isn't much impact we can have on these two situations without making other situations way worse. Extending the bundled intent of every color with RGB primaries and transfer functions will reduce Srgb::from_linear(color).to_pixel() to color.encode().to_pixel() and (in worst case) LinearRgb::<Srgb>::from(xyz) to (in best case) xyz.to_rgb(). The side effect would be that LinearRgb::<Srgb>from(liner_adobe_rgb.to_xyz()) would turn into something like LinearRgb::<Srgb>from(liner_adobe_rgb.to_xyz().interpret_as()) or worse.

Sure, most users would probably stick to only one RGB model, but I think they can survive having to specify it once or twice, if that would mean that almost every other scenario wouldn't get worse. The combination of carefully designed traits, a good selection of type aliases, and the usual type inference will probably be helpful enough.

That's the way I'm reasoning.

@sidred
Copy link
Contributor

sidred commented Mar 9, 2016

I thought they could be the same type. I don't see a reason why the type Srgb can't represent an an RGB standars, as set of primaries and a transfer function, at the same time.

I am not sure I understand. Are you saying that in my example there is no need to rename the Srgb imports? It's a compile error. I my experience name space collisions are never fun to deal with.

The other way to look at this is to use it like the error type and prefix the module name

import primaries;
import transfer_fn;

impl RgbStandard for Srgb {
    type WhitePoint = D65;
    type Primaries = primaries::Srgb
    type TransferFunciton = transfer_fn::Srgb;
}

I guess that should work too.

Converting to and from linear RGB should normally only happen on two occasions in a program; during input and output, and it will probably not be much worse than Srgb::from_linear(color).to_pixel(). Sometimes even less, since the output function may take Srgb as input, which makes type inference kick in and allows things like blit(x, y, color.encode()).

The two conversions might happen in different parts of the program and we are losing that information. Think of a pipeline that does multiple operations on a color and at the end with the linear color I have no way of encoding it into the original form without manually keeping track of it. Is this a common enough use case to need an explicit type? I leave it up to you to decide.

Finally, any reason to choose the trait over a type alias. I remember discussing this but I can't find it.

type Srgb = Rgb<SrgbTf, SrgbPrimaries, D65>


// vs

pub struct Srgb;

impl<T: Float> RgbColorSpace<T> for Srgb {
    type TransferFunction = SrgbTransferFunction;
    type Primaries = SrgbPrimaries;
    type WhitePoint = D65;
}

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 9, 2016

The two conversions might happen in different parts of the program and we are losing that information.

I think it's ok to lose some of that information, as long as the risk of any mix-ups and accidents is minimized. The transfer function is pretty insignificant during computations, since it doesn't affect how the color is perceived (except if it's misinterpreted, but that's always a risk), and it will be known again when the color is encoded. No big deal, IMO. The primaries are more critical, since they do have an effect on the colors, but I don't know if they are critical enough to be as ubiquitous as the white point.

Think of a pipeline that does multiple operations on a color and at the end with the linear color I have no way of encoding it into the original form without manually keeping track of it. Is this a common enough use case to need an explicit type?

I'm very tired and may have misunderstood everything, but I'll try to answer anyway. The user will know what they start out with and what they want to end up with. That should be enough for the type inference to fill in most of the blanks. Let's say you start out with Srgb and want to end up with Srgb. Then this tiny example is completely valid:

//`pix` is some pixel representation, like `[u8; 3]`.
//`Srgb<T = f32>` is an alias for `Rgb<rgb_standards::Srgb, T>`.
let color = Srgb::from_pixel(&pix).into_linear(); //We just lost the transfer function
let lab = color.into_lab(); //We just lost the primaries
let lab_shifted = lab.shift_hue(180.0.into());
//The compiler knows that this will become an `Srgb`,
//so it know the primaries from the future.
let new_color = lab_shifted.into_rgb();
//The transfer function will also become known here
pix = Srgb::from_linear(new_color).into_pixel();

Expanding this to a larger application should introduce enough input and return types to aid the type inference and keep it simple enough. Still, it's hard to be sure about anything without trying it for real.

Finally, any reason to choose the trait over a type alias

I'm not sure what you are asking, but I'll sleep on it.

@sidred
Copy link
Contributor

sidred commented Mar 10, 2016

I have an alternate proposal.

Currently this is an issue

// this works
let xyz: Xyz<D65> = Xyz::with_wp(...)
let rgb: LinearRgb<SrgbPrimaries, D65> = Xyz::with_wp(...)

// this does not
let xyz: Xyz<SrgbPrimaries, D65> = Xyz::with_wp(...)
let rgb: LinearRgb<TransferFunction, SrgbPrimaries, D65> = Xyz::with_wp(...)

So remove the trait bounds from all colors. Ex

Struct Xyz<C, T>{
    x: T,
   ...
   color: C // whatever you want to call this
}

Add the 3 basic traits

// the basic traits
trait WhitePoint
trait Primaries
trait TransferFunction

Add the composing traits

trait ColorSpace{
    type TransferFucntion,
    type Primaries,
    type WhitePoint
}
impl WhitePoint for ColorSpace
impl Primaries for ColorSpace
impl TransferFunction for ColorSpace


trait LinearColor{
    type Primaries,
    type WhitePoint
}
impl WhitePoint for LinearColor
impl Primaries for LinearColor

All the conversions and adaptation need to be modified to use the base traits
Example

FromColor for Hsl{
     from_xyz<C: Primaries + WhitePoint>(xyz: Xyz<C>) -> Hsl<C>{
          ....
     }

     // no trait bounds here. Should work for trait 
     from_rgb<C>(rgb: Rgb<C,T>) -> Hsl<C>{
         ....
     }
}

So this will work now

let xyz_d50: Xyz<D65> = Xyz::with_color(...)

let xyz_linear: Xyz<LinearSrgb> = Xyz::with_color(...)

let xyz_color: Xyz<SrgbColorSpace> = Xyz::with_color(...)

// should work
xyz_d50.to_lab
// will not work. needs primaries trait and will be a compile error
xyz_d50.to_hsl()

// both should work
xyz_linear.to_lab()
xyz_linear.to_rgb()

//should work for gamma encoding
xyz_color.into_encoded()
xyz_color.to_lab()
xyz_color.to_rgb()

So you only opt in to the traits required and higher order traits can be passed through conversions

Issues:

  • Not sure if there are any issue in implementation, especially for traits like from and Into color and chromatic adaptation
  • Color enum - what should be the trait bound. Should it be limited be to LinearColor trait? Or the Color Space trait? For simplicity, limiting to ColorSpace might be a good idea.

Further optimizations:

Can the trait made optional i.e ``Xyz and Xyz<D50,T> or Xyz<Srgb,T>` should work. I can't think of a way this will work. Maybe using a wrapper type? Might get too complicated

@Ogeon
Copy link
Owner Author

Ogeon commented Mar 10, 2016

That's a very interesting idea, and it's actually a quite good compromise. The only problems I can see is that Xyz<D65> and Xyz<Srgb> won't be the same type, but converting is trivial, and (above all) that there may be conflicts between implementations for SomeType<C: WhitePoint> and SomeType<C: ColorSpace>. I don't know, though. We may be lucky.

bors bot added a commit that referenced this issue Feb 17, 2018
60: [WIP] Generalize the RGB types over RGB standards r=Ogeon a=Ogeon

This changes the `Rgb` type to have a type parameter that implements the `RgbStandard` trait. This trait defines a set of primaries, white point and transfer function. It does also make the RGB types more uniform and type safe.

Closes #66, closes #31, closes #58
@bors bors bot closed this as completed in #60 Feb 17, 2018
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 a pull request may close this issue.

2 participants