Delay actual flush and reset in AVDA

content/common/gpu/media/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 "content/common/gpu/media/android_video_decode_accelerator.h"
 
 #include <stddef.h>
 
 #include "base/android/build_info.h"
 #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"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/task_runner_util.h"
 #include "base/trace_event/trace_event.h"
 #include "content/common/gpu/media/android_copying_backing_strategy.h"
 #include "content/common/gpu/media/android_deferred_rendering_backing_strategy.h"
 #include "content/common/gpu/media/avda_return_on_failure.h"
@@ skipping 262 matching lines @@
 
 AndroidVideoDecodeAccelerator::AndroidVideoDecodeAccelerator(
     const MakeGLContextCurrentCallback& make_context_current_cb,
     const GetGLES2DecoderCallback& get_gles2_decoder_cb)
     : client_(NULL),
       make_context_current_cb_(make_context_current_cb),
       get_gles2_decoder_cb_(get_gles2_decoder_cb),
       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),
       error_sequence_token_(0),
       defer_errors_(false),
       deferred_initialization_pending_(false),
       weak_this_factory_(this) {}
 
 AndroidVideoDecodeAccelerator::~AndroidVideoDecodeAccelerator() {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ skipping 216 matching lines @@
 
   DCHECK_NE(input_buf_index, -1);
 
   media::BitstreamBuffer bitstream_buffer = pending_bitstream_buffers_.front();
 
   if (bitstream_buffer.id() == -1) {
     pending_bitstream_buffers_.pop();
     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;
   }
 
   std::unique_ptr<SharedMemoryRegion> shm;
 
   if (pending_input_buf_index_ == -1) {
     // When |pending_input_buf_index_| is not -1, the buffer is already dequeued
     // from MediaCodec, filled with data and bitstream_buffer.handle() is
     // closed.
@@ skipping 98 matching lines @@
     TRACE_EVENT_BEGIN0("media", "AVDA::DequeueOutput");
     media::MediaCodecStatus status = media_codec_->DequeueOutputBuffer(
         NoWaitTimeOut(), &buf_index, &offset, &size, &presentation_timestamp,
         &eos, NULL);
     TRACE_EVENT_END2("media", "AVDA::DequeueOutput", "status", status,
                      "presentation_timestamp (ms)",
                      presentation_timestamp.InMilliseconds());
 
     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();
 
         // Don't request picture buffers if we already have some. This avoids
         // having to dismiss the existing buffers which may actively reference
         // decoded images. Breaking their connection to the decoded image will
         // cause rendering of black frames. Instead, we let the existing
         // PictureBuffers live on and we simply update their size the next time
         // they're attachted to an image of the new resolution. See the
         // size update in |SendDecodedFrameToClient| and https://crbug/587994.
@@ skipping 28 matching lines @@
   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()));
+    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
     // isn't supposed to happen (see EncodeDecodeTest.java#617).
     // Log a metric to see how common this is.
     RecordFormatChangedMetric(FormatChangedValue::MissingFormatChanged);
     media_codec_->ReleaseOutputBuffer(buf_index, false);
     POST_ERROR(PLATFORM_FAILURE, "Dequeued buffers before FORMAT_CHANGED.");
     return false;
   }
@@ skipping 108 matching lines @@
 void AndroidVideoDecodeAccelerator::DecodeBuffer(
     const media::BitstreamBuffer& bitstream_buffer) {
   pending_bitstream_buffers_.push(bitstream_buffer);
   TRACE_COUNTER1("media", "AVDA::PendingBitstreamBufferCount",
                  pending_bitstream_buffers_.size());
 
   DoIOTask(true);
 }
 
 void AndroidVideoDecodeAccelerator::RequestPictureBuffers() {
-  client_->ProvidePictureBuffers(kNumPictureBuffers, 1,
-                                 strategy_->GetPictureBufferSize(),
-                                 strategy_->GetTextureTarget());
+  if (client_)
+    client_->ProvidePictureBuffers(kNumPictureBuffers, 1,
+                                   strategy_->GetPictureBufferSize(),
+                                   strategy_->GetTextureTarget());
 }
 
 void AndroidVideoDecodeAccelerator::AssignPictureBuffers(
     const std::vector<media::PictureBuffer>& buffers) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(output_picture_buffers_.empty());
   DCHECK(free_picture_ids_.empty());
 
   if (buffers.size() < kNumPictureBuffers) {
     POST_ERROR(INVALID_ARGUMENT, "Not enough picture buffers assigned.");
@@ skipping 37 matching lines @@
     return;
   }
 
   strategy_->ReuseOnePictureBuffer(i->second);
   DoIOTask(true);
 }
 
 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() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // It's probably okay just to return here, since the codec will be configured
   // asynchronously.  It's unclear that any state for the new request could
   // be different, unless somebody modifies |codec_config_| while we're already
   // waiting for a codec.  One shouldn't do that for thread safety.
   DCHECK_NE(state_, WAITING_FOR_CODEC);
@@ skipping 67 matching lines @@
   if (!media_codec_) {
     POST_ERROR(PLATFORM_FAILURE, "Failed to create MediaCodec.");
     return;
   }
 
   state_ = NO_ERROR;
 
   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();
 
   if (pending_input_buf_index_ != -1) {
     // The data for that index exists in the input buffer, but corresponding
     // shm block been deleted. Check that it is safe to flush the coec, i.e.
     // |pending_bitstream_buffers_| is empty.
     // TODO(timav): keep shm block for that buffer and remove this restriction.
     DCHECK(pending_bitstream_buffers_.empty());
     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
   // state because of a codec error, then it would be okay.  Otherwise, it's
   // less obvious that we are exiting the error state.  Since deferred errors
   // are only intended for fullscreen transitions right now, we take the more
   // conservative approach and let the errors post.
   // TODO(liberato): revisit this once we sort out the error state a bit more.
 
   // When codec is not in error state we can quickly reset (internally calls
   // flush()) for JB-MR2 and beyond. Prior to JB-MR2, flush() had several bugs
   // (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();
     // Since we just flushed all the output buffers, make sure that nothing is
     // using them.
     strategy_->CodecChanged(media_codec_.get(), output_picture_buffers_);
   } else {
     DVLOG(3) << __FUNCTION__
              << " Deleting the MediaCodec and creating a new one.";
     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");
 
   while (!pending_bitstream_buffers_.empty()) {
     int32_t bitstream_buffer_id = pending_bitstream_buffers_.front().id();
     pending_bitstream_buffers_.pop();
 
     if (bitstream_buffer_id != -1) {
       base::MessageLoop::current()->PostTask(
           FROM_HERE,
           base::Bind(&AndroidVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer,
                      weak_this_factory_.GetWeakPtr(), bitstream_buffer_id));
     }
   }
   TRACE_COUNTER1("media", "AVDA::PendingBitstreamBufferCount", 0);
   bitstreams_notified_in_advance_.clear();
 
   // 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();
   if (!have_context)
     LOG(WARNING) << "Failed make GL context current for Destroy, continuing.";
 
   if (strategy_)
     strategy_->Cleanup(have_context, output_picture_buffers_);
 
   // If we have an OnFrameAvailable handler, tell it that we're going away.
   if (on_frame_available_handler_) {
     on_frame_available_handler_->ClearOwner();
     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.
   weak_this_factory_.InvalidateWeakPtrs();
   if (media_codec_) {
     g_avda_timer.Pointer()->StopTimer(this);
     media_codec_.reset();
   }
   delete this;
 }
@@ skipping 86 matching lines @@
 void AndroidVideoDecodeAccelerator::OnKeyAdded() {
   DVLOG(1) << __FUNCTION__;
 
   if (state_ == WAITING_FOR_KEY)
     state_ = NO_ERROR;
 
   DoIOTask(true);
 }
 
 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(
     media::VideoDecodeAccelerator::Error error,
     int token) {
   DVLOG(1) << __FUNCTION__ << ": error: " << error << " token: " << token
            << " current: " << error_sequence_token_;
   if (token != error_sequence_token_)
     return;
 
-  client_->NotifyError(error);
+  if (client_)
+    client_->NotifyError(error);
 }
 
 void AndroidVideoDecodeAccelerator::ManageTimer(bool did_work) {
   bool should_be_running = true;
 
   base::TimeTicks now = base::TimeTicks::Now();
   if (!did_work && !most_recent_work_.is_null()) {
     // Make sure that we have done work recently enough, else stop the timer.
     if (now - most_recent_work_ > IdleTimerTimeOut()) {
       most_recent_work_ = base::TimeTicks();
@@ skipping 77 matching lines @@
                          media::VideoDecodeAccelerator::Capabilities::
                              SUPPORTS_EXTERNAL_OUTPUT_SURFACE |
                          media::VideoDecodeAccelerator::Capabilities::
                              SUPPORTS_DEFERRED_INITIALIZATION;
   }
 
   return capabilities;
 }
 
 }  // namespace content