Fix MP4 mid-stream resolution changes for MSE on android spitzer.

content/common/gpu/media/android_video_decode_accelerator.cc

Index: content/common/gpu/media/android_video_decode_accelerator.cc
diff --git a/content/common/gpu/media/android_video_decode_accelerator.cc b/content/common/gpu/media/android_video_decode_accelerator.cc
index 2cc7c32c14d5d21f49b46ab24e38942bb15db94a..b67410a2c69836eb40e8498a702f43e570b268a6 100644
--- a/content/common/gpu/media/android_video_decode_accelerator.cc
+++ b/content/common/gpu/media/android_video_decode_accelerator.cc
@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include "base/android/build_info.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
@@ -332,8 +333,8 @@ bool AndroidVideoDecodeAccelerator::DequeueOutput() {
           // Dynamic resolution change support is not specified by the Android
           // platform at and before JB-MR1, so it's not possible to smoothly
           // continue playback at this point.  Instead, error out immediately,
-          // expecting clients to Reset() as appropriate to avoid this.
-          // b/7093648
+          // expecting clients to Flush() or Reset() as appropriate to avoid
+          // this. b/7093648
           RETURN_ON_FAILURE(this, size_ == gfx::Size(width, height),
                             "Dynamic resolution change is not supported.",
                             PLATFORM_FAILURE, false);
@@ -368,7 +369,9 @@ bool AndroidVideoDecodeAccelerator::DequeueOutput() {
                 : bitstream_buffers_in_decoder_.find(presentation_timestamp);
 
   if (it == bitstream_buffers_in_decoder_.end()) {
-    media_codec_->ReleaseOutputBuffer(buf_index, false);
+    DVLOG(3) << "AVDA::DequeueOutputBuffer: Resetting output state after EOS";
+    ResetOutputState();

[liberato (no reviews please), 2016/01/06 18:06:32]

we don't know that this is eos.  we might not have the bitstream buffer for the
presentation timestamp.  we don't want to ResetOutputState unless it's eos.

something like if(eos) { do this stuff } else if(it!=end) { do the else existing
clause } else should_try_again = true;

[chcunningham, 2016/01/07 02:14:48]

Done. Added a log and made sure to ReleaseOutputBuffer whenever we don't find
the   timestamp.

+
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&AndroidVideoDecodeAccelerator::NotifyFlushDone,
@@ -537,6 +540,39 @@ bool AndroidVideoDecodeAccelerator::ConfigureMediaCodec() {
   return true;
 }
 
+void AndroidVideoDecodeAccelerator::ResetOutputState() {

[liberato (no reviews please), 2016/01/06 18:06:32]

maybe ResetCodecState, or maybe ResetOrReplaceCodec?

[chcunningham, 2016/01/07 02:14:48]

Made this ResetCodecState

+  for (OutputBufferMap::iterator it = output_picture_buffers_.begin();

[watk, 2016/01/06 20:05:51]

Range based for-loop opportunity!

[chcunningham, 2016/01/07 02:14:48]

Done.

+       it != output_picture_buffers_.end(); ++it) {
+    strategy_->DismissOnePictureBuffer(it->second);

[liberato (no reviews please), 2016/01/06 18:06:32]

this will immediately make the images undrawable, including the most recent
frame.  DismissOnePictureBuffer disconnects the image from the texture, so all
future draw requests will fail even if they are in the SurfaceTexture's front
buffer already.  you might want to do two things:

Remove the call to SetImageForPicture in deferred::DismissOnePictureBuffer. 
This will keep the SurfaceTexture's front buffer attached.  The codec buffer
will still be released immediately, however, which is good.

Second, you might want to count how many of these picture buffers still have
codec buffers attached in DismissOne.  that gives us an idea of how many frames
are being affected.  This doesn't have to be part of the CL, but it would be
nice to have some measurement.  You can just count it in
ReleaseCodecBufferForPicture, but be sure to only count calls from DismissOne. 
There is a call to ReleaseCodecBufferForPicture from ReuseOnePictureBuffer that
we don't want to count.

[chcunningham, 2016/01/07 02:14:48]

SetImageForPicture appears to be a no-op whenever the image is 0. I don't see
that it actually unhooks anything?

But lets say it does do some unhooking and I stop calling it so that it can
remain attached to SurfaceTexture's front buffer... potentially n00b question:
how can we be sure that MediaCodec won't write to this front texture while we're
actively showing the frame? We've Released the ID to the codec, so shouldn't it
feel free to re-use the texture associated with that ID? Is there a potential
for graphics glitches if this occurs?

Dan and I hashed this out a bunch (I've still got a ton to learn about GL and
the gpu parts of our pipeline). His feeling is that this is that this is
unlikely to be a problem because Android likely does some ref counting on this
memory and will find that SurfaceTexture still holds a ref (front buffer) and
will not truly free it for writing until all refs are detached. This makes sense
to me, but I'm nervous and haven't seen mention of this in the docs. 

Going back to our design chat, we were worried that waiting for
AVDA::ReusePictureBuffer for all buffers would block forever as someone (not
sure who?) is likely to hold onto one. Questions:

1) Shouldn't AVDA reporting NEEDS_ALL_PICTURE_BUFFERS_TO_DECODE mean we can
expect to get them all back? Tracing this code path isn't trivial... can you
point me at the component you think may hold onto buffers?

2) If we really don't get all the PictureBuffers back, Dan mentioned we might be
able to do something like post a task whenever we UpdateTheSurfaceTexture and
basically wait for all the PictureBuffer's to be *used* before
MediaCodec.flush/Reset. WDYT
Happy to count. Do you just want a dvlog everytime this occurs? 

[liberato (no reviews please), 2016/01/07 14:48:55]

doesn't do anything for null: wow!  well then, i'd not worry about it.  good
catch.  not sure if it's a mistake or there's some subtle reason that i'm
forgetting.  probably best not to touch it.

front buffer issues: luckily, there's no issue.  the front buffer isn't
accessible to media codec.  i can go into more detail if you like next time i'm
in seattle (probably friday).

getting back picture buffers: i'm not sure if the last buffer is subject to
this, or how long it might take.  i think that VideoFrameCompositor might latch
it.  i'm really not sure though.

1) Personally, i wouldn't bother.  i think that your current approach of 'keep
the front buffer valid' is probably fine -- that's why i suggested counting the
number of unused decoder buffers.  it will give us an idea of whether we really
have a problem.  if we do, then we can figure out why.  but my first guesses
would be VideoFrameCompositor or VideoRendererAlgorithm, to answer your
question.

2) Yes -- i was thinking about this too, and i also believe that checking for
used is sufficient.  however, i'm not sure that it's necessary that we do
anything.

[chcunningham, 2016/01/08 22:49:36]

SGTM. I added a temporary count of dismissed-frames-in-use and found the count
is consistently 4 frames for both JB and K. Added a TODO in ResetCodecState to
describe potential future work.

+    client_->DismissPictureBuffer(it->first);
+    dismissed_picture_ids_.insert(it->first);
+  }
+  output_picture_buffers_.clear();
+  std::queue<int32_t> empty;
+  std::swap(free_picture_ids_, empty);
+  CHECK(free_picture_ids_.empty());

[watk, 2016/01/06 20:05:51]

Heh, this is a paranoid check.

[chcunningham, 2016/01/07 02:14:48]

Are you suggesting a downgrade to DCHECK?

+  picturebuffers_requested_ = false;
+  bitstream_buffers_in_decoder_.clear();
+
+  // 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 stop() and reconfigure MediaCodec. The
+  // full reconfigure is much slower and may cause visible freezing if done
+  // mid-stream.
+  if (state_ == NO_ERROR &&

[liberato (no reviews please), 2016/01/06 18:06:32]

not sure that it matters if |state_| is NO_ERROR or not.  or, conversely, if it
does matter, it's unclear that ConfigureMediaCodec would fix it while Reset
would not.

from a quick check, i think that it's okay to ignore |state_| and
unconditionally set it to NO_ERROR.

[chcunningham, 2016/01/07 02:14:48]

My thinking here comes from the state diagram in the MediaCodec docs. When we
hit an error we enter the "Stopped" mega-state and we need to call
MediaCodec.reset() to make the codec usable again... flush() won't cut it. I
think the ConfigureMediaCodec probably effectively resets things in that it
makes a completely new one. 

+      base::android::BuildInfo::GetInstance()->sdk_int() >= 18) {
+    DVLOG(3) << __FUNCTION__ << " Doing fast MediaCodec reset (flush).";
+    media_codec_->Reset();
+  } else {
+    DVLOG(3) << __FUNCTION__
+             << " Doing slow MediaCodec reset (stop/re-configure).";
+    io_timer_.Stop();
+    media_codec_->Stop();
+    ConfigureMediaCodec();

[liberato (no reviews please), 2016/01/06 18:06:32]

is this going to be deferred as part of sandersd@ work?  i don't think that we
want to re-allocate the codec if we don't know if another bitstream buffer is
coming.  as in, release it here and allocate it later.

for this CL, it's fine as it is.  just making sure we're all in sync.

[chcunningham, 2016/01/07 02:14:48]

sanders@, comment on your plans?

Agree, lazy would be better. 

+    state_ = NO_ERROR;
+  }
+}
+
 void AndroidVideoDecodeAccelerator::Reset() {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("media", "AVDA::Reset");
@@ -555,28 +591,7 @@ void AndroidVideoDecodeAccelerator::Reset() {
   TRACE_COUNTER1("media", "AVDA::PendingBitstreamBufferCount", 0);
   bitstreams_notified_in_advance_.clear();
 
-  for (OutputBufferMap::iterator it = output_picture_buffers_.begin();
-       it != output_picture_buffers_.end();
-       ++it) {
-    strategy_->DismissOnePictureBuffer(it->second);
-    client_->DismissPictureBuffer(it->first);
-    dismissed_picture_ids_.insert(it->first);
-  }
-  output_picture_buffers_.clear();
-  std::queue<int32_t> empty;
-  std::swap(free_picture_ids_, empty);
-  CHECK(free_picture_ids_.empty());
-  picturebuffers_requested_ = false;
-  bitstream_buffers_in_decoder_.clear();
-
-  // On some devices, and up to at least JB-MR1,
-  // - flush() can fail after EOS (b/8125974); and
-  // - mid-stream resolution change is unsupported (b/7093648).
-  // To cope with these facts, we always stop & restart the codec on Reset().
-  io_timer_.Stop();
-  media_codec_->Stop();
-  ConfigureMediaCodec();
-  state_ = NO_ERROR;
+  ResetOutputState();
 
   base::MessageLoop::current()->PostTask(
       FROM_HERE,