media: Ensure MediaCodecs are released before attached SurfaceTextures

media/gpu/android_video_decode_accelerator.cc

Index: media/gpu/android_video_decode_accelerator.cc
diff --git a/media/gpu/android_video_decode_accelerator.cc b/media/gpu/android_video_decode_accelerator.cc
index 16d347983ebd17cc4c3ef193c0c865998703b536..dc89848d89df815078c04d885727ed11fef855ea 100644
--- a/media/gpu/android_video_decode_accelerator.cc
+++ b/media/gpu/android_video_decode_accelerator.cc
@@ -875,7 +875,8 @@ void AndroidVideoDecodeAccelerator::ConfigureMediaCodecAsynchronously() {
 
   if (media_codec_) {
     AVDACodecAllocator::Instance()->ReleaseMediaCodec(
-        std::move(media_codec_), codec_config_->task_type, config_.surface_id);
+        std::move(media_codec_), codec_config_->task_type, config_.surface_id,
+        picture_buffer_manager_.surface_texture().get());

[liberato (no reviews please), 2017/01/13 17:22:04]

this looks a lot like the surface owner api.  it sends the owner in place of the
surface_id, including for SurfaceTexture cases, if it wants to free the surface
too.  i think that i'm not handling these cases correctly in that CL, though.

that's good -- we're converging :)

     picture_buffer_manager_.CodecChanged(nullptr);
   }
 
@@ -1144,13 +1145,19 @@ void AndroidVideoDecodeAccelerator::ActualDestroy() {
   g_avda_manager.Get().StopTimer(this);
   if (media_codec_) {
     AVDACodecAllocator::Instance()->ReleaseMediaCodec(
-        std::move(media_codec_), codec_config_->task_type, config_.surface_id);
+        std::move(media_codec_), codec_config_->task_type, config_.surface_id,
+        picture_buffer_manager_.surface_texture().get());

[liberato (no reviews please), 2017/01/13 17:22:04]

it's a little weird that we ask for the surface texture after Destroy.  not sure
if it's a problem, but it should probably be well commented in
picture_buffer_manager.  else, somebody will clear it in destroy or something. 
probably me.  :)

[watk, 2017/01/14 01:01:03]

Agreed. I fixed this in the latest PS. Now Destroy() does clear it :)

   }
 
   // We no longer care about |surface_id|, in case we did before.  It's okay
   // if we have no surface and/or weren't the owner or a waiter.
   AVDACodecAllocator::Instance()->DeallocateSurface(this, config_.surface_id);
 
+  if (picture_buffer_manager_.surface_texture()) {
+    AVDACodecAllocator::Instance()->ReleaseSurfaceTexture(
+        picture_buffer_manager_.surface_texture());
+  }
+
   delete this;
 }
 
@@ -1200,7 +1207,8 @@ void AndroidVideoDecodeAccelerator::OnSurfaceDestroyed() {
   state_ = SURFACE_DESTROYED;
   if (media_codec_) {
     AVDACodecAllocator::Instance()->ReleaseMediaCodec(
-        std::move(media_codec_), codec_config_->task_type, config_.surface_id);
+        std::move(media_codec_), codec_config_->task_type, config_.surface_id,
+        picture_buffer_manager_.surface_texture().get());
     picture_buffer_manager_.CodecChanged(nullptr);
   }
 
@@ -1466,29 +1474,34 @@ bool AndroidVideoDecodeAccelerator::UpdateSurface() {
     success = false;
   }
 
+  // Save the SurfaceTexture if any, so we can release it below.
+  scoped_refptr<gl::SurfaceTexture> previous_surface_texture =
+      picture_buffer_manager_.surface_texture();
+  gl::ScopedJavaSurface new_surface;
   if (success) {
-    codec_config_->surface = picture_buffer_manager_.Initialize(new_surface_id);
-    if (codec_config_->surface.IsEmpty()) {
+    new_surface = picture_buffer_manager_.Initialize(new_surface_id);
+    if (new_surface.IsEmpty()) {
       NOTIFY_ERROR(PLATFORM_FAILURE, "Failed to switch surfaces.");
       success = false;
     }
   }
 
   if (success && media_codec_ &&
-      !media_codec_->SetSurface(codec_config_->surface.j_surface().obj())) {
+      !media_codec_->SetSurface(new_surface.j_surface().obj())) {
     NOTIFY_ERROR(PLATFORM_FAILURE, "MediaCodec failed to switch surfaces.");
     success = false;
   }
 
   if (success) {
     config_.surface_id = new_surface_id;
+    codec_config_->surface = std::move(new_surface);
   } else {
     // This might be called from OnSurfaceDestroyed(), so we have to release the
     // MediaCodec if we failed to switch the surface.
     if (media_codec_) {
       AVDACodecAllocator::Instance()->ReleaseMediaCodec(
           std::move(media_codec_), codec_config_->task_type,
-          previous_surface_id);
+          previous_surface_id, previous_surface_texture.get());
       picture_buffer_manager_.CodecChanged(nullptr);
     }
     AVDACodecAllocator::Instance()->DeallocateSurface(this, new_surface_id);
@@ -1497,6 +1510,14 @@ bool AndroidVideoDecodeAccelerator::UpdateSurface() {
   // Regardless of whether we succeeded, we no longer own the previous surface.
   AVDACodecAllocator::Instance()->DeallocateSurface(this, previous_surface_id);
 
+  // Regardless of whether we succeded, we no longer want the previous
+  // SurfaceTexture. This ensures it's released after its attached codec (if
+  // applicable).
+  if (previous_surface_texture) {
+    AVDACodecAllocator::Instance()->ReleaseSurfaceTexture(
+        previous_surface_texture);
+  }
+
   return success;
 }