Set YCbCrInfo properly for Vulkan in VideoFrameFactoryImpl.

When VideoFrameFactoryImpl started initializing the CodecImage
itself rather than delegating to the SharedImageVideoProvider, the
YCbCrInfo for Vulkan wasn't being set; the provider asked the
(now uninitialized) CodecImage.

This CL makes VideoFrameFactoryImpl cache the YCbCr info itself,
using an extra thread hop to the gpu main thread.  It does this just
for the first few frames, until it receives the YCbCr info once.
Then, it re-uses it for all subsequent frames.

Change-Id: Ie167b4435b3ece6ec71dfd2f1a36250e7bc4d781
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1797107
Reviewed-by: Vikas Soni <vikassoni@google.com>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697738}

gpu/command_buffer/service/shared_image_video.cc

@@ -180,20 +180,22 @@ void SharedImageVideo::OnContextLost() {
   context_state_ = nullptr;
 }
 
-base::Optional<VulkanYCbCrInfo> SharedImageVideo::GetYcbcrInfo() {
+base::Optional<VulkanYCbCrInfo> SharedImageVideo::GetYcbcrInfo(
+    StreamTextureSharedImageInterface* stream_texture_sii,
+    scoped_refptr<SharedContextState> context_state) {
   // For non-vulkan context, return null.
-  if (!context_state_->GrContextIsVulkan())
+  if (!context_state->GrContextIsVulkan())
     return base::nullopt;
 
   // GetAHardwareBuffer() renders the latest image and gets AHardwareBuffer
   // from it.
-  auto scoped_hardware_buffer = stream_texture_sii_->GetAHardwareBuffer();
+  auto scoped_hardware_buffer = stream_texture_sii->GetAHardwareBuffer();
   if (!scoped_hardware_buffer) {
     return base::nullopt;
   }
 
   DCHECK(scoped_hardware_buffer->buffer());
-  auto* context_provider = context_state_->vk_context_provider();
+  auto* context_provider = context_state->vk_context_provider();
   VulkanImplementation* vk_implementation =
       context_provider->GetVulkanImplementation();
   VkDevice vk_device = context_provider->GetDeviceQueue()->GetVulkanDevice();

gpu/command_buffer/service/shared_image_video.h

@@ -54,7 +54,9 @@ class GPU_GLES2_EXPORT SharedImageVideo
 
   // Returns ycbcr information. This is only valid in vulkan context and
   // nullopt for other context.
-  base::Optional<VulkanYCbCrInfo> GetYcbcrInfo();
+  static base::Optional<VulkanYCbCrInfo> GetYcbcrInfo(
+      StreamTextureSharedImageInterface* stream_texture_sii,
+      scoped_refptr<SharedContextState> context_state);
 
  protected:
   std::unique_ptr<SharedImageRepresentationGLTexture> ProduceGLTexture(

media/base/android/test_destruction_observable.h

@@ -44,6 +44,9 @@ class DestructionObserver {
   // expectation.
   void DoNotAllowDestruction();
 
+  // Return if the object has been destroyed.
+  bool destructed() const { return destructed_; }
+
  private:
   void VerifyExpectations();
   void OnObservableDestructed();

media/gpu/BUILD.gn

@@ -133,6 +133,8 @@ component("gpu") {
       "android/video_frame_factory.h",
       "android/video_frame_factory_impl.cc",
       "android/video_frame_factory_impl.h",
+      "android/ycbcr_helper.cc",
+      "android/ycbcr_helper.h",
     ]
     libs += [ "android" ]
     deps += [

media/gpu/android/direct_shared_image_video_provider.cc

@@ -122,6 +122,8 @@ void GpuSharedImageVideoFactory::Initialize(
     return;
   }
 
+  is_vulkan_ = shared_context->GrContextIsVulkan();
+
   // Make the shared context current.
   auto scoped_current = std::make_unique<ui::ScopedMakeCurrent>(
       shared_context->context(), shared_context->surface());
@@ -172,7 +174,8 @@ void GpuSharedImageVideoFactory::CreateImage(
   SharedImageVideoProvider::ImageRecord record;
   record.mailbox = mailbox;
   record.release_cb = std::move(release_cb);
-  record.ycbcr_info = ycbcr_info_;
+  record.is_vulkan = is_vulkan_;
+
   // Since |codec_image|'s ref holders can be destroyed by stub destruction, we
   // create a ref to it for the MaybeRenderEarlyManager.  This is a hack; we
   // should not be sending the CodecImage at all.  The MaybeRenderEarlyManager
@@ -243,9 +246,6 @@ bool GpuSharedImageVideoFactory::CreateImageInternal(
       std::move(texture), std::move(shared_context),
       false /* is_thread_safe */);
 
-  if (!ycbcr_info_)
-    ycbcr_info_ = shared_image->GetYcbcrInfo();
-
   // Register it with shared image mailbox as well as legacy mailbox. This
   // keeps |shared_image| around until its destruction cb is called.
   // NOTE: Currently none of the video mailbox consumer uses shared image

media/gpu/android/direct_shared_image_video_provider.h

@@ -96,9 +96,7 @@ class GpuSharedImageVideoFactory
   // 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_;
+  bool is_vulkan_ = false;
 
   THREAD_CHECKER(thread_checker_);
 

media/gpu/android/shared_image_video_provider.h

@@ -53,16 +53,17 @@ class MEDIA_GPU_EXPORT SharedImageVideoProvider {
     // Mailbox to which this shared image is bound.
     gpu::Mailbox mailbox;
 
-    // Sampler conversion information which is used in vulkan context.
-    base::Optional<gpu::VulkanYCbCrInfo> ycbcr_info;
-
     // Release callback.  When this is called (or dropped), the image will be
     // considered to be unused.
     ReleaseCB release_cb;
 
     // CodecImage that one can use for MaybeRenderEarly.
     scoped_refptr<CodecImageHolder> codec_image_holder;
 
+    // Is the underlying context Vulkan?  If so, then one must provide YCbCrInfo
+    // with the VideoFrame.
+    bool is_vulkan = false;
+
    private:
     DISALLOW_COPY_AND_ASSIGN(ImageRecord);
   };

media/gpu/android/video_frame_factory_impl.cc

@@ -81,12 +81,14 @@ VideoFrameFactoryImpl::VideoFrameFactoryImpl(
     scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner,
     const gpu::GpuPreferences& gpu_preferences,
     std::unique_ptr<SharedImageVideoProvider> image_provider,
-    std::unique_ptr<MaybeRenderEarlyManager> mre_manager)
+    std::unique_ptr<MaybeRenderEarlyManager> mre_manager,
+    base::SequenceBound<YCbCrHelper> ycbcr_helper)
     : image_provider_(std::move(image_provider)),
       gpu_task_runner_(std::move(gpu_task_runner)),
       enable_threaded_texture_mailboxes_(
           gpu_preferences.enable_threaded_texture_mailboxes),
-      mre_manager_(std::move(mre_manager)) {}
+      mre_manager_(std::move(mre_manager)),
+      ycbcr_helper_(std::move(ycbcr_helper)) {}
 
 VideoFrameFactoryImpl::~VideoFrameFactoryImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -155,9 +157,9 @@ void VideoFrameFactoryImpl::CreateVideoFrame(
   SharedImageVideoProvider::ImageSpec spec(coded_size);
 
   auto image_ready_cb = base::BindOnce(
-      &VideoFrameFactoryImpl::OnImageReady, weak_factory_.GetWeakPtr(),
-      std::move(output_cb), timestamp, coded_size, natural_size,
-      std::move(output_buffer), codec_buffer_wait_coordinator_,
+      &VideoFrameFactoryImpl::CreateVideoFrame_OnImageReady,
+      weak_factory_.GetWeakPtr(), std::move(output_cb), timestamp, coded_size,
+      natural_size, std::move(output_buffer), codec_buffer_wait_coordinator_,
       std::move(promotion_hint_cb), pixel_format, overlay_mode_,
       enable_threaded_texture_mailboxes_, gpu_task_runner_);
 
@@ -169,7 +171,7 @@ void VideoFrameFactoryImpl::CreateVideoFrame(
 }
 
 // static
-void VideoFrameFactoryImpl::OnImageReady(
+void VideoFrameFactoryImpl::CreateVideoFrame_OnImageReady(
     base::WeakPtr<VideoFrameFactoryImpl> thiz,
     OnceOutputCb output_cb,
     base::TimeDelta timestamp,
@@ -198,8 +200,63 @@ void VideoFrameFactoryImpl::OnImageReady(
 
   // 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));
+  thiz->mre_manager()->AddCodecImage(record.codec_image_holder);
+
+  // In case we need to get the YCbCr info, take the image holder out of the
+  // record before we move it into |completion_cb|.
+  auto codec_image_holder = std::move(record.codec_image_holder);
+
+  // Doesn't need to be weak-ptr'd, since we're either calling it inline, or
+  // calling it from the YCbCr callback which is, itself weak-ptr'd.
+  auto completion_cb = base::BindOnce(
+      &VideoFrameFactoryImpl::CreateVideoFrame_Finish, thiz,
+      std::move(output_cb), timestamp, coded_size, natural_size,
+      std::move(codec_buffer_wait_coordinator), pixel_format, overlay_mode,
+      enable_threaded_texture_mailboxes, std::move(record));
+
+  // TODO(liberato): Use |ycbcr_helper_| as a signal about whether we're
+  // supposed to get YCbCr info or not, rather than requiring the provider to
+  // tell us.  Note that right now, we do have the helper even if we don't
+  // need it.  See GpuMojoMediaClient.
+  if (!thiz->ycbcr_info_ && record.is_vulkan) {
+    // We need YCbCr info to create the frame.  Post back to the gpu thread to
+    // do it.  Note that we might post multiple times before succeeding once,
+    // both because of failures and because we might get multiple requests to
+    // create frames on the mcvd thread, before the gpu thread returns one ycbcr
+    // info to us.  Either way, it's fine, since the helper also caches the
+    // info locally.  It won't render more frames than needed.
+    auto ycbcr_cb = BindToCurrentLoop(base::BindOnce(
+        &VideoFrameFactoryImpl::CreateVideoFrame_OnYCbCrInfo,
+        thiz->weak_factory_.GetWeakPtr(), std::move(completion_cb)));
+    thiz->ycbcr_helper_.Post(FROM_HERE, &YCbCrHelper::GetYCbCrInfo,
+                             std::move(codec_image_holder),
+                             std::move(ycbcr_cb));
+    return;
+  }
+
+  std::move(completion_cb).Run();
+}
+
+void VideoFrameFactoryImpl::CreateVideoFrame_OnYCbCrInfo(
+    base::OnceClosure completion_cb,
+    YCbCrHelper::OptionalInfo ycbcr_info) {
+  ycbcr_info_ = std::move(ycbcr_info);
+  // Clear the helper just to free it up, though we might continue to get
+  // callbacks from it if we've posted multiple requests.
+  ycbcr_helper_.Reset();
+  std::move(completion_cb).Run();
+}
 
+void VideoFrameFactoryImpl::CreateVideoFrame_Finish(
+    OnceOutputCb output_cb,
+    base::TimeDelta timestamp,
+    gfx::Size coded_size,
+    gfx::Size natural_size,
+    scoped_refptr<CodecBufferWaitCoordinator> codec_buffer_wait_coordinator,
+    VideoPixelFormat pixel_format,
+    OverlayMode overlay_mode,
+    bool enable_threaded_texture_mailboxes,
+    SharedImageVideoProvider::ImageRecord record) {
   gpu::MailboxHolder mailbox_holders[VideoFrame::kMaxPlanes];
   mailbox_holders[0] = gpu::MailboxHolder(record.mailbox, gpu::SyncToken(),
                                           GL_TEXTURE_EXTERNAL_OES);
@@ -222,7 +279,9 @@ void VideoFrameFactoryImpl::OnImageReady(
       pixel_format, mailbox_holders, VideoFrame::ReleaseMailboxCB(), coded_size,
       visible_rect, natural_size, timestamp);
 
-  frame->set_ycbcr_info(record.ycbcr_info);
+  // For Vulkan.
+  frame->set_ycbcr_info(ycbcr_info_);
+
   // If, for some reason, we failed to create a frame, then fail.  Note that we
   // don't need to call |release_cb|; dropping it is okay since the api says so.
   if (!frame) {

media/gpu/android/video_frame_factory_impl.h

@@ -10,6 +10,7 @@
 #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"
@@ -18,6 +19,7 @@
 #include "media/gpu/android/maybe_render_early_manager.h"
 #include "media/gpu/android/shared_image_video_provider.h"
 #include "media/gpu/android/video_frame_factory.h"
+#include "media/gpu/android/ycbcr_helper.h"
 #include "media/gpu/media_gpu_export.h"
 #include "ui/gl/gl_bindings.h"
 
@@ -44,7 +46,8 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
       scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner,
       const gpu::GpuPreferences& gpu_preferences,
       std::unique_ptr<SharedImageVideoProvider> image_provider,
-      std::unique_ptr<MaybeRenderEarlyManager> mre_manager);
+      std::unique_ptr<MaybeRenderEarlyManager> mre_manager,
+      base::SequenceBound<YCbCrHelper> ycbcr_helper);
   ~VideoFrameFactoryImpl() override;
 
   void Initialize(OverlayMode overlay_mode, InitCb init_cb) override;
@@ -76,7 +79,7 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
   //
   // Second, this way we don't care about the lifetime of |this|; |output_cb|
   // can worry about it.
-  static void OnImageReady(
+  static void CreateVideoFrame_OnImageReady(
       base::WeakPtr<VideoFrameFactoryImpl> thiz,
       OnceOutputCb output_cb,
       base::TimeDelta timestamp,
@@ -91,6 +94,23 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
       scoped_refptr<base::SequencedTaskRunner> gpu_task_runner,
       SharedImageVideoProvider::ImageRecord record);
 
+  // Callback to receive YCbCrInfo from |provider_| while creating a VideoFrame.
+  void CreateVideoFrame_OnYCbCrInfo(base::OnceClosure completion_cb,
+                                    YCbCrHelper::OptionalInfo ycbcr_info);
+
+  // Really create the VideoFrame, once we've tried to get the YCbCrInfo if it's
+  // needed for it.
+  void CreateVideoFrame_Finish(
+      OnceOutputCb output_cb,
+      base::TimeDelta timestamp,
+      gfx::Size coded_size,
+      gfx::Size natural_size,
+      scoped_refptr<CodecBufferWaitCoordinator> codec_buffer_wait_coordinator,
+      VideoPixelFormat pixel_format,
+      OverlayMode overlay_mode,
+      bool enable_threaded_texture_mailboxes,
+      SharedImageVideoProvider::ImageRecord record);
+
   MaybeRenderEarlyManager* mre_manager() const { return mre_manager_.get(); }
 
   std::unique_ptr<SharedImageVideoProvider> image_provider_;
@@ -111,6 +131,12 @@ class MEDIA_GPU_EXPORT VideoFrameFactoryImpl : public VideoFrameFactory {
 
   std::unique_ptr<MaybeRenderEarlyManager> mre_manager_;
 
+  // Sampler conversion information which is used in vulkan context.
+  YCbCrHelper::OptionalInfo ycbcr_info_;
+
+  // Optional helper to get the Vulkan YCbCrInfo.
+  base::SequenceBound<YCbCrHelper> ycbcr_helper_;
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   base::WeakPtrFactory<VideoFrameFactoryImpl> weak_factory_{this};