Remove gpu shader optimatization for solid white or trans black colors

Remove gpu shader optimatization for solid white or trans black colors

Running test on the added bench which draws a grid of all white paths, all blue paths, or alternating checkered white/blue paths.

With optimization in (ms):
         White       Blue        Checkered
Linux    ~80         ~80         ~160
N7       ~800        ~1100       ~1500
Moto-e   ~830        ~1100       ~2500

Without optimization in (ms):
         White       Blue        Checkered
Linux    ~80         ~80         ~80
N7       ~1100       ~1100       ~1100
Moto-e   ~1100       ~1100       ~1500

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/5f78d2251a440443c9eaa321dad058d7a32bfef7

Committed: https://skia.googlesource.com/skia/+/02cafcc1bf6e2968c2efdf459871167970da150e

[egdaniel, 2014-07-09 18:06:05]

So here is the change for removing the solid white shader optimization. As you
can see from the bench results above, the optimization has zero affect on the
desktop, but does show some speed up when comparing the all white and all blue
cases. However, the optimization is hurt in cases we need to do program
switching as in the checkered case.

So overall by removing the optimization, we will slow down on android devices
when drawing solid white, but we gain in having less shaders to compile, less
program switching (if we hit a bad checkered type case). Thoughts on whether to
remove it?

[bsalomon, 2014-07-09 20:49:54]

I'm a little concerned about the drawBitmap case where the SkPaint has
alpha=0xFF. We will always be doing four extra multiplies. Maybe we should make
your program do solid-alpha drawBitmap, fractional-alpha drawBitmap, and a mix?
That seems like more of a practical concern than drawing white things. If that
shows a slow down, let's talk tomorrow about a way to make this change but keep
bitmaps fast.

[egdaniel, 2014-07-11 13:57:11]

I've updated the bench to include the ability to use both solid colors and
bitmaps (with and without alpha). We see a similar trend in the numbers as
posted originally for the solid colors path. FYI, if we have a drawBitmap call,
it ends up being rendered as a drawRect call so we were never hitting the
optimization for drawBitmap to begin with so we shouldn't see any slow down from
removing it.

[bsalomon, 2014-07-11 14:29:53]

minor comments, otherwise lgtm.

https://codereview.chromium.org/375823005/diff/80001/bench/AlternatingColorPa...
File bench/AlternatingColorPatternBench.cpp (right):

https://codereview.chromium.org/375823005/diff/80001/bench/AlternatingColorPa...
bench/AlternatingColorPatternBench.cpp:22: SkColor         fColor;
Calling setAlpha on SkPaint actually changes SkPaint's alpha value. It's just a
shortcut for:

paint.setColor((paint.getColor() & 0x00FFFFFF) | (alpha << 24));

So, maybe this should just have a color entry and you put in 0x10000000 for the
alphaBitmap case?

https://codereview.chromium.org/375823005/diff/80001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (left):

https://codereview.chromium.org/375823005/diff/80001/src/gpu/gl/GrGLProgram.c...
src/gpu/gl/GrGLProgram.cpp:186: SkFAIL("Unknown color type.");
Since the enums are still defined but now not expected here, can you just change
the text to Unexpected instead of Unknown?

https://codereview.chromium.org/375823005/diff/80001/src/gpu/gl/GrGLProgram.c...
src/gpu/gl/GrGLProgram.cpp:225: SkFAIL("Unknown coverage type.");
ditto

[egdaniel, 2014-07-11 15:01:26]

https://codereview.chromium.org/375823005/diff/80001/bench/AlternatingColorPa...
File bench/AlternatingColorPatternBench.cpp (right):

https://codereview.chromium.org/375823005/diff/80001/bench/AlternatingColorPa...
bench/AlternatingColorPatternBench.cpp:22: SkColor         fColor;
Alpha field removed.
On 2014/07/11 14:29:52, bsalomon wrote:
> Calling setAlpha on SkPaint actually changes SkPaint's alpha value. It's just
a
> shortcut for:
> 
> paint.setColor((paint.getColor() & 0x00FFFFFF) | (alpha << 24));
> 
> So, maybe this should just have a color entry and you put in 0x10000000 for
the
> alphaBitmap case?

https://codereview.chromium.org/375823005/diff/80001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (left):

https://codereview.chromium.org/375823005/diff/80001/src/gpu/gl/GrGLProgram.c...
src/gpu/gl/GrGLProgram.cpp:186: SkFAIL("Unknown color type.");
done.
On 2014/07/11 14:29:52, bsalomon wrote:
> Since the enums are still defined but now not expected here, can you just
change
> the text to Unexpected instead of Unknown?

https://codereview.chromium.org/375823005/diff/80001/src/gpu/gl/GrGLProgram.c...
src/gpu/gl/GrGLProgram.cpp:225: SkFAIL("Unknown coverage type.");
done
On 2014/07/11 14:29:53, bsalomon wrote:
> ditto

[egdaniel, 2014-07-11 15:01:35]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-07-11 15:02:20]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/egdaniel@google.com/375823005/60002

[commit-bot: I haz the power, 2014-07-11 15:26:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)

[egdaniel, 2014-07-11 15:37:25]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-07-11 15:38:30]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/egdaniel@google.com/375823005/110001

[commit-bot: I haz the power, 2014-07-11 15:57:44]

Change committed as 5f78d2251a440443c9eaa321dad058d7a32bfef7

[egdaniel, 2014-07-11 16:12:53]

A revert of this CL has been created in
https://codereview.chromium.org/385163004/ by egdaniel@google.com.

The reason for reverting is: possible test and gm failures.

[egdaniel, 2014-07-14 16:12:07]

PTAL. The original commit failed for a couple reasons. First our random shader
test was still making shaders with solidWhite/transBlack color.

Second we were setting the color to be Uniform when defaultToUniformInputs &&
!requiresCoverageAttrib was set but inputcolorIsUsed was false so we were
getting unused uniform error.

[egdaniel, 2014-07-21 13:33:08]

Working version of the optimization removal.

Looking at the failed gm's from patch 11, all the failures that I was able to
reproduce were minimal single pixel type diffs. Some that I were not able to
reproduce I will assume they are similar in nature. I've added all the failed to
gms to the ignore list and will look closely at them once committed.

[bsalomon, 2014-07-21 14:28:51]

lgtm w/ name suggestion.

https://codereview.chromium.org/375823005/diff/230001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/375823005/diff/230001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:557: bool ignoreColorAttribute() const;
Can we rename to canIgnoreColorAttribute()? Seems a little clearer that the
implementation has the option to ignore it rather than a requirement.

[egdaniel, 2014-07-21 18:25:39]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-07-21 18:26:12]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/egdaniel@google.com/375823005/250001

[commit-bot: I haz the power, 2014-07-21 18:37:34]

Change committed as 02cafcc1bf6e2968c2efdf459871167970da150e