Split DirectSharedImageVideoProvider into separate files.

DirectSharedImageVideoProvider was in video_frame_factory_impl.cc,
mostly so that review of the refactor would be simpler.  This CL
moves it into separate files.  There is very little functional
change, except that TextureOwner creation is now moved out of the
SharedImageVideoProvider, and into VideoFrameFactoryImpl.  It's
still somewhat hacky, but at least it's hacky closer to where it
should actually be done.

This CL also adds more unittests for VideoFrameFactoryImpl, since
it's now much easier to inject mocks for everything.  It doesn't add
any tests for DirectSharedImageVideoProvider, since that depends on
a refactor to move codec output buffer management out of CodecImage.

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

media/gpu/BUILD.gn

@@ -112,6 +112,8 @@ component("gpu") {
       "android/codec_wrapper.h",
       "android/device_info.cc",
       "android/device_info.h",
+      "android/direct_shared_image_video_provider.cc",
+      "android/direct_shared_image_video_provider.h",
       "android/image_reader_gl_owner.cc",
       "android/image_reader_gl_owner.h",
       "android/maybe_render_early_manager.cc",

media/gpu/android/codec_wrapper.cc

@@ -107,14 +107,20 @@ class CodecWrapperImpl : public base::RefCountedThreadSafe<CodecWrapperImpl> {
 
 CodecOutputBuffer::CodecOutputBuffer(scoped_refptr<CodecWrapperImpl> codec,
                                      int64_t id,
-                                     gfx::Size size)
+                                     const gfx::Size& size)
     : codec_(std::move(codec)), id_(id), size_(size) {}
 
+// For testing.
+CodecOutputBuffer::CodecOutputBuffer(int64_t id, const gfx::Size& size)
+    : id_(id), size_(size) {}
+
 CodecOutputBuffer::~CodecOutputBuffer() {
   // While it will work if we re-release the buffer, since CodecWrapper handles
   // it properly, we can save a lock + (possibly) post by checking here if we
   // know that it has been rendered already.
-  if (!was_rendered_)
+  //
+  // |codec_| might be null, but only for tests.
+  if (!was_rendered_ && codec_)
     codec_->ReleaseCodecOutputBuffer(id_, false);
 }
 

media/gpu/android/codec_wrapper.h

@@ -22,6 +22,7 @@
 #include "media/gpu/media_gpu_export.h"
 
 namespace media {
+class CodecWrapper;
 class CodecWrapperImpl;
 
 using CodecSurfacePair = std::pair<std::unique_ptr<MediaCodecBridge>,
@@ -43,12 +44,24 @@ class MEDIA_GPU_EXPORT CodecOutputBuffer {
   // The size of the image.
   gfx::Size size() const { return size_; }
 
+  // Note that you can't use the first ctor, since CodecWrapperImpl isn't
+  // defined here.  Use the second, and it'll be nullptr.
+  template <typename... Args>
+  static std::unique_ptr<CodecOutputBuffer> CreateForTesting(Args&&... args) {
+    // std::make_unique can't access the constructor.
+    return std::unique_ptr<CodecOutputBuffer>(
+        new CodecOutputBuffer(std::forward<Args>(args)...));
+  }
+
  private:
   // Let CodecWrapperImpl call the constructor.
   friend class CodecWrapperImpl;
   CodecOutputBuffer(scoped_refptr<CodecWrapperImpl> codec,
                     int64_t id,
-                    gfx::Size size);
+                    const gfx::Size& size);
+
+  // For testing, since CodecWrapperImpl isn't available.  Uses nullptr.
+  CodecOutputBuffer(int64_t id, const gfx::Size& size);
 
   scoped_refptr<CodecWrapperImpl> codec_;
   int64_t id_;

media/gpu/android/direct_shared_image_video_provider.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_DIRECT_SHARED_IMAGE_VIDEO_PROVIDER_H_
+#define MEDIA_GPU_ANDROID_DIRECT_SHARED_IMAGE_VIDEO_PROVIDER_H_
+
+#include <memory>
+
+#include "base/memory/weak_ptr.h"
+#include "base/optional.h"
+#include "base/single_thread_task_runner.h"
+#include "base/threading/sequence_bound.h"
+#include "gpu/command_buffer/service/gles2_cmd_decoder.h"
+#include "gpu/command_buffer/service/shared_image_representation.h"
+#include "gpu/command_buffer/service/texture_manager.h"
+#include "gpu/ipc/common/vulkan_ycbcr_info.h"
+#include "gpu/ipc/service/command_buffer_stub.h"
+#include "media/base/video_frame.h"
+#include "media/gpu/android/codec_image.h"
+#include "media/gpu/android/maybe_render_early_manager.h"
+#include "media/gpu/android/shared_image_video_provider.h"
+#include "media/gpu/android/surface_texture_gl_owner.h"
+#include "media/gpu/android/video_frame_factory.h"
+#include "media/gpu/gles2_decoder_helper.h"
+#include "media/gpu/media_gpu_export.h"
+#include "ui/gl/gl_bindings.h"
+
+namespace media {
+class GpuSharedImageVideoFactory;
+
+// SharedImageVideoProvider implementation that lives on the thread that it's
+// created on, but hops to the GPU thread to create new shared images on demand.
+class MEDIA_GPU_EXPORT DirectSharedImageVideoProvider
+    : public SharedImageVideoProvider {
+ public:
+  DirectSharedImageVideoProvider(
+      scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner,
+      GetStubCB get_stub_cb);
+  ~DirectSharedImageVideoProvider() override;
+
+  // SharedImageVideoProvider
+  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;
+
+ 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_;
+
+  DISALLOW_COPY_AND_ASSIGN(DirectSharedImageVideoProvider);
+};
+
+// GpuSharedImageVideoFactory creates SharedImageVideo objects.  It must be run
+// on the gpu main thread.
+//
+// GpuSharedImageVideoFactory is an implementation detail of
+// DirectSharedImageVideoProvider.  It's here since we'll likely re-use it for
+// the pool.
+class GpuSharedImageVideoFactory
+    : public gpu::CommandBufferStub::DestructionObserver {
+ public:
+  explicit GpuSharedImageVideoFactory(
+      SharedImageVideoProvider::GetStubCB get_stub_cb);
+  ~GpuSharedImageVideoFactory() override;
+
+  // Will run |init_cb| with the shared context current.  |init_cb| should not
+  // post, else the context won't be current.
+  void Initialize(SharedImageVideoProvider::GpuInitCB init_cb);
+
+  // Similar to SharedImageVideoProvider::ImageReadyCB, but provides additional
+  // details for the provider that's using us.
+  using FactoryImageReadyCB = SharedImageVideoProvider::ImageReadyCB;
+
+  // Creates a SharedImage for |spec|, and returns it via the callback.
+  // TODO(liberato): |texture_owner| is only needed to get the service id, to
+  // create the per-frame texture.  All of that is only needed for legacy
+  // mailbox support, where we have to have one texture per CodecImage.
+  void CreateImage(FactoryImageReadyCB cb,
+                   const SharedImageVideoProvider::ImageSpec& spec,
+                   scoped_refptr<TextureOwner> texture_owner);
+
+ private:
+  // Creates a SharedImage for |mailbox|, and returns success or failure.
+  bool CreateImageInternal(const SharedImageVideoProvider::ImageSpec& spec,
+                           scoped_refptr<TextureOwner> texture_owner,
+                           gpu::Mailbox mailbox,
+                           scoped_refptr<CodecImage> image);
+
+  void OnWillDestroyStub(bool have_context) override;
+
+  gpu::CommandBufferStub* stub_ = nullptr;
+
+  // A helper for creating textures. Only valid while |stub_| is valid.
+  std::unique_ptr<GLES2DecoderHelper> decoder_helper_;
+
+  // Sampler conversion information which is used in vulkan context. This is
+  // constant for all the frames in a video and hence we cache it.
+  base::Optional<gpu::VulkanYCbCrInfo> ycbcr_info_;
+
+  THREAD_CHECKER(thread_checker_);
+
+  base::WeakPtrFactory<GpuSharedImageVideoFactory> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(GpuSharedImageVideoFactory);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_GPU_ANDROID_DIRECT_SHARED_IMAGE_VIDEO_PROVIDER_H_

media/gpu/android/shared_image_video_provider.h

@@ -14,6 +14,8 @@
 #include "ui/gfx/geometry/size.h"
 
 namespace gpu {
+class CommandBufferStub;
+class SharedContextState;
 struct SyncToken;
 }  // namespace gpu
 
@@ -22,6 +24,10 @@ namespace media {
 // Provider class for shared images.
 class MEDIA_GPU_EXPORT SharedImageVideoProvider {
  public:
+  using GetStubCB = base::RepeatingCallback<gpu::CommandBufferStub*()>;
+  using GpuInitCB =
+      base::OnceCallback<void(scoped_refptr<gpu::SharedContextState>)>;
+
   // Description of the underlying properties of the shared image.
   struct ImageSpec {
     ImageSpec(const gfx::Size& size);
@@ -66,6 +72,12 @@ class MEDIA_GPU_EXPORT SharedImageVideoProvider {
 
   using ImageReadyCB = base::OnceCallback<void(ImageRecord)>;
 
+  // Initialize this provider.  On success, |gpu_init_cb| will be run with the
+  // SharedContextState (and the context current), on the gpu main thread.  This
+  // is mostly a hack to allow VideoFrameFactoryImpl to create a TextureOwner on
+  // the right context.  Will call |gpu_init_cb| with nullptr otherwise.
+  virtual void Initialize(GpuInitCB gpu_init_cb) = 0;
+
   // 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

media/gpu/android/video_frame_factory.h

@@ -15,10 +15,6 @@
 #include "media/gpu/media_gpu_export.h"
 #include "ui/gfx/geometry/size.h"
 
-namespace gpu {
-class CommandBufferStub;
-}  // namespace gpu
-
 namespace media {
 
 class CodecOutputBuffer;
@@ -30,7 +26,6 @@ class VideoFrame;
 // safe. Virtual for testing; see VideoFrameFactoryImpl.
 class MEDIA_GPU_EXPORT VideoFrameFactory {
  public:
-  using GetStubCb = base::Callback<gpu::CommandBufferStub*()>;
   using InitCb = base::RepeatingCallback<void(scoped_refptr<TextureOwner>)>;
   using OnceOutputCb = base::OnceCallback<void(scoped_refptr<VideoFrame>)>;
 

media/gpu/android/video_frame_factory_impl.h

@@ -10,12 +10,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/single_thread_task_runner.h"
-#include "gpu/command_buffer/service/abstract_texture.h"
-#include "gpu/command_buffer/service/gles2_cmd_decoder.h"
-#include "gpu/command_buffer/service/shared_image_representation.h"
-#include "gpu/command_buffer/service/texture_manager.h"
-#include "gpu/ipc/common/vulkan_ycbcr_info.h"
-#include "gpu/ipc/service/command_buffer_stub.h"
+#include "gpu/config/gpu_preferences.h"
 #include "media/base/video_frame.h"
 #include "media/gpu/android/codec_image.h"
 #include "media/gpu/android/codec_wrapper.h"
@@ -23,7 +18,6 @@
 #include "media/gpu/android/shared_image_video_provider.h"
 #include "media/gpu/android/surface_texture_gl_owner.h"
 #include "media/gpu/android/video_frame_factory.h"
-#include "media/gpu/gles2_decoder_helper.h"
 #include "media/gpu/media_gpu_export.h"
 #include "ui/gl/gl_bindings.h"
 
@@ -48,8 +42,8 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
   // |get_stub_cb| will be run on |gpu_task_runner|.
   VideoFrameFactoryImpl(
       scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner,
-      GetStubCb get_stub_cb,
       const gpu::GpuPreferences& gpu_preferences,
+      std::unique_ptr<SharedImageVideoProvider> image_provider,
       std::unique_ptr<MaybeRenderEarlyManager> mre_manager);
   ~VideoFrameFactoryImpl() override;
 
@@ -93,7 +87,6 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
 
   std::unique_ptr<SharedImageVideoProvider> image_provider_;
   scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner_;
-  GetStubCb get_stub_cb_;
 
   // The texture owner that video frames should use, or nullptr.
   scoped_refptr<TextureOwner> texture_owner_;
@@ -117,61 +110,6 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
   DISALLOW_COPY_AND_ASSIGN(VideoFrameFactoryImpl);
 };
 
-// GpuSharedImageVideoFactory creates SharedImageVideo objects.  It must be run
-// on the gpu main thread.
-//
-// GpuSharedImageVideoFactory is an implementation detail of
-// DirectSharedImageVideoProvider.  It really should be split out into its own
-// file from here, but in the interest of making CL diffs more readable, that
-// is left for later.
-class GpuSharedImageVideoFactory
-    : public gpu::CommandBufferStub::DestructionObserver {
- public:
-  GpuSharedImageVideoFactory();
-  ~GpuSharedImageVideoFactory() override;
-
-  // TODO(liberato): Now that this is used as part of an image provider, it
-  // doesn't really make sense for it to call back with a TextureOwner.  That
-  // should be handled by VideoFrameFactoryImpl if it wants.
-  void Initialize(VideoFrameFactory::OverlayMode overlay_mode,
-                  VideoFrameFactory::GetStubCb get_stub_cb,
-                  VideoFrameFactory::InitCb init_cb);
-
-  // Similar to SharedImageVideoProvider::ImageReadyCB, but provides additional
-  // details for the provider that's using us.
-  using FactoryImageReadyCB = SharedImageVideoProvider::ImageReadyCB;
-
-  // Creates a SharedImage for |spec|, and returns it via the callback.
-  // TODO(liberato): |texture_owner| is only needed to get the service id, to
-  // create the per-frame texture.  All of that is only needed for legacy
-  // mailbox support, where we have to have one texture per CodecImage.
-  void CreateImage(FactoryImageReadyCB cb,
-                   const SharedImageVideoProvider::ImageSpec& spec,
-                   scoped_refptr<TextureOwner> texture_owner);
-
- private:
-  // Creates a SharedImage for |mailbox|, and returns success or failure.
-  bool CreateImageInternal(const SharedImageVideoProvider::ImageSpec& spec,
-                           scoped_refptr<TextureOwner> texture_owner,
-                           gpu::Mailbox mailbox,
-                           scoped_refptr<CodecImage> image);
-
-  void OnWillDestroyStub(bool have_context) override;
-
-  gpu::CommandBufferStub* stub_ = nullptr;
-
-  // A helper for creating textures. Only valid while |stub_| is valid.
-  std::unique_ptr<GLES2DecoderHelper> decoder_helper_;
-
-  // Sampler conversion information which is used in vulkan context. This is
-  // constant for all the frames in a video and hence we cache it.
-  base::Optional<gpu::VulkanYCbCrInfo> ycbcr_info_;
-
-  THREAD_CHECKER(thread_checker_);
-  base::WeakPtrFactory<GpuSharedImageVideoFactory> weak_factory_;
-  DISALLOW_COPY_AND_ASSIGN(GpuSharedImageVideoFactory);
-};
-
 }  // namespace media
 
 #endif  // MEDIA_GPU_ANDROID_VIDEO_FRAME_FACTORY_IMPL_

media/gpu/android/video_frame_factory_impl_unittest.cc

@@ -6,45 +6,209 @@
 
 #include "base/bind.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/gmock_callback_support.h"
+#include "base/test/mock_callback.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "gpu/command_buffer/service/shared_context_state.h"
 #include "gpu/config/gpu_preferences.h"
+#include "media/base/limits.h"
 #include "media/gpu/android/maybe_render_early_manager.h"
+#include "media/gpu/android/shared_image_video_provider.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::test::RunOnceCallback;
+using testing::_;
+using testing::SaveArg;
+
 namespace gpu {
 class CommandBufferStub;
 }  // namespace gpu
 
 namespace media {
 
-struct MockMaybeRenderEarlyManager : public MaybeRenderEarlyManager {
+class MockMaybeRenderEarlyManager : public MaybeRenderEarlyManager {
+ public:
   MOCK_METHOD1(SetSurfaceBundle, void(scoped_refptr<CodecSurfaceBundle>));
   MOCK_METHOD1(AddCodecImage, void(scoped_refptr<CodecImageHolder>));
   MOCK_METHOD0(MaybeRenderEarly, void());
 };
 
+class MockSharedImageVideoProvider : public SharedImageVideoProvider {
+ public:
+  MockSharedImageVideoProvider() : spec_(gfx::Size(0, 0)) {}
+
+  void Initialize(GpuInitCB gpu_init_cb) { Initialize_(gpu_init_cb); }
+
+  MOCK_METHOD1(Initialize_, void(GpuInitCB& gpu_init_cb));
+
+  void RequestImage(ImageReadyCB cb,
+                    const ImageSpec& spec,
+                    std::unique_ptr<CodecOutputBuffer> output_buffer,
+                    scoped_refptr<TextureOwner> texture_owner,
+                    PromotionHintAggregator::NotifyPromotionHintCB
+                        promotion_hint_cb) 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();
+  }
+
+  MOCK_METHOD0(MockRequestImage, void());
+
+  // 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 {
  public:
   VideoFrameFactoryImplTest()
-      : task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
+      : task_runner_(base::ThreadTaskRunnerHandle::Get()) {
+    auto get_stub_cb = base::BindRepeating(
+        []() -> gpu::CommandBufferStub* { return nullptr; });
+
+    auto image_provider = std::make_unique<MockSharedImageVideoProvider>();
+    image_provider_raw_ = image_provider.get();
+
+    auto mre_manager = std::make_unique<MockMaybeRenderEarlyManager>();
+    mre_manager_raw_ = mre_manager.get();
+
+    impl_ = std::make_unique<VideoFrameFactoryImpl>(
+        task_runner_, gpu_preferences_, std::move(image_provider),
+        std::move(mre_manager));
+  }
   ~VideoFrameFactoryImplTest() override = default;
 
+  void RequestVideoFrame() {
+    gfx::Size coded_size(100, 100);
+    gfx::Rect visible_rect(coded_size);
+    gfx::Size natural_size(coded_size);
+    auto output_buffer = CodecOutputBuffer::CreateForTesting(0, coded_size);
+    ASSERT_TRUE(
+        VideoFrame::IsValidConfig(PIXEL_FORMAT_ARGB, VideoFrame::STORAGE_OPAQUE,
+                                  coded_size, visible_rect, natural_size));
+
+    // We should get a call to the output callback, but no calls to the
+    // 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();
+    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_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
+  std::unique_ptr<VideoFrameFactoryImpl> impl_;
+
+  MockMaybeRenderEarlyManager* mre_manager_raw_ = nullptr;
+  MockSharedImageVideoProvider* image_provider_raw_ = nullptr;
+
+  // Sent to |impl_| by RequestVideoFrame..
+  base::MockCallback<VideoFrameFactory::OnceOutputCb> output_cb_;
+
   gpu::GpuPreferences gpu_preferences_;
 };
 
-TEST_F(VideoFrameFactoryImplTest, CreateDoesntCrash) {
-  // Placeholder test until we pull out the SharedImageVideoProvider.
-  auto get_stub_cb =
-      base::BindRepeating([]() -> gpu::CommandBufferStub* { return nullptr; });
-  std::unique_ptr<VideoFrameFactoryImpl> impl =
-      std::make_unique<VideoFrameFactoryImpl>(
-          task_runner_, get_stub_cb, gpu_preferences_,
-          std::make_unique<MockMaybeRenderEarlyManager>());
+TEST_F(VideoFrameFactoryImplTest, ImageProviderInitFailure) {
+  // If the image provider fails to init, then our init cb should be called with
+  // no TextureOwner.
+  EXPECT_CALL(*image_provider_raw_, Initialize_(_))
+      .Times(1)
+      .WillOnce(RunOnceCallback<0>(nullptr));
+  base::MockCallback<VideoFrameFactory::InitCb> init_cb;
+  EXPECT_CALL(init_cb, Run(scoped_refptr<TextureOwner>(nullptr)));
+  impl_->Initialize(VideoFrameFactory::OverlayMode::kDontRequestPromotionHints,
+                    init_cb.Get());
+  base::RunLoop().RunUntilIdle();
+
+  // TODO(liberato): for testing, we could just skip calling the gpu init cb,
+  // since |impl_| doesn't know or care if it's called.  that way, we don't need
+  // to mock out making the callback work.  would be nice, though.
+}
+
+TEST_F(VideoFrameFactoryImplTest,
+       SetSurfaceBundleForwardsToMaybeRenderEarlyManager) {
+  // Sending a non-null CodecSurfaceBundle should forward it to |mre_manager|.
+  scoped_refptr<CodecSurfaceBundle> surface_bundle =
+      base::MakeRefCounted<CodecSurfaceBundle>();
+  EXPECT_CALL(*mre_manager_raw_, SetSurfaceBundle(surface_bundle));
+  impl_->SetSurfaceBundle(surface_bundle);
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(VideoFrameFactoryImplTest, CreateVideoFrameFailsIfUnsupportedFormat) {
+  // Sending an unsupported format should cause an early failure, without a
+  // thread hop.
+  gfx::Size coded_size(limits::kMaxDimension + 1, limits::kMaxDimension + 1);
+  gfx::Rect visible_rect(coded_size);
+  gfx::Size natural_size(0, 0);
+  auto output_buffer = CodecOutputBuffer::CreateForTesting(0, coded_size);
+  ASSERT_FALSE(VideoFrame::IsValidConfig(PIXEL_FORMAT_ARGB,
+                                         VideoFrame::STORAGE_OPAQUE, coded_size,
+                                         visible_rect, natural_size));
+
+  // We should get a call to the output callback, but no calls to the provider.
+  base::MockCallback<VideoFrameFactory::OnceOutputCb> output_cb;
+  EXPECT_CALL(output_cb, Run(scoped_refptr<VideoFrame>(nullptr)));
+  EXPECT_CALL(*image_provider_raw_, MockRequestImage()).Times(0);
+
+  impl_->CreateVideoFrame(
+      std::move(output_buffer), base::TimeDelta(), natural_size,
+      PromotionHintAggregator::NotifyPromotionHintCB(), output_cb.Get());
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(VideoFrameFactoryImplTest, CreateVideoFrameSucceeds) {
+  // Creating a video frame calls through to the image provider, and forwards a
+  // VideoFrame to the output cb.
+  //
+  // TODO(liberato): Consider testing the metadata values.
+  RequestVideoFrame();
+
+  // Call the ImageReadyCB.
+  scoped_refptr<VideoFrame> frame;
+  EXPECT_CALL(output_cb_, Run(_)).WillOnce(SaveArg<0>(&frame));
+  SharedImageVideoProvider::ImageRecord record;
+  record.mailbox = gpu::Mailbox::Generate();
+  bool release_cb_called_flag = false;
+  record.release_cb =
+      base::BindOnce([](bool* flag, const gpu::SyncToken&) { *flag = true; },
+                     base::Unretained(&release_cb_called_flag));
+  record.codec_image_holder = nullptr;
+  std::move(image_provider_raw_->cb_).Run(std::move(record));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_NE(frame, nullptr);
+
+  // Destroy the VideoFrame, and verify that our release cb is called.
+  EXPECT_FALSE(release_cb_called_flag);
+  frame = nullptr;
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(release_cb_called_flag);
+}
+
+TEST_F(VideoFrameFactoryImplTest,
+       DestroyingFactoryDuringVideoFrameCreationDoesntCrash) {
+  // We should be able to destroy |impl_| while a VideoFrame is pending, and
+  // nothing bad should happen.
+  RequestVideoFrame();
+  impl_ = nullptr;
+  base::RunLoop().RunUntilIdle();
 }
 
 }  // namespace media

media/mojo/services/gpu_mojo_media_client.cc

@@ -30,6 +30,7 @@
 #include "media/filters/android/media_codec_audio_decoder.h"
 #include "media/gpu/android/android_video_surface_chooser_impl.h"
 #include "media/gpu/android/codec_allocator.h"
+#include "media/gpu/android/direct_shared_image_video_provider.h"
 #include "media/gpu/android/maybe_render_early_manager.h"
 #include "media/gpu/android/media_codec_video_decoder.h"
 #include "media/gpu/android/video_frame_factory_impl.h"
@@ -203,6 +204,8 @@ std::unique_ptr<VideoDecoder> GpuMojoMediaClient::CreateVideoDecoder(
   auto get_stub_cb =
       base::Bind(&GetCommandBufferStub, media_gpu_channel_manager_,
                  command_buffer_id->channel_token, command_buffer_id->route_id);
+  auto image_provider = std::make_unique<DirectSharedImageVideoProvider>(
+      gpu_task_runner_, std::move(get_stub_cb));
   video_decoder = std::make_unique<MediaCodecVideoDecoder>(
       gpu_preferences_, gpu_feature_info_, DeviceInfo::GetInstance(),
       CodecAllocator::GetInstance(gpu_task_runner_),
@@ -210,7 +213,7 @@ std::unique_ptr<VideoDecoder> GpuMojoMediaClient::CreateVideoDecoder(
           DeviceInfo::GetInstance()->IsSetOutputSurfaceSupported()),
       android_overlay_factory_cb_, std::move(request_overlay_info_cb),
       std::make_unique<VideoFrameFactoryImpl>(
-          gpu_task_runner_, std::move(get_stub_cb), gpu_preferences_,
+          gpu_task_runner_, gpu_preferences_, std::move(image_provider),
           MaybeRenderEarlyManager::Create(gpu_task_runner_)));
 
 #elif defined(OS_CHROMEOS)