media: Ensure MediaCodecs are released before attached SurfaceTextures

media/gpu/android_video_decode_accelerator.cc

 // Copyright (c) 2013 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 "media/gpu/android_video_decode_accelerator.h"
 
 #include <stddef.h>
 
 #include <memory>
 
@@ skipping 857 matching lines @@
 }
 
 void AndroidVideoDecodeAccelerator::ConfigureMediaCodecAsynchronously() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   DCHECK_NE(state_, WAITING_FOR_CODEC);
   state_ = WAITING_FOR_CODEC;
 
   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);
   }
 
   base::Optional<TaskType> task_type =
       AVDACodecAllocator::Instance()->TaskTypeForAllocation();
   if (!task_type) {
     // If there is no free thread, then just fail.
     OnCodecConfigured(nullptr);
     return;
   }
@@ skipping 248 matching lines @@
   DVLOG(1) << __func__;
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Note that async codec construction might still be in progress.  In that
   // case, the codec will be deleted when it completes once we invalidate all
   // our weak refs.
   weak_this_factory_.InvalidateWeakPtrs();
   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;
 }
 
 bool AndroidVideoDecodeAccelerator::TryToSetupDecodeOnSeparateThread(
     const base::WeakPtr<Client>& decode_client,
     const scoped_refptr<base::SingleThreadTaskRunner>& decode_task_runner) {
   return false;
 }
 
 const gfx::Size& AndroidVideoDecodeAccelerator::GetSize() const {
@@ skipping 29 matching lines @@
     UpdateSurface();
     return;
   }
 
   // If we're currently asynchronously configuring a codec, it will be destroyed
   // when configuration completes and it notices that |state_| has changed to
   // SURFACE_DESTROYED.
   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);
   }
 
   // If we're draining, signal completion now because the drain can no longer
   // proceed.
   if (drain_type_)
     OnDrainCompleted();
 }
 
 void AndroidVideoDecodeAccelerator::InitializeCdm() {
@@ skipping 245 matching lines @@
 
   // Ensure the current context is active when switching surfaces; we may need
   // to create a new texture.
   if (success && !make_context_current_cb_.Run()) {
     NOTIFY_ERROR(PLATFORM_FAILURE,
                  "Failed to make this decoder's GL context current when "
                  "switching surfaces.");
     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);
   }
 
   // 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;
 }
 
 }  // namespace media