Adding color space conversion code

Adding color space conversion code

This adds the missing color space conversion to the GPU path. Basically, whenever a color conversion is required, an SkColorFilter object is created and inserted into the filter tree to perform that color space conversion.

The luts used in ImageBuffer were changed to unsigned 8 bit luts (the extra bits were unused anyway) and made publicly available in that class so that they can be used as is by SkTableColorFilter.

R=senorblanco@chromium.org
BUG=229689
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151966

[sugoi1, 2013-05-02 18:03:28]

https://codereview.chromium.org/14856010/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp (right):

https://codereview.chromium.org/14856010/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp:182:
FilterColorSpacePair key((ptrdiff_t)effect, colorSpace);
I used ptrdiff_t here for simplicity. It adds less code than having to do all
the template instantiations required for FilterEffect* and pretty much does the
same thing. I can change it if that's an issue.

[Stephen White, 2013-05-02 18:45:17]

Looks like a great start.  Please add a new test, with subtests for the
different conversions, and null conversions.

https://codereview.chromium.org/14856010/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp (right):

https://codereview.chromium.org/14856010/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp:182:
FilterColorSpacePair key((ptrdiff_t)effect, colorSpace);
I'm not too fond of this c-style cast.  If possible, IWBN to actually
instantiate the right templates for FilterEffect* in the hashmap.

[sugoi1, 2013-05-02 20:00:18]

Will work on test next.

https://codereview.chromium.org/14856010/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp (right):

https://codereview.chromium.org/14856010/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp:182:
FilterColorSpacePair key((ptrdiff_t)effect, colorSpace);
On 2013/05/02 18:45:17, Stephen White wrote:
> I'm not too fond of this c-style cast.  If possible, IWBN to actually
> instantiate the right templates for FilterEffect* in the hashmap.

Done.

https://codereview.chromium.org/14856010/diff/5001/Source/core/platform/graph...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.h (right):

https://codereview.chromium.org/14856010/diff/5001/Source/core/platform/graph...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.h:38: typedef
PtrHash<WebCore::FilterEffect*> Hash;
Turns out there is a PtrHash. I don't know how I missed it the first time...

[Stephen White, 2013-05-02 20:22:58]

https://codereview.chromium.org/14856010/diff/5001/Source/core/platform/graph...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.h (right):

https://codereview.chromium.org/14856010/diff/5001/Source/core/platform/graph...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.h:36: namespace WTF
{
Maybe this chunk could be below the namespace WebCore below, and put the
FilterEffect forward-declaration back where it was, to save on one namespace?

[sugoi1, 2013-05-02 20:34:37]

On 2013/05/02 20:22:58, Stephen White wrote:
>
https://codereview.chromium.org/14856010/diff/5001/Source/core/platform/graph...
> File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.h (right):
> 
>
https://codereview.chromium.org/14856010/diff/5001/Source/core/platform/graph...
> Source/core/platform/graphics/filters/SkiaImageFilterBuilder.h:36: namespace
WTF
> {
> Maybe this chunk could be below the namespace WebCore below, and put the
FilterEffect forward-declaration back where it was, to save on one namespace?

Right, I made the change locally and it seems to work fine, so it'll be there
when I upload the next version.

[sugoi1, 2013-05-10 14:38:44]

- Added unit test
- Added new LayoutTests (and corresponding entry in TestExpectations)
- Fixed SkiaImageFilterBuilder::build() to work with NULL filters (AKA
SourceGraphic)
- Added conversion back to Device color space at the end of filter tree, if
needed

https://codereview.chromium.org/14856010/diff/13001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/ImageFilterBuilderTest.cpp (right):

https://codereview.chromium.org/14856010/diff/13001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageFilterBuilderTest.cpp:113:
EXPECT_EQ(filter->countInputs(), 1); // Should be CS (L->D)
I only check whether or not the filter is a color filter and how many children
it has. Seems enough for now, but I can add more checks, if needed.

https://codereview.chromium.org/14856010/diff/13001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/Filter.h (right):

https://codereview.chromium.org/14856010/diff/13001/Source/core/platform/grap...
Source/core/platform/graphics/filters/Filter.h:35: Filter(const AffineTransform&
absoluteTransform) : m_renderingMode(Unaccelerated),
m_absoluteTransform(absoluteTransform) { }
Just fixing a bad initialization order warning

[Stephen White, 2013-05-10 14:56:18]

https://codereview.chromium.org/14856010/diff/13001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp (right):

https://codereview.chromium.org/14856010/diff/13001/Source/core/platform/grap...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp:187: // Note
that we may still need the color transform even if the filter is null
Hmm.. so effect is always going to have two entries in the map, one mapping to
NULL and one mapping to CS(NULL)? I guess that makes sense, for when it's
optimized out or creation fails? Just wondering when you noticed this.

https://codereview.chromium.org/14856010/diff/13001/Source/core/rendering/Fil...
File Source/core/rendering/FilterEffectRenderer.cpp (left):

https://codereview.chromium.org/14856010/diff/13001/Source/core/rendering/Fil...
Source/core/rendering/FilterEffectRenderer.cpp:356:
effect->setOperatingColorSpace(ColorSpaceDeviceRGB);
I think we should still do this for non-reference filters, so please move this
into the if() below.

[sugoi1, 2013-05-10 14:58:03]

https://codereview.chromium.org/14856010/diff/21003/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/ImageFilterBuilderTest.cpp (right):

https://codereview.chromium.org/14856010/diff/21003/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageFilterBuilderTest.cpp:111: //                
Source Graphic (D)
The bots didn't like my ASCII art when a line ended with a '\', so I changed it
to use '|' instead. Less pretty, but less warnings.

[sugoi1, 2013-05-10 15:10:18]

https://codereview.chromium.org/14856010/diff/13001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp (right):

https://codereview.chromium.org/14856010/diff/13001/Source/core/platform/grap...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp:187: // Note
that we may still need the color transform even if the filter is null
On 2013/05/10 14:56:18, Stephen White wrote:
> Hmm.. so effect is always going to have two entries in the map, one mapping to
NULL and one mapping to CS(NULL)? I guess that makes sense, for when it's
optimized out or creation fails? Just wondering when you noticed this.

This is for SourceGraphic, which comes up as a NULL SkImageFilter, but may still
need to be set to the proper color space before going into the first filter, if
the first filter isn't in Device color space.

https://codereview.chromium.org/14856010/diff/13001/Source/core/rendering/Fil...
File Source/core/rendering/FilterEffectRenderer.cpp (left):

https://codereview.chromium.org/14856010/diff/13001/Source/core/rendering/Fil...
Source/core/rendering/FilterEffectRenderer.cpp:356:
effect->setOperatingColorSpace(ColorSpaceDeviceRGB);
On 2013/05/10 14:56:18, Stephen White wrote:
> I think we should still do this for non-reference filters, so please move this
> into the if() below.

Done.

[sugoi1, 2013-05-16 20:25:24]

Ok, I had a lot of problems with "git cl upload" constantly giving me the "Base
files missing." error, but I think the cl has uploaded properly anyway. So,
changes since path #4 :

- Important changes are in FilterEffectRenderer.cpp where I wrote some code to
be able to get the color-interpolation-filters information, even when the page
layout hasn't been done yet.
- There were some duplicate TestExpectations, so I fixed that
- Fixed typos where "color-interpolation=filters" was written in LayoutTests
instead of "color-interpolation-filters"
- Explicitly added color space in many LayoutTests to avoid regressions

[sugoi1, 2013-06-03 20:15:22]

Updated this cl after my 2 weeks off and fixed a small FEFlood colorspace issue.

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FEFlood.cpp (right):

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FEFlood.cpp:82:
FilterEffect::setOperatingColorSpace(ColorSpaceDeviceRGB);
This is necessary because FEFlood explicitly overrides setOperatingColorSpace()
and hard codes its operating color space to ColorSpaceDeviceRGB in
FEFlood::applySoftware().

[Stephen White, 2013-06-04 14:55:40]

https://codereview.chromium.org/14856010/diff/47001/LayoutTests/css3/filters/...
File LayoutTests/css3/filters/effect-reference-colorspace-hw.html (right):

https://codereview.chromium.org/14856010/diff/47001/LayoutTests/css3/filters/...
LayoutTests/css3/filters/effect-reference-colorspace-hw.html:3: <filter
id="blend" color-interpolation-filters="linearRGB">
We should probably have some test cases where color-interpolation-filters is
specified for the <filter> node, but not for the filter primitives, and vice
versa. We should also have some cases where it is specified differently for both
(and that the parent one is ignored).

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FEFlood.cpp (right):

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FEFlood.cpp:82:
FilterEffect::setOperatingColorSpace(ColorSpaceDeviceRGB);
On 2013/06/03 20:15:22, sugoi1 wrote:
> This is necessary because FEFlood explicitly overrides
setOperatingColorSpace()
> and hard codes its operating color space to ColorSpaceDeviceRGB in
> FEFlood::applySoftware().

I think it makes more sense to set result color space in the constructor as well
(as you've done for operating space here), and change the overrides in the
header to do nothing.

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp (left):

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp:25: #include
"config.h"
You should leave this in -- it should be the first #include in every .cpp file
in blink.

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp (right):

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp:293: // Go back
to device color space at the end
Nit: I'd word this something like "Transform to device color space at the end of
processing, if required"

https://codereview.chromium.org/14856010/diff/47001/Source/core/rendering/Fil...
File Source/core/rendering/FilterEffectRenderer.cpp (right):

https://codereview.chromium.org/14856010/diff/47001/Source/core/rendering/Fil...
Source/core/rendering/FilterEffectRenderer.cpp:96: RefPtr<CSSValue> cssValue =
svgStyledElement->getPresentationAttribute(
As discussed, let's use the "fast path" where available (ie., the attributes
have been already parsed). Then if we determine later that we can/should trigger
CSS parsing on external SVG references, we can use the fast path there too.

[sugoi1, 2013-06-05 14:57:30]

I fixed senorblanco's comments
Note that the layout tests seem to be constantly failing at the "apply patch"
step, which prevent them from being executed.

https://codereview.chromium.org/14856010/diff/47001/LayoutTests/css3/filters/...
File LayoutTests/css3/filters/effect-reference-colorspace-hw.html (right):

https://codereview.chromium.org/14856010/diff/47001/LayoutTests/css3/filters/...
LayoutTests/css3/filters/effect-reference-colorspace-hw.html:3: <filter
id="blend" color-interpolation-filters="linearRGB">
On 2013/06/04 14:55:41, Stephen White wrote:
> We should probably have some test cases where color-interpolation-filters is
> specified for the <filter> node, but not for the filter primitives, and vice
> versa. We should also have some cases where it is specified differently for
both
> (and that the parent one is ignored).

Done, but note that, for now, it's the child that is ignored, not the parent (so
the parent overrides all). That behavior can easily be modified in
FilterEffectRenderer.cpp if we want the opposite behavior.

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FEFlood.cpp (right):

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FEFlood.cpp:82:
FilterEffect::setOperatingColorSpace(ColorSpaceDeviceRGB);
On 2013/06/04 14:55:41, Stephen White wrote:
> On 2013/06/03 20:15:22, sugoi1 wrote:
> > This is necessary because FEFlood explicitly overrides
> setOperatingColorSpace()
> > and hard codes its operating color space to ColorSpaceDeviceRGB in
> > FEFlood::applySoftware().
> 
> I think it makes more sense to set result color space in the constructor as
well
> (as you've done for operating space here), and change the overrides in the
> header to do nothing.

Done.

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp (left):

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp:25: #include
"config.h"
On 2013/06/04 14:55:41, Stephen White wrote:
> You should leave this in -- it should be the first #include in every .cpp file
> in blink.

Done.

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp (right):

https://codereview.chromium.org/14856010/diff/47001/Source/core/platform/grap...
Source/core/platform/graphics/filters/SkiaImageFilterBuilder.cpp:293: // Go back
to device color space at the end
On 2013/06/04 14:55:41, Stephen White wrote:
> Nit: I'd word this something like "Transform to device color space at the end
of
> processing, if required"

Done.

https://codereview.chromium.org/14856010/diff/47001/Source/core/rendering/Fil...
File Source/core/rendering/FilterEffectRenderer.cpp (right):

https://codereview.chromium.org/14856010/diff/47001/Source/core/rendering/Fil...
Source/core/rendering/FilterEffectRenderer.cpp:96: RefPtr<CSSValue> cssValue =
svgStyledElement->getPresentationAttribute(
On 2013/06/04 14:55:41, Stephen White wrote:
> As discussed, let's use the "fast path" where available (ie., the attributes
> have been already parsed). Then if we determine later that we can/should
trigger
> CSS parsing on external SVG references, we can use the fast path there too.

Done.

[sugoi1, 2013-06-05 15:24:21]

Removed hueRotate.svg, as it is unfortunately tagged as a "binary" file which
causes issues with the trybots. I also added a line to TestExpectations for
effect-reference-external.html, which will fail until we manually update
hueRotate.svg after this patch goes in.

[commit-bot: I haz the power, 2013-06-05 21:04:55]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Stephen White, 2013-06-05 21:13:19]

Sorry, I think something is borked in Rietveld and/or git cl upload, but I can't
review this patch.

https://codereview.chromium.org/14856010/diff/66001/LayoutTests/css3/filters/...
File LayoutTests/css3/filters/effect-reference-composite-hw.html (right):

https://codereview.chromium.org/14856010/diff/66001/LayoutTests/css3/filters/...
LayoutTests/css3/filters/effect-reference-composite-hw.html:6: <feComposite
in2="transp" operator="over"/>
I think this is probably a git cl upload fail, but big chunks of this file are
missing as well.

https://codereview.chromium.org/14856010/diff/66001/LayoutTests/css3/filters/...
File LayoutTests/css3/filters/effect-reference-composite.html (right):

https://codereview.chromium.org/14856010/diff/66001/LayoutTests/css3/filters/...
LayoutTests/css3/filters/effect-reference-composite.html:6: <feComposite
in2="transp" operator="over"/>
Same here.

https://codereview.chromium.org/14856010/diff/66001/LayoutTests/css3/filters/...
File LayoutTests/css3/filters/effect-reference-hw.html (right):

https://codereview.chromium.org/14856010/diff/66001/LayoutTests/css3/filters/...
LayoutTests/css3/filters/effect-reference-hw.html:12: <feFuncG type="linear"
slope="0.5">
And here. (and probably more)

https://codereview.chromium.org/14856010/diff/66001/Source/WebKit/chromium/We...
File Source/WebKit/chromium/WebKit.gypi (right):

https://codereview.chromium.org/14856010/diff/66001/Source/WebKit/chromium/We...
Source/WebKit/chromium/WebKit.gypi:1: 'tests/IDBRequestTest.cpp',
I dunno what's going on here; it looks like Rietveld is showing only the diff'ed
lines, and no context and no rest-of-file.

[sugoi1, 2013-06-06 14:48:24]

I'll update / re-upload and if that fails again, I'll try creating a new cl from
scratch that's a copy of this one.

[Stephen White, 2013-06-06 15:14:35]

LGTM

[commit-bot: I haz the power, 2013-06-06 15:14:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/14856010/102001

[commit-bot: I haz the power, 2013-06-06 15:24:56]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[sugoi1, 2013-06-06 15:30:13]

Adding jochen@ as a reviewer to get an owner's LGTM on the WebKit.gypi file.

[jochen (gone - plz use gerrit), 2013-06-06 16:05:36]

WebKit.gypi lgtm

[commit-bot: I haz the power, 2013-06-06 16:09:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/14856010/102001

[commit-bot: I haz the power, 2013-06-06 16:59:01]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-06-06 19:40:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/14856010/102001

[commit-bot: I haz the power, 2013-06-06 20:09:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/14856010/102001

[commit-bot: I haz the power, 2013-06-06 21:21:41]

Change committed as 151966