Speculative fix for CodecImage crash.

It seems like CodecImages are sometimes being used for drawing after
they have been marked as unused for the MCVD shared image pool.  It
is likely that this happens when the renderer is torn down before
receiving returns from viz (e.g., renderer crash).

This CL causes CodecImage to handle these cases gracefully, rather
than crash.

Bug: 1010621
Change-Id: Ibd25fd13dc477f1702df2b11a3c4ff46413f0f3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1853113Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705689}

media/gpu/android/codec_image.cc

@@ -150,6 +150,11 @@ bool CodecImage::ScheduleOverlayPlane(
 
 void CodecImage::NotifyOverlayPromotion(bool promotion,
                                         const gfx::Rect& bounds) {
+  // Use-after-release.  It happens if the renderer crashes before getting
+  // returns from viz.
+  if (!promotion_hint_cb_)
+    return;
+
   if (!codec_buffer_wait_coordinator_ && promotion) {
     // When |CodecImage| is already backed by SurfaceView, and it should be used
     // as overlay.
@@ -339,7 +344,12 @@ void CodecImage::ReleaseCodecBuffer() {
 
 std::unique_ptr<base::android::ScopedHardwareBufferFenceSync>
 CodecImage::GetAHardwareBuffer() {
-  DCHECK(codec_buffer_wait_coordinator_);
+  // It would be nice if this didn't happen, but we can be incorrectly marked
+  // as free when viz is still using us for drawing.  This can happen if the
+  // renderer crashes before receiving returns.  It's hard to catch elsewhere,
+  // so just handle it gracefully here.
+  if (!codec_buffer_wait_coordinator_)
+    return nullptr;
 
   RenderToTextureOwnerFrontBuffer(BindingsMode::kDontRestoreIfBound);
   return codec_buffer_wait_coordinator_->texture_owner()->GetAHardwareBuffer();

media/gpu/android/codec_image_unittest.cc

@@ -404,4 +404,11 @@ TEST_F(CodecImageTest, GetAHardwareBuffer) {
   EXPECT_TRUE(i->was_rendered_to_front_buffer());
 }
 
+TEST_F(CodecImageTest, GetAHardwareBufferAfterRelease) {
+  // Make sure that we get a nullptr AHB once we've marked the image as unused.
+  auto i = NewImage(kTextureOwner);
+  i->NotifyUnused();
+  EXPECT_FALSE(i->GetAHardwareBuffer());
+}
+
 }  // namespace media

media/gpu/android/video_frame_factory_impl.cc

@@ -324,6 +324,26 @@ void VideoFrameFactoryImpl::CreateVideoFrame_Finish(
   frame->metadata()->SetBoolean(VideoFrameMetadata::TEXTURE_OWNER,
                                 !!codec_buffer_wait_coordinator);
 
+  // TODO(liberato): if this is run via being dropped, then it would be nice
+  // to find that out rather than treating the image as unused.  If the renderer
+  // is torn down, then this will be dropped rather than run.  While |provider_|
+  // allows this, it doesn't have enough information to understand if the image
+  // is free or not.  The problem only really affects the pool, since the
+  // direct provider destroys the SharedImage which works in either case.  Any
+  // use of the image (e.g., if viz is still using it after the renderer has
+  // been torn down unexpectedly), will just not draw anything.  That's fine.
+  //
+  // However, the pool will try to re-use the image, so the SharedImage remains
+  // valid.  However, it's not a good idea to draw with it until the CodecImage
+  // is re-initialized with a new frame.  If the renderer is torn down without
+  // getting returns from viz, then the pool does the wrong thing.  However,
+  // the pool really doesn't know anything about VideoFrames, and dropping the
+  // callback does, in fact, signal that it's unused now (as described in the
+  // api).  So, we probably should wrap the release cb in a default invoke, and
+  // if the default invoke happens, do something.  Unclear what, though.  Can't
+  // move it into the CodecImage (might hold a ref to the CodecImage in the cb),
+  // so it's unclear.  As it is, CodecImage just handles the case where it's
+  // used after release.
   frame->SetReleaseMailboxCB(std::move(record.release_cb));
 
   // Note that we don't want to handle the CodecImageGroup here.  It needs to be