Added a GLBench for testing performance of vec4 vs scalar for coverage in generated shaders.

bench/GLVec4ScalarBench.cpp

+/*
+ * Copyright 2015 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "Benchmark.h"
+#include "SkCanvas.h"
+#include "SkImageEncoder.h"

[tomhudson, 2015/07/10 19:23:54]

You don't seem to need any of these 3 includes any more?
You might want to explicitly include SkMatrix, SkPoint, SkString - it's open to
debate.

[wangyix, 2015/07/13 17:29:52]

Done.

+
+#if SK_SUPPORT_GPU
+#include "GLBench.h"
+#include "gl/GrGLGLSL.h"
+#include "gl/GrGLInterface.h"
+#include "gl/GrGLShaderVar.h"
+#include "gl/GrGLUtil.h"
+#include "glsl/GrGLSLCaps.h"
+
+#include <stdio.h>
+
+/*

[tomhudson, 2015/07/10 19:23:54]

Please make this /** so doxygen can pick it up.
Also, bring the comment up to date to describe what you're actually doing here,
and why.

[wangyix, 2015/07/13 17:29:53]

Done.

+ * This is a native GL benchmark for instanced arrays vs vertex buffer objects.  To benchmark this
+ * functionality, we draw n * kDrawMultipier triangles per run.  If this number is less than
+ * kNumTri then we do a single draw, either with instances, or drawArrays.  Otherwise we do
+ * multiple draws.
+ *
+ * Additionally, there is a divisor, which if > 0 will act as a multiplier for the number of draws
+ * issued.
+ */
+
+class GLVec4ScalarBench : public GLBench {
+public:
+    /*
+     * Use float or vec4 as GLSL data type for the output coverage
+     */
+    enum CoverageSetup {
+        kUseScalar_CoverageSetup,
+        kUseVec4_CoverageSetup,
+    };
+
+    /*
+     * drawDiv will act as a multiplier for the number of draws we issue if > 0. ie, 2 will issue
+     * 2x as many draws, 4 will issue 4x as many draws etc.  There is a limit however, which is
+     * kDrawMultipier.

[tomhudson, 2015/07/10 19:23:54]

Outdated comment; cut.
If the class comment is good enough and your variable names are good enough you
probably don't need to say anything here.

[wangyix, 2015/07/13 17:29:52]

Done.

+     */
+    GLVec4ScalarBench(CoverageSetup coverageSetup, uint32_t numStages)
+        : fCoverageSetup(coverageSetup)
+        , fNumStages(numStages)
+        , fProgram(0)
+        , fVAO(0) {

[tomhudson, 2015/07/10 19:23:55]

Why explicitly clear fVAO but not fVBO?

[wangyix, 2015/07/13 17:29:53]

Done.

+        fName = CoverageSetupToStr(coverageSetup, numStages);
+    }
+
+protected:
+    const char* onGetName() override {
+        return fName.c_str();
+    }
+
+    const GrGLContext* onGetGLContext(const GrGLContext*) override;

[tomhudson, 2015/07/13 16:00:13]

Once we've started reviewing, it's good for the reviewer to understand all your
changes. Why are you no longer overriding this?

OK, reading the text of the function definition it looks like it was only
relevant to the previous test, and the default behavior is sufficient here. But
then your PTAL message could have *also* mentioned this change.

[wangyix, 2015/07/13 17:29:52]

Yep that was the reason. The override was specific to the previous test.

+    void setup(const GrGLContext*) override;
+    void glDraw(const int loops, const GrGLContext*) override;
+    void teardown(const GrGLInterface*) override;
+
+private:
+    void setupSingleVbo(const GrGLInterface*, const SkMatrix*);
+    GrGLuint setupShader(const GrGLContext*, bool useVec4ForCoverage);
+
+
+    static SkString CoverageSetupToStr(CoverageSetup vboSetup, uint32_t numStages) {

[tomhudson, 2015/07/10 19:23:55]

Consider tweaking the function name, since you're doing more than just writing
"_scalar_" or "_vec4_".

[wangyix, 2015/07/13 17:29:52]

Done.

+        SkString name("GLVec4ScalarBench");
+        switch (vboSetup) {
+            default:
+            case kUseScalar_CoverageSetup:
+                name.appendf("_scalar_%u_stage", numStages);
+                break;
+            case kUseVec4_CoverageSetup:
+                name.appendf("_vec4_%u_stage", numStages);
+                break;
+        }
+        return name;
+    }
+
+    static const GrGLuint kScreenWidth = 800;
+    static const GrGLuint kScreenHeight = 600;
+    static const uint32_t kNumTriPerDraw = 512;
+    static const uint32_t kVerticesPerTri = 3;
+
+    SkString fName;
+    CoverageSetup fCoverageSetup;
+    uint32_t fNumStages;
+    GrGLuint fVBO;

[tomhudson, 2015/07/10 19:23:54]

Consider writing out name instead of using abbreviation.

[wangyix, 2015/07/13 17:29:52]

Vao and Vbo are extremely common occurrences in OpenGL code and should n't cause
any confusion.

+    GrGLuint fProgram;
+    GrGLuint fVAO;

[tomhudson, 2015/07/10 19:23:55]

... particularly when two abbreviations are very close together.

[wangyix, 2015/07/13 17:29:52]

Done.

+    GrGLuint fTexture;

[tomhudson, 2015/07/10 19:23:55]

It makes the name a bit long, but you might want to be explicit that this is a
ID or handle for a texture, and add a word to tell us the *semantics* - is this
texture the output? Ugh, fFrameBufferTextureID?

(Particularly since the object is never obviously used in this code - it's set
up, and then later on it's deleted.)

[wangyix, 2015/07/13 17:29:52]

Done.

+};
+
+///////////////////////////////////////////////////////////////////////////////////////////////////
+
+GrGLuint GLVec4ScalarBench::setupShader(const GrGLContext* ctx, bool useVec4ForCoverage) {

[tomhudson, 2015/07/10 19:23:55]

Instead of passing the bool here, pass the enum! The type name is good enough
your parameter name can be more concise, and the callsite simpler.

[wangyix, 2015/07/13 17:29:52]

Done.

+    const char* version = GrGLGetGLSLVersionDecl(*ctx);
+
+    // setup vertex shader

[tomhudson, 2015/07/10 19:23:54]

I'd like to see a "// This shader draws 4 overlapping circles of increasing
opacity and decreasing size"-type description somewhere in this function.

"// setup vertex shader" is a low-value comment.
Slightly better might be "// setup trivial vertex shader to pass through inputs"
here, and then the description I suggested above tacked onto the fragment
shader.

[wangyix, 2015/07/13 17:29:53]

Done.

+    GrGLShaderVar aPosition("a_position", kVec2f_GrSLType, GrShaderVar::kAttribute_TypeModifier);
+    GrGLShaderVar oPosition("o_position", kVec2f_GrSLType, GrShaderVar::kVaryingOut_TypeModifier);
+    GrGLShaderVar aColor("a_color", kVec3f_GrSLType, GrShaderVar::kAttribute_TypeModifier);
+    GrGLShaderVar oColor("o_color", kVec3f_GrSLType, GrShaderVar::kVaryingOut_TypeModifier);
+
+    SkString vshaderTxt(version);
+    aPosition.appendDecl(*ctx, &vshaderTxt);
+    vshaderTxt.append(";\n");
+    aColor.appendDecl(*ctx, &vshaderTxt);
+    vshaderTxt.append(";\n");
+    oPosition.appendDecl(*ctx, &vshaderTxt);
+    vshaderTxt.append(";\n");
+    oColor.appendDecl(*ctx, &vshaderTxt);
+    vshaderTxt.append(";\n");
+
+    vshaderTxt.append(
+            "void main()\n"
+            "{\n"
+            "    gl_Position = vec4(a_position, 0.f, 1.f);\n"
+            "    o_position = a_position;\n"
+            "    o_color = a_color;\n"
+            "}\n");
+
+    const GrGLInterface* gl = ctx->interface();
+
+    // setup fragment shader
+    GrGLShaderVar oFragColor("o_FragColor", kVec4f_GrSLType, GrShaderVar::kOut_TypeModifier);
+    SkString fshaderTxt(version);
+    GrGLAppendGLSLDefaultFloatPrecisionDeclaration(kDefault_GrSLPrecision, gl->fStandard,
+                                                   &fshaderTxt);
+    oPosition.setTypeModifier(GrShaderVar::kVaryingIn_TypeModifier);
+    oPosition.appendDecl(*ctx, &fshaderTxt);
+    fshaderTxt.append(";\n");
+    oColor.setTypeModifier(GrShaderVar::kVaryingIn_TypeModifier);
+    oColor.appendDecl(*ctx, &fshaderTxt);
+    fshaderTxt.append(";\n");
+
+    const char* fsOutName;
+    if (ctx->caps()->glslCaps()->mustDeclareFragmentShaderOutput()) {
+        oFragColor.appendDecl(*ctx, &fshaderTxt);
+        fshaderTxt.append(";\n");
+        fsOutName = oFragColor.c_str();
+    } else {
+        fsOutName = "gl_FragColor";
+    }
+
+    if (useVec4ForCoverage) {
+        fshaderTxt.appendf(
+                "void main()\n"
+                "{\n"
+                "    vec4 outputColor;\n"
+                "    vec4 outputCoverage;\n"
+                "    outputColor = vec4(%s, 1.0);\n"
+                "    outputCoverage = vec4(1.0);\n",
+                oColor.getName().c_str());
+    } else {
+        fshaderTxt.appendf(
+                "void main()\n"
+                "{\n"
+                "    vec4 outputColor;\n"
+                "    float outputCoverage;\n"
+                "    outputColor = vec4(%s, 1.0);\n"
+                "    outputCoverage = 1.0;\n",
+                oColor.getName().c_str());
+    }
+    float radius = 1.0f;
+    for (uint32_t i = 0; i < fNumStages; i++) {

[tomhudson, 2015/07/10 19:23:54]

Why pass in fConverageSetup (or a bool derived from it), but read this out of a
member variable?

It looks as though passing in both values would let you move this out of the
class into an anonymous namespace or static function?

[wangyix, 2015/07/13 17:29:52]

Done.

+        float centerX = 1.0f - radius;
+        float centerY = 1.0f - radius;
+        if (useVec4ForCoverage) {
+            fshaderTxt.appendf(
+                "    {\n"
+                "        float d = length(%s - vec2(%f, %f));\n"
+                "        float edgeAlpha = clamp(100.0 * (%f - d), 0.0, 1.0);\n"
+                "        outputCoverage = 0.5 * outputCoverage + 0.5 * vec4(edgeAlpha);\n"
+                "    }\n",
+                oPosition.getName().c_str(),
+                centerX, centerY,
+                radius
+                );
+        } else {
+            fshaderTxt.appendf(
+                "    {\n"
+                "        float d = length(%s - vec2(%f, %f));\n"
+                "        float edgeAlpha = clamp(100.0 * (%f - d), 0.0, 1.0);\n"
+                "        outputCoverage = 0.5 * outputCoverage + 0.5 * edgeAlpha;\n"
+                "    }\n",
+                oPosition.getName().c_str(),
+                centerX, centerY,
+                radius
+                );
+        }
+        radius *= 0.8f;
+    }
+    fshaderTxt.appendf(
+                "    {\n"
+                "        %s = outputColor * outputCoverage;\n"
+                "    }\n"
+                "}\n",
+                fsOutName);
+
+//printf("\n%s\n", fshaderTxt.c_str());

[tomhudson, 2015/07/10 19:23:55]

Remove commented-out code.

[wangyix, 2015/07/13 17:29:53]

Done.

+
+    return CreateProgram(gl, vshaderTxt.c_str(), fshaderTxt.c_str());
+}
+
+template<typename Func>
+static void setup_matrices(int numQuads, Func f) {
+    // We draw a really small triangle so we are not fill rate limited
+    for (int i = 0 ; i < numQuads; i++) {
+        SkMatrix m = SkMatrix::I();
+        m.setScale(0.01f, 0.01f);
+        f(m);
+    }
+}
+
+///////////////////////////////////////////////////////////////////////////////////////////////////
+
+const GrGLContext* GLVec4ScalarBench::onGetGLContext(const GrGLContext* ctx) {
+    // We only care about gpus with drawArraysInstanced support
+    if (!ctx->interface()->fFunctions.fDrawArraysInstanced) {
+        return NULL;
+    }
+    return ctx;
+}
+
+struct Vertex {
+    SkPoint fPositions;
+    GrGLfloat fColors[3];
+};
+
+void GLVec4ScalarBench::setupSingleVbo(const GrGLInterface* gl,
+                                                  const SkMatrix* viewMatrices) {

[tomhudson, 2015/07/10 19:23:54]

Nit: fix indentation

[wangyix, 2015/07/13 17:29:53]

Done.

+    // Constants for our various shader programs

[tomhudson, 2015/07/10 19:23:55]

This comment is not very high-value. Omit?

[wangyix, 2015/07/13 17:29:52]

Done.

+    Vertex vertices[kVerticesPerTri * kNumTriPerDraw];
+    for (uint32_t i = 0; i < kNumTriPerDraw; i++) {
+        Vertex* v = &vertices[i * kVerticesPerTri];
+        if (i % 2 == 0) {
+            v[0].fPositions.set(-1.0f, -1.0f);
+            v[1].fPositions.set( 1.0f, -1.0f);
+            v[2].fPositions.set( 1.0f,  1.0f);
+        } else {
+            v[0].fPositions.set(-1.0f, -1.0f);
+            v[1].fPositions.set( 1.0f, 1.0f);
+            v[2].fPositions.set( -1.0f, 1.0f);
+        }
+        SkPoint* position = reinterpret_cast<SkPoint*>(v);
+        viewMatrices[i].mapPointsWithStride(position, sizeof(Vertex), kVerticesPerTri);
+
+        // set colors

[tomhudson, 2015/07/10 19:23:55]

Bad evil comment. Expurgate!

[wangyix, 2015/07/13 17:29:53]

Done.

+        GrGLfloat color[3] = {1.0f, 0.0f, 1.0f};
+        for (uint32_t j = 0; j < kVerticesPerTri; j++) {
+            v->fColors[0] = color[0];
+            v->fColors[1] = color[1];
+            v->fColors[2] = color[2];
+            v++;
+        }
+    }
+
+    // setup VBO

[tomhudson, 2015/07/10 19:23:54]

Low-value comment. What is it? Or why are we doing these things?

[wangyix, 2015/07/13 17:29:53]

Done.

+    GR_GL_CALL(gl, GenBuffers(1, &fVBO));
+    GR_GL_CALL(gl, BindBuffer(GR_GL_ARRAY_BUFFER, fVBO));
+    GR_GL_CALL(gl, EnableVertexAttribArray(0));
+    GR_GL_CALL(gl, EnableVertexAttribArray(1));
+    GR_GL_CALL(gl, VertexAttribPointer(0, 2, GR_GL_FLOAT, GR_GL_FALSE, sizeof(Vertex),
+                                       (GrGLvoid*)0));
+    GR_GL_CALL(gl, VertexAttribPointer(1, 3, GR_GL_FLOAT, GR_GL_FALSE, sizeof(Vertex),
+                                       (GrGLvoid*)(sizeof(SkPoint))));
+    GR_GL_CALL(gl, BufferData(GR_GL_ARRAY_BUFFER, sizeof(vertices), vertices, GR_GL_STATIC_DRAW));
+}
+
+void GLVec4ScalarBench::setup(const GrGLContext* ctx) {
+    const GrGLInterface* gl = ctx->interface();
+    fTexture = SetupFramebuffer(gl, kScreenWidth, kScreenHeight);
+
+    fProgram = this->setupShader(ctx, fCoverageSetup == kUseVec4_CoverageSetup);
+
+    // setup matrices

[tomhudson, 2015/07/10 19:23:55]

Useless comment.
What matrices? Why? Or eliminate!

(Likewise the other fragmentary comments in this function.)

[wangyix, 2015/07/13 17:29:52]

Done.

+    int index = 0;
+    SkMatrix viewMatrices[kNumTriPerDraw];
+    setup_matrices(kNumTriPerDraw, [&index, &viewMatrices](const SkMatrix& m) {
+        viewMatrices[index++] = m;
+    });
+
+    // setup VAO
+    GR_GL_CALL(gl, GenVertexArrays(1, &fVAO));
+    GR_GL_CALL(gl, BindVertexArray(fVAO));
+
+    this->setupSingleVbo(gl, viewMatrices);
+
+    // set us up to draw
+    GR_GL_CALL(gl, UseProgram(fProgram));
+    GR_GL_CALL(gl, BindVertexArray(fVAO));
+}
+
+void GLVec4ScalarBench::glDraw(const int loops, const GrGLContext* ctx) {
+    const GrGLInterface* gl = ctx->interface();
+
+    for (int i = 0; i < loops; i++) {
+        GR_GL_CALL(gl, DrawArrays(GR_GL_TRIANGLES, 0, kVerticesPerTri * kNumTriPerDraw));
+    }
+
+
+#if 0
+    //const char* filename = "/data/local/tmp/out.png";

[tomhudson, 2015/07/10 19:23:55]

Delete commented-out code.
In theory you could probably leave the #if'd out code here, however, since it's
useful and clear - but don't VisualBench and nanobench have command line
arguments that say "write out the images you generate"?!

[tomhudson, 2015/07/13 16:00:13]

Again: How is your #0 different than the command-line args args to the bench
programs to force them to write out images? Isn't this redundant?

[wangyix, 2015/07/13 17:29:53]

Using -w when running nanobench does not write out the correct images for some
reason (I get blank white images), but changing the code here to #if 1 will
write out the correct images.  Josh was aware of this and  used this code for
debug purposes.

+    SkString filename("out");
+    filename.appendf("_%s.png", this->getName());
+    DumpImage(gl, kScreenWidth, kScreenHeight, filename.c_str());
+#endif
+}
+
+void GLVec4ScalarBench::teardown(const GrGLInterface* gl) {
+    GR_GL_CALL(gl, BindBuffer(GR_GL_ARRAY_BUFFER, 0));
+    GR_GL_CALL(gl, BindVertexArray(0));
+    GR_GL_CALL(gl, BindTexture(GR_GL_TEXTURE_2D, 0));
+    GR_GL_CALL(gl, BindFramebuffer(GR_GL_FRAMEBUFFER, 0));
+    GR_GL_CALL(gl, DeleteTextures(1, &fTexture));
+    GR_GL_CALL(gl, DeleteProgram(fProgram));
+    GR_GL_CALL(gl, DeleteBuffers(1, &fVBO));
+    GR_GL_CALL(gl, DeleteVertexArrays(1, &fVAO));
+}
+
+///////////////////////////////////////////////////////////////////////////////
+
+DEF_BENCH( return new GLVec4ScalarBench(GLVec4ScalarBench::kUseScalar_CoverageSetup, 1) )
+DEF_BENCH( return new GLVec4ScalarBench(GLVec4ScalarBench::kUseVec4_CoverageSetup, 1) )
+DEF_BENCH( return new GLVec4ScalarBench(GLVec4ScalarBench::kUseScalar_CoverageSetup, 2) )
+DEF_BENCH( return new GLVec4ScalarBench(GLVec4ScalarBench::kUseVec4_CoverageSetup, 2) )
+DEF_BENCH( return new GLVec4ScalarBench(GLVec4ScalarBench::kUseScalar_CoverageSetup, 4) )
+DEF_BENCH( return new GLVec4ScalarBench(GLVec4ScalarBench::kUseVec4_CoverageSetup, 4) )
+DEF_BENCH( return new GLVec4ScalarBench(GLVec4ScalarBench::kUseScalar_CoverageSetup, 6) )
+DEF_BENCH( return new GLVec4ScalarBench(GLVec4ScalarBench::kUseVec4_CoverageSetup, 6) )
+DEF_BENCH( return new GLVec4ScalarBench(GLVec4ScalarBench::kUseScalar_CoverageSetup, 8) )
+DEF_BENCH( return new GLVec4ScalarBench(GLVec4ScalarBench::kUseVec4_CoverageSetup, 8) )
+
+#endif