[webkit-reviews] review granted: [Bug 92059] [chromium] Combine color matrix filters and apply them in a single pass. : [Attachment 154532] Patch

[bugzilla-daemon at webkit.org]

Stephen White <senorblanco at chromium.org> has granted Antoine Labour
<piman at chromium.org>'s request for review:
Bug 92059: [chromium] Combine color matrix filters and apply them in a single
pass.
https://bugs.webkit.org/show_bug.cgi?id=92059

Attachment 154532: Patch
https://bugs.webkit.org/attachment.cgi?id=154532&action=review

------- Additional Comments from Stephen White <senorblanco at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154532&action=review


Looks good.  r=me

Note that your new test will likely cause the non-Linux WebKit canary bots to
go red (due to pixel differences), so please either land with a suppression in
TestExpectations, or rebaseline the tests after landing.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:198
> +    return (maxValue > 255) || (minValue < 0);

It just occurred to me.. if the matrix is really large, is there a possibility
for float overflow here?  I guess it'll just end up at inf or -inf, probably
safe for comparison purposes?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:388
> +	       SkCanvas* canvas = state.canvas();
> +	       if (!canvas)
> +		   return SkBitmap();

I'm going to be annoying and point out that, if we did this optimization as a
pre-processing pass, we'd know exactly how many textures we'd need to allocate,
and we could do all allocations up front, without the need for all these checks
and early-outs.  That could be done as a later change, though.