Don't reset the codec state for a flush; this kills the frames.

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 365 matching lines @@
       get_gles2_decoder_cb_(get_gles2_decoder_cb),
       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),
+      codec_needs_reset_(false),
       weak_this_factory_(this) {}
 
 AndroidVideoDecodeAccelerator::~AndroidVideoDecodeAccelerator() {
   DCHECK(thread_checker_.CalledOnValidThread());
   g_avda_timer.Pointer()->StopTimer(this);
   g_avda_timer.Pointer()->StopThread(this);
 
 #if defined(ENABLE_MOJO_MEDIA_IN_GPU_PROCESS)
   if (!media_drm_bridge_cdm_context_)
     return;
@@ skipping 497 matching lines @@
 
   // Connect the PictureBuffer to the decoded frame, via whatever mechanism the
   // strategy likes.
   strategy_->UseCodecBufferForPictureBuffer(codec_buffer_index, i->second);
 }
 
 void AndroidVideoDecodeAccelerator::Decode(
     const media::BitstreamBuffer& bitstream_buffer) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  // If we previously deferred a codec restart, take care of it now. This can
+  // happen on older devices where configuration changes require a codec reset.
+  if (codec_needs_reset_)
+    ResetCodecState(base::Closure());

[Tima Vaisburd, 2016/05/21 06:40:31]

Did you mean ConfigureMediaCodecAsynchronously() ? What else from
ResetCodecState() would have to be done here?

Set codec_needs_reset_ back to false?

[DaleCurtis, 2016/05/24 00:35:29]

ResetCodecState() does all the necessary things: Configure() and clears
|codec_needs_reset_|.

+
   if (bitstream_buffer.id() >= 0 && bitstream_buffer.size() > 0) {
     DecodeBuffer(bitstream_buffer);
     return;
   }
 
   if (base::SharedMemory::IsHandleValid(bitstream_buffer.handle()))
     base::SharedMemory::CloseHandle(bitstream_buffer.handle());
 
   if (bitstream_buffer.id() < 0) {
     POST_ERROR(INVALID_ARGUMENT,
@@ skipping 68 matching lines @@
   if (i == output_picture_buffers_.end()) {
     POST_ERROR(PLATFORM_FAILURE, "Can't find PictureBuffer id "
                                      << picture_buffer_id);
     return;
   }
 
   strategy_->ReuseOnePictureBuffer(i->second);
   DoIOTask(true);
 }
 
 void AndroidVideoDecodeAccelerator::Flush() {

[Tima Vaisburd, 2016/05/21 06:40:31]

Is this flush()  exists solely for the purpose to cope with dynamic resolution
change?
If so, then MediaCodec.flush() would not be needed after the drain for Android
>= K, indeed, and do we even need to drain? In other words, we either need to
drain because we need to recreate decoder after that, or we can
NotifyFlushDone() immediately. Does it sound right?

   DVLOG(1) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (state_ == SURFACE_DESTROYED)
     NotifyFlushDone();
   else
     StartCodecDrain(DRAIN_FOR_FLUSH);
 }
 
 void AndroidVideoDecodeAccelerator::ConfigureMediaCodecAsynchronously() {
@@ skipping 170 matching lines @@
     // 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;
 
+  // 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.
+  const bool codec_requires_reset_for_config_changes =

[chcunningham, 2016/05/21 00:25:33]

It used to be that

api >= 18 -> quick flush
api <  18 -> new media codec

Tima's comments seem to be that KitKat (19) is the starting point of good
adaptive support, so I think you want this check to be < 19? As it is, you will
not do the reset for JBMR2.

If I have this right, you still need to separately check >= 18 to decide when to
do a quick flush vs make a whole new media codec.

[DaleCurtis, 2016/05/24 00:35:29]

Actually, this needs to always reset since once we queue the EOS for flush
there's no turning back. In the future we can try to figure out when to flush vs
when to reset and how adaptive playback might be better supported.

+      base::android::BuildInfo::GetInstance()->sdk_int() < 18;
+
+  // Don't reset the codec if there's no error and we're only flushing. On older
+  // JellyBean devices we'll defer the reset until we get another Decode() call
+  // so that we don't prematurely nuke the last few frames of video.
+  codec_needs_reset_ = false;
+  if (drain_type_ == DRAIN_FOR_FLUSH && !did_codec_error_happen) {
+    codec_needs_reset_ = codec_requires_reset_for_config_changes;
+    if (!done_cb.is_null())
+      done_cb.Run();
+    return;
+  }
+
   // 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 (!did_codec_error_happen &&
-      base::android::BuildInfo::GetInstance()->sdk_int() >= 18) {
+  if (!did_codec_error_happen && !codec_requires_reset_for_config_changes) {
     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);
     // Changing the codec will also notify the strategy to forget about any
@@ skipping 387 matching lines @@
     if (media::MediaCodecUtil::IsSurfaceViewOutputSupported()) {
       capabilities.flags |= media::VideoDecodeAccelerator::Capabilities::
           SUPPORTS_EXTERNAL_OUTPUT_SURFACE;
     }
   }
 
   return capabilities;
 }
 
 }  // namespace media