media: Lazily reconfigure MediaCodecs in AVDA for JellyBean

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"
@@ skipping 390 matching lines @@
 
   NOTIMPLEMENTED();
   NotifyCdmAttached(false);
 
 #endif  // !defined(ENABLE_MOJO_MEDIA_IN_GPU_PROCESS)
 }
 
 void AndroidVideoDecodeAccelerator::DoIOTask() {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("media", "AVDA::DoIOTask");
+
   if (state_ == ERROR) {
     return;
+  } else if (state_ == PENDING_CODEC_RECONFIGURE) {

[watk, 2016/03/24 01:02:58]

Should probably put this in Decode().

[liberato (no reviews please), 2016/03/29 14:54:40]

probably, yes, since the codec can't take any action until a decode comes in.

alternatively, if you keep it here, then it might be a good idea to see if
|pending_bitstream_buffers_| is not empty before allocating the codec.  we call
DoIOTask a lot, though maybe not after a Reset.  still, makes more sense to
check if there's input to the codec before we bother.  then i think it's the
same as in Decode

+    // Defer errors here in case we're about to be destroyed because the
+    // pipeline is suspending.
+    base::AutoReset<bool> auto_reset(&defer_errors_, true);
+    if (!ConfigureMediaCodec()) {
+      POST_ERROR(PLATFORM_FAILURE, "Failed to create MediaCodec.");
+      return;
+    }
+    state_ = NO_ERROR;
   }
 
   bool did_work = QueueInput();
   while (DequeueOutput())
     did_work = true;
 
   ManageTimer(did_work);
 }
 
 bool AndroidVideoDecodeAccelerator::QueueInput() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_NE(state_, PENDING_CODEC_RECONFIGURE);
   TRACE_EVENT0("media", "AVDA::QueueInput");
   base::AutoReset<bool> auto_reset(&defer_errors_, true);
   if (bitstreams_notified_in_advance_.size() > kMaxBitstreamsNotifiedInAdvance)
     return false;
   if (pending_bitstream_buffers_.empty())
     return false;
   if (state_ == WAITING_FOR_KEY)
     return false;
 
   int input_buf_index = pending_input_buf_index_;
@@ skipping 94 matching lines @@
     DVLOG(1) << "QueueSecureInputBuffer failed: NO_KEY";
     pending_input_buf_index_ = input_buf_index;
     state_ = WAITING_FOR_KEY;
     return false;
   }
 
   pending_input_buf_index_ = -1;
   pending_bitstream_buffers_.pop();
   TRACE_COUNTER1("media", "AVDA::PendingBitstreamBufferCount",
                  pending_bitstream_buffers_.size());
-
-  if (status != media::MEDIA_CODEC_OK) {
-    POST_ERROR(PLATFORM_FAILURE, "Failed to QueueInputBuffer: " << status);
-    return false;
-  }
-
   // We should call NotifyEndOfBitstreamBuffer(), when no more decoded output
   // will be returned from the bitstream buffer. However, MediaCodec API is
   // not enough to guarantee it.
   // So, here, we calls NotifyEndOfBitstreamBuffer() in advance in order to
   // keep getting more bitstreams from the client, and throttle them by using
   // |bitstreams_notified_in_advance_|.
   // TODO(dwkang): check if there is a way to remove this workaround.
   base::MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(&AndroidVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer,
                  weak_this_factory_.GetWeakPtr(), bitstream_buffer.id()));
   bitstreams_notified_in_advance_.push_back(bitstream_buffer.id());
 
+  if (status != media::MEDIA_CODEC_OK) {
+    POST_ERROR(PLATFORM_FAILURE, "Failed to QueueInputBuffer: " << status);
+    return false;

[watk, 2016/03/24 01:02:58]

This has to be moved because otherwise we might pop an input buffer, and never
NotifyEnded, which will get GVD out of sync.

+  }
+
   return true;
 }
 
 bool AndroidVideoDecodeAccelerator::DequeueOutput() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_NE(state_, PENDING_CODEC_RECONFIGURE);
   TRACE_EVENT0("media", "AVDA::DequeueOutput");
   base::AutoReset<bool> auto_reset(&defer_errors_, true);
   if (picturebuffers_requested_ && output_picture_buffers_.empty())
     return false;
 
   if (!output_picture_buffers_.empty() && free_picture_ids_.empty()) {
     // Don't have any picture buffer to send. Need to wait more.
     return false;
   }
 
@@ skipping 332 matching lines @@
     // 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;
 

[liberato (no reviews please), 2016/03/29 14:54:40]

should this early out here if |state_| is already PENDING_CODEC_RECONFIGURE?  i
think it works as written, but it seems like a good idea anyway.  none of this
code was written with the idea that !|media_coded_|.  no need to re-notify the
strategy, nor re-stop the timer.

i'm not sure that it's even possible to get into this state right now. maybe one
can via EOS due to format changed, followed by a Reset before that completes. 
either way, that's not obvious and somewhat fragile, so an explicit early out
after clearing the buffers might be worthwhile.

   // 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 &&
       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);
     media_codec_.reset();
+    strategy_->CodecChanged(media_codec_.get(), output_picture_buffers_);
     // Changing the codec will also notify the strategy to forget about any
     // output buffers it has currently.
-    state_ = NO_ERROR;
-    if (!ConfigureMediaCodec())
-      POST_ERROR(PLATFORM_FAILURE, "Failed to create MediaCodec.");
+    state_ = PENDING_CODEC_RECONFIGURE;

[liberato (no reviews please), 2016/03/29 14:54:40]

you might consider not deferring the reset in the case of a call to
ResetCodecState for a format change -- it's a slow process, and we know that
more decodes will happen.  might as well start sooner to hide the latency. 
right now, it doesn't make much difference, because it's all synchronous.  with
async init, it could actually proceed in parallel.

alternatively, please add a comment about it here, else i'll forget to change it
later.  :)

   }
 }
 
 void AndroidVideoDecodeAccelerator::DismissPictureBuffers() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(3) << __FUNCTION__;
 
   for (const auto& pb : output_picture_buffers_) {
     strategy_->DismissOnePictureBuffer(pb.second);
     client_->DismissPictureBuffer(pb.first);
@@ skipping 254 matching lines @@
     capabilities.flags = media::VideoDecodeAccelerator::Capabilities::
                              NEEDS_ALL_PICTURE_BUFFERS_TO_DECODE |
                          media::VideoDecodeAccelerator::Capabilities::
                              SUPPORTS_EXTERNAL_OUTPUT_SURFACE;
   }
 
   return capabilities;
 }
 
 }  // namespace content