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

Common RGB struct #31

Closed
sidred opened this issue Feb 1, 2016 · 23 comments · Fixed by #60
Closed

Common RGB struct #31

sidred opened this issue Feb 1, 2016 · 23 comments · Fixed by #60
Milestone

Comments

@sidred
Copy link
Contributor

sidred commented Feb 1, 2016

Have a common rgb struct

struct RgbColor<RgbVariant, WP, T> {
    r: T,
    g: T,
    b: T,
    _color_space: PhantomData<RGBVariant>,
    _white_point: PhantomData<WP>,
}

then we can define the individual colors as

const Rgb = RgbColor<LinearSrgbSpace, D65, T>
const Srgb = RgbColor<SrgbSpace, D65, T>
const AdobeRgb = RgbColor<AdobeSpace, D65, T>

conversions will be

impl<RgbVariant, WP,T: Float> From<RgbColor<RgbVariant, WP,T>> for Xyz<T> {
    fn from(rgb: Rgb<T>) -> Xyz<T> {
      let conv_matrix = RgbVariant::get_matrix();
      let wp_xyz = WP::get_xyz();


        Xyz {
           .... calculations
        }
    }
}

impl<RgbVariant, WP,T: Float> From<Xyz<T>> for RgbColor<RgbVariant, WP,T> {
    fn from(xyz: Rgb<T>) -> RgbColor<RgbVariant, WP,T> {
      let conv_matrix = RgbVariant::get_matrix();
      let wp_xyz = WP::get_xyz();


        Rgb {
           .... calculations
        }
    }
}

@Ogeon
Copy link
Owner

Ogeon commented Feb 1, 2016

Something like this could be good for linear RGB. I'm not as sure when it comes to gamma encoded RGB, since it may have additional parameters (like the gamma in GammaRgb). Keeping linear and non-linear RGB separate could simplify the RgbColor type a bit:

struct RgbColor<P: Primaries, T: Float> {
    red: T,
    green: T,
    blue: T,
    _primaries: PhantomData<P>,
}

trait Primaries {
    type WhitePoint;

    //Maybe also functions returning the primaries as xyY, etc.
}

...assuming that white point and primaries are connected. I'm not sure if there are any odd stuff going on in that area, so it has to be checked first.

@Ogeon
Copy link
Owner

Ogeon commented Feb 1, 2016

Simple power law gamma is a thing, but it's not an "official" specification. Just another color compression/non-linearization method and it's sometimes used as a quick-and-dirty sRGB substitute.

@sidred
Copy link
Contributor Author

sidred commented Feb 1, 2016

Yeah, I misunderstood your comment.

@sidred
Copy link
Contributor Author

sidred commented Feb 1, 2016

I didn't realize that currently we are assuming all the rgb types are gamma corrected.

We will still need the WP trait on the RgbColor struct. It will be easier for conversions Rgb -> XYZ -> Lab using impls.

Not sure how to keep track of gamma. It is defined for each color type, so we can maybe get it on the rgb variant trait

trait RgbVariant {

    //functions returning the primaries as xyY, etc.
    // function returning gamma for the color space

   // provide default implementation for gamma corrrection
   fn gamma_correction()
}

// override the default gamma conversion for SrgbSpace which does not use a single value for gamma

One issue is that we are not keeping track of whether the color has been corrected for gamma and relying on the end user to do it.

@Ogeon
Copy link
Owner

Ogeon commented Feb 1, 2016

Rgb is currently always assumed to be linear and it's up to the user to find out what the source and destination gamma/encoding should be. Those may even be different, depending on what the program does, and I'm not sure how feasible it is to keep track of the source gamma throughout the pipeline. It doesn't affect any calculations, besides encoding/decoding, and an arbitrary gamma constant will just be dead weight. It's even possible that no encoding or decoding is performed at all.

We will still need the WP trait on the RgbColor struct. It will be easier for conversions Rgb -> XYZ -> Lab using impls.

That would be the type WhitePoint; in the Primaries trait. It's reachable through P as P::WhitePoint.

@Ogeon
Copy link
Owner

Ogeon commented Feb 1, 2016

HSV, HSL, etc. could also be made parametric in the same way as RGB, to reflect what they are based on.

@sidred
Copy link
Contributor Author

sidred commented Feb 2, 2016

I am more inclined towards separating rgb variant and white point. It is more flexible.
For most users this will not matter as they will use Srgb defined as either RgbColor<SrgbPrimaries, T> or RgbColor<SrgbSpace, D65, T>

Consider the case of when Srgb with D50 is needed. If using primaries, this needs to be explicitly defined. However if they are separated, it will be a simple RgbColor<SrgbSpace, D50, T> for the end user. Converison can work through impls on the RgbColor

The way I see conversions working is on the RgbColor itself, rgb points from the color space and white point xy's from the whitepoint trait. Depending on how things work out, we can auto derive conversions on the RgbVariants trait ( via bradford transfrom ) and provide the optimized matrices for the defined color spaces like sRgb-d65 and adobeRgb-d50 etc.

It will be good to have some implementation to see how it shapes up.

@Ogeon
Copy link
Owner

Ogeon commented Feb 2, 2016

Yes, a prototype would be good. Having the same primaries, but a different white point seems strange to me, but I guess it's mathematically possible if they are scaled differently. I'm trying to find something that explains exactly how they are connected (if they are)...

@sidred
Copy link
Contributor Author

sidred commented Feb 2, 2016

One use case I see is that most printers, monitors, camera's etc come with an ICC color profile and these are usually defined with the D50 white point. So Srgb D50 is definitely useful

@Ogeon
Copy link
Owner

Ogeon commented Feb 2, 2016

It's still weird, because the primaries wouldn't look the same. They would intuitively be more blue, or red, or whatever. I'm currently reading section B.2 in this specification to see what they have to say about chromatic adaption.

@Ogeon
Copy link
Owner

Ogeon commented Feb 2, 2016

Example: If D65 is selected as the sRGB adapted white, the chromatic adaptation transform will go from D65 to D50, the resulting D50 values will be encoded in the mediaWhitePoint tag, and the sRGB primaries will be adapted to D50 for encoding in the ICC profile. However, if D50 were selected as the sRGB adapted white, chromatic adaptation would not be necessary, D65 values would be encoded in the mediaWhitePoint tag, and the sRGB primaries would be used in the profile without adaptation.

It's a bit cryptic. At least the last sentence. What I think it's trying to say is that primaries and white point is completely disconnected, but I'm not 100% sure. If that's the case, then we should also disconnect them, as you are suggesting. Your SrgbSpace would basically represent the ITU-R BT.709 primaries.

@sidred
Copy link
Contributor Author

sidred commented Feb 13, 2016

After looking at the test data, maybe a good idea to encode the gamma/linear trait in the Rgb. Looking at the options:

Assume linear

  • Current behaviour
  • User needs to manually correct for gamma and this depends on the rgb space.
  • Easy to miss the gamma correction step.
  • Not clear when gamma correction is needed.

Assume Gamma

  • Most values shown are gamma corrected.
  • Every calculation like mix, add etc must convert to linear and from linear.
  • Not practical.

Add GammaEncoding trait

  • Srgba becomes RgbColor<Linear, Srgb, D65, T > or RgbColor<Gamma, Srgb, D65, T>
  • What should be the default?
  • How should it be made clear to the users that rgb's loaded from images or color list from somewhere are gamma encoded and should be loaded as such.

@Ogeon
Copy link
Owner

Ogeon commented Feb 13, 2016

I have some more questions:

  • How do we express the gamma exponent?
  • What happens with the gamma parameter when the color is converted to ex. XYZ? Does it go there, too?

What should be the default?

sRGB seem to be the big one.

How should it be made clear to the users that rgb's loaded from images or color list from somewhere are gamma encoded and should be loaded as such.

I'm not sure we can encode this into the types at all. Nothing will prevent anyone from just using Rgb::new and doing whatever they want. There is no way to detect an encoding and I wouldn't want to restrict the users too much in this area. The best option I know of is to have lots of information and examples where the proper procedure is used.

An additional way to discourage direct use of Rgb could be to rename it to LinearRgb. It's a long name and it's there, in your face, all the time. People would then hopefully think one more time before using it directly.

@sidred
Copy link
Contributor Author

sidred commented Feb 13, 2016

How do we express the gamma exponent?

It is part of the Rgb variant. The gamma values are listed here http://www.brucelindbloom.com/WorkingSpaceInfo.html#Specifications. Only Srgb has a non standard calculation. The rest all are color^gamma and its inverse. So we can enocde it directly in the RgbVariant trait.

What happens with the gamma parameter when the color is converted to ex. XYZ? Does it go there, too?

Gamma is not a parameter as such, it just indicates whether the color is gamma corrected or not. Xyz conversions are only to the Linear Rgb Types.

@Ogeon
Copy link
Owner

Ogeon commented Feb 13, 2016

It is part of the Rgb variant. The gamma values are listed here http://www.brucelindbloom.com/WorkingSpaceInfo.html#Specifications. Only Srgb has a non standard calculation. The rest all are color^gamma and its inverse. So we can enocde it directly in the RgbVariant trait.

Sure, but I meant the GammaRgb case. I'm having a feeling that it will be easier to keep encoded and linear RGB apart for some more time, and not cram everything into one type. I know that I'm usually promoting less duplication, but I think that this case may be fit for an exception. The encoded types are only used as a temporary state, so they will not have any fancy operations or anything. What about this structure:

//Encoded with standardized encodings, such as sRGB
//(maybe with a Whitepoint parameter)
struct EncodedRgb<Encoding, T> {...}

//Choose-your-own-gamma encoded RGB
struct GammaRgb<Primaries, Whitepoint, T> {...}

//Good old linear RGB
struct Rgb<Primaries, Whitepoint, T> {...}

EncodedRgb and GammaRgb could also bundle an alpha component, like they currently do, for the sake of simplicity. An other alternative is to make them wrappers, like Alpha, but that may be too much.

@sidred
Copy link
Contributor Author

sidred commented Feb 13, 2016

You'll still have to encode gamma as part of the RgbVariant trait for conversions to and from linear. Not sure if separating them buys us anything.

We'll most likely define

type Rgb<T> = RgbColor<Gamma, SrgbSpace, T>;
type LinearRgb<T> = RgbColor<Linear, SrgbSpace, T>;

which will cover most of the use cases. We can discuss this more when implementing it. It will give us a much clearer picture of how the types interact.

Another thing is the add, sub, mul traits and how will they work in the gamma space. I am not 100% clear on this.

@Ogeon
Copy link
Owner

Ogeon commented Feb 13, 2016

Another thing is the add, sub, mul traits and how will they work in the gamma space. I am not 100% clear on this.

They don't, really. That's the thing with this whole library. You won't get good results in gamma space, so it's greatly discouraged. The same goes for mixing, scaling pictures, rotating, whatever. There's still the possibility to force gamma encoded values to be taken as linear, as an escape hatch, but that's it. I would really like to keep the distinction as obvious as possible, but with a small escape hatch for when the user knows what's up.

Linear and encoded RGB will effectively be two different types with the same kind of content, if this same distinction is preserved, so the only duplication will more or less be the structs themselves, the constructors, and nothing else.

Those things, and the custom gamma exponents, are why I'm thinking that it could be less complex to keep them separated.

We can discuss this more when implementing it. It will give us a much clearer picture of how the types interact.

Possibly, but we must think of it as a prototype, and not necessarily the final implementation. No decision has been made, so no strings attached.

@sidred
Copy link
Contributor Author

sidred commented Feb 13, 2016

Possibly, but we must think of it as a prototype, and not necessarily the final implementation. No decision has been made, so no strings attached.

Exactly

@Ogeon
Copy link
Owner

Ogeon commented Feb 13, 2016

I'm just making sure we are on the same page 😄

@Ogeon
Copy link
Owner

Ogeon commented Feb 23, 2016

I think this fits within the scope of 0.3.0, so I'm scheduling it, but we can always reschedule if it turns out to be problematic in any way.

@Ogeon Ogeon added this to the 0.3.0 milestone Feb 23, 2016
@sidred
Copy link
Contributor Author

sidred commented Mar 5, 2016

Here is how I am planning to implement this

  • Existing Rgb will be the gamma encoded value (which seems to be the default in most cases) with the added primaries trait struct Rgb<Primaries, Whitepoint, T> {...}
  • Add linear rgb struct LinearRgb<Primaries, Whitepoint, T> {...}
  • Provide converisions between the two.
  • Define Srgb<T> = Rgb<SrgbSpace, D65, T>>, LinearSrgb<T> = LinearRgb<SrgbSpace, D65, T>> etc

Any thoughts?

What is still unclear is whether the operations like blend, add , multiply etc should be converted into linear before applying them or apply as is for each linear and gamma encoded types.

@Ogeon
Copy link
Owner

Ogeon commented Mar 5, 2016

I'm writing up an additional proposal for the pixel encoding types. It's very similar to what you are planning and will address some of the questions.

@Ogeon
Copy link
Owner

Ogeon commented Mar 5, 2016

I think it's good to take care of what's discussed in #58 in combination with this, since they are so closely connected. It could also be done as a two step process, where the linear RGB struct is changes separately from the encoded variants, in whatever order is best.

What is still unclear is whether the operations like blend, add , multiply etc should be converted into linear before applying them or apply as is for each linear and gamma encoded types.

My proposal is to not implement them for encoded types at all, and thereby prevent any accidental mixups between what's linear and what's not.

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