Create GrOptDrawState before recording draw in GrInOrderDrawBuffer

Create GrOptDrawState before recording draw in GrInOrderDrawBuffer

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

[bsalomon, 2014-11-18 22:23:08]

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

[joshualitt, 2014-11-19 14:22:40]

Just some nits, this looks good to me but someone else should definitely take a
look.

https://codereview.chromium.org/739673002/diff/140001/include/gpu/GrProgramEl...
File include/gpu/GrProgramElement.h (right):

https://codereview.chromium.org/739673002/diff/140001/include/gpu/GrProgramEl...
include/gpu/GrProgramElement.h:107: this->validate();
Is there anyway to refactor this->validate so we  don't need two calls?  I
realize it only effects debug and maybe there isn't.

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrOptDrawState.cpp
File src/gpu/GrOptDrawState.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:72: SkASSERT(0 == fFragmentStages.count());
Will this ever be not true?

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:74: -1 !=
descInfo.fFixedFunctionVertexAttribIndices[kLocalCoord_GrVertexAttribBinding];
descInfo.hasLocalCoordAttribute()

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:294: SkASSERT(this->numFragmentStages() ==
that.numFragmentStages());
does this get tested with the desc equality test?

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:331: void GrGLNvprProgram::setTransformData(const
GrPendingFragmentStage& proc, GrGLInstalledFragProc* ip) {
line wraps

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:372: GrGLLegacyNvprProgram::setTransformData(const
GrPendingFragmentStage& proc, GrGLInstalledFragProc* ip) {
line wraps

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLLegacyNvprProgramBuilder.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLLegacyNvprProgramBuilder.cpp:40: 
align

[joshualitt, 2014-11-19 14:55:35]

one more nit

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrInOrderDrawBu...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:517: }
Are these checks still necessary?

[joshualitt, 2014-11-19 14:55:36]

one more nit

[egdaniel, 2014-11-19 15:00:36]

lgtm with nits

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrInOrderDrawBu...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:265:
GrGpu::PrimTypeToDrawType(info.primitiveType()),
align

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLProgramBuilder.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.cpp:370: GrSLType varyingType =
can we remove some of the \n here?

[bsalomon, 2014-11-19 15:04:43]

https://codereview.chromium.org/739673002/diff/140001/include/gpu/GrProgramEl...
File include/gpu/GrProgramElement.h (right):

https://codereview.chromium.org/739673002/diff/140001/include/gpu/GrProgramEl...
include/gpu/GrProgramElement.h:107: this->validate();
On 2014/11/19 14:22:39, joshualitt wrote:
> Is there anyway to refactor this->validate so we  don't need two calls?  I
> realize it only effects debug and maybe there isn't.

It doesn't need to be called twice (or at all)... I was just doing pre- and
post- calls to be sure the state was correct before and after the call..

I could add and AutoValidate that validates in its cons and ~, but I think it
might increase the total line count

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrInOrderDrawBu...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:517: }
On 2014/11/19 14:55:35, joshualitt wrote:
> Are these checks still necessary?

oh nice, catch. No, they're not.

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrOptDrawState.cpp
File src/gpu/GrOptDrawState.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:72: SkASSERT(0 == fFragmentStages.count());
On 2014/11/19 14:22:39, joshualitt wrote:
> Will this ever be not true?

I'll remove it... it was for my own sanity as I was working on this. I noticed
that the old code called fFragmentStages.reset() when there were no color stages
to copy and was wondering how that could be necessary. I don't think it was.

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:74: -1 !=
descInfo.fFixedFunctionVertexAttribIndices[kLocalCoord_GrVertexAttribBinding];
On 2014/11/19 14:22:39, joshualitt wrote:
> descInfo.hasLocalCoordAttribute()

tx!

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:294: SkASSERT(this->numFragmentStages() ==
that.numFragmentStages());
On 2014/11/19 14:22:39, joshualitt wrote:
> does this get tested with the desc equality test?

Yeah, it has these fields:

        SkBool8                     fHasGeometryProcessor;
        int8_t                      fColorEffectCnt;
        int8_t                      fCoverageEffectCnt;

Will add a comment to that effect.

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:331: void GrGLNvprProgram::setTransformData(const
GrPendingFragmentStage& proc, GrGLInstalledFragProc* ip) {
On 2014/11/19 14:22:39, joshualitt wrote:
> line wraps

Done.

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:372: GrGLLegacyNvprProgram::setTransformData(const
GrPendingFragmentStage& proc, GrGLInstalledFragProc* ip) {
On 2014/11/19 14:22:39, joshualitt wrote:
> line wraps

Done.

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLLegacyNvprProgramBuilder.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLLegacyNvprProgramBuilder.cpp:40: 
On 2014/11/19 14:22:39, joshualitt wrote:
> align

Done.

[bsalomon, 2014-11-19 15:09:38]

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrInOrderDrawBu...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:265:
GrGpu::PrimTypeToDrawType(info.primitiveType()),
On 2014/11/19 15:00:36, egdaniel wrote:
> align

Done.

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLProgramBuilder.cpp (right):

https://codereview.chromium.org/739673002/diff/140001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.cpp:370: GrSLType varyingType =
On 2014/11/19 15:00:36, egdaniel wrote:
> can we remove some of the \n here?

Done.

[joshualitt, 2014-11-19 15:12:54]

On 2014/11/19 15:04:43, bsalomon wrote:
>
https://codereview.chromium.org/739673002/diff/140001/include/gpu/GrProgramEl...
> File include/gpu/GrProgramElement.h (right):
> 
>
https://codereview.chromium.org/739673002/diff/140001/include/gpu/GrProgramEl...
> include/gpu/GrProgramElement.h:107: this->validate();
> On 2014/11/19 14:22:39, joshualitt wrote:
> > Is there anyway to refactor this->validate so we  don't need two calls?  I
> > realize it only effects debug and maybe there isn't.
> 
> It doesn't need to be called twice (or at all)... I was just doing pre- and
> post- calls to be sure the state was correct before and after the call..
> 
> I could add and AutoValidate that validates in its cons and ~, but I think it
> might increase the total line count
> 
An autovalidate would also check the status on some of the early returns, which
might be nice.  You are the one who stares at this the most though so its your
call.

LGTM

[bsalomon, 2014-11-19 15:20:06]

https://codereview.chromium.org/739673002/diff/140001/include/gpu/GrProgramEl...
File include/gpu/GrProgramElement.h (right):

https://codereview.chromium.org/739673002/diff/140001/include/gpu/GrProgramEl...
include/gpu/GrProgramElement.h:107: this->validate();
On 2014/11/19 15:04:43, bsalomon wrote:
> On 2014/11/19 14:22:39, joshualitt wrote:
> > Is there anyway to refactor this->validate so we  don't need two calls?  I
> > realize it only effects debug and maybe there isn't.
> 
> It doesn't need to be called twice (or at all)... I was just doing pre- and
> post- calls to be sure the state was correct before and after the call..
> 
> I could add and AutoValidate that validates in its cons and ~, but I think it
> might increase the total line count

Actually I can't do that since some of the calls delete "this" before returning
and thus can't validate afterwards.

[bsalomon, 2014-11-19 15:20:10]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-11-19 15:20:16]

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

[commit-bot: I haz the power, 2014-11-19 15:20:25]

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

[commit-bot: I haz the power, 2014-11-19 15:20:26]

Failed to apply patch for include/gpu/GrFragmentStage.h:
While running git apply --index -3 -p1;
  error: patch failed: include/gpu/GrProcessorStage.h:129
  Falling back to three-way merge...
  Applied patch to 'include/gpu/GrFragmentStage.h' with conflicts.
  U include/gpu/GrFragmentStage.h

Patch:   NR  include/gpu/GrProcessorStage.h->include/gpu/GrFragmentStage.h
Index: include/gpu/GrFragmentStage.h
diff --git a/include/gpu/GrProcessorStage.h b/include/gpu/GrFragmentStage.h
similarity index 70%
rename from include/gpu/GrProcessorStage.h
rename to include/gpu/GrFragmentStage.h
index
6f8496db06d7e60fc7820047e1ed0a9622318dee..8089e9fab7a7819b94502a1d6ee4285e6248ff5a
100644
--- a/include/gpu/GrProcessorStage.h
+++ b/include/gpu/GrFragmentStage.h
@@ -1,4 +1,3 @@
-
 /*
  * Copyright 2010 Google Inc.
  *
@@ -6,23 +5,17 @@
  * found in the LICENSE file.
  */
 
+#ifndef GrFragmentStage_DEFINED
+#define GrFragmentStage_DEFINED
 
-
-#ifndef GrProcessorStage_DEFINED
-#define GrProcessorStage_DEFINED
-
-#include "GrBackendProcessorFactory.h"
-#include "GrCoordTransform.h"
 #include "GrFragmentProcessor.h"
-#include "GrProgramElementRef.h"
 #include "SkMatrix.h"
-#include "SkShader.h"
 
-// TODO: Make two variations on this class: One for GrDrawState that only owns
regular refs
-// and supports compatibility checks and changing local coords. The second is
for GrOptDrawState,
-// is immutable, and only owns pending execution refs. This requries removing
the common base
-// class from GrDrawState and GrOptDrawState called GrRODrawState and
converting to GrOptDrawState
-// when draws are enqueued in the GrInOrderDrawBuffer.
+/**
+ * Wraps a GrFragmentProcessor. It also contains a coord change matrix. This
matrix should be
+ * concat'ed with all the processor's coord transforms that apply to local
coords, unless
+ * explicit local coords are provided with the draw.
+ */
 class GrFragmentStage {
 public:
     explicit GrFragmentStage(const GrFragmentProcessor* proc)
@@ -35,9 +28,9 @@ public:
         if (other.fCoordChangeMatrixSet) {
             fCoordChangeMatrix = other.fCoordChangeMatrix;
         }
-        fProc.initAndRef(other.fProc);
+        fProc.reset(SkRef(other.fProc.get()));
     }
-    
+
     static bool AreCompatible(const GrFragmentStage& a, const GrFragmentStage&
b,
                               bool usingExplicitLocalCoords) {
         SkASSERT(a.fProc.get());
@@ -129,31 +122,12 @@ public:
         }
     }
 
-    bool isPerspectiveCoordTransform(int matrixIndex, bool
useExplicitLocalCoords) const {
-        const GrCoordTransform& coordTransform =
this->getProcessor()->coordTransform(matrixIndex);
-        SkMatrix::TypeMask type0 = coordTransform.getMatrix().getType();
-        SkMatrix::TypeMask type1 = SkMatrix::kIdentity_Mask;
-        if (kLocal_GrCoordSet == coordTransform.sourceCoords()) {
-          type1 = useExplicitLocalCoords ?
-                  SkMatrix::kIdentity_Mask :
this->getCoordChangeMatrix().getType();
-        }
-
-        int combinedTypes = type0 | type1;
-        if (SkMatrix::kPerspective_Mask & combinedTypes) {
-          return true;
-        } else {
-          return false;
-        }
-    }
-
     const GrFragmentProcessor* getProcessor() const { return fProc.get(); }
 
-    void convertToPendingExec() { fProc.convertToPendingExec(); }
-
 protected:
-    bool                                           fCoordChangeMatrixSet;
-    SkMatrix                                       fCoordChangeMatrix;
-    GrProgramElementRef<const GrFragmentProcessor> fProc;
+    bool                                    fCoordChangeMatrixSet;
+    SkMatrix                                fCoordChangeMatrix;
+    SkAutoTUnref<const GrFragmentProcessor> fProc;
 };
 
 #endif

[bsalomon, 2014-11-19 16:07:27]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-11-19 16:08:00]

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

[commit-bot: I haz the power, 2014-11-19 16:10:21]

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

[commit-bot: I haz the power, 2014-11-19 16:10:22]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)

[bsalomon, 2014-11-19 16:12:35]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-11-19 16:12:59]

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

[commit-bot: I haz the power, 2014-11-19 16:23:58]

Committed patchset #12 (id:220001) as
https://skia.googlesource.com/skia/+/ae59b77612c42ff6b793dc33e3d115e6a5db34cc