Defer deletion of our shaders until after linking the gl program to work around an...

R=bsalomon@google.com, brian@thesalomons.net, bsalomon
BUG=skia:

Author: george@mozilla.com

Review URL: https://codereview.chromium.org/164973002

git-svn-id: http://skia.googlecode.com/svn/trunk@13446 2bbb7eff-a529-9590-31e7-b0007b416f81

diff --git a/AUTHORS b/AUTHORS
index 27c0a4c..a51e7c6 100644 (file)
--- a/AUTHORS
+++ b/AUTHORS
@@ -13,6 +13,7 @@
 
 ACCESS CO., LTD. <*@access-company.com>
 ARM <*@arm.com>
+George Wright <george@mozilla.com>
 Google Inc. <*@google.com>
 Intel <*@intel.com>
 NVIDIA <*@nvidia.com>

diff --git a/src/gpu/gl/GrGLShaderBuilder.cpp b/src/gpu/gl/GrGLShaderBuilder.cpp
index 89f4cbf..264bc3b 100644 (file)
--- a/src/gpu/gl/GrGLShaderBuilder.cpp
+++ b/src/gpu/gl/GrGLShaderBuilder.cpp
@@ -572,7 +572,6 @@ const char* GrGLShaderBuilder::enableSecondaryOutput() {
     return dual_source_output_name();
 }
 
-
 bool GrGLShaderBuilder::finish(GrGLuint* outProgramId) {
     SK_TRACE_EVENT0("GrGLShaderBuilder::finish");
 
@@ -582,7 +581,9 @@ bool GrGLShaderBuilder::finish(GrGLuint* outProgramId) {
         return false;
     }
 
-    if (!this->compileAndAttachShaders(programId)) {
+    SkTDArray<GrGLuint> shadersToDelete;
+
+    if (!this->compileAndAttachShaders(programId, &shadersToDelete)) {
         GL_CALL(DeleteProgram(programId));
         return false;
     }
@@ -625,22 +626,27 @@ bool GrGLShaderBuilder::finish(GrGLuint* outProgramId) {
     if (!fUniformManager.isUsingBindUniform()) {
       fUniformManager.getUniformLocations(programId, fUniforms);
     }
+
+    for (int i = 0; i < shadersToDelete.count(); ++i) {
+      GL_CALL(DeleteShader(shadersToDelete[i]));
+    }
+
     *outProgramId = programId;
     return true;
 }
 
-// Compiles a GL shader, attaches it to a program, and releases the shader's reference.
-// (That way there's no need to hang on to the GL shader id and delete it later.)
-static bool attach_shader(const GrGLContext& glCtx,
-                          GrGLuint programId,
-                          GrGLenum type,
-                          const SkString& shaderSrc) {
+// Compiles a GL shader and attaches it to a program. Returns the shader ID if
+// successful, or 0 if not.
+static GrGLuint attach_shader(const GrGLContext& glCtx,
+                              GrGLuint programId,
+                              GrGLenum type,
+                              const SkString& shaderSrc) {
     const GrGLInterface* gli = glCtx.interface();
 
     GrGLuint shaderId;
     GR_GL_CALL_RET(gli, shaderId, CreateShader(type));
     if (0 == shaderId) {
-        return false;
+        return 0;
     }
 
     const GrGLchar* sourceStr = shaderSrc.c_str();
@@ -672,7 +678,7 @@ static bool attach_shader(const GrGLContext& glCtx,
             }
             SkDEBUGFAIL("Shader compilation failed!");
             GR_GL_CALL(gli, DeleteShader(shaderId));
-            return false;
+            return 0;
         }
     }
     if (c_PrintShaders) {
@@ -680,12 +686,16 @@ static bool attach_shader(const GrGLContext& glCtx,
         GrPrintf("\n");
     }
 
+    // Attach the shader, but defer deletion until after we have linked the program.
+    // This works around a bug in the Android emulator's GLES2 wrapper which
+    // will immediately delete the shader object and free its memory even though it's
+    // attached to a program, which then causes glLinkProgram to fail.
     GR_GL_CALL(gli, AttachShader(programId, shaderId));
-    GR_GL_CALL(gli, DeleteShader(shaderId));
-    return true;
+
+    return shaderId;
 }
 
-bool GrGLShaderBuilder::compileAndAttachShaders(GrGLuint programId) const {
+bool GrGLShaderBuilder::compileAndAttachShaders(GrGLuint programId, SkTDArray<GrGLuint>* shaderIds) const {
     SkString fragShaderSrc(GrGetGLSLVersionDecl(this->ctxInfo()));
     fragShaderSrc.append(fFSExtensions);
     append_default_precision_qualifier(kDefaultFragmentPrecision,
@@ -700,10 +710,14 @@ bool GrGLShaderBuilder::compileAndAttachShaders(GrGLuint programId) const {
     fragShaderSrc.append("void main() {\n");
     fragShaderSrc.append(fFSCode);
     fragShaderSrc.append("}\n");
-    if (!attach_shader(fGpu->glContext(), programId, GR_GL_FRAGMENT_SHADER, fragShaderSrc)) {
+
+    GrGLuint fragShaderId = attach_shader(fGpu->glContext(), programId, GR_GL_FRAGMENT_SHADER, fragShaderSrc);
+    if (!fragShaderId) {
         return false;
     }
 
+    *shaderIds->append() = fragShaderId;
+
     return true;
 }
 
@@ -870,7 +884,7 @@ GrGLProgramEffects* GrGLFullShaderBuilder::createAndEmitEffects(
     return programEffectsBuilder.finish();
 }
 
-bool GrGLFullShaderBuilder::compileAndAttachShaders(GrGLuint programId) const {
+bool GrGLFullShaderBuilder::compileAndAttachShaders(GrGLuint programId, SkTDArray<GrGLuint>* shaderIds) const {
     const GrGLContext& glCtx = this->gpu()->glContext();
     SkString vertShaderSrc(GrGetGLSLVersionDecl(this->ctxInfo()));
     this->appendUniformDecls(kVertex_Visibility, &vertShaderSrc);
@@ -879,9 +893,11 @@ bool GrGLFullShaderBuilder::compileAndAttachShaders(GrGLuint programId) const {
     vertShaderSrc.append("void main() {\n");
     vertShaderSrc.append(fVSCode);
     vertShaderSrc.append("}\n");
-    if (!attach_shader(glCtx, programId, GR_GL_VERTEX_SHADER, vertShaderSrc)) {
+    GrGLuint vertShaderId = attach_shader(glCtx, programId, GR_GL_VERTEX_SHADER, vertShaderSrc);
+    if (!vertShaderId) {
         return false;
     }
+    *shaderIds->append() = vertShaderId;
 
 #if GR_GL_EXPERIMENTAL_GS
     if (fDesc.getHeader().fExperimentalGS) {
@@ -907,13 +923,15 @@ bool GrGLFullShaderBuilder::compileAndAttachShaders(GrGLuint programId) const {
                              "\t}\n"
                              "\tEndPrimitive();\n");
         geomShaderSrc.append("}\n");
-        if (!attach_shader(glCtx, programId, GR_GL_GEOMETRY_SHADER, geomShaderSrc)) {
+        GrGLuint geomShaderId = attach_shader(glCtx, programId, GR_GL_GEOMETRY_SHADER, geomShaderSrc);
+        if (!geomShaderId) {
             return false;
         }
+        *shaderIds->append() = geomShaderId;
     }
 #endif
 
-    return this->INHERITED::compileAndAttachShaders(programId);
+    return this->INHERITED::compileAndAttachShaders(programId, shaderIds);
 }
 
 void GrGLFullShaderBuilder::bindProgramLocations(GrGLuint programId) const {

diff --git a/src/gpu/gl/GrGLShaderBuilder.h b/src/gpu/gl/GrGLShaderBuilder.h
index 103efa5..fb75d3a 100644 (file)
--- a/src/gpu/gl/GrGLShaderBuilder.h
+++ b/src/gpu/gl/GrGLShaderBuilder.h
@@ -248,7 +248,7 @@ protected:
                               int effectCnt,
                               GrGLSLExpr4* inOutFSColor);
 
-    virtual bool compileAndAttachShaders(GrGLuint programId) const;
+    virtual bool compileAndAttachShaders(GrGLuint programId, SkTDArray<GrGLuint>* shaderIds) const;
     virtual void bindProgramLocations(GrGLuint programId) const;
 
     void appendDecls(const VarArray&, SkString*) const;
@@ -422,7 +422,7 @@ public:
     }
 
 protected:
-    virtual bool compileAndAttachShaders(GrGLuint programId) const SK_OVERRIDE;
+    virtual bool compileAndAttachShaders(GrGLuint programId, SkTDArray<GrGLuint>* shaderIds) const SK_OVERRIDE;
     virtual void bindProgramLocations(GrGLuint programId) const SK_OVERRIDE;
 
 private: