Bug 910754 - [Skia] Defer deletion of our shader objects until after linking the program r=upstream(bsalomon)
authorGeorge Wright <george@mozilla.com>
Thu, 13 Feb 2014 01:19:51 -0500
changeset 169762 ae20e11c6a681611f767c121d42d8dcae375bf46
parent 169761 db240f69f90094a9b6cab9393f604f3731757f3f
child 169763 a35f514ea705b2d03e66895ad792cd44e248477e
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewersupstream
bugs910754
milestone30.0a1
Bug 910754 - [Skia] Defer deletion of our shader objects until after linking the program r=upstream(bsalomon)
gfx/skia/trunk/src/gpu/gl/GrGLShaderBuilder.cpp
gfx/skia/trunk/src/gpu/gl/GrGLShaderBuilder.h
--- a/gfx/skia/trunk/src/gpu/gl/GrGLShaderBuilder.cpp
+++ b/gfx/skia/trunk/src/gpu/gl/GrGLShaderBuilder.cpp
@@ -567,27 +567,28 @@ const char* GrGLShaderBuilder::enableSec
         fFSOutputs.push_back().set(kVec4f_GrSLType,
                                    GrGLShaderVar::kOut_TypeModifier,
                                    dual_source_output_name());
         fHasSecondaryOutput = true;
     }
     return dual_source_output_name();
 }
 
-
 bool GrGLShaderBuilder::finish(GrGLuint* outProgramId) {
     SK_TRACE_EVENT0("GrGLShaderBuilder::finish");
 
     GrGLuint programId = 0;
     GL_CALL_RET(programId, CreateProgram());
     if (!programId) {
         return false;
     }
 
-    if (!this->compileAndAttachShaders(programId)) {
+    SkTDArray<GrGLuint> shadersToDelete;
+
+    if (!this->compileAndAttachShaders(programId, &shadersToDelete)) {
         GL_CALL(DeleteProgram(programId));
         return false;
     }
 
     this->bindProgramLocations(programId);
     if (fUniformManager.isUsingBindUniform()) {
       fUniformManager.getUniformLocations(programId, fUniforms);
     }
@@ -620,32 +621,37 @@ bool GrGLShaderBuilder::finish(GrGLuint*
             GL_CALL(DeleteProgram(programId));
             return false;
         }
     }
 
     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();
     GrGLint sourceLength = static_cast<GrGLint>(shaderSrc.size());
     GR_GL_CALL(gli, ShaderSource(shaderId, 1, &sourceStr, &sourceLength));
     GR_GL_CALL(gli, CompileShader(shaderId));
 
     // Calling GetShaderiv in Chromium is quite expensive. Assume success in release builds.
@@ -667,48 +673,56 @@ static bool attach_shader(const GrGLCont
                 GrGLsizei length = GR_GL_INIT_ZERO;
                 GR_GL_CALL(gli, GetShaderInfoLog(shaderId, infoLen+1,
                                                  &length, (char*)log.get()));
                 GrPrintf(shaderSrc.c_str());
                 GrPrintf("\n%s", log.get());
             }
             SkDEBUGFAIL("Shader compilation failed!");
             GR_GL_CALL(gli, DeleteShader(shaderId));
-            return false;
+            return 0;
         }
     }
     if (c_PrintShaders) {
         GrPrintf(shaderSrc.c_str());
         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,
                                        fGpu->glStandard(),
                                        &fragShaderSrc);
     this->appendUniformDecls(kFragment_Visibility, &fragShaderSrc);
     this->appendDecls(fFSInputs, &fragShaderSrc);
     // We shouldn't have declared outputs on 1.10
     SkASSERT(k110_GrGLSLGeneration != fGpu->glslGeneration() || fFSOutputs.empty());
     this->appendDecls(fFSOutputs, &fragShaderSrc);
     fragShaderSrc.append(fFSFunctions);
     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;
 }
 
 void GrGLShaderBuilder::bindProgramLocations(GrGLuint programId) const {
     if (fHasCustomColorOutput) {
         GL_CALL(BindFragDataLocation(programId, 0, declared_color_output_name()));
     }
     if (fHasSecondaryOutput) {
@@ -865,28 +879,30 @@ GrGLProgramEffects* GrGLFullShaderBuilde
     this->INHERITED::createAndEmitEffects(&programEffectsBuilder,
                                           effectStages,
                                           effectKeys,
                                           effectCnt,
                                           inOutFSColor);
     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);
     this->appendDecls(fVSAttrs, &vertShaderSrc);
     this->appendDecls(fVSOutputs, &vertShaderSrc);
     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) {
         SkASSERT(this->ctxInfo().glslGeneration() >= k150_GrGLSLGeneration);
         SkString geomShaderSrc(GrGetGLSLVersionDecl(this->ctxInfo()));
         geomShaderSrc.append("layout(triangles) in;\n"
                              "layout(triangle_strip, max_vertices = 6) out;\n");
         this->appendDecls(fGSInputs, &geomShaderSrc);
@@ -902,23 +918,25 @@ bool GrGLFullShaderBuilder::compileAndAt
             geomShaderSrc.appendf("\t\t%s = %s[i];\n",
                                   fGSOutputs[i].getName().c_str(),
                                   fGSInputs[i].getName().c_str());
         }
         geomShaderSrc.append("\t\tEmitVertex();\n"
                              "\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 {
     this->INHERITED::bindProgramLocations(programId);
 
     const GrGLProgramDesc::KeyHeader& header = fDesc.getHeader();
 
     // Bind the attrib locations to same values for all shaders
--- a/gfx/skia/trunk/src/gpu/gl/GrGLShaderBuilder.h
+++ b/gfx/skia/trunk/src/gpu/gl/GrGLShaderBuilder.h
@@ -243,17 +243,17 @@ protected:
 
     // Helper for emitEffects().
     void createAndEmitEffects(GrGLProgramEffectsBuilder*,
                               const GrEffectStage* effectStages[],
                               const EffectKey effectKeys[],
                               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;
     void appendUniformDecls(ShaderVisibility, SkString*) const;
 
 private:
     class CodeStage : public SkNoncopyable {
     public:
@@ -417,17 +417,17 @@ public:
                 int effectCnt,
                 GrGLSLExpr4* inOutFSColor) SK_OVERRIDE;
 
     GrGLUniformManager::UniformHandle getViewMatrixUniform() const {
         return fViewMatrixUniform;
     }
 
 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:
     const GrGLProgramDesc&              fDesc;
     VarArray                            fVSAttrs;
     VarArray                            fVSOutputs;
     VarArray                            fGSInputs;
     VarArray                            fGSOutputs;