AVDACodecImages keep a reference to the SurfaceTexture backing them

content/common/gpu/media/android_deferred_rendering_backing_strategy.cc

Index: content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
diff --git a/content/common/gpu/media/android_deferred_rendering_backing_strategy.cc b/content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
index a76c98dece8b5c4c7e32e9d3d3812996963a2929..62ede7113cbbda790667df9646f3d7f9866b5404 100644
--- a/content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
+++ b/content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
@@ -16,20 +16,31 @@
 #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 =
@@ -40,6 +51,7 @@ gfx::ScopedJavaSurface AndroidDeferredRenderingBackingStrategy::Initialize(
     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
@@ -47,7 +59,10 @@ gfx::ScopedJavaSurface AndroidDeferredRenderingBackingStrategy::Initialize(
   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;
 }
@@ -55,20 +70,7 @@ gfx::ScopedJavaSurface AndroidDeferredRenderingBackingStrategy::Initialize(
 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>
@@ -123,6 +125,10 @@ void AndroidDeferredRenderingBackingStrategy::SetImageForPicture(
 
     // 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());