media: Handle output SurfaceView destruction in AVDA

content/common/gpu/media/android_video_decode_accelerator.cc

Index: content/common/gpu/media/android_video_decode_accelerator.cc
diff --git a/content/common/gpu/media/android_video_decode_accelerator.cc b/content/common/gpu/media/android_video_decode_accelerator.cc
index 55b1855a546d482fd89f87b95d6361c0d89e88ab..a4972c685b3b522292847c32dd52a069e60f8330 100644
--- a/content/common/gpu/media/android_video_decode_accelerator.cc
+++ b/content/common/gpu/media/android_video_decode_accelerator.cc
@@ -319,6 +319,7 @@ AndroidVideoDecodeAccelerator::AndroidVideoDecodeAccelerator(
       error_sequence_token_(0),
       defer_errors_(false),
       deferred_initialization_pending_(false),
+      surface_id_(media::VideoDecodeAccelerator::Config::kNoSurfaceID),
       weak_this_factory_(this) {}
 
 AndroidVideoDecodeAccelerator::~AndroidVideoDecodeAccelerator() {
@@ -416,13 +417,20 @@ bool AndroidVideoDecodeAccelerator::Initialize(const Config& config,
     return false;
   }
 
-  codec_config_->surface_ = strategy_->Initialize(config.surface_id);
+  surface_id_ = config.surface_id;
+  codec_config_->surface_ = strategy_->Initialize(surface_id_);
   if (codec_config_->surface_.IsEmpty()) {
     LOG(ERROR) << "Failed to initialize the backing strategy. The returned "
                   "Java surface is empty.";
     return false;
   }
 
+  on_destroying_surface_cb_ =

[liberato (no reviews please), 2016/04/27 16:50:04]

all of the callback code might be better in the deferred strategy.  as in, it
can register for the destruction callback, and just notify avda to change state.

this might fit better when the strategy chooses when / if to get a surface.

STATE_DESTROYED can stay in avda, but it can expose a "SurfaceWasDestroyed" or
something.  that call would end up moving into MediaCodecLoop, i think.

[watk, 2016/04/27 18:54:04]

I'm not sure if I agree. I think this is simpler in terms of less plumbing, and
conceptually in my mind. The reason we really care about surface destruction is
because we need to release the MediaCodec, and that's AVDAs job.

[liberato (no reviews please), 2016/04/27 21:00:37]

what brought it up is that AVDA didn't even record the surface_id before.  it
was all contained in the strategy.  now, avda peeks at the surface, and assumes
what the strategy is going to do with it.  granted, right now, the strategy has
no choice since WMPI also assumes it, but that's another issue.  :)

to be clear, i'm not suggesting moving anything beside the callback registration
/ unregistration itself.  it's not a big deal at this point, but will probably
happen anyway when the strategy starts worrying about when to allocate the
surface.

[watk, 2016/04/27 21:14:07]

Gotcha, well since you're not too worried I'll defer that change until it's more
clear to me how it will look.

+      base::Bind(&AndroidVideoDecodeAccelerator::OnDestroyingSurface,
+                 weak_this_factory_.GetWeakPtr());
+  AVDASurfaceTracker::GetInstance()->RegisterOnDestroyingSurfaceCallback(
+      on_destroying_surface_cb_);
+
   // TODO(watk,liberato): move this into the strategy.
   scoped_refptr<gfx::SurfaceTexture> surface_texture =
       strategy_->GetSurfaceTexture();
@@ -504,8 +512,10 @@ void AndroidVideoDecodeAccelerator::SetCdm(int cdm_id) {
 void AndroidVideoDecodeAccelerator::DoIOTask(bool start_timer) {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("media", "AVDA::DoIOTask");
-  if (state_ == ERROR || state_ == WAITING_FOR_CODEC)
+  if (state_ == ERROR || state_ == WAITING_FOR_CODEC ||
+      state_ == SURFACE_DESTROYED) {
     return;
+  }
 
   strategy_->MaybeRenderEarly();
   bool did_work = QueueInput();
@@ -1006,22 +1016,26 @@ AndroidVideoDecodeAccelerator::ConfigureMediaCodecOnAnyThread(
 void AndroidVideoDecodeAccelerator::OnCodecConfigured(
     std::unique_ptr<media::VideoCodecBridge> media_codec) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK_EQ(state_, WAITING_FOR_CODEC);
-
-  media_codec_ = std::move(media_codec);
+  DCHECK(state_ ==  WAITING_FOR_CODEC || state_ == SURFACE_DESTROYED);
 
   // Record one instance of the codec being initialized.
   RecordFormatChangedMetric(FormatChangedValue::CodecInitialized);
 
-  strategy_->CodecChanged(media_codec_.get());
-
   // If we are supposed to notify that initialization is complete, then do so
   // now.  Otherwise, this is a reconfiguration.
   if (deferred_initialization_pending_) {
-    NotifyInitializationComplete(!!media_codec_);
+    // Losing the output surface is not considered an error state, so notify
+    // success. The client will destroy this soon.
+    NotifyInitializationComplete(state_ == SURFACE_DESTROYED ? true
+                                                             : !!media_codec);
     deferred_initialization_pending_ = false;
   }
 
+  if (state_ == SURFACE_DESTROYED)

[liberato (no reviews please), 2016/04/27 16:50:04]

please add a comment that this will release the media codec and why we want
that.

[watk, 2016/04/27 18:54:04]

Done.

+    return;
+
+  media_codec_ = std::move(media_codec);
+  strategy_->CodecChanged(media_codec_.get());
   if (!media_codec_) {
     POST_ERROR(PLATFORM_FAILURE, "Failed to create MediaCodec.");
     return;
@@ -1097,7 +1111,8 @@ void AndroidVideoDecodeAccelerator::ResetCodecState(
 
   // If there is already a reset in flight, then that counts.  This can really
   // only happen if somebody calls Reset.
-  if (state_ == WAITING_FOR_CODEC) {
+  // If the surface is destroyed there's nothing to do.
+  if (state_ == WAITING_FOR_CODEC || state_ == SURFACE_DESTROYED) {
     if (!done_cb.is_null())
       done_cb.Run();
     return;
@@ -1224,6 +1239,11 @@ void AndroidVideoDecodeAccelerator::ActualDestroy() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  if (!on_destroying_surface_cb_.is_null()) {
+    AVDASurfaceTracker::GetInstance()->UnregisterOnDestroyingSurfaceCallback(
+        on_destroying_surface_cb_);
+  }
+
   // 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.
@@ -1277,6 +1297,24 @@ gpu::gles2::TextureRef* AndroidVideoDecodeAccelerator::GetTextureForPicture(
   return texture_ref;
 }
 
+void AndroidVideoDecodeAccelerator::OnDestroyingSurface(int surface_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  TRACE_EVENT0("media", "AVDA::OnDestroyingSurface");
+  DVLOG(1) << __FUNCTION__ << " surface_id: " << surface_id;
+
+  if (surface_id != surface_id_)
+    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_) {
+    media_codec_.reset();
+    strategy_->CodecChanged(media_codec_.get());

[liberato (no reviews please), 2016/04/27 16:50:04]

should this also clear |drain_type_|, for completeness?

i realize that it doesn't really matter, but it seems like a good idea.

[watk, 2016/04/27 18:54:04]

Good call, I think this was broken if a drain was ongoing (especially
DRAIN_FOR_DESTROY). Added logic to complete the drain. I also made flush return
early if the surface is lost.

[liberato (no reviews please), 2016/04/27 21:00:37]

actually, i didn't think about it that much.  good point on OnDrainCompleted.

+  }
+}
+
 void AndroidVideoDecodeAccelerator::OnFrameAvailable() {
   // Remember: this may be on any thread.
   DCHECK(strategy_);