From d328fb62938b1d7e43b07c619756dfdc781453b3 Mon Sep 17 00:00:00 2001 From: "commit-bot@chromium.org" Date: Fri, 14 Feb 2014 00:03:38 +0000 Subject: Defer deletion of our shaders until after linking the gl program to work around an Android emulator bug 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 --- AUTHORS | 1 + src/gpu/gl/GrGLShaderBuilder.cpp | 54 ++++++++++++++++++++++++++-------------- src/gpu/gl/GrGLShaderBuilder.h | 4 +-- 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/AUTHORS b/AUTHORS index 27c0a4cca9..a51e7c6abb 100644 --- a/AUTHORS +++ b/AUTHORS @@ -13,6 +13,7 @@ ACCESS CO., LTD. <*@access-company.com> ARM <*@arm.com> +George Wright 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 89f4cbf4a6..264bc3b005 100644 --- 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 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* 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* 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 103efa5aab..fb75d3a7db 100644 --- 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* 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* shaderIds) const SK_OVERRIDE; virtual void bindProgramLocations(GrGLuint programId) const SK_OVERRIDE; private: -- cgit v1.2.3