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 d8da2982bf2a15844cbfc44da7556591c6edeadc..e9b61ff88f0f3adb7e32eab7a9790f4435588286 100644
--- a/content/common/gpu/media/android_video_decode_accelerator.cc
+++ b/content/common/gpu/media/android_video_decode_accelerator.cc
@@ -19,6 +19,7 @@
 #include "content/common/gpu/gpu_channel.h"
 #include "content/common/gpu/media/android_copying_backing_strategy.h"
 #include "content/common/gpu/media/android_deferred_rendering_backing_strategy.h"
+#include "content/common/gpu/media/avda_return_on_failure.h"
 #include "content/public/common/content_switches.h"
 #include "gpu/command_buffer/service/gles2_cmd_decoder.h"
 #include "gpu/command_buffer/service/gpu_switches.h"
@@ -48,6 +49,11 @@
     PostError(FROM_HERE, media::VideoDecodeAccelerator::error_code); \
   } while (0)
 
+// Override definition from avda_return_on_failure.h to s/state_provider_/this/.
+#undef RETURN_NULL_IF_NULL
+#define RETURN_NULL_IF_NULL(ptr, ...) \
+  RETURN_ON_FAILURE(this, ptr, "Got null for " << #ptr, ILLEGAL_STATE, nullptr);
+
 namespace content {
 
 enum { kNumPictureBuffers = media::limits::kMaxVideoFrames + 1 };
@@ -585,25 +591,30 @@ bool AndroidVideoDecodeAccelerator::DequeueOutput() {
         return false;
 
       case media::MEDIA_CODEC_OUTPUT_FORMAT_CHANGED: {
-        if (!output_picture_buffers_.empty()) {
-          // 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();
-        }
-
         if (media_codec_->GetOutputSize(&size_) != media::MEDIA_CODEC_OK) {
           POST_ERROR(PLATFORM_FAILURE, "GetOutputSize failed.");
           return false;
         }
+        DVLOG(3) << __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;
+        }
 
-        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:
@@ -709,14 +720,21 @@ 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;
   }
 
+  bool size_changed = false;
+  if (i->second.size() != size_) {
+    // Size may have changed due to resolution change since the last time this
+    // PictureBuffer was used.
+    strategy_->UpdatePictureBufferSize(&i->second, size_);

[liberato (no reviews please), 2016/03/07 16:39:17]

an alternate idea is to add a "const gfx::Size*" to UseCodecBuffer... which is
non-NULL if and only if we'd call UpdatePictureBufferSize.  not sure if it's
better or worse than what you have.  i mention it only because it could be
viewed as part of UseCodecBuffer's job.

UpdatePictureSize would become a private method on the copying strategy, and
could be inlined into UseCodecBuffer in the deferred strategy.

then again, you'd have to also send in the Picture.  that would be unwieldy,
unless you want to move the Picture-setting code here.

either way seems fine.

[chcunningham, 2016/03/08 03:19:36]

"then again, you'd have to also send in the Picture.  that would be unwieldy,"

Little confused here. Both UseCodecBufferForPictureBuffer and
UpdatePictureBuffer size take in a PictureBuffer. I we make UpdatePBSize
private, and call in the top of UseCodecBufferForPictureBuffer, we should be
good without ever needing a Picture right? Picture gets constructed below with
the new size below. 

[chcunningham, 2016/03/08 22:22:13]

We talked over chat - decided not to do this because its not a clear win for
readability.

[liberato (no reviews please), 2016/03/09 15:32:06]

discussed offline.  you're right, i misread the signature as 'Picture'.

+    size_changed = true;
+  }
+
   // Connect the PictureBuffer to the decoded frame, via whatever
   // mechanism the strategy likes.
   strategy_->UseCodecBufferForPictureBuffer(codec_buffer_index, i->second);
@@ -726,7 +744,8 @@ void AndroidVideoDecodeAccelerator::SendDecodedFrameToClient(
       FROM_HERE, base::Bind(&AndroidVideoDecodeAccelerator::NotifyPictureReady,
                             weak_this_factory_.GetWeakPtr(),
                             media::Picture(picture_buffer_id, bitstream_id,
-                                           gfx::Rect(size_), allow_overlay)));
+                                           gfx::Rect(size_), allow_overlay,
+                                           size_changed)));
 }
 
 void AndroidVideoDecodeAccelerator::Decode(
@@ -1001,6 +1020,20 @@ AndroidVideoDecodeAccelerator::GetGlDecoder() const {
   return gl_decoder_;
 }
 
+gpu::gles2::TextureRef* AndroidVideoDecodeAccelerator::GetTextureForPicture(
+    const media::PictureBuffer& picture_buffer) {
+  RETURN_NULL_IF_NULL(GetGlDecoder());
+  RETURN_NULL_IF_NULL(GetGlDecoder()->GetContextGroup());
+  gpu::gles2::TextureManager* texture_manager =
+      GetGlDecoder()->GetContextGroup()->texture_manager();
+  RETURN_NULL_IF_NULL(texture_manager);
+  gpu::gles2::TextureRef* texture_ref =
+      texture_manager->GetTexture(picture_buffer.internal_texture_id());
+  RETURN_NULL_IF_NULL(texture_ref);
+
+  return texture_ref;
+}
+
 void AndroidVideoDecodeAccelerator::OnFrameAvailable() {
   // Remember: this may be on any thread.
   DCHECK(strategy_);