Bug 92059 – [chromium] Combine color matrix filters and apply them in a single pass.

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Labour]

[chromium] Combine color matrix filters and apply them in a single pass.

[Antoine Labour]

Created attachment 153949 [details]
Patch

[Antoine Labour]

See http://code.google.com/p/chromium/issues/detail?id=138645 for a use case for this optimization.

[Adrienne Walker]

Comment on attachment 153949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153949&action=review

R=me.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:224
> +class ScopedScratchCanvas {

Nice.  :)

[Stephen White]

Comment on attachment 153949 [details]
Patch

As tempting as this optimization is (and I have been tempted), I don't think it's correct for CSS filters.  The reason is that colour clamping must occur after each matrix is applied.  E.g., if a translation by +255 followed by -255 were applied, it should end up at black, not at the original colour.

I'd suggest two possibilities:  add a flag so that it is only enabled for your use case, and not CSS filters, or implement a generic colour matrix filter, and convert and concatenate your matrices outside this code.  Alternately, I suppose we could try to detect the case where clamping would occur, and avoid the optimization, but that might be tricky.

[Stephen White]

(In reply to comment #4)
> I'd suggest two possibilities:  add a flag so that it is only enabled for your use case, and not CSS filters, or implement a generic colour matrix filter, and convert and concatenate your matrices outside this code.  Alternately, I suppose we could try to detect the case where clamping would occur, and avoid the optimization, but that might be tricky.

After thinking about it a bit, I'd prefer the generic colour matrix filter option, since we'll need that for some upcoming work anyway.

[Dana Jansens]

It's tempting to just remove all the color matrix filter operations and keep a single "matrix" operation, but that may be shooting ourselves in the foot if we decide to animate filters inside the compositor someday, as each filter needs to be animated independently rather than interpolating between two matrices.

And in this case, the UI would want to use separate filters also, not a single matrix. So I'm a proponent of adding a useStrictClamping flag or somesuch to WebFilterOperations and doing these sorts of optimizations when it is false.

[Antoine Labour]

(In reply to comment #6)
> It's tempting to just remove all the color matrix filter operations and keep a single "matrix" operation, but that may be shooting ourselves in the foot if we decide to animate filters inside the compositor someday, as each filter needs to be animated independently rather than interpolating between two matrices.

Yeah, that's what happens.

> 
> And in this case, the UI would want to use separate filters also, not a single matrix. So I'm a proponent of adding a useStrictClamping flag or somesuch to WebFilterOperations and doing these sorts of optimizations when it is false.

I think I like this option. The flag can be kept off for web stuff.

[Stephen White]

Comment on attachment 153949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153949&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:307
> +        if (getColorMatrix(op, matrix)) {
> +            if (hasMatrix) {
> +                SkScalar newMatrix[20];
> +                multColorMatrix(matrix, combinedMatrix, newMatrix);
> +                memcpy(combinedMatrix, newMatrix, sizeof(combinedMatrix));
> +            } else {
> +                memcpy(combinedMatrix, matrix, sizeof(combinedMatrix));
> +                hasMatrix = true;
> +            }
> +        } else {
> +            if (hasMatrix) {
> +                ScopedScratchCanvas canvas(gr, desc, &source);
> +                if (!canvas.isValid())
> +                    return SkBitmap();
> +                applyColorMatrix(canvas.canvas(), source, combinedMatrix);
> +                hasMatrix = false;
> +            }

This seems to be a lot of special cases (and separate texture allocations) inside what's essentially a pretty simple loop.

Could we instead pre-process the list of filters into an optimized list, consisting of matrix and non-matrix operations?

Also please add a test in css3/filters to test for the colour clamping I mentioned.  You'll need a regular and -hw flavour of the test.

[Stephen White]

(In reply to comment #7)
> (In reply to comment #6)
> > It's tempting to just remove all the color matrix filter operations and keep a single "matrix" operation, but that may be shooting ourselves in the foot if we decide to animate filters inside the compositor someday, as each filter needs to be animated independently rather than interpolating between two matrices.
> 
> Yeah, that's what happens.
> 
> > 
> > And in this case, the UI would want to use separate filters also, not a single matrix. So I'm a proponent of adding a useStrictClamping flag or somesuch to WebFilterOperations and doing these sorts of optimizations when it is false.
> 
> I think I like this option. The flag can be kept off for web stuff.

Here's another option:  create a matrix filter type, but for now only use it inside CCRenderSurfaceFilters.cpp (we can expose it later).  Pre-process the list of filters into a list of collapsed matrices and non-matrix operations.  Then the processing loop can be simple, as it was before, with only a single backing store allocation.

[Antoine Labour]

Created attachment 154196 [details]
Patch

[Antoine Labour]

As discussed directly, this new patch does:
- automatically figures out if we need clamping, in which case it will apply filters separately.
- reuse textures across filter passes (flip-flops)
- add layout tests for the clamping case
- a few minor simplifications to make the loop body state machine more readable.

[WebKit Review Bot]

Attachment 154196 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/css3/filters/effect-brightness..." exit_code: 1
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dana Jansens]

Comment on attachment 154196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154196&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:170
> +    for (int j = 0; j < 4; ++j)

this should have a { as well as the for loop inside it

[Antoine Labour]

Created attachment 154199 [details]
Patch

[Antoine Labour]

Created attachment 154201 [details]
Patch

[Antoine Labour]

Comment on attachment 154196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154196&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:170
>> +    for (int j = 0; j < 4; ++j)
> 
> this should have a { as well as the for loop inside it

Done.

[Dana Jansens]

Comment on attachment 154201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154201&action=review

Sorry for the horde of nits, also I think I see an extra copy now?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:164
> +// c = a*b;

maybe rename "c" to something like "out" or "result" to make this clear instead of requiring this comment?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:253
> +    FilterBufferState(GrContext* gr, const WebCore::FloatSize& size, unsigned textureId)
> +        : m_gr(gr)

s/gr/grContext/ ?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:255
> +        // Wrap the RenderSurface's texture in a Ganesh platform texture.

nit: let's use terminology of "input texture" or something, rather than RenderSurface explicitly, since that will be wrong once we allow filters without a surface in the single-layer case.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:256
> +        GrPlatformTextureDesc platformTexDesc;

There's lots of abbreviations in this file which we avoid generally. Sorry to bikeshed on them, but maybe we can clean them up now? eg textureDescription?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:345
> +    SkScalar combinedMatrix[20];

combinedColorMatrix?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:346
> +    bool hasMatrix = false;

Can we call this "haveCombinedColorMatrix" or "combinedColorMatrixIsEmpty" or "combinedColorMatrixHasValue" or something? I feel like hasMatrix is a bit more ambiguous about which matrix its meaning.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:376
> +        if (hasMatrix && !applyColorMatrix(&state, combinedMatrix))
> +            return SkBitmap();
> +        hasMatrix = false;
> +
> +        SkCanvas* canvas = state.canvas();
> +        if (!canvas)
>              return SkBitmap();
> -        SkGpuDevice device(gr, destination.get());
> -        SkCanvas canvas(&device);
> -        canvas.clear(0x0);
>          switch (op.type()) {

If we have a combinedColorMatrix and succeed at applying it, should we do a "continue" after marking the combinedColorMatrix as being non-present? Right now, we'll fall into the default: case of the switch statement after applying the color matrix, which will do a copy of the source to the new destination texture (swapped in at the end of the apply).

[Stephen White]

Comment on attachment 154201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154201&action=review

Thanks very much for implementing this optimization.  It's great that we can apply it to web content as well.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:189
> +bool componentNeedsClamping(SkScalar line[5])

Nit:  Could we call this "row" rather than "line"?  Matrices usually have rows.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:316
> +    OwnPtr<SkGpuDevice> m_device;
> +    OwnPtr<SkCanvas> m_canvas;

I seem to recall problems with using OwnPtr and Skia stuff in the past, since it has its own ref counting.  Consider using an SkAutoTUnref on the device & canvas as well (if you like.. your call).

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:346
>> +    bool hasMatrix = false;
> 
> Can we call this "haveCombinedColorMatrix" or "combinedColorMatrixIsEmpty" or "combinedColorMatrixHasValue" or something? I feel like hasMatrix is a bit more ambiguous about which matrix its meaning.

Perhaps "accumulated" instead of "combined"?
"accumulatedColorMatrix" and "haveAccumulatedColorMatrix"?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:376
>          switch (op.type()) {

Good catch, Dana (although I think it's only a problem in the clamping case).  Maybe we could apply the matrix right away above if it needs clamping, and continue in both the clamping and non-clamping cases:

if (matrixNeedsClamping(combinedMatrix)) && !applyColorMatrix(&state, combinedMatrix))
    return SkBitmap();
continue;

Alternately, we could probably get rid of the identity draw in the default case now since it'll just pick up the previous source, although it would require moving the swap() into the blur and drop-shadow draws so it wouldn't happen in the default case either.

> LayoutTests/css3/filters/effect-brightness-clamping-hw.html:13
> +<img style="-webkit-filter: brightness(1) brightness(-1)" src="resources/reference.png">

I'd like to see more coverage here, e.g., brightness followed by blur or drop-shadow.

[Dana Jansens]

(In reply to comment #18)
> (From update of attachment 154201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154201&action=review
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:376
> >          switch (op.type()) {
> 
> Good catch, Dana (although I think it's only a problem in the clamping case).  Maybe we could apply the matrix right away above if it needs clamping, and continue in both the clamping and non-clamping cases:
> 
> if (matrixNeedsClamping(combinedMatrix)) && !applyColorMatrix(&state, combinedMatrix))
>     return SkBitmap();
> continue;
> 
> Alternately, we could probably get rid of the identity draw in the default case now since it'll just pick up the previous source, although it would require moving the swap() into the blur and drop-shadow draws so it wouldn't happen in the default case either.

If we can get rid of the default case, (and add back all the color matrix cases going to the same case-body) that could possibly be nice. That way we would also get compiler errors if a new operation type is added to the enum but not to the switch.

[Antoine Labour]

Created attachment 154481 [details]
Patch

[Antoine Labour]

Comment on attachment 154201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154201&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:164
>> +// c = a*b;
> 
> maybe rename "c" to something like "out" or "result" to make this clear instead of requiring this comment?

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:189
>> +bool componentNeedsClamping(SkScalar line[5])
> 
> Nit:  Could we call this "row" rather than "line"?  Matrices usually have rows.

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:253
>> +        : m_gr(gr)
> 
> s/gr/grContext/ ?

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:255
>> +        // Wrap the RenderSurface's texture in a Ganesh platform texture.
> 
> nit: let's use terminology of "input texture" or something, rather than RenderSurface explicitly, since that will be wrong once we allow filters without a surface in the single-layer case.

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:256
>> +        GrPlatformTextureDesc platformTexDesc;
> 
> There's lots of abbreviations in this file which we avoid generally. Sorry to bikeshed on them, but maybe we can clean them up now? eg textureDescription?

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:316
>> +    OwnPtr<SkCanvas> m_canvas;
> 
> I seem to recall problems with using OwnPtr and Skia stuff in the past, since it has its own ref counting.  Consider using an SkAutoTUnref on the device & canvas as well (if you like.. your call).

Done. I'm not familiar with Skia's flavor of refcounting / smart pointers - my understanding is that a new class starts with a ref of 1 and SkAutoTUnref doesn't add a ref on assignment - so please double-check my usage.

>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:346
>>> +    bool hasMatrix = false;
>> 
>> Can we call this "haveCombinedColorMatrix" or "combinedColorMatrixIsEmpty" or "combinedColorMatrixHasValue" or something? I feel like hasMatrix is a bit more ambiguous about which matrix its meaning.
> 
> Perhaps "accumulated" instead of "combined"?
> "accumulatedColorMatrix" and "haveAccumulatedColorMatrix"?

Done.

>>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:376
>>>>          switch (op.type()) {
>>> 
>>> If we have a combinedColorMatrix and succeed at applying it, should we do a "continue" after marking the combinedColorMatrix as being non-present? Right now, we'll fall into the default: case of the switch statement after applying the color matrix, which will do a copy of the source to the new destination texture (swapped in at the end of the apply).
>> 
>> Good catch, Dana (although I think it's only a problem in the clamping case).  Maybe we could apply the matrix right away above if it needs clamping, and continue in both the clamping and non-clamping cases:
>> 
>> if (matrixNeedsClamping(combinedMatrix)) && !applyColorMatrix(&state, combinedMatrix))
>>     return SkBitmap();
>> continue;
>> 
>> Alternately, we could probably get rid of the identity draw in the default case now since it'll just pick up the previous source, although it would require moving the swap() into the blur and drop-shadow draws so it wouldn't happen in the default case either.
> 
> If we can get rid of the default case, (and add back all the color matrix cases going to the same case-body) that could possibly be nice. That way we would also get compiler errors if a new operation type is added to the enum but not to the switch.

Done.

>> LayoutTests/css3/filters/effect-brightness-clamping-hw.html:13
>> +<img style="-webkit-filter: brightness(1) brightness(-1)" src="resources/reference.png">
> 
> I'd like to see more coverage here, e.g., brightness followed by blur or drop-shadow.

Done.

[Dana Jansens]

Comment on attachment 154481 [details]
Patch

nice, thanks for all the renames!

[Antoine Labour]

Created attachment 154532 [details]
Patch

[Antoine Labour]

PTAL, I noticed the previous patch was creating a canvas that it never used in one case (when the last filter is a matrix that needs clamping), causing possibly an extra alloc and a clear. New patch addresses that.

[Stephen White]

Comment on attachment 154532 [details]
Patch

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.

[Antoine Labour]

Comment on attachment 154532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154532&action=review

>> 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?

Correct, float overflow is a lot nicer than unsigned int (let alone int!) for this sort of things.

FYI, during my testing I noticed that some matrices (at least the grayscale one depending on the amount value) trigger this because of precision issues. I think I can change the way it's computed to make sure it's stable (will do in a follow up), or possibly add a fudge factor here (I'd rather not).

[Antoine Labour]

Created attachment 154718 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 154718 [details]
Patch for landing

Clearing flags on attachment: 154718

Committed r123790: <http://trac.webkit.org/changeset/123790>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.