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 <memory>
 
 #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 221 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 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();
 
         // 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 19 matching lines @@
                  << " size:" << size << " eos:" << eos;
         break;
 
       default:
         NOTREACHED();
         break;
     }
   } while (buf_index < 0);
 
   if (eos) {
-    DVLOG(3) << __FUNCTION__ << ": Resetting codec state after EOS";
+    OnDrainCompleted();
+    return false;
+  }
 
-    // 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()));
-    }
-    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
     // 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.");
@@ skipping 110 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_) {

[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(
     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 35 matching lines @@
     POST_ERROR(PLATFORM_FAILURE, "Can't find PictureBuffer id "
                                      << picture_buffer_id);
     return;
   }
 
   strategy_->ReuseOnePictureBuffer(i->second);
   DoIOTask(true);
 }
 
 void AndroidVideoDecodeAccelerator::Flush() {
+  DVLOG(1) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  DecodeBuffer(media::BitstreamBuffer(-1, base::SharedMemoryHandle(), 0));
+  StartCodecDrain(DRAIN_FOR_FLUSH);
 }
 
 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::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();
 
   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);

[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
   // 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 (!done_cb.is_null())
+    done_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_);

[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();
   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;

[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.
   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 79 matching lines @@
     if (media::MediaCodecUtil::IsSurfaceViewOutputSupported()) {
       capabilities.flags |= media::VideoDecodeAccelerator::Capabilities::
           SUPPORTS_EXTERNAL_OUTPUT_SURFACE;
     }
   }
 
   return capabilities;
 }
 
 }  // namespace content