Simplify GrGLPathRendering interface

Simplify GrGLPathRendering interface

Simplify GrGLPathRendering interface by removing polymorphism and functions that
simply wrap GL functions unnecessarily.

Replace the polymorphism by if -condition.

Call the unconditional GL functions directly. GrGLPath, GrGLPathRange and
GrGLPathRendering are part of the same logical subsystem. This means that the
subsystem implementation details are taken into account within these
classes. Example: if support for using conics would be added, the feature flag
would go to GrGLPathRendering::Caps, the emulation would go to GrGLPath instead of
GrGLPathRendering::pathCommands. Wrapping glPathCommandsNV is not useful.

Try to expose the interface fully in same logical level; rename fragment input
function to reflect this.

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

[Kimmo Kinnunen, 2014-08-08 12:45:03]

Depends:
https://codereview.chromium.org/444223002/ (reland of GrGLPathRendering)
https://codereview.chromium.org/452823002/ (Separate GL path rendering state
from GrGpuGL to GrGLPathRendering)

[Chris Dalton, 2014-08-14 17:03:45]

The GrGLPathRendering class serves a completely different purpose now, which is
OK since GrGpuGL was getting drowned out by all the nvpr and it was good to do
something about it.

But, I do miss having a class that wraps the nvpr API. I liked how before you
just called 'pathRendering->whatever()', whereas now there are implicit rules
about which calls you make directly on the GrGLInterface (stencilFillPath,
pathTexGen, etc.), and which go through GrGLPathRendering
(stencilThenCoverFillPath, genPaths, etc.).

Note that wrapping extensions is also the direction WebGL has taken:

  gl.coreFunction();
  gl.getExtension().extensionFunction();

But of course, the original patch didn't do that puristically either, since the
nvpr functions all existed on the GrGLInterface. There was just an expectation
that you always went through GrGLPathRendering instead of calling them directly.
And it didn't provide a really clean solution for our pathFragmentInputGen vs.
pathTexGen problem.

I think this is the type of thing that could benefit from a long whiteboard
discussion. Do we want to try and organize a phone conference with the
interested parties from nvidia and google? I believe a design decision here
could be helpful for more than just nvpr.

https://chromiumcodereview.appspot.com/450283002/diff/1/src/gpu/gl/GrGLPathRe...
File src/gpu/gl/GrGLPathRendering.h (right):

https://chromiumcodereview.appspot.com/450283002/diff/1/src/gpu/gl/GrGLPathRe...
src/gpu/gl/GrGLPathRendering.h:101: bool thenFunctionsSupport : 1;
I think "stencilThenCoverSupport" might be more clear

[Kimmo Kinnunen, 2014-08-15 07:02:04]

On 2014/08/14 17:03:45, Chris Dalton wrote:
> But, I do miss having a class that wraps the nvpr API. I liked how before you
> just called 'pathRendering->whatever()', whereas now there are implicit rules
> about which calls you make directly on the GrGLInterface (stencilFillPath,
> pathTexGen, etc.), and which go through GrGLPathRendering
> (stencilThenCoverFillPath, genPaths, etc.).

But the question is: what actual benefit does this bring?
It does not abstract anything meaningful. The abstraction stays within
GrGLPathRendering. There is hardly any complexity here to abstract out. Creating
3 classes just to have 2 method calls be textually different, is it really the
thing we need to beautify?
 
This patch removes 100 lines of code and has an arguable runtime benefit.

> Note that wrapping extensions is also the direction WebGL has taken:
>   gl.coreFunction();
>   gl.getExtension().extensionFunction();

Yes, very "beautiful"? Our corresponding thing would be GL API, where we have
similar thing: glCoreFunction() and glExtensionFunction().
Personally, I don't see WebGL API design as a valid argument for any Skia
internal implementation choice..
Though, personally, I don't see "I prefer to type this instead of that" either.

> But of course, the original patch didn't do that puristically either, since
the
> nvpr functions all existed on the GrGLInterface. There was just an expectation
> that you always went through GrGLPathRendering instead of calling them
directly.

The expectation still holds, as is explained in the commit message. Outside the
GrGLPath* system, you go through the "abstract" GrGLPath* subsystem methods.
Inside of the subsystem is such a trivial, small and static code that I don't
see it justified to add complexity there.
 
> I think this is the type of thing that could benefit from a long whiteboard
> discussion. Do we want to try and organize a phone conference with the
> interested parties from nvidia and google? I believe a design decision here
> could be helpful for more than just nvpr.

I think not. Mere scheduling an conf call for this little thing delays this
patch for a week. I think we should just move on. (I'd prefer also try to not
add pointless code, but if there's a choise to be made with pointless conf calls
or pointless code, we can just abandon this patch.)

[Kimmo Kinnunen, 2014-08-15 07:02:16]

https://chromiumcodereview.appspot.com/450283002/diff/1/src/gpu/gl/GrGLPathRe...
File src/gpu/gl/GrGLPathRendering.h (right):

https://chromiumcodereview.appspot.com/450283002/diff/1/src/gpu/gl/GrGLPathRe...
src/gpu/gl/GrGLPathRendering.h:101: bool thenFunctionsSupport : 1;
On 2014/08/14 17:03:45, Chris Dalton wrote:
> I think "stencilThenCoverSupport" might be more clear

Done.

[Chris Dalton, 2014-08-15 17:56:41]

lgtm

lgtm

[Chris Dalton, 2014-08-15 18:01:38]

> But the question is: what actual benefit does this bring?

The benefit is that it protects against subtle bugs in the future (for example,
somebody calls DeletePaths on the GrGLInterface for a path that was created with
genPaths on GrGLPathRendering, or somebody calls StencilThenCover on the
GrGLInterface and only tests on drivers that support that function, etc.) Having
all the api calls consolidated into one place is less bug prone than unclear
rules about which methods you call on which object.

But you are right that the path rendering subsystem is pretty small for now, so
it might not be too big of a deal.

[bsalomon, 2014-08-19 21:22:58]

lgtm

[Kimmo Kinnunen, 2014-08-20 11:04:50]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-08-20 11:05:05]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/450283002/20001

[commit-bot: I haz the power, 2014-08-20 11:05:08]

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

[commit-bot: I haz the power, 2014-08-20 11:05:09]

Failed to apply patch for src/gpu/gl/GrGLPathRendering.h:
While running git apply --index -p1;
  error: patch failed: src/gpu/gl/GrGLPathRendering.h:43
  error: src/gpu/gl/GrGLPathRendering.h: patch does not apply

Patch:       src/gpu/gl/GrGLPathRendering.h
Index: src/gpu/gl/GrGLPathRendering.h
diff --git a/src/gpu/gl/GrGLPathRendering.h b/src/gpu/gl/GrGLPathRendering.h
index
260daa800775b6952cbe31b4038e95bc2de1b83d..422b1dc7612af595163a02405552a97ee83a764c
100644
--- a/src/gpu/gl/GrGLPathRendering.h
+++ b/src/gpu/gl/GrGLPathRendering.h
@@ -27,11 +27,9 @@ class GrGpuGL;
 class GrGLPathRendering : public GrPathRendering {
 public:
     /**
-     * Create a new GrGLPathRendering object from a given GrGpuGL. Unless
-     * otherwise specified in the caps, every method will work properly, even
-     * if it did not exist in the GL interface of the gpu.
+     * Create a new GrGLPathRendering object from a given GrGpuGL.
      */
-    static GrGLPathRendering* Create(GrGpuGL* gpu);
+    GrGLPathRendering(GrGpuGL* gpu);
     virtual ~GrGLPathRendering();
 
     // GrPathRendering implementations.
@@ -43,16 +41,6 @@ public:
                            const float transforms[], PathTransformType,
                            SkPath::FillType) SK_OVERRIDE;
 
-    /**
-     * Mark certain functionality as not supported if the driver version is too
-     * old and a backup implementation is not practical.
-     */
-    struct Caps {
-        bool fragmentInputGenSupport : 1;
-    };
-    const Caps& caps() const { return fCaps; }
-
-
     /* Called when the 3D context state is unknown. */
     void onResetContext();
 
@@ -62,6 +50,25 @@ public:
      */
     void abandonGpuResources();
 
+
+    enum TexturingMode {
+        FixedFunction_TexturingMode,
+        SeparableShaders_TexturingMode
+    };
+
+    /** Specifies whether texturing should use fixed fuction pipe or separable
shaders
+     * Specifies whether texturing should use fixed fuction pipe or whether
+     * it is ok to use normal vertex and fragment shaders, and for path
rendering
+     * populate fragment shaders with setProgramPathFragmentInputTransform.
+     * The fixed function mode will be removed once the other mode is more
widely
+     * available.
+     */
+    TexturingMode texturingMode() const  {
+        return caps().fragmentInputGenSupport ?
+            SeparableShaders_TexturingMode : FixedFunction_TexturingMode;
+    }
+
+    // Functions for fixed function texturing support.
     enum PathTexGenComponents {
         kS_PathTexGenComponents = 1,
         kST_PathTexGenComponents = 2,
@@ -70,65 +77,50 @@ public:
     void enablePathTexGen(int unitIdx, PathTexGenComponents, const GrGLfloat*
coefficients);
     void enablePathTexGen(int unitIdx, PathTexGenComponents, const SkMatrix&
matrix);
     void flushPathTexGenSettings(int numUsedTexCoordSets);
+
+    // Functions for "separable shader" texturing support.
+    void setProgramPathFragmentInputTransform(GrGLuint program, GrGLint
location,
+                                              GrGLenum genMode, GrGLint
components,
+                                              const SkMatrix&);
+
+    /* Sets the projection matrix for path rendering */
     void setProjectionMatrix(const SkMatrix& matrix,
                              const SkISize& renderTargetSize,
                              GrSurfaceOrigin renderTargetOrigin);
 
-
-    // NV_path_rendering
     GrGLuint genPaths(GrGLsizei range);
     GrGLvoid deletePaths(GrGLuint path, GrGLsizei range);
-    GrGLvoid pathCommands(GrGLuint path, GrGLsizei numCommands, const GrGLubyte
*commands,
-                          GrGLsizei numCoords, GrGLenum coordType, const
GrGLvoid *coords);
-    GrGLvoid pathCoords(GrGLuint path, GrGLsizei numCoords,
-                        GrGLenum coordType, const GrGLvoid *coords);
-    GrGLvoid pathParameteri(GrGLuint path, GrGLenum pname, GrGLint value);
-    GrGLvoid pathParameterf(GrGLuint path, GrGLenum pname, GrGLfloat value);
-    GrGLboolean isPath(GrGLuint path);
-    GrGLvoid pathStencilFunc(GrGLenum func, GrGLint ref, GrGLuint mask);
-    GrGLvoid stencilFillPath(GrGLuint path, GrGLenum fillMode, GrGLuint mask);
-    GrGLvoid stencilStrokePath(GrGLuint path, GrGLint reference, GrGLuint
mask);
-    GrGLvoid stencilFillPathInstanced(GrGLsizei numPaths, GrGLenum
pathNameType,
-                                      const GrGLvoid *paths, GrGLuint pathBase,
GrGLenum fillMode,
-                                      GrGLuint mask, GrGLenum transformType,
-                                      const GrGLfloat *transformValues);
-    GrGLvoid stencilStrokePathInstanced(GrGLsizei numPaths, GrGLenum
pathNameType,
-                                        const GrGLvoid *paths, GrGLuint
pathBase,
-                                        GrGLint reference, GrGLuint mask,
GrGLenum transformType,
-                                        const GrGLfloat *transformValues);
-    GrGLvoid pathTexGen(GrGLenum texCoordSet, GrGLenum genMode,
-                        GrGLint components, const GrGLfloat *coeffs);
-    GrGLvoid coverFillPath(GrGLuint path, GrGLenum coverMode);
-    GrGLvoid coverStrokePath(GrGLuint name, GrGLenum coverMode);
-    GrGLvoid coverFillPathInstanced(GrGLsizei numPaths, GrGLenum pathNameType,
-                                    const GrGLvoid *paths, GrGLuint pathBase,
GrGLenum coverMode,
-                                    GrGLenum transformType, const GrGLfloat
*transformValues);
-    GrGLvoid coverStrokePathInstanced(GrGLsizei numPaths, GrGLenum
pathNameType,
-                                      const GrGLvoid *paths, GrGLuint pathBase,
GrGLenum coverMode,
-                                      GrGLenum transformType, const GrGLfloat*
transformValues);
+
+private:
+    /**
+     * Mark certain functionality as not supported if the driver version is too
+     * old and a backup implementation is not practical.
+     */
+    struct Caps {
+        bool stencilThenCoverSupport : 1;
+        bool fragmentInputGenSupport : 1;
+    };
+    const Caps& caps() const { return fCaps; }
+
+    void flushPathStencilSettings(SkPath::FillType fill);
 
     // NV_path_rendering v1.2
-    virtual GrGLvoid stencilThenCoverFillPath(GrGLuint path, GrGLenum fillMode,
-                                              GrGLuint mask, GrGLenum
coverMode);
-    virtual GrGLvoid stencilThenCoverStrokePath(GrGLuint path, GrGLint
reference,
-                                                GrGLuint mask, GrGLenum
coverMode);
-    virtual GrGLvoid stencilThenCoverFillPathInstanced(
-                         GrGLsizei numPaths, GrGLenum pathNameType, const
GrGLvoid *paths,
+    void stencilThenCoverFillPath(GrGLuint path, GrGLenum fillMode,
+                                  GrGLuint mask, GrGLenum coverMode);
+
+    void stencilThenCoverStrokePath(GrGLuint path, GrGLint reference,
+                                    GrGLuint mask, GrGLenum coverMode);
+
+    void stencilThenCoverFillPathInstanced(
+        GrGLsizei numPaths, GrGLenum pathNameType, const GrGLvoid *paths,
                          GrGLuint pathBase, GrGLenum fillMode, GrGLuint mask,
GrGLenum coverMode,
                          GrGLenum transformType, const GrGLfloat
*transformValues);
-    virtual GrGLvoid stencilThenCoverStrokePathInstanced(
+
+    void stencilThenCoverStrokePathInstanced(
                          GrGLsizei numPaths, GrGLenum pathNameType, const
GrGLvoid *paths,
                          GrGLuint pathBase, GrGLint reference, GrGLuint mask,
GrGLenum coverMode,
                          GrGLenum transformType, const GrGLfloat
*transformValues);
 
-    // NV_path_rendering v1.3
-    virtual GrGLvoid programPathFragmentInputGen(GrGLuint program, GrGLint
location,
-                                                 GrGLenum genMode, GrGLint
components,
-                                                 const GrGLfloat *coeffs);
-
-protected:
-    GrGLPathRendering(GrGpuGL* gpu);
-
     GrGpuGL* fGpu;
     SkAutoTDelete<GrGLNameAllocator> fPathNameAllocator;
     Caps fCaps;
@@ -141,10 +133,6 @@ protected:
     };
     int fHWActivePathTexGenSets;
     SkTArray<PathTexGenData, true> fHWPathTexGenSettings;
-
-private:
-    void flushPathStencilSettings(SkPath::FillType fill);
-
 };
 
 #endif

[Kimmo Kinnunen, 2014-08-20 11:05:19]

The CQ bit was unchecked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2014-08-20 11:06:01]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-08-20 11:06:55]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/450283002/40001

[commit-bot: I haz the power, 2014-08-20 11:13:32]

Committed patchset #3 (40001) as 5b653577994fe298e08e5f7a5c1fa39fe53c9203