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

media/gpu/android_video_decode_accelerator.cc

Index: media/gpu/android_video_decode_accelerator.cc
diff --git a/media/gpu/android_video_decode_accelerator.cc b/media/gpu/android_video_decode_accelerator.cc
index a46176d0502a8b752ebd44d6fc4ba255def771e3..165b68bebc5b1f0387f6c81597f6d21e864bd391 100644
--- a/media/gpu/android_video_decode_accelerator.cc
+++ b/media/gpu/android_video_decode_accelerator.cc
@@ -1185,18 +1185,23 @@ void AndroidVideoDecodeAccelerator::ResetCodecState(
   }
 
   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
@@ -1211,8 +1216,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.

[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
@@ -1222,9 +1226,12 @@ void AndroidVideoDecodeAccelerator::ResetCodecState(
     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())