Fix Android black frames from MSE config changes.

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 7deb6f3dc81cbe36be666f2354ac8591e5b71950..65710ed1389523f26e359aa752b63f895f796c81 100644
--- a/content/common/gpu/media/android_video_decode_accelerator.cc
+++ b/content/common/gpu/media/android_video_decode_accelerator.cc
@@ -574,23 +574,29 @@ bool AndroidVideoDecodeAccelerator::DequeueOutput() {
         return false;
 
       case media::MEDIA_CODEC_OUTPUT_FORMAT_CHANGED: {
-        if (!output_picture_buffers_.empty()) {

[liberato (no reviews please), 2016/03/01 17:13:08]

i prefer that you ignore this comment, but i leave it in case the info is
useful.

for the copying strategy, there is another option.  we could be asymmetric about
when we dismiss picture buffers between strategies.  in particular, one chould
continue to dismiss picture buffers when FORMAT_CHANGED shows up for the copying
strategy, but not for the deferred strategy.  all the frames that have been sent
to the pipeline as VideoFrames, but not returned yet, will be kept alive
automatically by GVD in the copying case.

i'm not sure there's an advantage to doing this over the method described below,
but it's good to have options.

-          // TODO(chcunningham): This will likely dismiss a handful of decoded
-          // frames that have not yet been drawn and returned to us for re-use.
-          // Consider a more complicated design that would wait for them to be
-          // drawn before dismissing.
-          DismissPictureBuffers();
-        }
-
-        picturebuffers_requested_ = true;
         int32_t width, height;
         media_codec_->GetOutputFormat(&width, &height);
         size_ = gfx::Size(width, height);
-        base::MessageLoop::current()->PostTask(
-            FROM_HERE,
-            base::Bind(&AndroidVideoDecodeAccelerator::RequestPictureBuffers,
-                       weak_this_factory_.GetWeakPtr()));
-        return false;
+        DVLOG(4) << __FUNCTION__
+                 << " OUTPUT_FORMAT_CHANGED, new size: " << size_.ToString();
+
+        // Don't request picture buffers if we already have some. This avoids
+        // having to dismiss the existing buffers which may actively reference
+        // decoded images. Breaking their connection to the decoded image will
+        // cause rendering of black frames. Instead, we let the existing
+        // PictureBuffers live on and we simply update their size the next time
+        // they're attachted to an image of the new resolution. See the
+        // size update in |SendDecodedFrameToClient| and https://crbug/587994.
+        if (output_picture_buffers_.empty()) {
+          picturebuffers_requested_ = true;
+          base::MessageLoop::current()->PostTask(
+              FROM_HERE,
+              base::Bind(&AndroidVideoDecodeAccelerator::RequestPictureBuffers,
+                         weak_this_factory_.GetWeakPtr()));
+          return false;
+        }
+
+        return true;
       }
 
       case media::MEDIA_CODEC_OUTPUT_BUFFERS_CHANGED:
@@ -685,14 +691,24 @@ void AndroidVideoDecodeAccelerator::SendDecodedFrameToClient(
   free_picture_ids_.pop();
   TRACE_COUNTER1("media", "AVDA::FreePictureIds", free_picture_ids_.size());
 
-  OutputBufferMap::const_iterator i =
-      output_picture_buffers_.find(picture_buffer_id);
+  OutputBufferMap::iterator i = output_picture_buffers_.find(picture_buffer_id);
   if (i == output_picture_buffers_.end()) {
     POST_ERROR(PLATFORM_FAILURE,
                "Can't find PictureBuffer id: " << picture_buffer_id);
     return;
   }
 
+  if (i->second.size() != size_) {
+    // Size may have changed due to resolution change since the last time this
+    // PictureBuffer was used. The size of the PictureBuffer is purely
+    // meta-data and is not associated with OES texture it refers to.

[liberato (no reviews please), 2016/03/01 17:13:08]

whether it's an external texture or not depends on the backing strategy.  for
the copying strategy, it won't be.  PictureBuffer textures are 2D, not external,
when we're using the copying strategy.

also, for the copying strategy, updating just the metadata won't be enough.  we
have to bind a big enough 2D texture to hold the copy.  however, if we dismiss /
re-acquire picture buffers in the copying case (as suggested in another comment
as the 'asymmetric' case), then this will happen automatically and no action is
needed.

if you decide to do it that way, then having a size mismatch here is a DCHECK
when using the copying strategy.

if, instead, you want to keep deferred / copying strategies symmetric and never
dismiss picture buffers on FORMAT_CHANGED, then one can still fix up the
textures here for the copying strategy.  one could
strategy::UpdatePictureBufferSize(picture_buffer, new_size).  deferred would do
nothing.  copying would issue a glTexImage2D, and also update the
gpu::gles2::Texture object to reflect the new size.

the code @709 would stay here (same for both copying and deferred), as would the
additional code to get GVD's PictureBuffer size in sync. this is described @709.

[chcunningham, 2016/03/01 19:54:25]

I like this symmetric proposal better. Patch coming shortly.

+    DVLOG (3) << __FUNCTION__
+                << " Updating size for PictureBuffer[" << i->first << "]"
+                << " from:" << i->second.size().ToString()
+                << " to:" << size_.ToString();
+    i->second.set_size(size_);

[liberato (no reviews please), 2016/03/01 17:13:08]

i don't think that this is enough to get the size metadata updated everywhere.

the PictureBuffers are created by the GVD in response to ProvidePictureBuffers
call from us.  each is assigned an integer handle.  after that, we only ever use
that integer handle to refer to them; we never send a PictureBuffer back to GVD
via IPC.  instead, we send back a Picture object (not PictureBuffer) on every
frame that references which PictureBuffer contains the image data.

this CL will end up changing (a) the size in our local PictureBuffer structure,
and (b) the visible size of the Picture in GVD.  it doesn't update the
PictureBuffer size in GVD, as far as i can tell.  see
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_...
.

it can probably be done via a flag on Picture that tells the GVD to copy the
size back into its copy of the PictureBuffer.

+  }
+
   // Connect the PictureBuffer to the decoded frame, via whatever
   // mechanism the strategy likes.
   strategy_->UseCodecBufferForPictureBuffer(codec_buffer_index, i->second);