Make AVDA errors terminal; don't destruct until Decode() for Reset().

media/gpu/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 "media/gpu/android_video_decode_accelerator.h"
 
 #include <stddef.h>
 
 #include <memory>
 
@@ skipping 1167 matching lines @@
   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;
   }
 
   const bool did_codec_error_happen = state_ == ERROR;
-  state_ = NO_ERROR;
+  if (!did_codec_error_happen)
+    state_ = NO_ERROR;
 
-  // Don't reset the codec here if there's no error and we're only flushing;
-  // instead defer until the next decode call; this prevents us from unbacking
-  // frames that might be out for display at end of stream.
-  codec_needs_reset_ = false;
-  if (drain_type_ == DRAIN_FOR_FLUSH && !did_codec_error_happen) {
+  // Don't reset the codec here if there's no error and we're only flushing or
+  // we have to destruct the codec to reset; instead defer until the next decode
+  // call; this prevents us from unbacking frames that might be out for display
+  // at end of stream or reconstructing a codec which is never used.
+  const bool codec_requires_destruction =
+      base::android::BuildInfo::GetInstance()->sdk_int() <= 18;
+  if (!codec_needs_reset_ && !did_codec_error_happen &&
+      (codec_requires_destruction || drain_type_ == DRAIN_FOR_FLUSH)) {
     codec_needs_reset_ = true;
     if (!done_cb.is_null())
       done_cb.Run();
     return;
   }
+  codec_needs_reset_ = false;

[liberato (no reviews please), 2016/06/02 22:10:44]

the idea here is to make the first call defer, and the second call wait, right?

it might be better to send in a flag with each call "may defer reset / new
codec", so that Decode can indicate that it always wants a ready-to-use codec. 
it seems more clear.

 
   // 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.

[watk, 2016/05/26 20:53:28]

Could probably delete this whole comment since it's talking about recovering
from errors other than fullscreen transitions. I don't think we need to consider
that any 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.

[watk, 2016/05/26 20:53:28]

Do you think this comment belongs on codec_requires_destruction?

-  if (!did_codec_error_happen &&
-      base::android::BuildInfo::GetInstance()->sdk_int() >= 18) {
+  if (!codec_requires_destruction && !did_codec_error_happen) {
     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());
   } else {
     DVLOG(3) << __FUNCTION__
              << " Deleting the MediaCodec and creating a new one.";
     g_avda_timer.Pointer()->StopTimer(this);

[watk, 2016/05/26 20:53:28]

unrelated, but it's a bit weird that the other branch doesn't also stop the
timer..

[liberato (no reviews please), 2016/06/02 22:10:44]

i don't think that it should.  the next decode will restart it, but i don't
think that we want to depend on that.  it'll be okay now since there aren't any
cases where we'd need the timer and decoding is paused waiting for flush, i
think.  but, soon, there may be for switching SV <-> ST.  in that case, we don't
want to stop the timer in the reset case.

in this branch, it's fine.  when the codec is ready, it'll restart it in
OnCodecConfigured.

-    // Changing the codec will also notify the strategy to forget about any
-    // output buffers it has currently.
-    ConfigureMediaCodecAsynchronously();
+
+    // Only recreate the codec if we're not in the error state.
+    media_codec_.reset();
+    strategy_->CodecChanged(nullptr);
+    if (!did_codec_error_happen)
+      ConfigureMediaCodecAsynchronously();
   }
 
   if (!done_cb.is_null())
     done_cb.Run();
 }
 
 void AndroidVideoDecodeAccelerator::Reset() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("media", "AVDA::Reset");
@@ skipping 375 matching lines @@
     if (media::MediaCodecUtil::IsSurfaceViewOutputSupported()) {
       capabilities.flags |= media::VideoDecodeAccelerator::Capabilities::
           SUPPORTS_EXTERNAL_OUTPUT_SURFACE;
     }
   }
 
   return capabilities;
 }
 
 }  // namespace media