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 ccc3ef84639cecd268370fae7d3dc1ad9fdc03ef..bd79478d8097c11704072f3aef53f6415cb6e4c0 100644
--- a/content/common/gpu/media/android_video_decode_accelerator.cc
+++ b/content/common/gpu/media/android_video_decode_accelerator.cc
@@ -10,6 +10,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"
@@ -292,6 +293,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),
@@ -528,8 +530,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;
   }
@@ -648,17 +648,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. Instead, try

[watk, 2016/04/18 21:12:18]

Remove "try" since we are unconditionally running it.

[Tima Vaisburd, 2016/04/20 01:54:19]

Done.

+        // to run the drain completion callback.
+        if (IsDrainingForReset()) {
+          state_ = ERROR;
+          DCHECK(!drain_completed_cb_.is_null());
+          base::ResetAndReturn(&drain_completed_cb_).Run();
+        } 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 (IsDrainingForReset())
+          return true;
+
         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();
 
@@ -707,19 +719,21 @@ bool AndroidVideoDecodeAccelerator::DequeueOutput() {
     // 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()));
+    if (drain_completed_cb_.is_null()) {
+      // Unexpected EOS.
+      state_ = ERROR;
+      ResetCodecState(base::Closure());
+    } else {
+      base::ResetAndReturn(&drain_completed_cb_).Run();
     }
     return false;
   }
 
+  if (IsDrainingForReset()) {
+    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
@@ -848,9 +862,10 @@ void AndroidVideoDecodeAccelerator::DecodeBuffer(
 }
 
 void AndroidVideoDecodeAccelerator::RequestPictureBuffers() {
-  client_->ProvidePictureBuffers(kNumPictureBuffers, 1,
-                                 strategy_->GetPictureBufferSize(),
-                                 strategy_->GetTextureTarget());
+  if (client_)
+    client_->ProvidePictureBuffers(kNumPictureBuffers, 1,
+                                   strategy_->GetPictureBufferSize(),
+                                   strategy_->GetTextureTarget());
 }
 
 void AndroidVideoDecodeAccelerator::AssignPictureBuffers(
@@ -908,7 +923,13 @@ void AndroidVideoDecodeAccelerator::ReusePictureBuffer(
 void AndroidVideoDecodeAccelerator::Flush() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  DecodeBuffer(media::BitstreamBuffer(-1, base::SharedMemoryHandle(), 0));
+  base::Closure notification_cb = media::BindToCurrentLoop(
+      base::Bind(&AndroidVideoDecodeAccelerator::NotifyFlushDone,
+                 weak_this_factory_.GetWeakPtr()));
+
+  RequestDrain(DRAIN_FOR_FLUSH,
+               base::Bind(&AndroidVideoDecodeAccelerator::ResetCodecState,
+                          weak_this_factory_.GetWeakPtr(), notification_cb));
 }
 
 void AndroidVideoDecodeAccelerator::ConfigureMediaCodecAsynchronously() {
@@ -996,13 +1017,36 @@ void AndroidVideoDecodeAccelerator::OnCodecConfigured(
   ManageTimer(true);
 }
 
-void AndroidVideoDecodeAccelerator::ResetCodecState() {
+void AndroidVideoDecodeAccelerator::RequestDrain(

[watk, 2016/04/18 21:12:18]

IMO RequestDrain makes it sound like there's a chance the request will be
rejected. What about DrainCodec? Or BeginDrainingCodec.

[Tima Vaisburd, 2016/04/20 01:54:19]

Renamed to StartCodecDrain(). I have to strong preference, can do any if you
still like yours better.

+    DrainType drain_type,
+    base::Closure drain_completed_cb) {

[watk, 2016/04/18 21:12:18]

I'm wondering if it would be clearer to structure it like this: you have a
OnCodecDrained which gets called whenever we dequeue EOS. It switches on
drain_type to call Notify{Flush,Reset}Done, ActualDestroy and ResetCodecState as
required. It can also reset the drain_type back to NONE.

I can't tell if it's better without writing the code, but maybe you already
tried?

[Tima Vaisburd, 2016/04/20 01:54:19]

Yes, thank you! I tried now, and I think this way is better.

+  DVLOG(2) << __FUNCTION__ << " drain_type:" << drain_type;
+
+  // 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_completed_cb_.is_null() || drain_type == DRAIN_FOR_DESTROY);
+
+  drain_completed_cb_ = drain_completed_cb;
+  drain_type_ = drain_type;
+  DecodeBuffer(media::BitstreamBuffer(-1, base::SharedMemoryHandle(), 0));

[watk, 2016/04/18 21:12:18]

If there's an EOS already queued for a DRAIN_FOR_FLUSH, for example, and this is
DRAIN_FOR_DESTROY, should we queue another EOS? It seems like we may as well
wait for the one already queued. It probably doesn't matter but worth testing.

[Tima Vaisburd, 2016/04/20 01:54:19]

Done.

+}
+
+bool AndroidVideoDecodeAccelerator::IsDrainingForReset() const {
+  return !drain_completed_cb_.is_null() &&
+         (drain_type_ == DRAIN_FOR_RESET || drain_type_ == DRAIN_FOR_DESTROY);
+}
+
+void AndroidVideoDecodeAccelerator::ResetCodecState(
+    base::Closure notification_cb) {

[watk, 2016/04/18 21:12:18]

I would be inclined to name this consistently with drain_completed_cb even if it
is a bit long. Maybe done_cb if you want to make it shorter?

[Tima Vaisburd, 2016/04/20 01:54:19]

Done.

   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 (!notification_cb.is_null())
+      notification_cb.Run();
     return;
+  }
 
   bitstream_buffers_in_decoder_.clear();
 
@@ -1015,8 +1059,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);
+  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
@@ -1031,7 +1075,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();
@@ -1044,12 +1088,16 @@ 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 (!notification_cb.is_null())
+    notification_cb.Run();
 }
 
 void AndroidVideoDecodeAccelerator::Reset() {
+  DVLOG(1) << __FUNCTION__;
+
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("media", "AVDA::Reset");
 
@@ -1070,16 +1118,28 @@ void AndroidVideoDecodeAccelerator::Reset() {
   // Any error that is waiting to post can be ignored.
   error_sequence_token_++;
 
-  ResetCodecState();
+  DCHECK(strategy_);
+  strategy_->ReleaseCodecBuffers(output_picture_buffers_);
 
-  // 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()));
+  base::Closure notification_cb = media::BindToCurrentLoop(

[watk, 2016/04/18 21:12:18]

I don't think you need BindToCurrentLoop because this is all single threaded
right?

[Tima Vaisburd, 2016/04/20 01:54:19]

Removed BindToCurrentLoop. The original code always posted the task, I'm not
quite sure why though.

+      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 (codec_config_->codec_ == media::kCodecVP8) {
+    // Postpone ResetCodecState() after the drain.
+    RequestDrain(DRAIN_FOR_RESET,
+                 base::Bind(&AndroidVideoDecodeAccelerator::ResetCodecState,
+                            weak_this_factory_.GetWeakPtr(), notification_cb));
+  } else {
+    ResetCodecState(notification_cb);
+  }
 }
 
 void AndroidVideoDecodeAccelerator::Destroy() {
+  DVLOG(1) << __FUNCTION__;
+

[watk, 2016/04/18 21:12:18]

nit: Remove empty line to keep the LOG and thread DCHECK as out of the way as
possible.

[Tima Vaisburd, 2016/04/20 01:54:19]

Done.

   DCHECK(thread_checker_.CalledOnValidThread());
 
   bool have_context = make_context_current_cb_.Run();
@@ -1095,6 +1155,27 @@ void AndroidVideoDecodeAccelerator::Destroy() {
     on_frame_available_handler_ = nullptr;
   }
 
+  client_ = nullptr;
+
+  // 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.
+    RequestDrain(DRAIN_FOR_DESTROY,
+                 media::BindToCurrentLoop(
+                     base::Bind(&AndroidVideoDecodeAccelerator::ActualDestroy,
+                                weak_this_factory_.GetWeakPtr())));
+  } else {
+    ActualDestroy();
+  }
+}
+
+void AndroidVideoDecodeAccelerator::ActualDestroy() {
+  DVLOG(1) << __FUNCTION__;
   // 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.
@@ -1201,25 +1282,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(
@@ -1230,7 +1316,8 @@ void AndroidVideoDecodeAccelerator::NotifyError(
   if (token != error_sequence_token_)
     return;
 
-  client_->NotifyError(error);
+  if (client_)
+    client_->NotifyError(error);
 }
 
 void AndroidVideoDecodeAccelerator::ManageTimer(bool did_work) {