Move DstCopy on gpu into the GrXferProcessor.

Move DstCopy on gpu into the GrXferProcessor.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/74a11753604768bf461b80cabb66060e8564d82c

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

[egdaniel, 2015-01-29 20:51:21]

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

[egdaniel, 2015-01-29 20:51:22]

First pass at moving the dst copy to GrXP. The actual dst copy still happens
outside the xp in draw target.

[bsalomon, 2015-01-30 14:59:15]

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:39: * sepcific subclasses key.
specific subclass's ?

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:109: bool hasDstCopy() const;
different than (bool)getDstCopyTexture()?

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:110: bool dstCopyIsTopDown() const;
can't we just canonicalize this?

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:112: const GrTexture* getDstCopyTexture() const {
return fDstCopy.getTexture(); }
comments?

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:114: const SkIPoint& offset() const { return
fOffset; }
offset() is a bit vague...

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:267: virtual bool willReadDst() const = 0;
maybe make it clear that this refers to reading the dst in the fragment shader
(as opposed to in the blend equation).

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrInOrderDrawBuf...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrInOrderDrawBuf...
src/gpu/GrInOrderDrawBuffer.cpp:584: if
(!this->setupDstReadIfNecessary(pipelineBuilder, &dstCopy, devBounds)) {
Is it possible to set up the dst read in GrDrawTarget rather than the subclass?

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrXferProcessor.cpp
File src/gpu/GrXferProcessor.cpp (right):

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrXferProcessor....
src/gpu/GrXferProcessor.cpp:16: kTopLeftOrigin_DstReadKeyBit    = 0x4, // Set if
dst-copy origin is top-left.
canonicalize on always tl or bl?

[joshualitt, 2015-02-02 15:42:26]

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrDrawTarget.cpp...
src/gpu/GrDrawTarget.cpp:661: // support NV_blend_equation_advanced.
Comment no longer applies

https://codereview.chromium.org/885923002/diff/20001/src/gpu/gl/builders/GrGL...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp (left):

https://codereview.chromium.org/885923002/diff/20001/src/gpu/gl/builders/GrGL...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:200:
fProgramBuilder->fUniformHandles.fDstCopySamplerUni =
Delete the dstcopy stuff from BuiltInUniformHandles struct.

[egdaniel, 2015-02-03 16:47:44]

The main changes to note are I always make the copy texture to be top left so
there is no need to check for that in XP.

With that I was able to reduce the key to simply be reading dst or not, so I
dropped the whole key flags and simply use a boolean instead.

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
File include/gpu/GrXferProcessor.h (right):

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:39: * sepcific subclasses key.
On 2015/01/30 14:59:14, bsalomon wrote:
> specific subclass's ?

Done.

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:109: bool hasDstCopy() const;
On 2015/01/30 14:59:14, bsalomon wrote:
> different than (bool)getDstCopyTexture()?

Removed hasDstCopy

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:110: bool dstCopyIsTopDown() const;
On 2015/01/30 14:59:14, bsalomon wrote:
> can't we just canonicalize this?

done

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:112: const GrTexture* getDstCopyTexture() const {
return fDstCopy.getTexture(); }
On 2015/01/30 14:59:14, bsalomon wrote:
> comments?

Done.

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:114: const SkIPoint& offset() const { return
fOffset; }
On 2015/01/30 14:59:14, bsalomon wrote:
> offset() is a bit vague...

made name more descriptive "DstcopyTextureOffset"

https://codereview.chromium.org/885923002/diff/20001/include/gpu/GrXferProces...
include/gpu/GrXferProcessor.h:267: virtual bool willReadDst() const = 0;
On 2015/01/30 14:59:14, bsalomon wrote:
> maybe make it clear that this refers to reading the dst in the fragment shader
> (as opposed to in the blend equation).

Done.

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrDrawTarget.cpp...
src/gpu/GrDrawTarget.cpp:661: // support NV_blend_equation_advanced.
On 2015/02/02 15:42:25, joshualitt wrote:
> Comment no longer applies

Done.

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrInOrderDrawBuf...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrInOrderDrawBuf...
src/gpu/GrInOrderDrawBuffer.cpp:584: if
(!this->setupDstReadIfNecessary(pipelineBuilder, &dstCopy, devBounds)) {
On 2015/01/30 14:59:14, bsalomon wrote:
> Is it possible to set up the dst read in GrDrawTarget rather than the
subclass?

Yes it is definitely better to do this in the base class (as well creating the
pipeline and other similar tasks). For now, as per our conversation, this will
be left for follow up work.

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrXferProcessor.cpp
File src/gpu/GrXferProcessor.cpp (right):

https://codereview.chromium.org/885923002/diff/20001/src/gpu/GrXferProcessor....
src/gpu/GrXferProcessor.cpp:16: kTopLeftOrigin_DstReadKeyBit    = 0x4, // Set if
dst-copy origin is top-left.
On 2015/01/30 14:59:15, bsalomon wrote:
> canonicalize on always tl or bl?

Done.

[bsalomon, 2015-02-03 17:28:24]

lgtm, some comments and questions.

https://codereview.chromium.org/885923002/diff/80001/src/effects/SkArithmetic...
File src/effects/SkArithmeticMode_gpu.h (right):

https://codereview.chromium.org/885923002/diff/80001/src/effects/SkArithmetic...
src/effects/SkArithmeticMode_gpu.h:74: class GrArithmeticXP : public
GrXferProcessor {
Curious, can the XP be moved to a cpp, seems like only the XPF needs to be
exposed in a header, right?

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrDrawTarget.cpp...
src/gpu/GrDrawTarget.cpp:419: desc.fOrigin = kTopLeft_GrSurfaceOrigin;
maybe a comment here that our contract with XP is that the dst copy is always
kTopLeft?

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrPipeline.cpp
File src/gpu/GrPipeline.cpp (right):

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrPipeline.cpp#n...
src/gpu/GrPipeline.cpp:139: fReadsFragPosition = false;
do we need to check in here if the xp reads the frag pos (for impl of dst copy)

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrXferProcessor.cpp
File src/gpu/GrXferProcessor.cpp (right):

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrXferProcessor....
src/gpu/GrXferProcessor.cpp:20: this->addTextureAccess(&fDstCopy);
Assert that fDstCopy has kTopLeft origin?

[bsalomon, 2015-02-03 17:28:26]

lgtm, some comments and questions.

[egdaniel, 2015-02-03 19:50:57]

https://codereview.chromium.org/885923002/diff/80001/src/effects/SkArithmetic...
File src/effects/SkArithmeticMode_gpu.h (right):

https://codereview.chromium.org/885923002/diff/80001/src/effects/SkArithmetic...
src/effects/SkArithmeticMode_gpu.h:74: class GrArithmeticXP : public
GrXferProcessor {
On 2015/02/03 17:28:24, bsalomon wrote:
> Curious, can the XP be moved to a cpp, seems like only the XPF needs to be
> exposed in a header, right?

Yeah you are right and this is the case with all the XP implementations. I'll
put a follow up CL together that does all this moving around.

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrDrawTarget.cpp...
src/gpu/GrDrawTarget.cpp:419: desc.fOrigin = kTopLeft_GrSurfaceOrigin;
On 2015/02/03 17:28:24, bsalomon wrote:
> maybe a comment here that our contract with XP is that the dst copy is always
> kTopLeft?

Done.

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrPipeline.cpp
File src/gpu/GrPipeline.cpp (right):

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrPipeline.cpp#n...
src/gpu/GrPipeline.cpp:139: fReadsFragPosition = false;
On 2015/02/03 17:28:24, bsalomon wrote:
> do we need to check in here if the xp reads the frag pos (for impl of dst
copy)

No we don't really need to check it, even though we do read the frag position if
we do a dst copy. First in terms of shader keys, if we are doing a dst read, we
set the key to reflect that. This is actually unique enough to tell if the XP
will also look at frag pos since a given gpu will either always read frag pos
with a dst read or it will never read it with dst read (if fbFetch). The only
other thing that is in the FragPos key whether the texture is top left or bottom
left. Well as per changes in this cl for the dst copy case we will only have top
left.

Now in terms of emitted code, the call to
FragmentShaderBuilder::fragmetPosition() is always made directly in the emitcode
of subclasses, and for the XP the only check on whether to call this or not is
again if we are doing a dstCopy.

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrXferProcessor.cpp
File src/gpu/GrXferProcessor.cpp (right):

https://codereview.chromium.org/885923002/diff/80001/src/gpu/GrXferProcessor....
src/gpu/GrXferProcessor.cpp:20: this->addTextureAccess(&fDstCopy);
On 2015/02/03 17:28:24, bsalomon wrote:
> Assert that fDstCopy has kTopLeft origin?

Done.

[egdaniel, 2015-02-03 21:41:17]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2015-02-03 21:42:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/885923002/120001

[commit-bot: I haz the power, 2015-02-03 21:42:13]

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

[commit-bot: I haz the power, 2015-02-03 21:42:14]

Failed to apply patch for src/gpu/GrBatchTarget.cpp:
While running git apply --index -3 -p1;
  error: patch failed: src/gpu/GrBatchTarget.cpp:40
  Falling back to three-way merge...
  Applied patch to 'src/gpu/GrBatchTarget.cpp' with conflicts.
  U src/gpu/GrBatchTarget.cpp

Patch:       src/gpu/GrBatchTarget.cpp
Index: src/gpu/GrBatchTarget.cpp
diff --git a/src/gpu/GrBatchTarget.cpp b/src/gpu/GrBatchTarget.cpp
index
1b68923472e97ccc09b01f9e83c9c135933d640d..9d96e612c3fa547d14a229b3264da3977af509a0
100644
--- a/src/gpu/GrBatchTarget.cpp
+++ b/src/gpu/GrBatchTarget.cpp
@@ -21,8 +21,7 @@ void GrBatchTarget::flush() {
         BufferedFlush* bf = iter.get();
         const GrPipeline* pipeline = bf->fPipeline;
         const GrPrimitiveProcessor* primProc = bf->fPrimitiveProcessor.get();
-        fGpu->buildProgramDesc(&desc, *primProc, *pipeline,
pipeline->descInfo(),
-                               bf->fBatchTracker);
+        fGpu->buildProgramDesc(&desc, *primProc, *pipeline, bf->fBatchTracker);
 
         GrGpu::DrawArgs args(primProc, pipeline, &desc, &bf->fBatchTracker);
         for (int i = 0; i < bf->fDraws.count(); i++) {
@@ -40,8 +39,7 @@ void GrBatchTarget::flushNext(int n) {
         BufferedFlush* bf = fIter.get();
         const GrPipeline* pipeline = bf->fPipeline;
         const GrPrimitiveProcessor* primProc = bf->fPrimitiveProcessor.get();
-        fGpu->buildProgramDesc(&desc, *primProc, *pipeline,
pipeline->descInfo(),
-                               bf->fBatchTracker);
+        fGpu->buildProgramDesc(&desc, *primProc, *pipeline, bf->fBatchTracker);
 
         GrGpu::DrawArgs args(primProc, pipeline, &desc, &bf->fBatchTracker);
         for (int i = 0; i < bf->fDraws.count(); i++) {

[egdaniel, 2015-02-03 21:45:02]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2015-02-03 21:45:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/885923002/140001

[commit-bot: I haz the power, 2015-02-03 23:02:47]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/74a11753604768bf461b80cabb66060e8564d82c

[egdaniel, 2015-02-04 14:04:22]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/900883002/ by egdaniel@google.com.

The reason for reverting is: Breaking xfermodes on some android devices.

[egdaniel, 2015-02-04 20:15:03]

changed back to checking the origin in the XP instead of forcing dstCopies to be
topLeft

[egdaniel, 2015-02-05 19:00:09]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2015-02-05 19:00:33]

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

[commit-bot: I haz the power, 2015-02-05 19:11:18]

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

[bsalomon, 2015-02-05 22:16:40]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/901663007/ by bsalomon@google.com.

The reason for reverting is: Testing to see if reverting fixes 10.9 bots..