Add support for converting Colors to linear RGB; Fix relevant filters

Add support for converting Colors to linear RGB; Fix relevant filters

Moves get{Linear,Device}RgbLUT out of ImageBuffer.cpp and into the (new)
ColorSpace.cpp and a (new) namespace ColorSpaceUtilities.
Implement ColorSpaceUtilities::convertColor and getConversionLUT, and use them in a
new FilterEffect::adaptColorToOperatingColorSpace method, that derived
classes can use.

With the above infrastructure in place, use it in FEDropShadow to make
sure the result of the (conceptual) flood operation is in correct color
space (== the operating colorspace of the FEDropShadow).
Also use it to transform the lighting-color of the lighting primitives.

BUG=336440
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165767

[Stephen Chennney, 2014-01-22 14:55:01]

There are other places where we do color space conversions for filters, I think.
Are there additional places where you can switch to the new adaptTo... method?

[Stephen Chennney, 2014-01-22 14:56:00]

https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/fil...
File Source/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/fil...
Source/platform/graphics/filters/FilterEffect.h:187: Color
adaptColorToOperatingColorSpace(const Color& deviceColor);
This is the method that could be used in more places, maybe.

[f(malita), 2014-01-22 15:09:32]

https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/Col...
File Source/platform/graphics/Color.h (right):

https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/Col...
Source/platform/graphics/Color.h:127: Color convertToLinear() const;
This is just a bit odd - Color feels too low-level for color space assumptions.

I think at this level we should have a generic convertToColorSpace(ColorSpace),
and relocate the assumption to FilterEffect::adaptColorToOperatingColorSpace().

Or maybe there's a better home for the color space conversion method (ColorSpace
class? see below)...

https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/Col...
File Source/platform/graphics/ColorSpace.h (right):

https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/Col...
Source/platform/graphics/ColorSpace.h:42: const Vector<uint8_t>&
getLinearRgbLUT();
Now that these have external visibility, they should probably be encapsulated
somehow.

Time for a ColorSpace class maybe? It would be home for:

* ColorSpaceType enum
* static get*LUT methods
* static createColorSpaceFilter() (to be relocated from ImageBuffer, where it
doesn't really belong)
* static convertColor() (to be relocated from Color)

[fs, 2014-01-22 16:06:07]

On 2014/01/22 14:55:01, Stephen Chenney wrote:
> There are other places where we do color space conversions for filters, I
think.
> Are there additional places where you can switch to the new adaptTo... method?

TLDR; I don't think there're additional places.

There's two *-color properties on (SVG) filters - flood-color and
lighting-color. The former is a bit special since as specified,
color-interpolation-filters don't apply to feFlood (with the reasoning being "no
interpolation"). feDropShadow however - which is essentially a "composite
filter", c-i-f will apply to (some of) the intermediate steps, and hence there
would be a conversion from sRGB/DeviceRGB to LinearRGB. lighting-color is not
exceptioned in any such way. (feFlood could potentially use it if all the
referencing primitives are using linearRGB, to save a conversion step, but
that's an optimization, and needs the described context to "work".)

On 2014/01/22 15:09:32, Florin Malita wrote:
>
https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/Col...
> File Source/platform/graphics/Color.h (right):
> 
>
https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/Col...
> Source/platform/graphics/Color.h:127: Color convertToLinear() const;
> This is just a bit odd - Color feels too low-level for color space
assumptions.
>
> I think at this level we should have a generic
convertToColorSpace(ColorSpace),
> and relocate the assumption to
FilterEffect::adaptColorToOperatingColorSpace().

Ok, I think I catch your drift here. Keep Color as an unlabeled blob. (Let
callers tag, assume etc.)
 
> Or maybe there's a better home for the color space conversion method
(ColorSpace
> class? see below)...
> 
>
https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/Col...
> File Source/platform/graphics/ColorSpace.h (right):
> 
>
https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/Col...
> Source/platform/graphics/ColorSpace.h:42: const Vector<uint8_t>&
> getLinearRgbLUT();
> Now that these have external visibility, they should probably be encapsulated
> somehow.
> 
> Time for a ColorSpace class maybe? It would be home for:
> 
> * ColorSpaceType enum
> * static get*LUT methods
> * static createColorSpaceFilter() (to be relocated from ImageBuffer, where it
> doesn't really belong)
> * static convertColor() (to be relocated from Color)

How about a ColorSpaceUtilities namespace (no state yet, and only static
methods), and leaving the enum as ColorSpace (I'd hate to turn that nice-looking
name into Foo::Type or similar...). And if ok, I'd like to leave the
createColorSpaceFilter out of this CL (can relocate that one in a follow-up CL)
- I had some qualms about that one because it returns a Skia type...

Sound OK?

[f(malita), 2014-01-22 16:54:27]

On 2014/01/22 16:06:07, fs wrote:
>
https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/Col...
> > Source/platform/graphics/ColorSpace.h:42: const Vector<uint8_t>&
> > getLinearRgbLUT();
> > Now that these have external visibility, they should probably be
encapsulated
> > somehow.
> > 
> > Time for a ColorSpace class maybe? It would be home for:
> > 
> > * ColorSpaceType enum
> > * static get*LUT methods
> > * static createColorSpaceFilter() (to be relocated from ImageBuffer, where
it
> > doesn't really belong)
> > * static convertColor() (to be relocated from Color)
> 
> How about a ColorSpaceUtilities namespace (no state yet, and only static
> methods), and leaving the enum as ColorSpace (I'd hate to turn that
nice-looking
> name into Foo::Type or similar...). And if ok, I'd like to leave the
> createColorSpaceFilter out of this CL (can relocate that one in a follow-up
CL)
> - I had some qualms about that one because it returns a Skia type...
> 
> Sound OK?

SGTM, thanks.

[Stephen Chennney, 2014-01-22 20:44:54]

On 2014/01/22 16:54:27, Florin Malita wrote:
> On 2014/01/22 16:06:07, fs wrote:
> >
>
https://codereview.chromium.org/134733016/diff/1/Source/platform/graphics/Col...
> > > Source/platform/graphics/ColorSpace.h:42: const Vector<uint8_t>&
> > > getLinearRgbLUT();
> > > Now that these have external visibility, they should probably be
> encapsulated
> > > somehow.
> > > 
> > > Time for a ColorSpace class maybe? It would be home for:
> > > 
> > > * ColorSpaceType enum
> > > * static get*LUT methods
> > > * static createColorSpaceFilter() (to be relocated from ImageBuffer, where
> it
> > > doesn't really belong)
> > > * static convertColor() (to be relocated from Color)
> > 
> > How about a ColorSpaceUtilities namespace (no state yet, and only static
> > methods), and leaving the enum as ColorSpace (I'd hate to turn that
> nice-looking
> > name into Foo::Type or similar...). And if ok, I'd like to leave the
> > createColorSpaceFilter out of this CL (can relocate that one in a follow-up
> CL)
> > - I had some qualms about that one because it returns a Skia type...
> > 
> > Sound OK?
> 
> SGTM, thanks.

SGTM too.

[fs, 2014-01-23 11:24:37]

Updated according to discussion.

[Stephen Chennney, 2014-01-23 16:04:04]

Personally I would prefer that the ColorSpaceUtilities be a class in a separate
file. That would let you avoid the unnamed namespace. I don't feel very
strongly, however.

https://codereview.chromium.org/134733016/diff/220001/Source/platform/graphic...
File Source/platform/graphics/ColorSpace.cpp (right):

https://codereview.chromium.org/134733016/diff/220001/Source/platform/graphic...
Source/platform/graphics/ColorSpace.cpp:63: DEFINE_STATIC_LOCAL(Vector<uint8_t>,
deviceRgbLUT, ());
Given that the return value gets converted immediately to a const unit_8 array,
why not just define it that way, and return a const unit_8* here too? I'm not
seeing any value in the Vector in this case.

[fs, 2014-01-23 18:22:46]

On 2014/01/23 16:04:04, Stephen Chenney wrote:
> Personally I would prefer that the ColorSpaceUtilities be a class in a
separate
> file. That would let you avoid the unnamed namespace. I don't feel very
> strongly, however.

I can get rid of the anonymous namespace - turning them (back) into static...
 
>
https://codereview.chromium.org/134733016/diff/220001/Source/platform/graphic...
> File Source/platform/graphics/ColorSpace.cpp (right):
> 
>
https://codereview.chromium.org/134733016/diff/220001/Source/platform/graphic...
> Source/platform/graphics/ColorSpace.cpp:63:
DEFINE_STATIC_LOCAL(Vector<uint8_t>,
> deviceRgbLUT, ());
> Given that the return value gets converted immediately to a const unit_8
array,
> why not just define it that way, and return a const unit_8* here too? I'm not
> seeing any value in the Vector in this case.

Sure, done.

[Stephen Chennney, 2014-01-24 16:14:43]

LGTM. It's a pleasure to work with you.

[commit-bot: I haz the power, 2014-01-24 16:46:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/134733016/360001

[commit-bot: I haz the power, 2014-01-24 18:30:46]

Change committed as 165767