Fix unbind and teardown behavior of media::CommandBufferHelper.

At stub destruction, CommandBufferHelper forgets all of its textures,
which leads to crashes on Mac. The fix is to stop doing that, now that
AbstractTextures remove the need to do so.

Additionally, the behavior of calling BindImage(nullptr) was changed in
the conversion to AbstractTexture; this change restores the original
behavior.

Bug: 869186
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I27fb81408ac90d28752a6bb6cfbb5e3c56975c70
Reviewed-on: https://chromium-review.googlesource.com/1155812Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580384}

gpu/command_buffer/service/abstract_texture.h

@@ -77,12 +77,6 @@ class GPU_GLES2_EXPORT AbstractTexture {
   // The context must be current.
   virtual void BindImage(gl::GLImage* image, bool client_managed) = 0;
 
-  // Unbind and release any image bound to this texture, and return it to an
-  // uncleared state.
-  //
-  // The context must be current.
-  virtual void ReleaseImage() = 0;
-
   // Return the image, if any.
   virtual gl::GLImage* GetImage() const = 0;
 

gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc

@@ -325,7 +325,7 @@ TEST_P(GLES2DecoderTest, CreateAbstractTexture) {
   EXPECT_EQ(texture->GetLevelImage(target, 0), image.get());
 
   // Unbinding should make it not renderable.
-  abstract_texture->ReleaseImage();
+  abstract_texture->BindImage(nullptr, false);
   EXPECT_EQ(texture->SafeToRenderFrom(), false);
   EXPECT_EQ(abstract_texture->GetImage(), nullptr);
 

gpu/command_buffer/service/passthrough_abstract_texture_impl.cc

@@ -51,8 +51,23 @@ void PassthroughAbstractTextureImpl::BindImage(gl::GLImage* image,
   if (!texture_passthrough_)
     return;
 
-  texture_passthrough_->set_is_bind_pending(!client_managed);
-  texture_passthrough_->SetLevelImage(texture_passthrough_->target(), 0, image);
+  const GLuint target = texture_passthrough_->target();
+  const GLuint level = 0;
+
+  // If there is a decoder-managed image bound, release it.
+  if (decoder_managed_image_) {
+    gl::GLImage* current_image =
+        texture_passthrough_->GetLevelImage(target, level);
+    // TODO(sandersd): This isn't correct if CopyTexImage() was used.
+    bool is_bound = !texture_passthrough_->is_bind_pending();
+    if (current_image && is_bound)
+      current_image->ReleaseTexImage(target);
+  }
+
+  // Configure the new image.
+  decoder_managed_image_ = image && !client_managed;
+  texture_passthrough_->set_is_bind_pending(decoder_managed_image_);
+  texture_passthrough_->SetLevelImage(target, level, image);
 }
 
 void PassthroughAbstractTextureImpl::BindStreamTextureImage(
@@ -61,19 +76,6 @@ void PassthroughAbstractTextureImpl::BindStreamTextureImage(
   NOTREACHED();
 }
 
-void PassthroughAbstractTextureImpl::ReleaseImage() {
-  if (!texture_passthrough_)
-    return;
-
-  texture_passthrough_->set_is_bind_pending(false);
-  const GLuint target = texture_passthrough_->target();
-  const GLuint level = 0;
-  if (gl::GLImage* image = GetImage()) {
-    image->ReleaseTexImage(target);
-    texture_passthrough_->SetLevelImage(target, level, nullptr);
-  }
-}
-
 gl::GLImage* PassthroughAbstractTextureImpl::GetImage() const {
   if (!texture_passthrough_)
     return nullptr;

gpu/command_buffer/service/passthrough_abstract_texture_impl.h

@@ -33,7 +33,6 @@ class GPU_GLES2_EXPORT PassthroughAbstractTextureImpl : public AbstractTexture {
   void BindImage(gl::GLImage* image, bool client_managed) override;
   void BindStreamTextureImage(GLStreamTextureImage* image,
                               GLuint service_id) override;
-  void ReleaseImage() override;
   gl::GLImage* GetImage() const override;
   void SetCleared() override;
   void SetCleanupCallback(CleanupCallback cb) override;
@@ -43,6 +42,7 @@ class GPU_GLES2_EXPORT PassthroughAbstractTextureImpl : public AbstractTexture {
 
  private:
   scoped_refptr<TexturePassthrough> texture_passthrough_;
+  bool decoder_managed_image_ = false;
   gl::GLApi* gl_api_;
   GLES2DecoderPassthroughImpl* decoder_;
   CleanupCallback cleanup_cb_;

gpu/command_buffer/service/validating_abstract_texture_impl.cc

@@ -52,26 +52,37 @@ void ValidatingAbstractTextureImpl::SetParameteri(GLenum pname, GLint param) {
 
 void ValidatingAbstractTextureImpl::BindImage(gl::GLImage* image,
                                               bool client_managed) {
-  DCHECK(image);
-
   if (!texture_ref_)
     return;
 
+  const GLuint target = texture_ref_->texture()->target();
   const GLint level = 0;
 
-  Texture::ImageState state = client_managed ? Texture::ImageState::BOUND
-                                             : Texture::ImageState::UNBOUND;
+  // If there is a decoder-managed image bound, release it.
+  if (decoder_managed_image_) {
+    Texture::ImageState image_state;
+    gl::GLImage* current_image =
+        texture_ref_->texture()->GetLevelImage(target, 0, &image_state);
+    if (current_image && image_state == Texture::BOUND)
+      current_image->ReleaseTexImage(target);
+  }
 
-  GetTextureManager()->SetLevelImage(texture_ref_.get(),
-                                     texture_ref_->texture()->target(), level,
-                                     image, state);
-  SetCleared();
+  // Configure the new image.
+  decoder_managed_image_ = image && !client_managed;
+  Texture::ImageState state = image && client_managed
+                                  ? Texture::ImageState::BOUND
+                                  : Texture::ImageState::UNBOUND;
+  GetTextureManager()->SetLevelImage(texture_ref_.get(), target, level, image,
+                                     state);
+  GetTextureManager()->SetLevelCleared(texture_ref_.get(), target, level,
+                                       image);
 }
 
 void ValidatingAbstractTextureImpl::BindStreamTextureImage(
     GLStreamTextureImage* image,
     GLuint service_id) {
   DCHECK(image);
+  DCHECK(!decoder_managed_image_);
 
   if (!texture_ref_)
     return;
@@ -86,31 +97,6 @@ void ValidatingAbstractTextureImpl::BindStreamTextureImage(
   SetCleared();
 }
 
-void ValidatingAbstractTextureImpl::ReleaseImage() {
-  if (!texture_ref_)
-    return;
-
-  GLuint target = texture_ref_->texture()->target();
-  Texture::ImageState image_state;
-  gl::GLImage* image =
-      texture_ref_->texture()->GetLevelImage(target, 0, &image_state);
-  if (!image)
-    return;
-
-  // TODO(liberato): Suppress errors here?
-  if (image_state == Texture::BOUND)
-    image->ReleaseTexImage(target);
-
-  GetTextureManager()->SetLevelImage(texture_ref_.get(), target, 0, nullptr,
-                                     Texture::UNBOUND);
-
-  // Mark the texture as uncleared, in case it's only cleared because of the
-  // image binding.
-  const GLint level = 0;
-  GetTextureManager()->SetLevelCleared(texture_ref_.get(), target, level,
-                                       false);
-}
-
 gl::GLImage* ValidatingAbstractTextureImpl::GetImage() const {
   if (!texture_ref_)
     return nullptr;

gpu/command_buffer/service/validating_abstract_texture_impl.h

@@ -38,7 +38,6 @@ class GPU_GLES2_EXPORT ValidatingAbstractTextureImpl : public AbstractTexture {
   void BindStreamTextureImage(GLStreamTextureImage* image,
                               GLuint service_id) override;
   void BindImage(gl::GLImage* image, bool client_managed) override;
-  void ReleaseImage() override;
   gl::GLImage* GetImage() const override;
   void SetCleared() override;
   void SetCleanupCallback(CleanupCallback cb) override;
@@ -58,6 +57,7 @@ class GPU_GLES2_EXPORT ValidatingAbstractTextureImpl : public AbstractTexture {
   void SetLevelInfo();
 
   scoped_refptr<TextureRef> texture_ref_;
+  bool decoder_managed_image_ = false;
 
   DecoderContext* decoder_context_ = nullptr;
   DestructionCB destruction_cb_;

media/gpu/command_buffer_helper.cc

@@ -174,10 +174,6 @@ class CommandBufferHelperImpl
     DVLOG(3) << __func__;
     DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
-    // Drop all textures.  Note that it's okay if the context isn't current,
-    // since AbstractTexture handles that case.
-    textures_.clear();
-
     decoder_helper_ = nullptr;
 
     // If the last reference to |this| is in a |done_cb|, destroying the wait

media/gpu/fake_command_buffer_helper.cc

@@ -28,7 +28,6 @@ void FakeCommandBufferHelper::StubLost() {
   has_stub_ = false;
   is_context_lost_ = true;
   is_context_current_ = false;
-  service_ids_.clear();
   waits_.clear();
 }
 
@@ -110,7 +109,6 @@ bool FakeCommandBufferHelper::BindImage(GLuint service_id,
   DVLOG(2) << __func__ << "(" << service_id << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(service_ids_.count(service_id));
-  DCHECK(image);
   return has_stub_;
 }