Change internal color representation to SRGBA

[jeffreyrosenbluth]

Please do not merge. This is a preliminary implementation of the proposal below.

[bergey]

From IRC:

<bergey> I'm having trouble finding the Data.Colour function that actually
         does the same thing as toRGBAUsingSpace.  Can you point it out?
<Martingale> bergey there is no function actually but `toSRGB` is all that's
             really needed.
<bergey> Your patch does 2 different things.  It changes colorToSRGBA to use a
         faster code path in Colour.  And it removes the exported function
         toRGBAUsingSpace.
<bergey> Even though our Backends don't use the latter, I don't see the reason
         for removing it.  [15:50]
<Martingale> bergey I dont see why any potential use for it, but if you want
             to keep it I wont object.

[jeffreyrosenbluth]

Proposal

By using Data.Colour the way we do, that is using AlphaColour Double as our internal representation of color, we are essentially choosing linear ITU-R BT.709 RGB color space (hence forth called Linear 709). I propose we switch to non-linear sRGBA. In code that would be something like this.

data SRGBA = SRGBA {_red :: !Double, _green :: !Double, _red :: !Double, _alpha :: !Double}

class Color c where
  toSRGBA :: c -> SRGBA
  fromSRGBA :: SRGBA -> c

Here are the reasons I think this makes more sense than what we do currently:

• Every single backend we have both official and unofficial takes an sRGB(A) value for rendering. And many of our color inputs are in sRGB. Converting from sRGBA to Linear 709 and back to sRGBA, or just from Linear 709 to sRGBA is time consuming and wasteful. Even if we dramatically speed up Data.Colour, there is no need for diagrams to be doing this conversion.
• sRGB and Linear 709 are very similar, they share the same chromaticities (i.e. vales for red, green and blue), white point and gamut. The only difference it the linearity and gamma correction (i.e. transfer function). So it is not as if Linear 709 gives us a larger range of color.
• The primary benefit of Data.Colour is blending, (because it operates in a linear space) which I have not seen come up often in diagrams code. However there is nothing preventing a user from utilizing Linear 709 and the functions in Data.Colour, since we will have an instance of Color c for AlphaColour Double and others.
• sRGB is somewhat of a de-facto standard for color these days, both in digital photography and computer graphics. This is a bit of an exaggeration, but I believe mostly accurate.

[byorgey]

I don't know much about all of this color stuff, but this proposal sounds sensible to me (modulo seeing the details of how it works out in practice). One worry re: practice is that we would no longer be able to use all the color names from Data.Colour.Names (since they construct Colour values instead of our new SRGBA type). But hopefully there's a sensible way around that.

[jeffreyrosenbluth]

Yes, I believe there are sensible ways around it, I think we should make Colour and instance of Color so that we could just use the names as is - although this puts us right back to our performance issue. But i think it would be better to create a module with the same names defined as SRGBA, and import qualified if need be, it's not hard there are only something like 332 of them. I would be willing to make the conversion - easy to do in a good text editor.

[byorgey]

True, duplicating the names in a module of our own is not really that bad (since we can just do it once using search & replace and then be done with it).

[bergey]

This proposal seems to be that Diagrams should sacrifice correctly modeling color in favor of a small performance boost. I don't think this is a good tradeoff.

My understanding is that Colour tries hard to provide a type which is abstract, not representing a particular color space, and to do blending in CIE perceptual space. @jeffreyrosenbluth, what makes you think that it leaks its internal representation into the interface?

I think it does leak a little, in that toSRGB is faster than toRGBUsingSpace RGBSpace (by inspection; I haven't measured this). But the first patch (d7b3ecc) should let all current Diagrams code use the fast version. I don't think there's much performance to gain by skipping the roundtrip to linear space. For example, when I run a factorization diagram with profiling, 9.7% of runtime is spent in matrix inverse and multiplication (which are avoided by the first patch), but less than 0.5% is spent converting between linear and sRGB spaces.

I am looking forward to using the linear space output in a future OpenGL 3D backend, per recommendations in Colour

[jeffreyrosenbluth]

Neither sRGB or Linear 709 is more "accurate" they are just different choices. Each one has it's advantages and disadvantages. Linear is what you need for correct blending and non-linear is what you need for correct rendering (at least in the current backends). In a linear space there is not enough definition in the darker shades and too much in the lighter (i.e. more than the eye can detect). If we never converted to non-linear, that is never made a gamma correction we would need about 11 bits per channel as opposed to 8 to get the quality we now have. Since we don't do any blending there is no need to convert all of our color values to linear, which is how they are stored in Data.Colour.

I don't see why the first patch would avoid the matrix inversion and multiplication, it shouldn't. It should the same as before. If it is than I'm misunderstanding something about Data.Colour. Did you profile it after the patch?

That being said I'm not seeing a tremendous speed increase by using the version in this branch. And @fryguybob's Mandelbrot example only takes him 7 seconds to run on his hardware. So perhaps the whole impetus for faster color is not necessary.

I'll test both versions on Factorisaton tomorrow and see how much speed up we get.

[bergey]

I didn't mean to suggest that Linear 709 was more accurate. I meant to say that I think Colour does give us a wider range of representable colors than are in gamut for sRGB. I thought that was the whole point of the library.

Clearly this isn't important to the majority of users who will use named colors from colour or palette, but I like having the infrastructure there.

toSRGB calls toRGB instead of using toRGBUsingGamut. toRGB uses knowledge of the internal representation, whereas toRGBUsingGamut has the expensive operations. I haven't profiled yet, but that's why I expect it to be much faster.

[bergey]

diagrams-lib master, note the cost centers for inverse and mult:

    total time  =        3.07 secs   (3072 ticks @ 1000 us, 1 processor)
    total alloc = 3,409,488,472 bytes  (excludes profiling overheads)

COST CENTRE     MODULE                  %time %alloc

renderSeg       Graphics.Rendering.SVG   17.8   26.4
atParam         Diagrams.Segment         11.7   14.0
lapp            Diagrams.Core.Transform   7.9   10.1
inverse         Data.Colour.Matrix        5.9    3.2
quadForm        Diagrams.Solve            5.6    3.8
getEnvelope.\   Diagrams.Segment          5.5    5.3
quadForm.q      Diagrams.Solve            4.2    3.2
moveOriginTo.\  Diagrams.Core.Envelope    3.8    5.8
mult            Data.Colour.Matrix        3.8    2.7
*^              Diagrams.TwoD.Types       3.4    2.7
<.>             Diagrams.TwoD.Types       3.1    1.6

Now with the patch d7b3ecc applied, using toSRGB and avoiding the inverse:

    total time  =        2.69 secs   (2690 ticks @ 1000 us, 1 processor)
    total alloc = 3,162,115,344 bytes  (excludes profiling overheads)

COST CENTRE     MODULE                  %time %alloc

renderSeg       Graphics.Rendering.SVG   20.6   28.5
atParam         Diagrams.Segment         12.3   15.0
lapp            Diagrams.Core.Transform  10.2   10.8
quadForm        Diagrams.Solve            7.0    4.1
getEnvelope.\   Diagrams.Segment          6.3    5.7
quadForm.q      Diagrams.Solve            4.4    3.5
<.>             Diagrams.TwoD.Types       3.9    1.7
moveOriginTo.\  Diagrams.Core.Envelope    3.8    6.2
*^              Diagrams.TwoD.Types       3.3    3.0
transform.\.v'  Diagrams.Core.Envelope    3.0    2.4

I should really have run these several times to make comparing the wall clock legitimate. But I think the absence of expensive cost centers in Data.Colour after the patch is convincing. I haven't profiled 20b2af0 because it requires further patches to diagrams-contrib.

[jeffreyrosenbluth]

Ah that's great, so I guess the first patch was enough after all. Which means, we should probably revert back that point and merge.

One thing I'm not clear on is that the documentation for Data.Colour says that the internal representation is Linear 709, which has the same gamut as sRGB, but I haven''t look at his CIE module so I'm not sure how he handles this bigger space. Anyway it's not important since my first patch seems to have done the trick and openGL will likely use a lot of blending.

[bergey]

As I was discussing with luite on IRC, I think what's going on is that the internal representation isn't actually clipped to the 709 gamut. So 709 provides the basis vectors, but they're a basis for a larger space. But I could easily be misunderstanding.

[jeffreyrosenbluth]

That would make sense. It would be interesting to talk to Russ O'Connor and find out what's really going on. Either way I'll roll back the changes and then we just have to decide whether or not to keep and export toRGBAUsingSpace. I'm fine either way but I think this function really belongs inData.Colour so if we are not using it in diagrams we should not have it. But as I say, I don't feel strongly about it.

[bergey]

You make a good point about toRGBUsingSpace really belonging it Data.Colour. I'm fine if you want to merge now, or we could wait for a third opinion.

[jeffreyrosenbluth]

I'll go ahead and merge, we can always make changes down the road.

On Mon, Jan 27, 2014 at 8:42 AM, Daniel Bergey notifications@github.comwrote:

You make a good point about toRGBUsingSpace really belonging it
Data.Colour. I'm fine if you want to merge now, or we could wait for a
third opinion.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/149#issuecomment-33367998
.