Revert "Reland "Fix video quad rect and surface output rect""

This reverts commit 56f3274d.

Reason for revert: This broke VideoFrameSubmitterTest on Linux ChromiumOS MSan Tests.

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20ChromiumOS%20MSan%20Tests/9636


Original change's description:
> 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/1338418
> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
> Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#608596}

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

Change-Id: I55b589ea5325317bc271928d9d2b388bec344174
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 904035
Reviewed-on: https://chromium-review.googlesource.com/c/1338586Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608653}

cc/layers/video_layer_impl.cc

@@ -122,8 +122,6 @@ 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_) {
@@ -145,19 +143,20 @@ 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(quad_rect);
+      occlusion_in_video_space.GetUnoccludedContentRect(
+          gfx::Rect(rotated_size));
   if (visible_quad_rect.IsEmpty())
     return;
 
-  updater_->AppendQuads(
-      render_pass, frame_, transform, quad_rect, visible_quad_rect, clip_rect(),
-      is_clipped(), contents_opaque(), draw_opacity(), GetSortingContextId());
+  updater_->AppendQuads(render_pass, frame_, transform, rotated_size,
+                        visible_quad_rect, clip_rect(), is_clipped(),
+                        contents_opaque(), draw_opacity(),
+                        GetSortingContextId(), visible_quad_rect);
 }
 
 void VideoLayerImpl::DidDraw(viz::ClientResourceProvider* resource_provider) {

media/renderers/video_resource_updater.cc

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

media/renderers/video_resource_updater.h

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

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

@@ -72,51 +72,57 @@ 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();
-  // 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());
+  gfx::Size rotated_size = frame->coded_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, -quad_rect.height());
+      transform.Translate(0.0, -rotated_size.height());
       break;
     case media::VIDEO_ROTATION_180:
       transform.Rotate(180.0);
-      transform.Translate(-quad_rect.width(), -quad_rect.height());
+      transform.Translate(-rotated_size.width(), -rotated_size.height());
       break;
     case media::VIDEO_ROTATION_270:
+      rotated_size = gfx::Size(rotated_size.height(), rotated_size.width());
       transform.Rotate(270.0);
-      transform.Translate(-quad_rect.width(), 0);
+      transform.Translate(-rotated_size.width(), 0);
       break;
     case media::VIDEO_ROTATION_0:
       break;
   }
 
-  gfx::Rect visible_quad_rect = quad_rect;
-  gfx::Rect clip_rect;
+  // 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());
   bool is_clipped = false;
   float draw_opacity = 1.0f;
   int sorting_context_id = 0;
 
-  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);
+  // 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);
 }
 
 void VideoFrameResourceProvider::ReleaseFrameResources() {

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

@@ -242,22 +242,16 @@ bool VideoFrameSubmitter::SubmitFrame(
   if (!compositor_frame_sink_ || !ShouldSubmit())
     return false;
 
-  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_ != gfx::Rect(video_frame->coded_size())) {
     if (!frame_size_.IsEmpty())
       child_local_surface_id_allocator_.GenerateId();
-    frame_size_ = frame_size;
+    frame_size_ = gfx::Rect(video_frame->coded_size());
   }
 
   viz::CompositorFrame compositor_frame;
   std::unique_ptr<viz::RenderPass> render_pass = viz::RenderPass::Create();
 
-  render_pass->SetNew(1, gfx::Rect(frame_size_), gfx::Rect(frame_size_),
-                      gfx::Transform());
+  render_pass->SetNew(1, frame_size_, frame_size_, gfx::Transform());
   render_pass->filters = cc::FilterOperations();
   resource_provider_->AppendQuads(render_pass.get(), video_frame, rotation_,
                                   is_opaque_);
@@ -308,8 +302,7 @@ void VideoFrameSubmitter::SubmitEmptyFrame() {
   compositor_frame.metadata.may_contain_video = true;
 
   std::unique_ptr<viz::RenderPass> render_pass = viz::RenderPass::Create();
-  render_pass->SetNew(1, gfx::Rect(frame_size_), gfx::Rect(frame_size_),
-                      gfx::Transform());
+  render_pass->SetNew(1, frame_size_, 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::Size frame_size_;
+  gfx::Rect 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,10 +54,6 @@ 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());
 
@@ -68,8 +64,7 @@ class MockCompositorFrameSink : public viz::mojom::blink::CompositorFrameSink {
       viz::CompositorFrame frame,
       viz::mojom::blink::HitTestRegionListPtr hit_test_region_list,
       uint64_t submit_time) override {
-    last_submitted_compositor_frame_ = std::move(frame);
-    DoSubmitCompositorFrame(id, &last_submitted_compositor_frame_);
+    DoSubmitCompositorFrame(id, &frame);
   }
   void SubmitCompositorFrameSync(
       const viz::LocalSurfaceId& id,
@@ -77,8 +72,7 @@ class MockCompositorFrameSink : public viz::mojom::blink::CompositorFrameSink {
       viz::mojom::blink::HitTestRegionListPtr hit_test_region_list,
       uint64_t submit_time,
       const SubmitCompositorFrameSyncCallback callback) override {
-    last_submitted_compositor_frame_ = std::move(frame);
-    DoSubmitCompositorFrame(id, &last_submitted_compositor_frame_);
+    DoSubmitCompositorFrame(id, &frame);
   }
 
   MOCK_METHOD1(DidNotProduceFrame, void(const viz::BeginFrameAck&));
@@ -99,8 +93,6 @@ 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);
 };
 
@@ -810,7 +802,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::Size(8, 8), submitter_->frame_size_);
+    EXPECT_EQ(gfx::Rect(8, 8), submitter_->frame_size_);
   }
 
   EXPECT_CALL(*video_frame_provider_, GetCurrentFrame())
@@ -835,112 +827,7 @@ 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::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);
+    EXPECT_EQ(gfx::Rect(2, 2), submitter_->frame_size_);
   }
 }