Moved CodecImage init from image provider to VideoFrameFactoryImpl.

VideoFrameFactoryImpl can call CodecImage::Initialize when it gets
the CodecImage from the SharedImageVideoProvider.  Previously, it
forwarded the output buffer, etc., to the provider instead.

The reason that the provider was responsible for it was to hide the
fact that a CodecImage was involved at all; this is an
implementation detail of the current SharedImageVideo.  However,
since VideoFrameFactory already has to have the CodecImage for early
rendering, it might as well handle initialization too.

If we can deprecate early rendering, then we could just provide a
callback in the ImageRecord instead, to hide CodecImage entirely.

When we introduce image pooling, it's convenient to allocate images
from a provider without actually initializing them at all; the pool
can handle it when it hands them out to clients.

Change-Id: I6a7c9d61306fd126cb77ba087f82e5a6ccb8d968
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1707549
Commit-Queue: Frank Liberato <liberato@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678876}

media/gpu/BUILD.gn

@@ -473,6 +473,8 @@ source_set("android_video_decode_accelerator_unittests") {
       "android/mock_abstract_texture.h",
       "android/mock_android_video_surface_chooser.cc",
       "android/mock_android_video_surface_chooser.h",
+      "android/mock_codec_image.cc",
+      "android/mock_codec_image.h",
       "android/mock_device_info.cc",
       "android/mock_device_info.h",
       "android/mock_promotion_hint_aggregator.cc",

media/gpu/android/codec_image.h

@@ -118,6 +118,10 @@ class MEDIA_GPU_EXPORT CodecImage : public gpu::gles2::GLStreamTextureImage {
   // Release any codec buffer without rendering, if we have one.
   virtual void ReleaseCodecBuffer();
 
+  CodecOutputBuffer* get_codec_output_buffer_for_testing() const {
+    return output_buffer_.get();
+  }
+
  protected:
   ~CodecImage() override;
 

media/gpu/android/codec_image_group_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/threading/thread.h"
 #include "media/base/android/mock_android_overlay.h"
 #include "media/gpu/android/codec_surface_bundle.h"
+#include "media/gpu/android/mock_codec_image.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -40,17 +41,6 @@ class CodecImageGroupWithDestructionHook : public CodecImageGroup {
   base::OnceClosure destruction_cb_;
 };
 
-// CodecImage with a mocked ReleaseCodecBuffer.
-class MockCodecImage : public CodecImage {
- public:
-  MockCodecImage() = default;
-
-  MOCK_METHOD0(ReleaseCodecBuffer, void());
-
- protected:
-  ~MockCodecImage() override {}
-};
-
 }  // namespace
 
 class CodecImageGroupTest : public testing::Test {

media/gpu/android/direct_shared_image_video_provider.cc

@@ -73,14 +73,7 @@ void DirectSharedImageVideoProvider::Initialize(GpuInitCB gpu_init_cb) {
 void DirectSharedImageVideoProvider::RequestImage(
     ImageReadyCB cb,
     const ImageSpec& spec,
-    std::unique_ptr<CodecOutputBuffer> output_buffer,
-    scoped_refptr<TextureOwner> texture_owner,
-    PromotionHintAggregator::NotifyPromotionHintCB promotion_hint_cb) {
-  auto image_ready_cb = BindToCurrentLoop(
-      base::BindOnce(&DirectSharedImageVideoProvider::OnImageReady,
-                     std::move(cb), std::move(output_buffer), texture_owner,
-                     std::move(promotion_hint_cb), gpu_task_runner_));
-
+    scoped_refptr<TextureOwner> texture_owner) {
   // It's unclear that we should handle the image group, but since CodecImages
   // have to be registered on it, we do.  If the CodecImage is ever re-used,
   // then part of that re-use would be to call the (then mis-named)
@@ -91,25 +84,8 @@ void DirectSharedImageVideoProvider::RequestImage(
   // care about, and that doesn't have anything to do with GLImage.
 
   gpu_factory_.Post(FROM_HERE, &GpuSharedImageVideoFactory::CreateImage,
-                    std::move(image_ready_cb), spec, std::move(texture_owner));
-}
-
-// static
-void DirectSharedImageVideoProvider::OnImageReady(
-    ImageReadyCB cb,
-    std::unique_ptr<CodecOutputBuffer> output_buffer,
-    scoped_refptr<TextureOwner> texture_owner,
-    PromotionHintAggregator::NotifyPromotionHintCB promotion_hint_cb,
-    scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner,
-    ImageRecord record) {
-  // It's okay to access |codec_image| here, since it's not used anywhere.
-  // We could let |cb| do this, but, in the long term, we want our caller to
-  // provide the output buffer / etc. to us, so we can hide how we use it.
-  record.codec_image_holder->codec_image_raw()->Initialize(
-      std::move(output_buffer), std::move(texture_owner),
-      std::move(promotion_hint_cb));
-
-  std::move(cb).Run(std::move(record));
+                    BindToCurrentLoop(std::move(cb)), spec,
+                    std::move(texture_owner));
 }
 
 GpuSharedImageVideoFactory::GpuSharedImageVideoFactory(

media/gpu/android/direct_shared_image_video_provider.h

@@ -43,20 +43,9 @@ class MEDIA_GPU_EXPORT DirectSharedImageVideoProvider
   void Initialize(GpuInitCB get_stub_cb) override;
   void RequestImage(ImageReadyCB cb,
                     const ImageSpec& spec,
-                    std::unique_ptr<CodecOutputBuffer> output_buffer,
-                    scoped_refptr<TextureOwner> texture_owner,
-                    PromotionHintAggregator::NotifyPromotionHintCB
-                        promotion_hint_cb) override;
+                    scoped_refptr<TextureOwner> texture_owner) override;
 
  private:
-  static void OnImageReady(
-      ImageReadyCB cb,
-      std::unique_ptr<CodecOutputBuffer> output_buffer,
-      scoped_refptr<TextureOwner> texture_owner,
-      PromotionHintAggregator::NotifyPromotionHintCB promotion_hint_cb,
-      scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner,
-      ImageRecord record);
-
   base::SequenceBound<GpuSharedImageVideoFactory> gpu_factory_;
 
   scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner_;

media/gpu/android/mock_codec_image.cc

+// Copyright 2019 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 "media/gpu/android/mock_codec_image.h"
+
+namespace media {
+
+MockCodecImage::MockCodecImage() = default;
+
+MockCodecImage::~MockCodecImage() = default;
+
+}  // namespace media

media/gpu/android/mock_codec_image.h

+// Copyright 2019 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.
+
+#ifndef MEDIA_GPU_ANDROID_MOCK_CODEC_IMAGE_H_
+#define MEDIA_GPU_ANDROID_MOCK_CODEC_IMAGE_H_
+
+#include "base/macros.h"
+#include "media/gpu/android/codec_image.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+
+// CodecImage with a mocked ReleaseCodecBuffer.
+class MockCodecImage : public CodecImage {
+ public:
+  MockCodecImage();
+
+  MOCK_METHOD0(ReleaseCodecBuffer, void());
+
+ protected:
+  ~MockCodecImage() override;
+
+  DISALLOW_COPY_AND_ASSIGN(MockCodecImage);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_GPU_ANDROID_MOCK_CODEC_IMAGE_H_

media/gpu/android/shared_image_video_provider.h

@@ -80,16 +80,9 @@ class MEDIA_GPU_EXPORT SharedImageVideoProvider {
 
   // Call |cb| when we have a shared image that matches |spec|.  We may call
   // |cb| back before returning, or we might post it for later.
-  // |output_buffer|, |texture_owner|, and |promotion_hint_cb| probably should
-  // not be provided to the provider, but need to be refactored a bit first.
-  // They will be used to configure the SharedImageVideo to display the buffer
-  // on the given texture, and return promotion hints.
-  virtual void RequestImage(
-      ImageReadyCB cb,
-      const ImageSpec& spec,
-      std::unique_ptr<CodecOutputBuffer> output_buffer,
-      scoped_refptr<TextureOwner> texture_owner,
-      PromotionHintAggregator::NotifyPromotionHintCB promotion_hint_cb) = 0;
+  virtual void RequestImage(ImageReadyCB cb,
+                            const ImageSpec& spec,
+                            scoped_refptr<TextureOwner> texture_owner) = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SharedImageVideoProvider);

media/gpu/android/video_frame_factory_impl.cc

@@ -154,13 +154,13 @@ void VideoFrameFactoryImpl::CreateVideoFrame(
 
   auto image_ready_cb = base::BindOnce(
       &VideoFrameFactoryImpl::OnImageReady, weak_factory_.GetWeakPtr(),
-      std::move(output_cb), timestamp, coded_size, natural_size, texture_owner_,
+      std::move(output_cb), timestamp, coded_size, natural_size,
+      std::move(output_buffer), texture_owner_, std::move(promotion_hint_cb),
       pixel_format, overlay_mode_, enable_threaded_texture_mailboxes_,
       gpu_task_runner_);
 
   image_provider_->RequestImage(std::move(image_ready_cb), spec,
-                                std::move(output_buffer), texture_owner_,
-                                std::move(promotion_hint_cb));
+                                texture_owner_);
 }
 
 // static
@@ -170,7 +170,9 @@ void VideoFrameFactoryImpl::OnImageReady(
     base::TimeDelta timestamp,
     gfx::Size coded_size,
     gfx::Size natural_size,
+    std::unique_ptr<CodecOutputBuffer> output_buffer,
     scoped_refptr<TextureOwner> texture_owner,
+    PromotionHintAggregator::NotifyPromotionHintCB promotion_hint_cb,
     VideoPixelFormat pixel_format,
     OverlayMode overlay_mode,
     bool enable_threaded_texture_mailboxes,
@@ -181,6 +183,13 @@ void VideoFrameFactoryImpl::OnImageReady(
   if (!thiz)
     return;
 
+  // Initialize the CodecImage to use this output buffer.  Note that we're not
+  // on the gpu main thread here, but it's okay since CodecImage is not being
+  // used at this point.  Alternatively, we could post it, or hand it off to the
+  // MaybeRenderEarlyManager to save a post.
+  record.codec_image_holder->codec_image_raw()->Initialize(
+      std::move(output_buffer), texture_owner, std::move(promotion_hint_cb));
+
   // Send the CodecImage (via holder, since we can't touch the refcount here) to
   // the MaybeRenderEarlyManager.
   thiz->mre_manager()->AddCodecImage(std::move(record.codec_image_holder));

media/gpu/android/video_frame_factory_impl.h

@@ -76,7 +76,9 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
       base::TimeDelta timestamp,
       gfx::Size coded_size,
       gfx::Size natural_size,
+      std::unique_ptr<CodecOutputBuffer> output_buffer,
       scoped_refptr<TextureOwner> texture_owner,
+      PromotionHintAggregator::NotifyPromotionHintCB promotion_hint_cb,
       VideoPixelFormat pixel_format,
       OverlayMode overlay_mode,
       bool enable_threaded_texture_mailboxes,

media/gpu/android/video_frame_factory_impl_unittest.cc

@@ -14,6 +14,7 @@
 #include "gpu/config/gpu_preferences.h"
 #include "media/base/limits.h"
 #include "media/gpu/android/maybe_render_early_manager.h"
+#include "media/gpu/android/mock_codec_image.h"
 #include "media/gpu/android/shared_image_video_provider.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -45,15 +46,10 @@ class MockSharedImageVideoProvider : public SharedImageVideoProvider {
 
   void RequestImage(ImageReadyCB cb,
                     const ImageSpec& spec,
-                    std::unique_ptr<CodecOutputBuffer> output_buffer,
-                    scoped_refptr<TextureOwner> texture_owner,
-                    PromotionHintAggregator::NotifyPromotionHintCB
-                        promotion_hint_cb) override {
+                    scoped_refptr<TextureOwner> texture_owner) override {
     cb_ = std::move(cb);
     spec_ = spec;
-    output_buffer_ = std::move(output_buffer);
     texture_owner_ = std::move(texture_owner);
-    promotion_hint_cb_ = std::move(promotion_hint_cb);
 
     MockRequestImage();
   }
@@ -63,9 +59,7 @@ class MockSharedImageVideoProvider : public SharedImageVideoProvider {
   // Most recent arguments to RequestImage.
   ImageReadyCB cb_;
   ImageSpec spec_;
-  std::unique_ptr<CodecOutputBuffer> output_buffer_;
   scoped_refptr<TextureOwner> texture_owner_;
-  PromotionHintAggregator::NotifyPromotionHintCB promotion_hint_cb_;
 };
 
 class VideoFrameFactoryImplTest : public testing::Test {
@@ -100,14 +94,13 @@ class VideoFrameFactoryImplTest : public testing::Test {
     // provider.
     // TODO(liberato): Verify that it's sending the proper TextureOwner.
     // However, we haven't actually given it a TextureOwner yet.
-    CodecOutputBuffer* output_buffer_raw = output_buffer.get();
+    output_buffer_raw_ = output_buffer.get();
     EXPECT_CALL(*image_provider_raw_, MockRequestImage());
 
     impl_->CreateVideoFrame(
         std::move(output_buffer), base::TimeDelta(), natural_size,
         PromotionHintAggregator::NotifyPromotionHintCB(), output_cb_.Get());
     base::RunLoop().RunUntilIdle();
-    EXPECT_EQ(image_provider_raw_->output_buffer_.get(), output_buffer_raw);
   }
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
@@ -118,6 +111,9 @@ class VideoFrameFactoryImplTest : public testing::Test {
   MockMaybeRenderEarlyManager* mre_manager_raw_ = nullptr;
   MockSharedImageVideoProvider* image_provider_raw_ = nullptr;
 
+  // Most recently created CodecOutputBuffer.
+  CodecOutputBuffer* output_buffer_raw_ = nullptr;
+
   // Sent to |impl_| by RequestVideoFrame..
   base::MockCallback<VideoFrameFactory::OnceOutputCb> output_cb_;
 
@@ -189,11 +185,17 @@ TEST_F(VideoFrameFactoryImplTest, CreateVideoFrameSucceeds) {
   record.release_cb =
       base::BindOnce([](bool* flag, const gpu::SyncToken&) { *flag = true; },
                      base::Unretained(&release_cb_called_flag));
-  record.codec_image_holder = nullptr;
+  auto codec_image = base::MakeRefCounted<MockCodecImage>();
+  record.codec_image_holder =
+      base::MakeRefCounted<CodecImageHolder>(task_runner_, codec_image);
   std::move(image_provider_raw_->cb_).Run(std::move(record));
   base::RunLoop().RunUntilIdle();
   EXPECT_NE(frame, nullptr);
 
+  // Make sure that it set the output buffer properly.
+  EXPECT_EQ(codec_image->get_codec_output_buffer_for_testing(),
+            output_buffer_raw_);
+
   // Destroy the VideoFrame, and verify that our release cb is called.
   EXPECT_FALSE(release_cb_called_flag);
   frame = nullptr;