Fix Android black frames from MSE config changes.

content/common/gpu/media/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 "content/common/gpu/media/android_video_decode_accelerator.h"
 
 #include <stddef.h>
 
 #include "base/android/build_info.h"
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/trace_event/trace_event.h"
 #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"
 #include "gpu/command_buffer/service/mailbox_manager.h"
 #include "media/base/android/media_codec_bridge.h"
 #include "media/base/android/media_codec_util.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/bitstream_buffer.h"
 #include "media/base/limits.h"
 #include "media/base/media.h"
 #include "media/base/media_switches.h"
 #include "media/base/timestamp_constants.h"
 #include "media/base/video_decoder_config.h"
 #include "media/video/picture.h"
 #include "ui/gl/android/scoped_java_surface.h"
 #include "ui/gl/android/surface_texture.h"
 #include "ui/gl/gl_bindings.h"
 
 #if defined(ENABLE_MOJO_MEDIA_IN_GPU_PROCESS)
 #include "media/base/media_keys.h"
 #include "media/mojo/services/mojo_cdm_service.h"
 #endif
 
 #define POST_ERROR(error_code, error_message)                        \
   do {                                                               \
     DLOG(ERROR) << error_message;                                    \
     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 };
 
 // Max number of bitstreams notified to the client with
 // NotifyEndOfBitstreamBuffer() before getting output from the bitstream.
 enum { kMaxBitstreamsNotifiedInAdvance = 32 };
 
 // MediaCodec is only guaranteed to support baseline, but some devices may
 // support others. Advertise support for all H264 profiles and let the
@@ skipping 517 matching lines @@
 
     switch (status) {
       case media::MEDIA_CODEC_ERROR:
         POST_ERROR(PLATFORM_FAILURE, "DequeueOutputBuffer failed.");
         return false;
 
       case media::MEDIA_CODEC_DEQUEUE_OUTPUT_AGAIN_LATER:
         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();
 
-        picturebuffers_requested_ = true;
-        base::MessageLoop::current()->PostTask(
-            FROM_HERE,
-            base::Bind(&AndroidVideoDecodeAccelerator::RequestPictureBuffers,
-                       weak_this_factory_.GetWeakPtr()));
-        return false;
+        // 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:
         break;
 
       case media::MEDIA_CODEC_OK:
         DCHECK_GE(buf_index, 0);
         DVLOG(3) << __FUNCTION__ << ": pts:" << presentation_timestamp
                  << " buf_index:" << buf_index << " offset:" << offset
                  << " size:" << size << " eos:" << eos;
@@ skipping 85 matching lines @@
 
   if (!make_context_current_.Run()) {
     POST_ERROR(PLATFORM_FAILURE, "Failed to make the GL context current.");
     return;
   }
 
   int32_t picture_buffer_id = free_picture_ids_.front();
   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);
 
   const bool allow_overlay = strategy_->ArePicturesOverlayable();
   base::MessageLoop::current()->PostTask(
       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(
     const media::BitstreamBuffer& bitstream_buffer) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (bitstream_buffer.id() >= 0 && bitstream_buffer.size() > 0) {
     DecodeBuffer(bitstream_buffer);
     return;
   }
@@ skipping 254 matching lines @@
 const base::ThreadChecker& AndroidVideoDecodeAccelerator::ThreadChecker()
     const {
   return thread_checker_;
 }
 
 base::WeakPtr<gpu::gles2::GLES2Decoder>
 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_);
   strategy_->OnFrameAvailable();
 }
 
 void AndroidVideoDecodeAccelerator::PostError(
     const ::tracked_objects::Location& from_here,
     media::VideoDecodeAccelerator::Error error) {
   base::MessageLoop::current()->PostDelayedTask(
@@ skipping 134 matching lines @@
     capabilities.flags = media::VideoDecodeAccelerator::Capabilities::
                              NEEDS_ALL_PICTURE_BUFFERS_TO_DECODE |
                          media::VideoDecodeAccelerator::Capabilities::
                              SUPPORTS_EXTERNAL_OUTPUT_SURFACE;
   }
 
   return capabilities;
 }
 
 }  // namespace content