AVDACodecImages keep a reference to the SurfaceTexture backing them

content/common/gpu/media/android_deferred_rendering_backing_strategy.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/common/gpu/media/android_deferred_rendering_backing_strategy.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "content/common/gpu/gpu_channel.h"
 #include "content/common/gpu/gpu_surface_lookup.h"
 #include "content/common/gpu/media/avda_codec_image.h"
 #include "content/common/gpu/media/avda_return_on_failure.h"
 #include "content/common/gpu/media/avda_shared_state.h"
 #include "gpu/command_buffer/service/texture_manager.h"
 #include "ui/gl/android/surface_texture.h"
 #include "ui/gl/gl_bindings.h"
+#include "ui/gl/scoped_make_current.h"
+
+namespace {
+void DeleteTexture(const scoped_refptr<gfx::GLContext>& context,
+                   const scoped_refptr<gfx::GLSurface>& surface,
+                   GLuint service_id) {
+  auto scoped_make_current =
+      make_scoped_ptr(new ui::ScopedMakeCurrent(context.get(), surface.get()));
+  glDeleteTextures(1, &service_id);

[liberato (no reviews please), 2016/02/11 15:55:41]

the more i think about it after our conversation offline, there might still be a
problem with relying on the strong refs to destroy the textures before the
glDeleteTextures happens.  if one binds a new glimage to replace the
AVCodecImage, then the texture could outlive the shared state while its unowned
service id is still set to |service_id|.  at least, i think so, since the
Texture doesn't directly reference the shared state.

while we won't cause that here, the renderer could also do that via glBindImage.
 it's a potential security issue.

since we're relying on the textures to be destroyed to trigger cleanup, we can't
keep a (texture)ref to them.  i don't believe that there's any notification when
a glimage changes.

maybe we could force the unowned service id back to 0 whenever the glimage is
changed, but that seems awful.

here's an alternate approach.  during AVDA::Cleanup, make a copy of the
SurfaceTexture into a teximage2D.  destroy the surface texture, unset all the
UnownedServiceIds to 0, and glDeleteTextures the surface texture's old client
texture.

each PictureBuffer texture will be an unused texture object.  bind the EGLImage
to each of the picturebuffer textures.  reveman tells me that this works --
binding an eglimage created from a 2D texture onto one that will be bound to an
external sampler.

also, one can delete the EGLImage and the newly created 2D texture immediately,
i think.  the PictureBuffer textures don't rely on it (have to check that too --
i think that's how it works).  if that's not true, then we've traded one clean
up problem for another :)

if you can think of a way to fix the cleanup story without all that work, that
would be okay too.  i'll give it more thought, but i don't see a way to do it at
the moment.

[watk, 2016/02/11 21:21:58]

:( Thanks for catching this. I'll have to look into the copy approach you
suggested and pray we don't have the same cleanup problem.

+}
+}
 
 namespace content {
 
 AndroidDeferredRenderingBackingStrategy::
     AndroidDeferredRenderingBackingStrategy(AVDAStateProvider* state_provider)
-    : state_provider_(state_provider), media_codec_(nullptr) {}
+    : shared_state_(new AVDASharedState),
+      state_provider_(state_provider),
+      media_codec_(nullptr) {}
 
 AndroidDeferredRenderingBackingStrategy::
     ~AndroidDeferredRenderingBackingStrategy() {}
 
 gfx::ScopedJavaSurface AndroidDeferredRenderingBackingStrategy::Initialize(
     int surface_view_id) {
-  shared_state_ = new AVDASharedState();
-
   gfx::ScopedJavaSurface surface;
   if (surface_view_id != media::VideoDecodeAccelerator::Config::kNoSurfaceID) {
     surface =
         GpuSurfaceLookup::GetInstance()->AcquireJavaSurface(surface_view_id);
   } else {
     // Create a detached SurfaceTexture. Detaching it will silently fail to
     // delete texture 0.
     surface_texture_ = gfx::SurfaceTexture::Create(0);
     surface_texture_->DetachFromGLContext();
     surface = gfx::ScopedJavaSurface(surface_texture_.get());
+    shared_state_->set_surface_texture(surface_texture_);
   }
 
   // Create a texture for the SurfaceTexture to use.  We don't attach it here
   // so that it gets attached in the compositor gl context in the common case.
   GLuint service_id = 0;
   glGenTextures(1, &service_id);
   DCHECK(service_id);
-  shared_state_->set_surface_texture_service_id(service_id);
+  scoped_refptr<gfx::GLContext> glcontext = gfx::GLContext::GetCurrent();
+  scoped_refptr<gfx::GLSurface> glsurface = gfx::GLSurface::GetCurrent();
+  shared_state_->SetSurfaceTextureServiceID(

[liberato (no reviews please), 2016/02/11 15:55:41]

good idea.

+      service_id, base::Bind(&DeleteTexture, glcontext, glsurface));
 
   return surface;
 }
 
 void AndroidDeferredRenderingBackingStrategy::Cleanup(
     bool have_context,
     const AndroidVideoDecodeAccelerator::OutputBufferMap& buffers) {
-  // If we failed before Initialize, then do nothing.
-  if (!shared_state_)
-    return;
-
-  // Make sure that no PictureBuffer textures refer to the SurfaceTexture or to
-  // the service_id that we created for it.
-  for (const std::pair<int, media::PictureBuffer>& entry : buffers)
-    SetImageForPicture(entry.second, nullptr);
-
-  // Now that no AVDACodecImages refer to the SurfaceTexture's texture, delete
-  // the texture name.
-  GLuint service_id = shared_state_->surface_texture_service_id();
-  if (service_id > 0 && have_context)
-    glDeleteTextures(1, &service_id);
+  CodecChanged(nullptr, buffers);
 }
 
 scoped_refptr<gfx::SurfaceTexture>
 AndroidDeferredRenderingBackingStrategy::GetSurfaceTexture() const {
   return surface_texture_;
 }
 
 uint32_t AndroidDeferredRenderingBackingStrategy::GetTextureTarget() const {
   return GL_TEXTURE_EXTERNAL_OES;
 }
@@ skipping 34 matching lines @@
   if (image) {
     // Also set the parameters for the level if we're not clearing
     // the image.
     const gfx::Size size = state_provider_->GetSize();
     texture_manager->SetLevelInfo(texture_ref, GetTextureTarget(), 0, GL_RGBA,
                                   size.width(), size.height(), 1, 0, GL_RGBA,
                                   GL_UNSIGNED_BYTE, gfx::Rect());
 
     // Override the texture's service_id, so that it will use the one that
     // will be / is attached to the SurfaceTexture.
+    // It's okay to never reset the unowned service id as long as the Textures
+    // that refer to it have an AVDACodecImage attached. The AVDACodecImage will
+    // ensure that the surface texture service id is valid as long as the
+    // Texture is.
     DCHECK(shared_state_->surface_texture_service_id());
     texture_ref->texture()->SetUnownedServiceId(
         shared_state_->surface_texture_service_id());
 
     static_cast<AVDACodecImage*>(image.get())
         ->SetTexture(texture_ref->texture());
   } else {
     // Clear the unowned service_id, so that this texture is no longer going
     // to depend on the surface texture at all.
     texture_ref->texture()->SetUnownedServiceId(0);
@@ skipping 92 matching lines @@
   shared_state_->SignalFrameAvailable();
 }
 
 bool AndroidDeferredRenderingBackingStrategy::ArePicturesOverlayable() {
   // SurfaceView frames are always overlayable because that's the only way to
   // display them.
   return !surface_texture_;
 }
 
 }  // namespace content