Implement gradient simplification for 0,0,1 and 0,1,1 gradients

Implement gradient simplification for 0,0,1 and 0,1,1 gradients

Depends on https://codereview.chromium.org/2259823005/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2256843004

Committed: https://skia.googlesource.com/skia/+/68d952cf4061dc455d6a6040ce1e4625e4f2ab29

[fmenozzi, 2016-08-18 21:14:25]

Description was changed from

==========
Implement gradient simplification for 0,0,1 and 0,1,1 gradients

Depends on https://codereview.chromium.org/2259823005/

BUG=skia:
==========

to

==========
Implement gradient simplification for 0,0,1 and 0,1,1 gradients

Depends on https://codereview.chromium.org/2259823005/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2256843004
==========

[fmenozzi, 2016-08-18 21:17:09]

fmenozzi@google.com changed reviewers:
+ fmalita@chromium.org

[fmenozzi, 2016-08-18 21:17:09]

Simplified cases of 0,0,1 and 0,1,1 gradients where colors at hard stop are
identical OR tile mode isn't clamp.

Depends on https://codereview.chromium.org/2256843004

ptal

[f(malita), 2016-08-18 22:54:17]

https://codereview.chromium.org/2256843004/diff/20001/src/effects/gradients/S...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/2256843004/diff/20001/src/effects/gradients/S...
src/effects/gradients/SkGradientShader.cpp:779: colorCount = 2;
If we fall through here, I think we'll also take the second conditional
unnecessarily (after the optimization, pos is [0, 1, 1]).

I would either return or make the the checks exclusive.

https://codereview.chromium.org/2256843004/diff/20001/src/effects/gradients/S...
src/effects/gradients/SkGradientShader.cpp:831: }
It would be nice to

a) avoid these allocations and copies when the optimization is not applicable
b) only allocate/copy the minimum amount (2), on stack

Can you try refactoring simplify() as a RAII helper, something along these
lines:

struct ColorStopOptimizer {
    
    ColorStopOptimizer(const SkColor* colors, const SkScalar* pos, int count,
SkShader::TileMode mode)
        : fColors(colors)
        , fPos(pos)
        , fCount(count) {

        if (count != 3 || !pos) {
            // the optimization is not applicable, use original data
            return;
        }

        if (<left edge optimization is applicable>) {
            // optimization fired, switch to internal storage
            fColorStorage[0] = colors[1];
            fColorStorage[1] = colors[2];
            fPosStorage[0] = 0;
            fPosStorage[1] = 1;

            fColors = fColorStorage;
            fPos    = fPosStorage;
            fCount  = 2;
        } else if (<right edge optimization is applicable>) {
            ...
        }
    }

    const SkColor*  fColors;
    const SkScalar* fPos;
    int             fCount

    SkColor         fColorStorage[2];
    SkScalar        fPosStorage[2];
}


Then the factories could just do something like this


    ColorStopOptimizer opt(colors, pos, count, mode);
    SkGradientShaderBase::Descriptor desc;
    desc_init(&desc, opt.fColors, opt.fPos, opt.fCount, mode, flags,
localMatrix);

[fmenozzi, 2016-08-19 14:54:54]

ptal

[f(malita), 2016-08-19 15:26:20]

Thanks!

LGTM % pos null check.

https://codereview.chromium.org/2256843004/diff/40001/src/effects/gradients/S...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/2256843004/diff/40001/src/effects/gradients/S...
src/effects/gradients/SkGradientShader.cpp:777: if (count != 3) {
We should also bail here if pos is null (which I think is a legal factory
value).

[fmenozzi, 2016-08-19 15:33:37]

The CQ bit was checked by fmenozzi@google.com

[fmenozzi, 2016-08-19 15:33:38]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2256843004/#ps60001
(title: "Add null pos check")

[commit-bot: I haz the power, 2016-08-19 15:33:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 15:56:58]

Description was changed from

==========
Implement gradient simplification for 0,0,1 and 0,1,1 gradients

Depends on https://codereview.chromium.org/2259823005/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2256843004
==========

to

==========
Implement gradient simplification for 0,0,1 and 0,1,1 gradients

Depends on https://codereview.chromium.org/2259823005/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2256843004

Committed:
https://skia.googlesource.com/skia/+/68d952cf4061dc455d6a6040ce1e4625e4f2ab29
==========

[commit-bot: I haz the power, 2016-08-19 15:56:59]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/68d952cf4061dc455d6a6040ce1e4625e4f2ab29

[reed1, 2016-08-21 15:18:31]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2016-08-21 15:18:32]

do we have a test that exercises/confirms this check?

[f(malita), 2016-08-21 15:23:30]

On 2016/08/21 15:18:32, reed1 wrote:
> do we have a test that exercises/confirms this check?

I couldn't think of a good way to check the internal shader data, but now that
you bring it up I recall we do have SkShader::asGradient().  Let me see if I can
use that for a test.