Fix ordering of GetFrameInfo requests

This CL adds requests queue to FrameInfoHelperImpl to make sure all
requests are ordered to keep VideoFrameFactoryImpl pipeline ordered.

Before this change VFFI would cache FrameInfo to avoid thread hops to
GPU thread and it was possible that some CreateVideoFrame requests will
be completed out-of-order.

After this change VFFI doesn't cache FrameInfo and always call
FrameInfoHelper. FrameInfoHelper does all necessary caching to avoid
unnecessary hops to GPU thread and guarantees that callbacks with
FrameInfo will come in order.

Change-Id: Iacb63cf3746991c911f1f0d5e3667db3dbd56c68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2304092
Commit-Queue: Vasiliy Telezhnikov <vasilyt@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790127}

media/gpu/android/frame_info_helper.cc

@@ -4,10 +4,13 @@
 
 #include "media/gpu/android/frame_info_helper.h"
 
+#include "base/threading/sequence_bound.h"
 #include "gpu/command_buffer/service/shared_image_video.h"
 #include "gpu/ipc/service/command_buffer_stub.h"
 #include "gpu/ipc/service/gpu_channel.h"
 #include "gpu/ipc/service/gpu_channel_manager.h"
+#include "media/base/bind_to_current_loop.h"
+#include "media/gpu/android/codec_output_buffer_renderer.h"
 
 namespace media {
 
@@ -21,106 +24,184 @@ FrameInfoHelper::FrameInfo& FrameInfoHelper::FrameInfo::operator=(
 
 // Concrete implementation of FrameInfoHelper that renders output buffers and
 // gets the FrameInfo they need.
-class FrameInfoHelperImpl : public FrameInfoHelper,
-                            public gpu::CommandBufferStub::DestructionObserver {
+class FrameInfoHelperImpl : public FrameInfoHelper {
  public:
-  FrameInfoHelperImpl(SharedImageVideoProvider::GetStubCB get_stub_cb) {
-    stub_ = get_stub_cb.Run();
-    if (stub_)
-      stub_->AddDestructionObserver(this);
+  FrameInfoHelperImpl(scoped_refptr<base::SequencedTaskRunner> gpu_task_runner,
+                      SharedImageVideoProvider::GetStubCB get_stub_cb) {
+    on_gpu_ = base::SequenceBound<OnGpu>(std::move(gpu_task_runner),
+                                         std::move(get_stub_cb));
   }
 
-  FrameInfoHelperImpl() = default;
+  ~FrameInfoHelperImpl() override = default;
 
-  ~FrameInfoHelperImpl() override {
-    if (stub_)
-      stub_->RemoveDestructionObserver(this);
+  void GetFrameInfo(std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
+                    FrameInfoReadyCB callback) override {
+    Request request = {.buffer_renderer = std::move(buffer_renderer),
+                       .callback = std::move(callback)};
+    requests_.push(std::move(request));
+    // If there were no pending requests start processing queue now.
+    if (requests_.size() == 1)
+      ProcessRequestsQueue();
   }
 
-  void GetFrameInfo(
-      std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
-      base::OnceCallback<
-          void(std::unique_ptr<CodecOutputBufferRenderer>, FrameInfo, bool)> cb)
-      override {
-    if (!buffer_renderer) {
-      std::move(cb).Run(nullptr, FrameInfo(), false);
-      return;
+ private:
+  struct Request {
+    std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer;
+    FrameInfoReadyCB callback;
+  };
+
+  class OnGpu : public gpu::CommandBufferStub::DestructionObserver {
+   public:
+    OnGpu(SharedImageVideoProvider::GetStubCB get_stub_cb) {
+      stub_ = get_stub_cb.Run();
+      if (stub_)
+        stub_->AddDestructionObserver(this);
+    }
+
+    ~OnGpu() override {
+      if (stub_)
+        stub_->RemoveDestructionObserver(this);
     }
 
-    auto texture_owner = buffer_renderer->texture_owner();
+    void OnWillDestroyStub(bool have_context) override {
+      DCHECK(stub_);
+      stub_ = nullptr;
+    }
 
-    FrameInfo info;
+    void GetFrameInfo(
+        std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
+        base::OnceCallback<void(std::unique_ptr<CodecOutputBufferRenderer>,
+                                base::Optional<FrameInfo>)> cb) {
+      DCHECK(buffer_renderer);
 
-    // Indicates that the FrameInfo is reliable and can be cached by caller.
-    // It's true if we either return cached values or we attempted to render
-    // frame and succeeded.
-    bool success = true;
-
-    // We default to visible size if if we can't get real size
-    info.coded_size = buffer_renderer->size();
-    info.visible_rect = gfx::Rect(info.coded_size);
-
-    if (texture_owner) {
-      if (visible_size_ == buffer_renderer->size()) {
-        info = frame_info_;
-      } else if (buffer_renderer->RenderToTextureOwnerFrontBuffer(
-                     CodecOutputBufferRenderer::BindingsMode::
-                         kDontRestoreIfBound)) {
-        visible_size_ = buffer_renderer->size();
+      auto texture_owner = buffer_renderer->texture_owner();
+      DCHECK(texture_owner);
+
+      base::Optional<FrameInfo> info;
+
+      if (buffer_renderer->RenderToTextureOwnerFrontBuffer(
+              CodecOutputBufferRenderer::BindingsMode::kDontRestoreIfBound)) {
+        info.emplace();
         texture_owner->GetCodedSizeAndVisibleRect(
-            visible_size_, &frame_info_.coded_size, &frame_info_.visible_rect);
+            buffer_renderer->size(), &info->coded_size, &info->visible_rect);
 
-        frame_info_.ycbcr_info = GetYCbCrInfo(texture_owner.get());
-        info = frame_info_;
-      } else {
-        // We attempted to render frame and failed, mark request as failed so
-        // caller won't cache best-guess values.
-        success = false;
+        info->ycbcr_info = GetYCbCrInfo(texture_owner.get());
       }
+
+      std::move(cb).Run(std::move(buffer_renderer), info);
+    }
+
+   private:
+    // Gets YCbCrInfo from last rendered frame.
+    base::Optional<gpu::VulkanYCbCrInfo> GetYCbCrInfo(
+        gpu::TextureOwner* texture_owner) {
+      gpu::ContextResult result;
+
+      if (!stub_)
+        return base::nullopt;
+
+      auto shared_context =
+          stub_->channel()->gpu_channel_manager()->GetSharedContextState(
+              &result);
+      auto context_provider =
+          (result == gpu::ContextResult::kSuccess) ? shared_context : nullptr;
+      if (!context_provider)
+        return base::nullopt;
+
+      return gpu::SharedImageVideo::GetYcbcrInfo(texture_owner,
+                                                 context_provider);
     }
 
-    std::move(cb).Run(std::move(buffer_renderer), info, success);
+    gpu::CommandBufferStub* stub_ = nullptr;
+  };
+
+  FrameInfo GetFrameInfoWithVisibleSize(const gfx::Size& visible_size) {
+    FrameInfo info;
+    info.coded_size = visible_size;
+    info.visible_rect = gfx::Rect(visible_size);
+    return info;
   }
 
-  void OnWillDestroyStub(bool have_context) override {
-    DCHECK(stub_);
-    stub_ = nullptr;
+  void OnFrameInfoReady(
+      std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
+      base::Optional<FrameInfo> frame_info) {
+    DCHECK(buffer_renderer);
+    DCHECK(!requests_.empty());
+
+    auto& request = requests_.front();
+
+    if (frame_info) {
+      visible_size_ = buffer_renderer->size();
+      frame_info_ = *frame_info;
+      std::move(request.callback).Run(std::move(buffer_renderer), frame_info_);
+    } else {
+      // It's possible that we will fail to render frame and so weren't able to
+      // obtain FrameInfo. In this case we don't cache new values and complete
+      // current request with visible size, we will attempt to render next frame
+      // with next request.
+      auto info = GetFrameInfoWithVisibleSize(buffer_renderer->size());
+      std::move(request.callback)
+          .Run(std::move(buffer_renderer), std::move(info));
+    }
+    requests_.pop();
+    ProcessRequestsQueue();
   }
 
- private:
-  // Gets YCbCrInfo from last rendered frame.
-  base::Optional<gpu::VulkanYCbCrInfo> GetYCbCrInfo(
-      gpu::TextureOwner* texture_owner) {
-    gpu::ContextResult result;
-    if (!stub_)
-      return base::nullopt;
-
-    auto shared_context =
-        stub_->channel()->gpu_channel_manager()->GetSharedContextState(&result);
-    auto context_provider =
-        (result == gpu::ContextResult::kSuccess) ? shared_context : nullptr;
-    if (!context_provider)
-      return base::nullopt;
-
-    return gpu::SharedImageVideo::GetYcbcrInfo(texture_owner, context_provider);
+  void ProcessRequestsQueue() {
+    while (!requests_.empty()) {
+      auto& request = requests_.front();
+
+      if (!request.buffer_renderer) {
+        // If we don't have buffer_renderer we can Run callback immediately.
+        std::move(request.callback).Run(nullptr, FrameInfo());
+      } else if (!request.buffer_renderer->texture_owner()) {
+        // If there is no texture_owner (SurfaceView case), we can't render
+        // frame and get proper size. But as Display Compositor won't render
+        // this frame the actual size is not important, assume coded_size =
+        // visible_size.
+        auto info =
+            GetFrameInfoWithVisibleSize(request.buffer_renderer->size());
+        std::move(request.callback)
+            .Run(std::move(request.buffer_renderer), std::move(info));
+      } else if (visible_size_ == request.buffer_renderer->size()) {
+        // We have cached the results of last frame info request with the same
+        // size. We assume that coded_size doesn't change if the visible_size
+        // stays the same.
+        std::move(request.callback)
+            .Run(std::move(request.buffer_renderer), frame_info_);
+      } else {
+        // We have texture_owner and we don't have cached value, so we need to
+        // hop to GPU thread and render the frame to get proper size.
+        auto cb = BindToCurrentLoop(
+            base::BindOnce(&FrameInfoHelperImpl::OnFrameInfoReady,
+                           weak_factory_.GetWeakPtr()));
+
+        on_gpu_.Post(FROM_HERE, &OnGpu::GetFrameInfo,
+                     std::move(request.buffer_renderer), std::move(cb));
+        // We didn't complete this request quite yet, so we can't process queue
+        // any further.
+        break;
+      }
+      requests_.pop();
+    }
   }
 
-  gpu::CommandBufferStub* stub_ = nullptr;
+  base::SequenceBound<OnGpu> on_gpu_;
+  std::queue<Request> requests_;
 
+  // Cached values.
   FrameInfo frame_info_;
   gfx::Size visible_size_;
+
+  base::WeakPtrFactory<FrameInfoHelperImpl> weak_factory_{this};
 };
 
 // static
-base::SequenceBound<FrameInfoHelper> FrameInfoHelper::Create(
+std::unique_ptr<FrameInfoHelper> FrameInfoHelper::Create(
     scoped_refptr<base::SequencedTaskRunner> gpu_task_runner,
     SharedImageVideoProvider::GetStubCB get_stub_cb) {
-  return base::SequenceBound<FrameInfoHelperImpl>(std::move(gpu_task_runner),
-                                                  std::move(get_stub_cb));
-}
-
-std::unique_ptr<FrameInfoHelper> FrameInfoHelper::CreateForTesting() {
-  return std::make_unique<FrameInfoHelperImpl>();
+  return std::make_unique<FrameInfoHelperImpl>(std::move(gpu_task_runner),
+                                               std::move(get_stub_cb));
 }
 
 }  // namespace media

media/gpu/android/frame_info_helper.h

@@ -6,12 +6,11 @@
 #define MEDIA_GPU_ANDROID_FRAME_INFO_HELPER_H_
 
 #include "base/optional.h"
-#include "base/threading/sequence_bound.h"
-#include "media/gpu/android/codec_image.h"
 #include "media/gpu/android/shared_image_video_provider.h"
 #include "media/gpu/media_gpu_export.h"
 
 namespace media {
+class CodecOutputBufferRenderer;
 
 // Helper class to fetch YCbCrInfo for Vulkan from a CodecImage.
 class MEDIA_GPU_EXPORT FrameInfoHelper {
@@ -29,12 +28,14 @@ class MEDIA_GPU_EXPORT FrameInfoHelper {
     base::Optional<gpu::VulkanYCbCrInfo> ycbcr_info;
   };
 
-  static base::SequenceBound<FrameInfoHelper> Create(
+  using FrameInfoReadyCB =
+      base::OnceCallback<void(std::unique_ptr<CodecOutputBufferRenderer>,
+                              FrameInfo)>;
+
+  static std::unique_ptr<FrameInfoHelper> Create(
       scoped_refptr<base::SequencedTaskRunner> gpu_task_runner,
       SharedImageVideoProvider::GetStubCB get_stub_cb);
 
-  static std::unique_ptr<FrameInfoHelper> CreateForTesting();
-
   virtual ~FrameInfoHelper() = default;
 
   // Call |cb| with the FrameInfo.  Will render |buffer_renderer| to the front
@@ -42,9 +43,11 @@ class MEDIA_GPU_EXPORT FrameInfoHelper {
   // attempt to get YCbCrInfo and cache it.  If all necessary info is cached the
   // call will leave buffer_renderer intact and it can be rendered later.
   // Rendering can fail for reasons. This function will make best efforts to
-  // fill FrameInfo which can be used to create VideoFrame, but shouldn't be
-  // cached by caller. Last parameter in |cb| is bool that indicates that info
-  // is reliable.
+  // fill FrameInfo which can be used to create VideoFrame.
+  //
+  // Callbacks will be executed and on callers sequence and guaranteed to be
+  // called in order of GetFrameInfo calls. Callback can be called before this
+  // function returns if all necessary info is available right away.
   //
   // While this API might seem to be out of its Vulkan mind, it's this
   // complicated to (a) prevent rendering frames out of order to the front
@@ -52,9 +55,7 @@ class MEDIA_GPU_EXPORT FrameInfoHelper {
   // can't get a YCbCrInfo from a CodecImage due to timeouts.
   virtual void GetFrameInfo(
       std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
-      base::OnceCallback<void(std::unique_ptr<CodecOutputBufferRenderer>,
-                              FrameInfo,
-                              bool)> cb) = 0;
+      FrameInfoReadyCB callback) = 0;
 
  protected:
   FrameInfoHelper() = default;

media/gpu/android/frame_info_helper_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/test/bind_test_util.h"
 #include "base/test/task_environment.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "gpu/command_buffer/service/mock_texture_owner.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -18,11 +19,19 @@ namespace {
 constexpr gfx::Size kTestVisibleSize(100, 100);
 constexpr gfx::Size kTestVisibleSize2(110, 110);
 constexpr gfx::Size kTestCodedSize(128, 128);
+
+std::unique_ptr<FrameInfoHelper> CreateHelper() {
+  auto task_runner = base::ThreadTaskRunnerHandle::Get();
+  auto get_stub_cb =
+      base::Bind([]() -> gpu::CommandBufferStub* { return nullptr; });
+  return FrameInfoHelper::Create(std::move(task_runner),
+                                 std::move(get_stub_cb));
+}
 }  // namespace
 
 class FrameInfoHelperTest : public testing::Test {
  public:
-  FrameInfoHelperTest() : helper_(FrameInfoHelper::CreateForTesting()) {}
+  FrameInfoHelperTest() : helper_(CreateHelper()) {}
 
  protected:
   void GetFrameInfo(
@@ -31,13 +40,13 @@ class FrameInfoHelperTest : public testing::Test {
     bool called = false;
     auto callback = base::BindLambdaForTesting(
         [&](std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
-            FrameInfoHelper::FrameInfo info, bool success) {
+            FrameInfoHelper::FrameInfo info) {
           ASSERT_EQ(buffer_renderer_raw, buffer_renderer.get());
           called = true;
-          last_get_frame_info_succeeded_ = success;
           last_frame_info_ = info;
         });
     helper_->GetFrameInfo(std::move(buffer_renderer), callback);
+    base::RunLoop().RunUntilIdle();
     ASSERT_TRUE(called);
   }
 
@@ -67,15 +76,12 @@ class FrameInfoHelperTest : public testing::Test {
   base::test::SingleThreadTaskEnvironment task_environment_;
 
   std::unique_ptr<FrameInfoHelper> helper_;
-  bool last_get_frame_info_succeeded_ = false;
   FrameInfoHelper::FrameInfo last_frame_info_;
 };
 
 TEST_F(FrameInfoHelperTest, NoBufferRenderer) {
-  // If there is no buffer renderer we shouldn't crash and report that request
-  // failed.
+  // If there is no buffer renderer we shouldn't crash.
   GetFrameInfo(nullptr);
-  EXPECT_FALSE(last_get_frame_info_succeeded_);
 }
 
 TEST_F(FrameInfoHelperTest, TextureOwner) {
@@ -93,7 +99,6 @@ TEST_F(FrameInfoHelperTest, TextureOwner) {
   // as failed.
   EXPECT_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _)).Times(0);
   GetFrameInfo(std::move(buffer1));
-  EXPECT_FALSE(last_get_frame_info_succeeded_);
   EXPECT_EQ(last_frame_info_.coded_size, kTestVisibleSize);
   Mock::VerifyAndClearExpectations(texture_owner.get());
 
@@ -101,7 +106,6 @@ TEST_F(FrameInfoHelperTest, TextureOwner) {
   // be called and result should be kTestCodedSize instead of kTestVisibleSize.
   EXPECT_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _)).Times(1);
   GetFrameInfo(CreateBufferRenderer(kTestVisibleSize, texture_owner));
-  EXPECT_TRUE(last_get_frame_info_succeeded_);
   EXPECT_EQ(last_frame_info_.coded_size, kTestCodedSize);
   Mock::VerifyAndClearExpectations(texture_owner.get());
 
@@ -109,22 +113,102 @@ TEST_F(FrameInfoHelperTest, TextureOwner) {
   // size. GetCodedSizeAndVisibleRect should not be called.
   EXPECT_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _)).Times(0);
   GetFrameInfo(CreateBufferRenderer(kTestVisibleSize, texture_owner));
-  EXPECT_TRUE(last_get_frame_info_succeeded_);
   EXPECT_EQ(last_frame_info_.coded_size, kTestCodedSize);
   Mock::VerifyAndClearExpectations(texture_owner.get());
 
   // Verify that we render if the visible size changed.
   EXPECT_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _)).Times(1);
   GetFrameInfo(CreateBufferRenderer(kTestVisibleSize2, texture_owner));
-  EXPECT_TRUE(last_get_frame_info_succeeded_);
   EXPECT_EQ(last_frame_info_.coded_size, kTestCodedSize);
 }
 
 TEST_F(FrameInfoHelperTest, Overlay) {
   // In overlay case we always use visible size.
   GetFrameInfo(CreateBufferRenderer(kTestVisibleSize, nullptr));
-  EXPECT_TRUE(last_get_frame_info_succeeded_);
   EXPECT_EQ(last_frame_info_.coded_size, kTestVisibleSize);
 }
 
+TEST_F(FrameInfoHelperTest, SwitchBetweenOverlayAndTextureOwner) {
+  auto texture_owner = base::MakeRefCounted<NiceMock<gpu::MockTextureOwner>>(
+      0, nullptr, nullptr, true);
+
+  // Return CodedSize when GetCodedSizeAndVisibleRect is called.
+  ON_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _))
+      .WillByDefault(SetArgPointee<1>(kTestCodedSize));
+
+  // In overlay case we always use visible size.
+  GetFrameInfo(CreateBufferRenderer(kTestVisibleSize, nullptr));
+  EXPECT_EQ(last_frame_info_.coded_size, kTestVisibleSize);
+
+  // Verify that when we switch to TextureOwner we request new size.
+  EXPECT_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _)).Times(1);
+  GetFrameInfo(CreateBufferRenderer(kTestVisibleSize, texture_owner));
+  EXPECT_EQ(last_frame_info_.coded_size, kTestCodedSize);
+  Mock::VerifyAndClearExpectations(texture_owner.get());
+
+  // Switch back to overlay and verify that we falled back to visible size and
+  // didn't try to render image.
+  EXPECT_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _)).Times(0);
+  GetFrameInfo(CreateBufferRenderer(kTestVisibleSize, nullptr));
+  EXPECT_EQ(last_frame_info_.coded_size, kTestVisibleSize);
+  Mock::VerifyAndClearExpectations(texture_owner.get());
+
+  // Verify that when we switch to TextureOwner we use cached size.
+  EXPECT_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _)).Times(0);
+  GetFrameInfo(CreateBufferRenderer(kTestVisibleSize, texture_owner));
+  EXPECT_EQ(last_frame_info_.coded_size, kTestCodedSize);
+  Mock::VerifyAndClearExpectations(texture_owner.get());
+
+  // Switch back to overlay again.
+  EXPECT_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _)).Times(0);
+  GetFrameInfo(CreateBufferRenderer(kTestVisibleSize, nullptr));
+  EXPECT_EQ(last_frame_info_.coded_size, kTestVisibleSize);
+  Mock::VerifyAndClearExpectations(texture_owner.get());
+
+  // Verify that when we switch to TextureOwner with resize we render another
+  // frame.
+  EXPECT_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _)).Times(1);
+  GetFrameInfo(CreateBufferRenderer(kTestVisibleSize2, texture_owner));
+  EXPECT_EQ(last_frame_info_.coded_size, kTestCodedSize);
+  Mock::VerifyAndClearExpectations(texture_owner.get());
+}
+
+TEST_F(FrameInfoHelperTest, OrderingTest) {
+  auto texture_owner = base::MakeRefCounted<NiceMock<gpu::MockTextureOwner>>(
+      0, nullptr, nullptr, true);
+
+  // Return CodedSize when GetCodedSizeAndVisibleRect is called.
+  ON_CALL(*texture_owner, GetCodedSizeAndVisibleRect(_, _, _))
+      .WillByDefault(SetArgPointee<1>(kTestCodedSize));
+
+  auto buffer_renderer = CreateBufferRenderer(kTestVisibleSize, texture_owner);
+
+  // Create first callback and request frame.
+  const auto* buffer_renderer_raw = buffer_renderer.get();
+  bool called = false;
+  auto callback = base::BindLambdaForTesting(
+      [&](std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
+          FrameInfoHelper::FrameInfo info) {
+        ASSERT_EQ(buffer_renderer_raw, buffer_renderer.get());
+        called = true;
+        last_frame_info_ = info;
+      });
+  helper_->GetFrameInfo(std::move(buffer_renderer), callback);
+
+  // Create run after callback.
+  bool run_after_called = false;
+  auto run_after_callback = base::BindLambdaForTesting(
+      [&](std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
+          FrameInfoHelper::FrameInfo info) {
+        ASSERT_EQ(buffer_renderer.get(), nullptr);
+        // Expect first callback was called before this.
+        EXPECT_TRUE(called);
+        run_after_called = true;
+      });
+  helper_->GetFrameInfo(nullptr, run_after_callback);
+
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(called);
+  ASSERT_TRUE(run_after_called);
+}
 }  // namespace media

media/gpu/android/video_frame_factory_impl.cc

@@ -81,7 +81,7 @@ VideoFrameFactoryImpl::VideoFrameFactoryImpl(
     const gpu::GpuPreferences& gpu_preferences,
     std::unique_ptr<SharedImageVideoProvider> image_provider,
     std::unique_ptr<MaybeRenderEarlyManager> mre_manager,
-    base::SequenceBound<FrameInfoHelper> frame_info_helper)
+    std::unique_ptr<FrameInfoHelper> frame_info_helper)
     : image_provider_(std::move(image_provider)),
       gpu_task_runner_(std::move(gpu_task_runner)),
       enable_threaded_texture_mailboxes_(
@@ -113,10 +113,6 @@ void VideoFrameFactoryImpl::SetSurfaceBundle(
   // changing the TextureOwner.  This is temporary.  See ImageSpec.
   image_spec_.generation_id++;
 
-  // Reset cached visible size as we might switched between overlay and texture
-  // owner mode.
-  visible_size_ = gfx::Size();
-
   if (!surface_bundle) {
     // Clear everything, just so we're not holding a reference.
     codec_buffer_wait_coordinator_ = nullptr;
@@ -185,48 +181,20 @@ void VideoFrameFactoryImpl::CreateVideoFrame(
 void VideoFrameFactoryImpl::RequestImage(
     std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
     ImageWithInfoReadyCB image_ready_cb) {
-  if (buffer_renderer && visible_size_ == buffer_renderer->size()) {
-    auto cb = base::BindOnce(std::move(image_ready_cb),
-                             std::move(buffer_renderer), frame_info_);
-
-    image_provider_->RequestImage(
-        std::move(cb), image_spec_,
-        codec_buffer_wait_coordinator_
-            ? codec_buffer_wait_coordinator_->texture_owner()
-            : nullptr);
-    return;
-  }
-
-  // We need to reset size to make sure VFFI pipeline is still ordered.
-  // e.g: CreateVideoFrame is called with new size. We post task to GPU thread
-  // to get new frame info. While we wait CreateVideoFrame might be called with
-  // old size again and if we don't reset size here we will skip GPU hop and new
-  // frame will be created earlier than first one.
-  visible_size_ = gfx::Size();
-
-  auto info_cb = BindToCurrentLoop(
+  auto info_cb =
       base::BindOnce(&VideoFrameFactoryImpl::CreateVideoFrame_OnFrameInfoReady,
                      weak_factory_.GetWeakPtr(), std::move(image_ready_cb),
-                     codec_buffer_wait_coordinator_));
+                     codec_buffer_wait_coordinator_);
 
-  frame_info_helper_.Post(FROM_HERE, &FrameInfoHelper::GetFrameInfo,
-                          std::move(buffer_renderer), std::move(info_cb));
+  frame_info_helper_->GetFrameInfo(std::move(buffer_renderer),
+                                   std::move(info_cb));
 }
 
 void VideoFrameFactoryImpl::CreateVideoFrame_OnFrameInfoReady(
     ImageWithInfoReadyCB image_ready_cb,
     scoped_refptr<CodecBufferWaitCoordinator> codec_buffer_wait_coordinator,
     std::unique_ptr<CodecOutputBufferRenderer> output_buffer_renderer,
-    FrameInfoHelper::FrameInfo frame_info,
-    bool success) {
-  // To get frame info we need to render frame which might fail for variety of
-  // reason. FrameInfoHelper will provide best values we can proceed with, but
-  // we should not cache it and attempt to get info for next frame.
-  if (success) {
-    frame_info_ = frame_info;
-    visible_size_ = output_buffer_renderer->size();
-  }
-
+    FrameInfoHelper::FrameInfo frame_info) {
   // If we don't have output buffer here we can't rely on reply from
   // FrameInfoHelper as there might be not cached value and we can't render
   // nothing. But in this case call comes from RunAfterPendingVideoFrames and we

media/gpu/android/video_frame_factory_impl.h

@@ -10,7 +10,6 @@
 #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/config/gpu_preferences.h"
 #include "media/base/video_frame.h"
 #include "media/gpu/android/codec_buffer_wait_coordinator.h"
@@ -52,7 +51,7 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
       const gpu::GpuPreferences& gpu_preferences,
       std::unique_ptr<SharedImageVideoProvider> image_provider,
       std::unique_ptr<MaybeRenderEarlyManager> mre_manager,
-      base::SequenceBound<FrameInfoHelper> frame_info_helper);
+      std::unique_ptr<FrameInfoHelper> frame_info_helper);
   ~VideoFrameFactoryImpl() override;
 
   void Initialize(OverlayMode overlay_mode, InitCB init_cb) override;
@@ -105,8 +104,7 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
       ImageWithInfoReadyCB image_ready_cb,
       scoped_refptr<CodecBufferWaitCoordinator> codec_buffer_wait_coordinator,
       std::unique_ptr<CodecOutputBufferRenderer> output_buffer_renderer,
-      FrameInfoHelper::FrameInfo frame_info,
-      bool success);
+      FrameInfoHelper::FrameInfo frame_info);
 
   MaybeRenderEarlyManager* mre_manager() const { return mre_manager_.get(); }
 
@@ -128,12 +126,8 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
 
   std::unique_ptr<MaybeRenderEarlyManager> mre_manager_;
 
-  // Caches FrameInfo and visible size it was cached for.
-  gfx::Size visible_size_;
-  FrameInfoHelper::FrameInfo frame_info_;
-
-  // Optional helper to get the Vulkan YCbCrInfo.
-  base::SequenceBound<FrameInfoHelper> frame_info_helper_;
+  // Helper to get coded_size and optional Vulkan YCbCrInfo.
+  std::unique_ptr<FrameInfoHelper> frame_info_helper_;
 
   // The current image spec that we'll use to request images.
   SharedImageVideoProvider::ImageSpec image_spec_;

media/gpu/android/video_frame_factory_impl_unittest.cc

@@ -44,46 +44,14 @@ class MockMaybeRenderEarlyManager : public MaybeRenderEarlyManager {
 class MockFrameInfoHelper : public FrameInfoHelper,
                             public DestructionObservable {
  public:
-  MockFrameInfoHelper(MockFrameInfoHelper** thiz) { *thiz = this; }
-
-  void GetFrameInfo(
-      std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
-      base::OnceCallback<
-          void(std::unique_ptr<CodecOutputBufferRenderer>, FrameInfo, bool)> cb)
-      override {
-    MockGetFrameInfo(buffer_renderer.get());
-    cb_ = std::move(cb);
-    buffer_renderer_ = std::move(buffer_renderer);
-
-    if (run_callback_automatically_) {
-      RunWithYcbCrInfo(true);
-      base::RunLoop().RunUntilIdle();
-    }
-  }
-
-  void RunWithYcbCrInfo(bool success) {
-    DCHECK(buffer_renderer_);
-
+  void GetFrameInfo(std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer,
+                    FrameInfoReadyCB cb) override {
     FrameInfo info;
-    info.coded_size = buffer_renderer_->size();
+    info.coded_size = buffer_renderer->size();
     info.visible_rect = gfx::Rect(info.coded_size);
 
-    std::move(cb_).Run(std::move(buffer_renderer_), info, success);
-  }
-
-  void set_run_callback_automatically(bool run_callback_automatically) {
-    run_callback_automatically_ = run_callback_automatically;
+    std::move(cb).Run(std::move(buffer_renderer), info);
   }
-
-  MOCK_METHOD1(MockGetFrameInfo,
-               void(CodecOutputBufferRenderer* buffer_renderer));
-
- private:
-  bool run_callback_automatically_ = true;
-  base::OnceCallback<
-      void(std::unique_ptr<CodecOutputBufferRenderer>, FrameInfo, bool)>
-      cb_;
-  std::unique_ptr<CodecOutputBufferRenderer> buffer_renderer_;
 };
 
 class VideoFrameFactoryImplTest : public testing::Test {
@@ -96,15 +64,11 @@ class VideoFrameFactoryImplTest : public testing::Test {
     auto mre_manager = std::make_unique<MockMaybeRenderEarlyManager>();
     mre_manager_raw_ = mre_manager.get();
 
-    auto ycbcr_helper = base::SequenceBound<MockFrameInfoHelper>(
-        task_runner_, &ycbcr_helper_raw_);
-    base::RunLoop().RunUntilIdle();  // Init |ycbcr_helper_raw_|.
-    ycbcr_destruction_observer_ =
-        ycbcr_helper_raw_->CreateDestructionObserver();
+    auto info_helper = std::make_unique<MockFrameInfoHelper>();
 
     impl_ = std::make_unique<VideoFrameFactoryImpl>(
         task_runner_, gpu_preferences_, std::move(image_provider),
-        std::move(mre_manager), std::move(ycbcr_helper));
+        std::move(mre_manager), std::move(info_helper));
     auto texture_owner = base::MakeRefCounted<NiceMock<gpu::MockTextureOwner>>(
         0, nullptr, nullptr, true);
     auto codec_buffer_wait_coordinator =
@@ -177,7 +141,6 @@ class VideoFrameFactoryImplTest : public testing::Test {
   // Sent to |impl_| by RequestVideoFrame..
   base::MockCallback<VideoFrameFactory::OnceOutputCB> output_cb_;
 
-  MockFrameInfoHelper* ycbcr_helper_raw_ = nullptr;
   std::unique_ptr<DestructionObserver> ycbcr_destruction_observer_;
 
   gpu::GpuPreferences gpu_preferences_;
@@ -272,75 +235,4 @@ TEST_F(VideoFrameFactoryImplTest,
   impl_ = nullptr;
   base::RunLoop().RunUntilIdle();
 }
-
-TEST_F(VideoFrameFactoryImplTest, DoesCallFrameInfoHelperIfVulkan) {
-  // We will be driving callback by ourselves in this test.
-  ycbcr_helper_raw_->set_run_callback_automatically(false);
-  // Expect call to get info for the first frame.
-  EXPECT_CALL(*ycbcr_helper_raw_, MockGetFrameInfo(_)).Times(1);
-
-  RequestVideoFrame();
-
-  // Provide info.  It should send image request.
-  ycbcr_helper_raw_->RunWithYcbCrInfo(true);
-  base::RunLoop().RunUntilIdle();
-
-  testing::Mock::VerifyAndClearExpectations(ycbcr_helper_raw_);
-
-  // Fulfilling image request should provide video frame.
-  EXPECT_CALL(output_cb_, Run(_)).Times(1);
-
-  auto image_record = MakeImageRecord();
-  image_provider_raw_->ProvideOneRequestedImage(&image_record);
-  base::RunLoop().RunUntilIdle();
-
-  // Verify that no more calls happen, since we don't want thread hops on every
-  // frame.  Note that multiple could be dispatched before now.  It should still
-  // send along a VideoFrame, though.
-  EXPECT_CALL(*ycbcr_helper_raw_, MockGetFrameInfo(_)).Times(0);
-  EXPECT_CALL(output_cb_, Run(_)).Times(1);
-
-  RequestVideoFrame();
-  auto other_image_record = MakeImageRecord();
-  // If the helper hasn't been destroyed, then we don't expect it to be called.
-  image_provider_raw_->ProvideOneRequestedImage(&other_image_record);
-  base::RunLoop().RunUntilIdle();
-}
-
-TEST_F(VideoFrameFactoryImplTest, NullYCbCrInfoDoesntCrash) {
-  // We will be driving callback by ourselves in this test.
-  ycbcr_helper_raw_->set_run_callback_automatically(false);
-
-  // Expect call to get info for the first frame.
-  EXPECT_CALL(*ycbcr_helper_raw_, MockGetFrameInfo(_)).Times(1);
-
-  RequestVideoFrame();
-
-  // Provide info.  It should send image request.
-  ycbcr_helper_raw_->RunWithYcbCrInfo(false);
-  base::RunLoop().RunUntilIdle();
-
-  testing::Mock::VerifyAndClearExpectations(ycbcr_helper_raw_);
-
-  // Fulfilling image request should provide video frame.
-  EXPECT_CALL(output_cb_, Run(_)).Times(1);
-
-  auto image_record = MakeImageRecord();
-  image_provider_raw_->ProvideOneRequestedImage(&image_record);
-  base::RunLoop().RunUntilIdle();
-
-  // Verify that we will get call to GetFrameInfo as previous one failed.
-  EXPECT_CALL(*ycbcr_helper_raw_, MockGetFrameInfo(_)).Times(1);
-  EXPECT_CALL(output_cb_, Run(_)).Times(1);
-
-  RequestVideoFrame();
-  ycbcr_helper_raw_->RunWithYcbCrInfo(true);
-  base::RunLoop().RunUntilIdle();
-
-  auto other_image_record = MakeImageRecord();
-  // If the helper hasn't been destroyed, then we don't expect it to be called.
-  image_provider_raw_->ProvideOneRequestedImage(&other_image_record);
-  base::RunLoop().RunUntilIdle();
-}
-
 }  // namespace media