Reland "Fix video quad rect and surface output rect"

This is a reland of 1dfd4418

Original change's description:
> Fix video quad rect and surface output rect
>
> Video surface output rect didn't take rotation into account so when
> SurfaceAggregator would stretch content to fill bounds, it would end up
> with the wrong scaling factors (squash in one dimension and expand in
> another) since the surface quad's bounds (in the embedder) were rotated.
>
> To work around this (or by accident), VideoFrameResourceProvider was
> passing the rotated size as the quad's rect which doesn't make sense
> intuitively.  This also worked by accident with DirectComposition
> overlays because of applying another scaling which would fix the aspect
> ratio again.
>
> This change makes it possible to use the |quad_to_target_transform| as
> is, without having to apply an aspect ratio correcting scaling, and to
> assume that the quad's rect is in the pre-transform space.
>
> Bug: 904035
> Change-Id: Ia55e44f1f2b49b8d368a97af54f3ce9d90a81234
> Reviewed-on: https://chromium-review.googlesource.com/c/1334971
> Reviewed-by: enne <enne@chromium.org>
> Reviewed-by: Kenneth Russell <kbr@chromium.org>
> Reviewed-by: Frank Liberato <liberato@chromium.org>
> Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#608495}

TBR=enne@chromium.org,kbr@chromium.org,liberato@chromium.org

Bug: 904035
Change-Id: I278a0ea2c5507f01ea13b31d2090b5f99537c7c3
Reviewed-on: https://chromium-review.googlesource.com/c/1338418Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608596}

cc/layers/video_layer_impl.cc

@@ -122,6 +122,8 @@ void VideoLayerImpl::AppendQuads(viz::RenderPass* render_pass,
   DCHECK(frame_.get());
 
   gfx::Transform transform = DrawTransform();
+  // bounds() is in post-rotation space so quad rect in content space must be
+  // in pre-rotation space
   gfx::Size rotated_size = bounds();
 
   switch (video_rotation_) {
@@ -143,20 +145,19 @@ void VideoLayerImpl::AppendQuads(viz::RenderPass* render_pass,
       break;
   }
 
+  gfx::Rect quad_rect(rotated_size);
   Occlusion occlusion_in_video_space =
       draw_properties()
           .occlusion_in_content_space.GetOcclusionWithGivenDrawTransform(
               transform);
   gfx::Rect visible_quad_rect =
-      occlusion_in_video_space.GetUnoccludedContentRect(
-          gfx::Rect(rotated_size));
+      occlusion_in_video_space.GetUnoccludedContentRect(quad_rect);
   if (visible_quad_rect.IsEmpty())
     return;
 
-  updater_->AppendQuads(render_pass, frame_, transform, rotated_size,
-                        visible_quad_rect, clip_rect(), is_clipped(),
-                        contents_opaque(), draw_opacity(),
-                        GetSortingContextId(), visible_quad_rect);
+  updater_->AppendQuads(
+      render_pass, frame_, transform, quad_rect, visible_quad_rect, clip_rect(),
+      is_clipped(), contents_opaque(), draw_opacity(), GetSortingContextId());
 }
 
 void VideoLayerImpl::DidDraw(viz::ClientResourceProvider* resource_provider) {

media/renderers/video_resource_updater.cc

@@ -404,26 +404,24 @@ void VideoResourceUpdater::ReleaseFrameResources() {
 void VideoResourceUpdater::AppendQuads(viz::RenderPass* render_pass,
                                        scoped_refptr<VideoFrame> frame,
                                        gfx::Transform transform,
-                                       gfx::Size rotated_size,
-                                       gfx::Rect visible_layer_rect,
+                                       gfx::Rect quad_rect,
+                                       gfx::Rect visible_quad_rect,
                                        gfx::Rect clip_rect,
                                        bool is_clipped,
                                        bool contents_opaque,
                                        float draw_opacity,
-                                       int sorting_context_id,
-                                       gfx::Rect visible_quad_rect) {
+                                       int sorting_context_id) {
   DCHECK(frame.get());
 
   viz::SharedQuadState* shared_quad_state =
       render_pass->CreateAndAppendSharedQuadState();
-  gfx::Rect rotated_size_rect(rotated_size);
-  shared_quad_state->SetAll(
-      transform, rotated_size_rect, visible_layer_rect, clip_rect, is_clipped,
-      contents_opaque, draw_opacity, SkBlendMode::kSrcOver, sorting_context_id);
+  shared_quad_state->SetAll(transform, quad_rect, visible_quad_rect, clip_rect,
+                            is_clipped, contents_opaque, draw_opacity,
+                            SkBlendMode::kSrcOver, sorting_context_id);
 
-  gfx::Rect quad_rect(rotated_size);
-  gfx::Rect visible_rect = frame->visible_rect();
   bool needs_blending = !contents_opaque;
+
+  gfx::Rect visible_rect = frame->visible_rect();
   gfx::Size coded_size = frame->coded_size();
 
   const float tex_width_scale =

media/renderers/video_resource_updater.h

@@ -105,14 +105,13 @@ class MEDIA_EXPORT VideoResourceUpdater
   void AppendQuads(viz::RenderPass* render_pass,
                    scoped_refptr<VideoFrame> frame,
                    gfx::Transform transform,
-                   gfx::Size rotated_size,
-                   gfx::Rect visible_layer_rect,
+                   gfx::Rect quad_rect,
+                   gfx::Rect visible_quad_rect,
                    gfx::Rect clip_rect,
                    bool is_clipped,
                    bool context_opaque,
                    float draw_opacity,
-                   int sorting_context_id,
-                   gfx::Rect visible_quad_rect);
+                   int sorting_context_id);
 
   // TODO(kylechar): This is only public for testing, make private.
   VideoFrameExternalResources CreateExternalResourcesFromVideoFrame(

third_party/blink/renderer/platform/graphics/video_frame_resource_provider.cc

@@ -72,57 +72,51 @@ void VideoFrameResourceProvider::AppendQuads(
   DCHECK(resource_updater_);
   DCHECK(resource_provider_);
 
+  // When obtaining frame resources, we end up having to wait. See
+  // https://crbug/878070.
+  // Unfortunately, we have no idea if blocking is allowed on the current thread
+  // or not.  If we're on the cc impl thread, the answer is yes, and further
+  // the thread is marked as not allowing blocking primitives.  On the various
+  // media threads, however, blocking is not allowed but the blocking scopes
+  // are.  So, we use ScopedAllow only if we're told that we should do so.
+  if (use_sync_primitives_) {
+    base::ScopedAllowBaseSyncPrimitives allow_base_sync_primitives;
+    resource_updater_->ObtainFrameResources(frame);
+  } else {
+    resource_updater_->ObtainFrameResources(frame);
+  }
+
   gfx::Transform transform = gfx::Transform();
-  gfx::Size rotated_size = frame->coded_size();
+  // The quad's rect is in pre-transform space so that applying the transform on
+  // it will produce the bounds in target space.
+  gfx::Rect quad_rect = gfx::Rect(frame->natural_size());
 
   switch (rotation) {
     case media::VIDEO_ROTATION_90:
-      rotated_size = gfx::Size(rotated_size.height(), rotated_size.width());
       transform.Rotate(90.0);
-      transform.Translate(0.0, -rotated_size.height());
+      transform.Translate(0.0, -quad_rect.height());
       break;
     case media::VIDEO_ROTATION_180:
       transform.Rotate(180.0);
-      transform.Translate(-rotated_size.width(), -rotated_size.height());
+      transform.Translate(-quad_rect.width(), -quad_rect.height());
       break;
     case media::VIDEO_ROTATION_270:
-      rotated_size = gfx::Size(rotated_size.height(), rotated_size.width());
       transform.Rotate(270.0);
-      transform.Translate(-rotated_size.width(), 0);
+      transform.Translate(-quad_rect.width(), 0);
       break;
     case media::VIDEO_ROTATION_0:
       break;
   }
 
-  // When obtaining frame resources, we end up having to wait. See
-  // https://crbug/878070.
-  // Unfortunately, we have no idea if blocking is allowed on the current thread
-  // or not.  If we're on the cc impl thread, the answer is yes, and further
-  // the thread is marked as not allowing blocking primitives.  On the various
-  // media threads, however, blocking is not allowed but the blocking scopes
-  // are.  So, we use ScopedAllow only if we're told that we should do so.
-  if (use_sync_primitives_) {
-    base::ScopedAllowBaseSyncPrimitives allow_base_sync_primitives;
-    resource_updater_->ObtainFrameResources(frame);
-  } else {
-    resource_updater_->ObtainFrameResources(frame);
-  }
-
-  // TODO(lethalantidote) : update with true value;
-  gfx::Rect visible_layer_rect = gfx::Rect(rotated_size);
-  gfx::Rect clip_rect = gfx::Rect(frame->coded_size());
+  gfx::Rect visible_quad_rect = quad_rect;
+  gfx::Rect clip_rect;
   bool is_clipped = false;
   float draw_opacity = 1.0f;
   int sorting_context_id = 0;
 
-  // Internal to this compositor frame, this video quad is never occluded,
-  // thus the full quad is visible.
-  gfx::Rect visible_quad_rect = gfx::Rect(rotated_size);
-
-  resource_updater_->AppendQuads(render_pass, std::move(frame), transform,
-                                 rotated_size, visible_layer_rect, clip_rect,
-                                 is_clipped, is_opaque, draw_opacity,
-                                 sorting_context_id, visible_quad_rect);
+  resource_updater_->AppendQuads(
+      render_pass, std::move(frame), transform, quad_rect, visible_quad_rect,
+      clip_rect, is_clipped, is_opaque, draw_opacity, sorting_context_id);
 }
 
 void VideoFrameResourceProvider::ReleaseFrameResources() {

third_party/blink/renderer/platform/graphics/video_frame_submitter.cc

@@ -242,16 +242,22 @@ bool VideoFrameSubmitter::SubmitFrame(
   if (!compositor_frame_sink_ || !ShouldSubmit())
     return false;
 
-  if (frame_size_ != gfx::Rect(video_frame->coded_size())) {
+  gfx::Size frame_size(video_frame->natural_size());
+  if (rotation_ == media::VIDEO_ROTATION_90 ||
+      rotation_ == media::VIDEO_ROTATION_270) {
+    frame_size = gfx::Size(frame_size.height(), frame_size.width());
+  }
+  if (frame_size_ != frame_size) {
     if (!frame_size_.IsEmpty())
       child_local_surface_id_allocator_.GenerateId();
-    frame_size_ = gfx::Rect(video_frame->coded_size());
+    frame_size_ = frame_size;
   }
 
   viz::CompositorFrame compositor_frame;
   std::unique_ptr<viz::RenderPass> render_pass = viz::RenderPass::Create();
 
-  render_pass->SetNew(1, frame_size_, frame_size_, gfx::Transform());
+  render_pass->SetNew(1, gfx::Rect(frame_size_), gfx::Rect(frame_size_),
+                      gfx::Transform());
   render_pass->filters = cc::FilterOperations();
   resource_provider_->AppendQuads(render_pass.get(), video_frame, rotation_,
                                   is_opaque_);
@@ -302,7 +308,8 @@ void VideoFrameSubmitter::SubmitEmptyFrame() {
   compositor_frame.metadata.may_contain_video = true;
 
   std::unique_ptr<viz::RenderPass> render_pass = viz::RenderPass::Create();
-  render_pass->SetNew(1, frame_size_, frame_size_, gfx::Transform());
+  render_pass->SetNew(1, gfx::Rect(frame_size_), gfx::Rect(frame_size_),
+                      gfx::Transform());
   compositor_frame.render_pass_list.push_back(std::move(render_pass));
 
   compositor_frame_sink_->SubmitCompositorFrame(

third_party/blink/renderer/platform/graphics/video_frame_submitter.h

@@ -142,7 +142,7 @@ class PLATFORM_EXPORT VideoFrameSubmitter
   // Size of the video frame being submitted. It is set the first time a frame
   // is submitted. Every time there is a change in the video frame size, the
   // child component of the LocalSurfaceId will be updated.
-  gfx::Rect frame_size_;
+  gfx::Size frame_size_;
 
   // Used to updated the LocalSurfaceId when detecting a change in video frame
   // size.

third_party/blink/renderer/platform/graphics/video_frame_submitter_test.cc

@@ -54,6 +54,10 @@ class MockCompositorFrameSink : public viz::mojom::blink::CompositorFrameSink {
       : binding_(this, std::move(*request)) {}
   ~MockCompositorFrameSink() override = default;
 
+  const viz::CompositorFrame& last_submitted_compositor_frame() const {
+    return last_submitted_compositor_frame_;
+  }
+
   MOCK_METHOD1(SetNeedsBeginFrame, void(bool));
   MOCK_METHOD0(SetWantsAnimateOnlyBeginFrames, void());
 
@@ -64,7 +68,8 @@ class MockCompositorFrameSink : public viz::mojom::blink::CompositorFrameSink {
       viz::CompositorFrame frame,
       viz::mojom::blink::HitTestRegionListPtr hit_test_region_list,
       uint64_t submit_time) override {
-    DoSubmitCompositorFrame(id, &frame);
+    last_submitted_compositor_frame_ = std::move(frame);
+    DoSubmitCompositorFrame(id, &last_submitted_compositor_frame_);
   }
   void SubmitCompositorFrameSync(
       const viz::LocalSurfaceId& id,
@@ -72,7 +77,8 @@ class MockCompositorFrameSink : public viz::mojom::blink::CompositorFrameSink {
       viz::mojom::blink::HitTestRegionListPtr hit_test_region_list,
       uint64_t submit_time,
       const SubmitCompositorFrameSyncCallback callback) override {
-    DoSubmitCompositorFrame(id, &frame);
+    last_submitted_compositor_frame_ = std::move(frame);
+    DoSubmitCompositorFrame(id, &last_submitted_compositor_frame_);
   }
 
   MOCK_METHOD1(DidNotProduceFrame, void(const viz::BeginFrameAck&));
@@ -93,6 +99,8 @@ class MockCompositorFrameSink : public viz::mojom::blink::CompositorFrameSink {
  private:
   mojo::Binding<viz::mojom::blink::CompositorFrameSink> binding_;
 
+  viz::CompositorFrame last_submitted_compositor_frame_;
+
   DISALLOW_COPY_AND_ASSIGN(MockCompositorFrameSink);
 };
 
@@ -802,7 +810,7 @@ TEST_F(VideoFrameSubmitterTest, FrameSizeChangeUpdatesLocalSurfaceId) {
     EXPECT_EQ(11u, local_surface_id.parent_sequence_number());
     EXPECT_EQ(viz::kInitialChildSequenceNumber,
               local_surface_id.child_sequence_number());
-    EXPECT_EQ(gfx::Rect(8, 8), submitter_->frame_size_);
+    EXPECT_EQ(gfx::Size(8, 8), submitter_->frame_size_);
   }
 
   EXPECT_CALL(*video_frame_provider_, GetCurrentFrame())
@@ -827,7 +835,112 @@ TEST_F(VideoFrameSubmitterTest, FrameSizeChangeUpdatesLocalSurfaceId) {
     EXPECT_EQ(11u, local_surface_id.parent_sequence_number());
     EXPECT_EQ(viz::kInitialChildSequenceNumber + 1,
               local_surface_id.child_sequence_number());
-    EXPECT_EQ(gfx::Rect(2, 2), submitter_->frame_size_);
+    EXPECT_EQ(gfx::Size(2, 2), submitter_->frame_size_);
+  }
+}
+
+TEST_F(VideoFrameSubmitterTest, VideoRotationOutputRect) {
+  MakeSubmitter();
+  EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
+  submitter_->StartRendering();
+  EXPECT_TRUE(submitter_->Rendering());
+
+  gfx::Size coded_size(1280, 720);
+  gfx::Size natural_size(1280, 1024);
+  gfx::Size rotated_size(1024, 1280);
+
+  {
+    submitter_->SetRotation(media::VideoRotation::VIDEO_ROTATION_90);
+
+    EXPECT_CALL(*video_frame_provider_, UpdateCurrentFrame(_, _))
+        .WillOnce(Return(true));
+    EXPECT_CALL(*video_frame_provider_, GetCurrentFrame())
+        .WillOnce(Return(media::VideoFrame::CreateFrame(
+            media::PIXEL_FORMAT_YV12, coded_size, gfx::Rect(coded_size),
+            natural_size, base::TimeDelta())));
+    EXPECT_CALL(*sink_, DoSubmitCompositorFrame(_, _));
+    EXPECT_CALL(*video_frame_provider_, PutCurrentFrame());
+    EXPECT_CALL(*resource_provider_,
+                AppendQuads(_, _, media::VideoRotation::VIDEO_ROTATION_90, _));
+    EXPECT_CALL(*resource_provider_, PrepareSendToParent(_, _));
+    EXPECT_CALL(*resource_provider_, ReleaseFrameResources());
+
+    viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
+        BEGINFRAME_FROM_HERE, now_src_.get());
+    submitter_->OnBeginFrame(args, {});
+    scoped_task_environment_.RunUntilIdle();
+
+    EXPECT_EQ(sink_->last_submitted_compositor_frame().size_in_pixels(),
+              rotated_size);
+
+    submitter_->DidReceiveFrame();
+
+    WTF::Vector<viz::ReturnedResource> resources;
+    EXPECT_CALL(*resource_provider_, ReceiveReturnsFromParent(_));
+    submitter_->DidReceiveCompositorFrameAck(resources);
+  }
+
+  {
+    submitter_->SetRotation(media::VideoRotation::VIDEO_ROTATION_180);
+
+    EXPECT_CALL(*video_frame_provider_, UpdateCurrentFrame(_, _))
+        .WillOnce(Return(true));
+    EXPECT_CALL(*video_frame_provider_, GetCurrentFrame())
+        .WillOnce(Return(media::VideoFrame::CreateFrame(
+            media::PIXEL_FORMAT_YV12, coded_size, gfx::Rect(coded_size),
+            natural_size, base::TimeDelta())));
+    EXPECT_CALL(*sink_, DoSubmitCompositorFrame(_, _));
+    EXPECT_CALL(*video_frame_provider_, PutCurrentFrame());
+    EXPECT_CALL(*resource_provider_,
+                AppendQuads(_, _, media::VideoRotation::VIDEO_ROTATION_180, _));
+    EXPECT_CALL(*resource_provider_, PrepareSendToParent(_, _));
+    EXPECT_CALL(*resource_provider_, ReleaseFrameResources());
+
+    viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
+        BEGINFRAME_FROM_HERE, now_src_.get());
+    submitter_->OnBeginFrame(args, {});
+    scoped_task_environment_.RunUntilIdle();
+
+    // 180 deg rotation has same size.
+    EXPECT_EQ(sink_->last_submitted_compositor_frame().size_in_pixels(),
+              natural_size);
+
+    submitter_->DidReceiveFrame();
+
+    WTF::Vector<viz::ReturnedResource> resources;
+    EXPECT_CALL(*resource_provider_, ReceiveReturnsFromParent(_));
+    submitter_->DidReceiveCompositorFrameAck(resources);
+  }
+
+  {
+    submitter_->SetRotation(media::VideoRotation::VIDEO_ROTATION_270);
+
+    EXPECT_CALL(*video_frame_provider_, UpdateCurrentFrame(_, _))
+        .WillOnce(Return(true));
+    EXPECT_CALL(*video_frame_provider_, GetCurrentFrame())
+        .WillOnce(Return(media::VideoFrame::CreateFrame(
+            media::PIXEL_FORMAT_YV12, coded_size, gfx::Rect(coded_size),
+            natural_size, base::TimeDelta())));
+    EXPECT_CALL(*sink_, DoSubmitCompositorFrame(_, _));
+    EXPECT_CALL(*video_frame_provider_, PutCurrentFrame());
+    EXPECT_CALL(*resource_provider_,
+                AppendQuads(_, _, media::VideoRotation::VIDEO_ROTATION_270, _));
+    EXPECT_CALL(*resource_provider_, PrepareSendToParent(_, _));
+    EXPECT_CALL(*resource_provider_, ReleaseFrameResources());
+
+    viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
+        BEGINFRAME_FROM_HERE, now_src_.get());
+    submitter_->OnBeginFrame(args, {});
+    scoped_task_environment_.RunUntilIdle();
+
+    EXPECT_EQ(sink_->last_submitted_compositor_frame().size_in_pixels(),
+              rotated_size);
+
+    submitter_->DidReceiveFrame();
+
+    WTF::Vector<viz::ReturnedResource> resources;
+    EXPECT_CALL(*resource_provider_, ReceiveReturnsFromParent(_));
+    submitter_->DidReceiveCompositorFrameAck(resources);
   }
 }