Make skpaint->grpaint flow work for composing draws

Make skpaint->grpaint flow work for composing draws (verts and atlas)

One side effect is that the SkShader's (or implicit shader's) fragment processor is responsible for the transition from an unpremul paint color to a premul color.

Committed: https://skia.googlesource.com/skia/+/f1b7a1d82860e106ed7d3e0e876419e65783fb84

[bsalomon, 2015-09-25 18:12:06]

bsalomon@google.com changed reviewers:
+ egdaniel@google.com, robertphillips@google.com

[bsalomon, 2015-09-25 18:12:44]

I think if you start with SkGr.cpp the rest of this will make sense. Many of the
other files are just changing to the new SkToGr signatures.

[bsalomon, 2015-09-25 18:54:09]

Please ignore the changes in gm/vertices.cpp. That will be its own CL and needs
a fix on the cpu side to land.

[egdaniel, 2015-09-25 19:23:15]

initial skgr comments

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:30: #include "effects/GrConstColorProcessor.h"
alphabetize

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:754: grPaint->setColor(GrColor_WHITE);
seems a little strange to me to stay the GrPaint's color will be ignored and
then set it to white. Can we set it to some invalid color if it has to be set?

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:792: *primColorMode));
does this not need to look at primitiveIsSrc?

[robertphillips, 2015-09-25 19:52:17]

https://codereview.chromium.org/1348583002/diff/120001/gm/vertices.cpp
File gm/vertices.cpp (right):

https://codereview.chromium.org/1348583002/diff/120001/gm/vertices.cpp#newcod...
gm/vertices.cpp:157: for (size_t j = 0; j < SK_ARRAY_COUNT(modes); ++j) {
SkAutoTUnref ?

https://codereview.chromium.org/1348583002/diff/120001/gm/vertices.cpp#newcod...
gm/vertices.cpp:163: paint.setAlpha(rec[i].fAlpha);
rm commented code ?

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/GrFragment...
File include/gpu/GrFragmentProcessor.h (right):

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/GrFragment...
include/gpu/GrFragmentProcessor.h:47: */
OverrideInputColor ?

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/GrInvarian...
File include/gpu/GrInvariantOutput.h (right):

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/GrInvarian...
include/gpu/GrInvariantOutput.h:224: }
rm this else ?

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/SkGr.h
File include/gpu/SkGr.h (right):

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/SkGr.h#new...
include/gpu/SkGr.h:65: 
We used to always be able to say GrColor was premul. :(

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1556: if (GrPixelConfigIsAlphaOnly(devTex->config())) {
I don't think so

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1760: if (texs && colors && paint.getShader()) {
Missing period before 'A' ?

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:28: #include "effects/GrPorterDuffXferProcessor.h"
Fix order ?

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:713: 
for to -> to ?

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:780: // The shader's FP sees the paint unpremul color
So in this case the GrPaint's color isn't premul?

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:815: if (colorFilter->asFragmentProcessors(context,
grPaint->getProcessorDataManager(),
xtra space ?

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:817: for (int i = 0; i < array.count(); ++i) {
grPaint->addColorFragmentProcessor(array[i])->unref(); ?

[egdaniel, 2015-09-25 20:01:55]

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/GrFragmentProc...
File src/gpu/GrFragmentProcessor.cpp (right):

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/GrFragmentProc...
src/gpu/GrFragmentProcessor.cpp:11: #include "gl/builders/GrGLProgramBuilder.h"
this should go below the gl/*.cpp

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1015: grPaint.setColor(paintColor);
something seems weird about the SkToGr setting the grPaint color and then we
immediately override it again here (possibly with the same value if we are
Alpha8)

[bsalomon, 2015-09-25 20:45:08]

https://codereview.chromium.org/1348583002/diff/120001/gm/vertices.cpp
File gm/vertices.cpp (right):

https://codereview.chromium.org/1348583002/diff/120001/gm/vertices.cpp#newcod...
gm/vertices.cpp:157: for (size_t j = 0; j < SK_ARRAY_COUNT(modes); ++j) {
On 2015/09/25 19:52:17, robertphillips wrote:
> SkAutoTUnref ?

This file shouldn't be included in the CL.

https://codereview.chromium.org/1348583002/diff/120001/gm/vertices.cpp#newcod...
gm/vertices.cpp:163: paint.setAlpha(rec[i].fAlpha);
On 2015/09/25 19:52:17, robertphillips wrote:
> rm commented code ?

This file shouldn't be included in the CL.

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/GrFragment...
File include/gpu/GrFragmentProcessor.h (right):

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/GrFragment...
include/gpu/GrFragmentProcessor.h:47: */
On 2015/09/25 19:52:17, robertphillips wrote:
> OverrideInputColor ?

Made it OverrideInput

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/GrInvarian...
File include/gpu/GrInvariantOutput.h (right):

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/GrInvarian...
include/gpu/GrInvariantOutput.h:224: }
On 2015/09/25 19:52:17, robertphillips wrote:
> rm this else ?

Done.

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/SkGr.h
File include/gpu/SkGr.h (right):

https://codereview.chromium.org/1348583002/diff/120001/include/gpu/SkGr.h#new...
include/gpu/SkGr.h:65: 
On 2015/09/25 19:52:17, robertphillips wrote:
> We used to always be able to say GrColor was premul. :(

Sorry, but it cannot be anymore if we want to correctly support compose and
drawVertices. drawVertices is supposed to interpolate unpremul vertex colors and
then premul the interpolated color.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/GrFragmentProc...
File src/gpu/GrFragmentProcessor.cpp (right):

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/GrFragmentProc...
src/gpu/GrFragmentProcessor.cpp:11: #include "gl/builders/GrGLProgramBuilder.h"
On 2015/09/25 20:01:55, egdaniel wrote:
> this should go below the gl/*.cpp 

ok

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1015: grPaint.setColor(paintColor);
On 2015/09/25 20:01:55, egdaniel wrote:
> something seems weird about the SkToGr setting the grPaint color and then we
> immediately override it again here (possibly with the same value if we are
> Alpha8)

overlooked deleting this.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1556: if (GrPixelConfigIsAlphaOnly(devTex->config())) {
On 2015/09/25 19:52:17, robertphillips wrote:
> I don't think so

I thought we sometimes internally created A8 devices... but maybe they never
have layers. I don't think it hurts to leave it here.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1760: if (texs && colors && paint.getShader()) {
On 2015/09/25 19:52:17, robertphillips wrote:
> Missing period before 'A' ?

Done.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:28: #include "effects/GrPorterDuffXferProcessor.h"
On 2015/09/25 19:52:17, robertphillips wrote:
> Fix order ?

Done.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:30: #include "effects/GrConstColorProcessor.h"
On 2015/09/25 19:23:15, egdaniel wrote:
> alphabetize

Done.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:713: 
On 2015/09/25 19:52:17, robertphillips wrote:
> for to -> to ?

Done.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:754: grPaint->setColor(GrColor_WHITE);
On 2015/09/25 19:23:15, egdaniel wrote:
> seems a little strange to me to stay the GrPaint's color will be ignored and
> then set it to white. Can we set it to some invalid color if it has to be set?

I'll just delete the line.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:780: // The shader's FP sees the paint unpremul color
On 2015/09/25 19:52:17, robertphillips wrote:
> So in this case the GrPaint's color isn't premul?

Yep, SkShader's FPs are now responsible for ingesting a unpremul color and
providing a premul output.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:792: *primColorMode));
On 2015/09/25 19:23:15, egdaniel wrote:
> does this not need to look at primitiveIsSrc?

Done.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:815: if (colorFilter->asFragmentProcessors(context,
grPaint->getProcessorDataManager(),
On 2015/09/25 19:52:17, robertphillips wrote:
> xtra space ?

Done.

https://codereview.chromium.org/1348583002/diff/120001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:817: for (int i = 0; i < array.count(); ++i) {
On 2015/09/25 19:52:17, robertphillips wrote:
> grPaint->addColorFragmentProcessor(array[i])->unref(); ?

Done.

[bsalomon, 2015-09-28 13:03:03]

ping

[bsalomon, 2015-09-28 13:04:18]

The CQ bit was checked by bsalomon@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-28 13:04:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348583002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348583002/180001

[commit-bot: I haz the power, 2015-09-28 13:05:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-28 13:05:19]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[egdaniel, 2015-09-28 13:13:22]

outside of failing trybots, lgtm, but might want rob to agree as well

[bsalomon, 2015-09-28 13:15:07]

The CQ bit was checked by bsalomon@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-28 13:15:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348583002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348583002/200001

[commit-bot: I haz the power, 2015-09-28 13:21:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-28 13:21:25]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2015-09-28 13:23:05]

lgtm

[bsalomon, 2015-09-28 13:25:27]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-09-28 13:25:27]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/1348583002/#ps200001
(title: "remove unused inheriteds to satisfy clang")

[commit-bot: I haz the power, 2015-09-28 13:25:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348583002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348583002/200001

[commit-bot: I haz the power, 2015-09-28 13:26:39]

Committed patchset #11 (id:200001) as
https://skia.googlesource.com/skia/+/f1b7a1d82860e106ed7d3e0e876419e65783fb84