Compile shader upon deletion if attached.

[email protected], [email protected] BUG=459778 TEST=chromote locally with enableVideoDecodeRenderer Review URL: https://codereview.chromium.org/954073002 Cr-Commit-Position: refs/heads/master@{#317985} NOTRY=true Review URL: https://codereview.chromium.org/957403004 Cr-Commit-Position: refs/branch-heads/2311@{#43} Cr-Branched-From: 09b7de5-refs/heads/master@{#317474}
crosswalk-project · Feb 26, 2015 · 3415626 · 3415626
1 parent 95335ac
commit 3415626
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 16 deletions.
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -5133,8 +5133,7 @@ error::Error GLES2DecoderImpl::HandleDeleteShader(uint32 immediate_data_size,
     Shader* shader = GetShader(client_id);
     if (shader) {
       if (!shader->IsDeleted()) {
-        glDeleteShader(shader->service_id());
-        shader_manager()->MarkAsDeleted(shader);
+        shader_manager()->Delete(shader);
       }
     } else {
       LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, "glDeleteShader", "unknown shader");

diff --git a/gpu/command_buffer/service/shader_manager.cc b/gpu/command_buffer/service/shader_manager.cc
@@ -133,8 +133,13 @@ void Shader::DecUseCount() {
   DCHECK_GE(use_count_, 0);
 }
 
-void Shader::MarkAsDeleted() {
+void Shader::Delete() {
+  if (use_count_ > 0) {
+    // If attached, compile the shader before we delete it.
+    DoCompile();
+  }
   DCHECK_NE(service_id_, 0u);
+  glDeleteShader(service_id_);
   service_id_ = 0;
 }
 
@@ -185,8 +190,7 @@ void ShaderManager::Destroy(bool have_context) {
     if (have_context) {
       Shader* shader = shaders_.begin()->second.get();
       if (!shader->IsDeleted()) {
-        glDeleteShader(shader->service_id());
-        shader->MarkAsDeleted();
+        shader->Delete();
       }
     }
     shaders_.erase(shaders_.begin());
@@ -247,10 +251,10 @@ void ShaderManager::RemoveShader(Shader* shader) {
   }
 }
 
-void ShaderManager::MarkAsDeleted(Shader* shader) {
+void ShaderManager::Delete(Shader* shader) {
   DCHECK(shader);
   DCHECK(IsOwned(shader));
-  shader->MarkAsDeleted();
+  shader->Delete();
   RemoveShader(shader);
 }
 
@@ -269,5 +273,3 @@ void ShaderManager::UnuseShader(Shader* shader) {
 
 }  // namespace gles2
 }  // namespace gpu
-
-
diff --git a/gpu/command_buffer/service/shader_manager.h b/gpu/command_buffer/service/shader_manager.h
@@ -147,7 +147,7 @@ class GPU_EXPORT Shader : public base::RefCounted<Shader> {
 
   void IncUseCount();
   void DecUseCount();
-  void MarkAsDeleted();
+  void Delete();
 
   int use_count_;
 
@@ -215,7 +215,7 @@ class GPU_EXPORT ShaderManager {
   // Gets a client id for a given service id.
   bool GetClientId(GLuint service_id, GLuint* client_id) const;
 
-  void MarkAsDeleted(Shader* shader);
+  void Delete(Shader* shader);
 
   // Mark a shader as used
   void UseShader(Shader* shader);

diff --git a/gpu/command_buffer/service/shader_manager_unittest.cc b/gpu/command_buffer/service/shader_manager_unittest.cc
@@ -43,7 +43,10 @@ TEST_F(ShaderManagerTest, Basic) {
   // Check we get nothing for a non-existent shader.
   EXPECT_TRUE(manager_.GetShader(kClient2Id) == NULL);
   // Check we can't get the shader after we remove it.
-  manager_.MarkAsDeleted(shader1);
+  EXPECT_CALL(*gl_, DeleteShader(kService1Id))
+      .Times(1)
+      .RetiresOnSaturation();
+  manager_.Delete(shader1);
   EXPECT_TRUE(manager_.GetShader(kClient1Id) == NULL);
 }
 
@@ -79,8 +82,14 @@ TEST_F(ShaderManagerTest, DeleteBug) {
   ASSERT_TRUE(shader1.get());
   ASSERT_TRUE(shader2.get());
   manager_.UseShader(shader1.get());
-  manager_.MarkAsDeleted(shader1.get());
-  manager_.MarkAsDeleted(shader2.get());
+  EXPECT_CALL(*gl_, DeleteShader(kService1Id))
+      .Times(1)
+      .RetiresOnSaturation();
+  manager_.Delete(shader1.get());
+  EXPECT_CALL(*gl_, DeleteShader(kService2Id))
+      .Times(1)
+      .RetiresOnSaturation();
+  manager_.Delete(shader2.get());
   EXPECT_TRUE(manager_.IsOwned(shader1.get()));
   EXPECT_FALSE(manager_.IsOwned(shader2.get()));
 }
@@ -249,7 +258,10 @@ TEST_F(ShaderManagerTest, ShaderInfoUseCount) {
   EXPECT_TRUE(shader1->InUse());
   manager_.UseShader(shader1);
   EXPECT_TRUE(shader1->InUse());
-  manager_.MarkAsDeleted(shader1);
+  EXPECT_CALL(*gl_, DeleteShader(kService1Id))
+      .Times(1)
+      .RetiresOnSaturation();
+  manager_.Delete(shader1);
   EXPECT_TRUE(shader1->IsDeleted());
   Shader* shader2 = manager_.GetShader(kClient1Id);
   EXPECT_EQ(shader1, shader2);
@@ -272,7 +284,10 @@ TEST_F(ShaderManagerTest, ShaderInfoUseCount) {
   EXPECT_FALSE(shader1->InUse());
   shader2 = manager_.GetShader(kClient1Id);
   EXPECT_EQ(shader1, shader2);
-  manager_.MarkAsDeleted(shader1);  // this should delete the shader.
+  EXPECT_CALL(*gl_, DeleteShader(kService1Id))
+      .Times(1)
+      .RetiresOnSaturation();
+  manager_.Delete(shader1);  // this should delete the shader.
   shader2 = manager_.GetShader(kClient1Id);
   EXPECT_TRUE(shader2 == NULL);
 }

diff --git a/gpu/command_buffer/tests/gl_program_unittest.cc b/gpu/command_buffer/tests/gl_program_unittest.cc
@@ -251,5 +251,38 @@ TEST_F(GLProgramTest, DeferCompileWithExt) {
   EXPECT_NE(0u, program_good2);
 }
 
+TEST_F(GLProgramTest, DeleteAttachedShaderLinks) {
+  static const char* v_shdr_str = R"(
+      attribute vec4 vPosition;
+      void main()
+      {
+        gl_Position = vPosition;
+      }
+  )";
+  static const char* f_shdr_str = R"(
+      void main()
+      {
+        gl_FragColor = vec4(1, 1, 1, 1);
+      }
+  )";
+
+  // Compiling the shaders, attaching, then deleting before linking should work.
+  GLuint vs = GLTestHelper::CompileShader(GL_VERTEX_SHADER, v_shdr_str);
+  GLuint fs = GLTestHelper::CompileShader(GL_FRAGMENT_SHADER, f_shdr_str);
+
+  GLuint program = glCreateProgram();
+  glAttachShader(program, vs);
+  glAttachShader(program, fs);
+
+  glDeleteShader(vs);
+  glDeleteShader(fs);
+
+  glLinkProgram(program);
+
+  GLint linked = 0;
+  glGetProgramiv(program, GL_LINK_STATUS, &linked);
+  EXPECT_NE(0, linked);
+}
+
 }  // namespace gpu