Delay actual flush and reset 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 2c062af38735a4d12d5984394712a59e400a8ea3..ae4410039bb4f704846557e0a959f00e40d632fb 100644
--- a/content/common/gpu/media/android_video_decode_accelerator.cc
+++ b/content/common/gpu/media/android_video_decode_accelerator.cc
@@ -12,6 +12,7 @@
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
@@ -294,6 +295,7 @@ AndroidVideoDecodeAccelerator::AndroidVideoDecodeAccelerator(
       is_encrypted_(false),
       state_(NO_ERROR),
       picturebuffers_requested_(false),
+      drain_type_(DRAIN_TYPE_NONE),
       media_drm_bridge_cdm_context_(nullptr),
       cdm_registration_id_(0),
       pending_input_buf_index_(-1),
@@ -535,8 +537,6 @@ bool AndroidVideoDecodeAccelerator::QueueInput() {
     TRACE_COUNTER1("media", "AVDA::PendingBitstreamBufferCount",
                    pending_bitstream_buffers_.size());
 
-    DCHECK_NE(state_, ERROR);
-    state_ = WAITING_FOR_EOS;
     media_codec_->QueueEOS(input_buf_index);
     return true;
   }
@@ -655,17 +655,29 @@ bool AndroidVideoDecodeAccelerator::DequeueOutput() {
 
     switch (status) {
       case media::MEDIA_CODEC_ERROR:
-        POST_ERROR(PLATFORM_FAILURE, "DequeueOutputBuffer failed.");
+        // Do not post an error if we are draining for reset and destroy.
+        // Instead, run the drain completion task.
+        if (IsDrainingForResetOrDestroy()) {
+          DVLOG(1) << __FUNCTION__ << ": error while codec draining";
+          state_ = ERROR;
+          OnDrainCompleted();
+        } else {
+          POST_ERROR(PLATFORM_FAILURE, "DequeueOutputBuffer failed.");
+        }
         return false;
 
       case media::MEDIA_CODEC_DEQUEUE_OUTPUT_AGAIN_LATER:
         return false;
 
       case media::MEDIA_CODEC_OUTPUT_FORMAT_CHANGED: {
+        if (IsDrainingForResetOrDestroy())
+          return true;  // ignore
+
         if (media_codec_->GetOutputSize(&size_) != media::MEDIA_CODEC_OK) {
           POST_ERROR(PLATFORM_FAILURE, "GetOutputSize failed.");
           return false;
         }
+
         DVLOG(3) << __FUNCTION__
                  << " OUTPUT_FORMAT_CHANGED, new size: " << size_.ToString();
 
@@ -705,28 +717,15 @@ bool AndroidVideoDecodeAccelerator::DequeueOutput() {
   } while (buf_index < 0);
 
   if (eos) {
-    DVLOG(3) << __FUNCTION__ << ": Resetting codec state after EOS";
-
-    // If we were waiting for an EOS, clear the state and reset the MediaCodec
-    // as normal. Otherwise, enter the ERROR state which will force destruction
-    // of MediaCodec during ResetCodecState().
-    //
-    // Some Android platforms seem to send an EOS buffer even when we're not
-    // expecting it. In this case, destroy and reset the codec but don't notify
-    // flush done since it violates the state machine. http://crbug.com/585959.
-    const bool was_waiting_for_eos = state_ == WAITING_FOR_EOS;
-    state_ = was_waiting_for_eos ? NO_ERROR : ERROR;
-
-    ResetCodecState();
-    // |media_codec_| might still be null.
-    if (was_waiting_for_eos) {
-      base::MessageLoop::current()->PostTask(
-          FROM_HERE, base::Bind(&AndroidVideoDecodeAccelerator::NotifyFlushDone,
-                                weak_this_factory_.GetWeakPtr()));
-    }
+    OnDrainCompleted();
     return false;
   }
 
+  if (IsDrainingForResetOrDestroy()) {
+    media_codec_->ReleaseOutputBuffer(buf_index, false);
+    return true;
+  }
+
   if (!picturebuffers_requested_) {
     // If, somehow, we get a decoded frame back before a FORMAT_CHANGED
     // message, then we might not have any picture buffers to use.  This
@@ -855,9 +854,11 @@ void AndroidVideoDecodeAccelerator::DecodeBuffer(
 }
 
 void AndroidVideoDecodeAccelerator::RequestPictureBuffers() {
-  client_->ProvidePictureBuffers(kNumPictureBuffers, 1,
-                                 strategy_->GetPictureBufferSize(),
-                                 strategy_->GetTextureTarget());
+  if (client_) {

[DaleCurtis, 2016/04/21 00:16:07]

Why this?

[Tima Vaisburd, 2016/04/21 00:28:45]

See below.

+    client_->ProvidePictureBuffers(kNumPictureBuffers, 1,
+                                   strategy_->GetPictureBufferSize(),
+                                   strategy_->GetTextureTarget());
+  }
 }
 
 void AndroidVideoDecodeAccelerator::AssignPictureBuffers(
@@ -913,9 +914,10 @@ void AndroidVideoDecodeAccelerator::ReusePictureBuffer(
 }
 
 void AndroidVideoDecodeAccelerator::Flush() {
+  DVLOG(1) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  DecodeBuffer(media::BitstreamBuffer(-1, base::SharedMemoryHandle(), 0));
+  StartCodecDrain(DRAIN_FOR_FLUSH);
 }
 
 void AndroidVideoDecodeAccelerator::ConfigureMediaCodecAsynchronously() {
@@ -1003,13 +1005,73 @@ void AndroidVideoDecodeAccelerator::OnCodecConfigured(
   ManageTimer(true);
 }
 
-void AndroidVideoDecodeAccelerator::ResetCodecState() {
+void AndroidVideoDecodeAccelerator::StartCodecDrain(DrainType drain_type) {
+  DVLOG(2) << __FUNCTION__ << " drain_type:" << drain_type;
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // We assume that DRAIN_FOR_FLUSH and DRAIN_FOR_RESET cannot come while
+  // another drain request is present, but DRAIN_FOR_DESTROY can.
+  DCHECK(drain_type != DRAIN_TYPE_NONE);

[DaleCurtis, 2016/04/21 00:16:07]

DCHECK_NE

[Tima Vaisburd, 2016/04/21 21:38:22]

Done.

+  DCHECK(drain_type_ == DRAIN_TYPE_NONE || drain_type == DRAIN_FOR_DESTROY);

[DaleCurtis, 2016/04/21 00:16:07]

add << drain_type_ print so this DCHECK will print useful info.

[Tima Vaisburd, 2016/04/21 21:38:22]

Done.

+
+  const bool enqueue_eos = (drain_type_ == DRAIN_TYPE_NONE);

[DaleCurtis, 2016/04/21 00:16:07]

No unnecessary parens.

[Tima Vaisburd, 2016/04/21 21:38:21]

Done.

+  drain_type_ = drain_type;
+
+  if (enqueue_eos)
+    DecodeBuffer(media::BitstreamBuffer(-1, base::SharedMemoryHandle(), 0));
+}
+
+bool AndroidVideoDecodeAccelerator::IsDrainingForResetOrDestroy() const {
+  return drain_type_ == DRAIN_FOR_RESET || drain_type_ == DRAIN_FOR_DESTROY;
+}
+
+void AndroidVideoDecodeAccelerator::OnDrainCompleted() {
+  DVLOG(2) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // If we were waiting for an EOS, clear the state and reset the MediaCodec
+  // as normal. Otherwise, enter the ERROR state which will force destruction
+  // of MediaCodec during ResetCodecState().
+  //
+  // Some Android platforms seem to send an EOS buffer even when we're not
+  // expecting it. In this case, destroy and reset the codec but don't notify
+  // flush done since it violates the state machine. http://crbug.com/585959.
+
+  switch (drain_type_) {
+    case DRAIN_TYPE_NONE:
+      // Unexpected EOS.
+      state_ = ERROR;
+      ResetCodecState(base::Closure());
+      break;
+    case DRAIN_FOR_FLUSH:
+      ResetCodecState(media::BindToCurrentLoop(
+          base::Bind(&AndroidVideoDecodeAccelerator::NotifyFlushDone,
+                     weak_this_factory_.GetWeakPtr())));
+      break;
+    case DRAIN_FOR_RESET:
+      ResetCodecState(media::BindToCurrentLoop(
+          base::Bind(&AndroidVideoDecodeAccelerator::NotifyResetDone,
+                     weak_this_factory_.GetWeakPtr())));
+      break;
+    case DRAIN_FOR_DESTROY:
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE, base::Bind(&AndroidVideoDecodeAccelerator::ActualDestroy,
+                                weak_this_factory_.GetWeakPtr()));
+      break;
+  }
+  drain_type_ = DRAIN_TYPE_NONE;
+}
+
+void AndroidVideoDecodeAccelerator::ResetCodecState(base::Closure done_cb) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // 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 (state_ == WAITING_FOR_CODEC) {
+    if (!done_cb.is_null())
+      done_cb.Run();
     return;
+  }
 
   bitstream_buffers_in_decoder_.clear();
 
@@ -1022,8 +1084,8 @@ void AndroidVideoDecodeAccelerator::ResetCodecState() {
     pending_input_buf_index_ = -1;
   }
 
-  if (state_ == WAITING_FOR_KEY)
-    state_ = NO_ERROR;
+  const bool did_codec_error_happen = (state_ == ERROR);

[DaleCurtis, 2016/04/21 00:16:07]

No unnecessary parens.

[Tima Vaisburd, 2016/04/21 21:38:22]

Done.

+  state_ = NO_ERROR;
 
   // We might increment error_sequence_token here to cancel any delayed errors,
   // but right now it's unclear that it's safe to do so.  If we are in an error
@@ -1038,7 +1100,7 @@ void AndroidVideoDecodeAccelerator::ResetCodecState() {
   // (b/8125974, b/8347958) so we must delete the MediaCodec and create a new
   // one. The full reconfigure is much slower and may cause visible freezing if
   // done mid-stream.
-  if (state_ == NO_ERROR &&
+  if (!did_codec_error_happen &&
       base::android::BuildInfo::GetInstance()->sdk_int() >= 18) {
     DVLOG(3) << __FUNCTION__ << " Doing fast MediaCodec reset (flush).";
     media_codec_->Reset();
@@ -1051,12 +1113,15 @@ void AndroidVideoDecodeAccelerator::ResetCodecState() {
     g_avda_timer.Pointer()->StopTimer(this);
     // Changing the codec will also notify the strategy to forget about any
     // output buffers it has currently.
-    state_ = NO_ERROR;
     ConfigureMediaCodecAsynchronously();
   }
+
+  if (!done_cb.is_null())
+    done_cb.Run();
 }
 
 void AndroidVideoDecodeAccelerator::Reset() {
+  DVLOG(1) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("media", "AVDA::Reset");
 
@@ -1077,16 +1142,23 @@ void AndroidVideoDecodeAccelerator::Reset() {
   // Any error that is waiting to post can be ignored.
   error_sequence_token_++;
 
-  ResetCodecState();
+  DCHECK(strategy_);

[DaleCurtis, 2016/04/21 00:16:07]

Don't think this can every be null?

[Tima Vaisburd, 2016/04/21 21:38:22]

As far as I understand |strategy_| can be null only before initialization. There
is another such DCHECK on l.1252

+  strategy_->ReleaseCodecBuffers(output_picture_buffers_);

[DaleCurtis, 2016/04/21 00:16:07]

Did we decide to do this for all codecs and not just VP8?

[Tima Vaisburd, 2016/04/21 00:28:45]

No, we have not, but I went ahead myself because I think this is the right thing
to do in general (in other words I do not trust MediaCodec implementations that
much) and pretty cheap.

 
-  // Note that |media_codec_| might not yet be ready, but we can still post
-  // this anyway.
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE, base::Bind(&AndroidVideoDecodeAccelerator::NotifyResetDone,
-                            weak_this_factory_.GetWeakPtr()));
+  // Some VP8 files require complete MediaCodec drain before we can call
+  // MediaCodec.flush() or MediaCodec.reset(). http://crbug.com/598963.
+  if (media_codec_ && codec_config_->codec_ == media::kCodecVP8) {
+    // Postpone ResetCodecState() after the drain.
+    StartCodecDrain(DRAIN_FOR_RESET);
+  } else {
+    ResetCodecState(media::BindToCurrentLoop(
+        base::Bind(&AndroidVideoDecodeAccelerator::NotifyResetDone,
+                   weak_this_factory_.GetWeakPtr())));
+  }
 }
 
 void AndroidVideoDecodeAccelerator::Destroy() {
+  DVLOG(1) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
 
   bool have_context = make_context_current_cb_.Run();
@@ -1102,6 +1174,26 @@ void AndroidVideoDecodeAccelerator::Destroy() {
     on_frame_available_handler_ = nullptr;
   }
 
+  client_ = nullptr;

[DaleCurtis, 2016/04/21 00:16:07]

Despite clearing this, none of the nullptr checks on it should be necessary
right? Did you find that these methods were actually getting invoked?

[Tima Vaisburd, 2016/04/21 00:28:45]

Yes, I think I did. Before this change I observed crashes which could be
explained as invalid |client_| pointers in Notify... methods. I believe that the
|client_| is invalid at the time of Destroy(). Before this CL a weak pointer
invalidation was enough to prevent them, but now machinery keeps running until
we drain.

[DaleCurtis, 2016/04/21 00:32:36]

I see. It's risky to invalidate the weakptrs and cross your fingers, so this
sounds better.

+
+  // Some VP8 files require complete MediaCodec drain before we can call
+  // MediaCodec.flush() or MediaCodec.reset(). http://crbug.com/598963.
+  if (media_codec_ && codec_config_->codec_ == media::kCodecVP8) {
+    // Clear pending_bitstream_buffers_.
+    while (!pending_bitstream_buffers_.empty())
+      pending_bitstream_buffers_.pop();
+
+    // Postpone ActualDestroy after the drain.
+    StartCodecDrain(DRAIN_FOR_DESTROY);
+  } else {
+    ActualDestroy();
+  }
+}
+
+void AndroidVideoDecodeAccelerator::ActualDestroy() {
+  DVLOG(1) << __FUNCTION__;
+  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.
@@ -1208,25 +1300,30 @@ void AndroidVideoDecodeAccelerator::OnKeyAdded() {
 }
 
 void AndroidVideoDecodeAccelerator::NotifyInitializationComplete(bool success) {
-  client_->NotifyInitializationComplete(success);
+  if (client_)
+    client_->NotifyInitializationComplete(success);
 }
 
 void AndroidVideoDecodeAccelerator::NotifyPictureReady(
     const media::Picture& picture) {
-  client_->PictureReady(picture);
+  if (client_)
+    client_->PictureReady(picture);
 }
 
 void AndroidVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer(
     int input_buffer_id) {
-  client_->NotifyEndOfBitstreamBuffer(input_buffer_id);
+  if (client_)
+    client_->NotifyEndOfBitstreamBuffer(input_buffer_id);
 }
 
 void AndroidVideoDecodeAccelerator::NotifyFlushDone() {
-  client_->NotifyFlushDone();
+  if (client_)
+    client_->NotifyFlushDone();
 }
 
 void AndroidVideoDecodeAccelerator::NotifyResetDone() {
-  client_->NotifyResetDone();
+  if (client_)
+    client_->NotifyResetDone();
 }
 
 void AndroidVideoDecodeAccelerator::NotifyError(
@@ -1237,7 +1334,8 @@ void AndroidVideoDecodeAccelerator::NotifyError(
   if (token != error_sequence_token_)
     return;
 
-  client_->NotifyError(error);
+  if (client_)
+    client_->NotifyError(error);
 }
 
 void AndroidVideoDecodeAccelerator::ManageTimer(bool did_work) {