Make GrGLShaderBuilder responsible for enabling GLSL extensions

Make GrGLShaderBuilder responsible for enabling GLSL extensions
Committed: https://code.google.com/p/skia/source/detail?r=8459

[robertphillips, 2013-03-26 17:26:05]

lgtm + 1 question

https://codereview.chromium.org/12668019/diff/4002/src/gpu/gl/GrGLShaderBuild...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/12668019/diff/4002/src/gpu/gl/GrGLShaderBuild...
src/gpu/gl/GrGLShaderBuilder.cpp:148: 
"require" vs. "enable"

[jvanverth1, 2013-04-01 13:40:41]

Do you want to commit this before I commit issue 13344002?

On 2013/03/26 17:26:05, robertphillips wrote:
> lgtm + 1 question
> 
>
https://codereview.chromium.org/12668019/diff/4002/src/gpu/gl/GrGLShaderBuild...
> File src/gpu/gl/GrGLShaderBuilder.cpp (right):
> 
>
https://codereview.chromium.org/12668019/diff/4002/src/gpu/gl/GrGLShaderBuild...
> src/gpu/gl/GrGLShaderBuilder.cpp:148: 
> "require" vs. "enable"

[commit-bot: I haz the power, 2013-04-01 17:11:23]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/bsalomon@google.com/12668019/4002

[commit-bot: I haz the power, 2013-04-01 17:11:25]

Failed to apply patch for src/gpu/gl/GrGLShaderBuilder.cpp:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file src/gpu/gl/GrGLShaderBuilder.cpp
  Hunk #1 FAILED at 96.
  Hunk #2 succeeded at 165 (offset 54 lines).
  Hunk #3 succeeded at 430 (offset 54 lines).
  Hunk #4 succeeded at 547 (offset 54 lines).
  Hunk #5 succeeded at 561 (offset 54 lines).
  Hunk #6 succeeded at 573 (offset 54 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
src/gpu/gl/GrGLShaderBuilder.cpp.rej

Patch:       src/gpu/gl/GrGLShaderBuilder.cpp
Index: src/gpu/gl/GrGLShaderBuilder.cpp
===================================================================
--- src/gpu/gl/GrGLShaderBuilder.cpp	(revision 8392)
+++ src/gpu/gl/GrGLShaderBuilder.cpp	(working copy)
@@ -96,6 +96,7 @@
     , fCtxInfo(ctxInfo)
     , fUniformManager(uniformManager)
     , fCurrentStageIdx(kNonStageIdx)
+    , fFSFeaturesAddedMask(0)
     , fSetupFragPosition(false)
     , fRTHeightUniform(GrGLUniformManager::kInvalidUniformHandle) {
 
@@ -111,6 +112,48 @@
     }
 }
 
+bool GrGLShaderBuilder::enableFeature(GLSLFeature feature) {
+    switch (feature) {
+        case kStandardDerivatives_GLSLFeature:
+            if (!fCtxInfo.caps()->shaderDerivativeSupport()) {
+                return false;
+            }
+            if (kES2_GrGLBinding == fCtxInfo.binding()) {
+                this->addFSFeature(1 << kStandardDerivatives_GLSLFeature,
+                                   "GL_OES_standard_derivatives");
+            }
+            return true;
+        default:
+            GrCrash("Unexpected GLSLFeature requested.");
+            return false;
+    }
+}
+
+bool GrGLShaderBuilder::enablePrivateFeature(GLSLPrivateFeature feature) {
+    switch (feature) {
+        case kFragCoordConventions_GLSLPrivateFeature:
+            if (!fCtxInfo.caps()->fragCoordConventionsSupport()) {
+                return false;
+            }
+            if (fCtxInfo.glslGeneration() < k150_GrGLSLGeneration) {
+                this->addFSFeature(1 <<
kFragCoordConventions_GLSLPrivateFeature,
+                                   "GL_ARB_fragment_coord_conventions");
+            }
+            return true;
+        default:
+            GrCrash("Unexpected GLSLPrivateFeature requested.");
+            return false;
+    }
+}
+
+void GrGLShaderBuilder::addFSFeature(uint32_t featureBit, const char*
extensionName) {
+    if (!(featureBit & fFSFeaturesAddedMask)) {
+        fFSHeader.appendf("#extension %s: require\n", extensionName);
+        fFSFeaturesAddedMask |= featureBit;
+    }
+}
+
+
 void GrGLShaderBuilder::codeAppendf(ShaderType type, const char format[],
va_list args) {
     SkString* string = NULL;
     switch (type) {
@@ -334,9 +377,7 @@
 #if 1
     if (fCtxInfo.caps()->fragCoordConventionsSupport()) {
         if (!fSetupFragPosition) {
-            if (fCtxInfo.glslGeneration() < k150_GrGLSLGeneration) {
-                fFSHeader.append("#extension GL_ARB_fragment_coord_conventions:
require\n");
-            }
+           
SkAssertResult(this->enablePrivateFeature(kFragCoordConventions_GLSLPrivateFeature));
             fFSInputs.push_back().set(kVec4f_GrSLType,
                                       GrGLShaderVar::kIn_TypeModifier,
                                       "gl_FragCoord",
@@ -453,9 +494,11 @@
 }
 
 void GrGLShaderBuilder::getShader(ShaderType type, SkString* shaderStr) const {
+    const char* version = GrGetGLSLVersionDecl(fCtxInfo.binding(),
fCtxInfo.glslGeneration());
+
     switch (type) {
         case kVertex_ShaderType:
-            *shaderStr = fHeader;
+            *shaderStr = version;
             this->appendUniformDecls(kVertex_ShaderType, shaderStr);
             this->appendDecls(fVSAttrs, shaderStr);
             this->appendDecls(fVSOutputs, shaderStr);
@@ -465,7 +508,7 @@
             break;
         case kGeometry_ShaderType:
             if (fUsesGS) {
-                *shaderStr = fHeader;
+                *shaderStr = version;
                 shaderStr->append(fGSHeader);
                 this->appendDecls(fGSInputs, shaderStr);
                 this->appendDecls(fGSOutputs, shaderStr);
@@ -477,7 +520,7 @@
             }
             break;
         case kFragment_ShaderType:
-            *shaderStr = fHeader;
+            *shaderStr = version;
             append_default_precision_qualifier(kDefaultFragmentPrecision,
                                                fCtxInfo.binding(),
                                                shaderStr);

[bsalomon, 2013-04-01 17:45:37]

Make GrGLShaderBuilder responsible for enabling GLSL extensions

[bsalomon, 2013-04-01 17:48:38]

Committed patchset #4 manually as r8459 (presubmit successful).

[bsalomon, 2013-04-01 17:49:31]

https://codereview.chromium.org/12668019/diff/4002/src/gpu/gl/GrGLShaderBuild...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/12668019/diff/4002/src/gpu/gl/GrGLShaderBuild...
src/gpu/gl/GrGLShaderBuilder.cpp:148: 
On 2013/03/26 17:26:05, robertphillips wrote:
> "require" vs. "enable"

require is more correct. We need the extension to be present for the shader to
compile. The effect gets back a false if the extension isn't support and no
#extenion is added. It can then choose to emit alternative code.

[robertphillips, 2013-04-01 22:23:24]

This was reverted in r8474 due to a shader compiler error on the Nexus 10
(http://108.170.217.252:10117/builders/Skia_Nexus10_4-1_Float_Debug_32/builds/...).