VideoSurfaceLayer: use ChildLocalSurfaceIdAllocator in VideoFrameSubmitter.

This allows the submitter to update the LocalSurfaceId (by modifying the
child component) before submitting a new frame. The SurfaceLayerBridge
will be notified of the change and update its internal surface id.

Bug: 868008
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iee17f20c753cc236b9ccce5d5bdfe05fb655b08e
Reviewed-on: https://chromium-review.googlesource.com/1157882Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579812}

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

@@ -58,12 +58,14 @@ void VideoFrameSubmitter::SetIsOpaque(bool is_opaque) {
 }
 
 void VideoFrameSubmitter::EnableSubmission(
-    viz::SurfaceId id,
+    viz::SurfaceId surface_id,
     WebFrameSinkDestroyedCallback frame_sink_destroyed_callback) {
   // TODO(lethalantidote): Set these fields earlier in the constructor. Will
   // need to construct VideoFrameSubmitter later in order to do this.
-  surface_id_ = id;
+  frame_sink_id_ = surface_id.frame_sink_id();
   frame_sink_destroyed_callback_ = frame_sink_destroyed_callback;
+  child_local_surface_id_allocator_.UpdateFromParent(
+      surface_id.local_surface_id());
   if (resource_provider_->IsInitialized())
     StartSubmitting();
 }
@@ -180,13 +182,13 @@ void VideoFrameSubmitter::OnReceivedContextProvider(
     resource_provider_->Initialize(nullptr, this);
   }
 
-  if (surface_id_.is_valid())
+  if (frame_sink_id_.is_valid())
     StartSubmitting();
 }
 
 void VideoFrameSubmitter::StartSubmitting() {
   DCHECK_CALLED_ON_VALID_THREAD(media_thread_checker_);
-  DCHECK(surface_id_.is_valid());
+  DCHECK(frame_sink_id_.is_valid());
 
   mojom::blink::EmbeddedFrameSinkProviderPtr provider;
   Platform::Current()->GetInterfaceProvider()->GetInterface(
@@ -195,7 +197,7 @@ void VideoFrameSubmitter::StartSubmitting() {
   viz::mojom::blink::CompositorFrameSinkClientPtr client;
   binding_.Bind(mojo::MakeRequest(&client));
   provider->CreateCompositorFrameSink(
-      surface_id_.frame_sink_id(), std::move(client),
+      frame_sink_id_, std::move(client),
       mojo::MakeRequest(&compositor_frame_sink_));
 
   compositor_frame_sink_.set_connection_error_handler(base::BindOnce(
@@ -212,10 +214,11 @@ void VideoFrameSubmitter::SubmitFrame(
   if (!compositor_frame_sink_ || !ShouldSubmit())
     return;
 
-  // TODO(mlamouri): the `frame_size_` is expected to be consistent but seems to
-  // change in some cases. Ideally, it should be set when empty and a DCHECK
-  // should make sure it stays consistent.
-  frame_size_ = gfx::Rect(video_frame->coded_size());
+  if (frame_size_ != gfx::Rect(video_frame->coded_size())) {
+    if (!frame_size_.IsEmpty())
+      child_local_surface_id_allocator_.GenerateId();
+    frame_size_ = gfx::Rect(video_frame->coded_size());
+  }
 
   viz::CompositorFrame compositor_frame;
   std::unique_ptr<viz::RenderPass> render_pass = viz::RenderPass::Create();
@@ -242,7 +245,8 @@ void VideoFrameSubmitter::SubmitFrame(
 
   // TODO(lethalantidote): Address third/fourth arg in SubmitCompositorFrame.
   compositor_frame_sink_->SubmitCompositorFrame(
-      surface_id_.local_surface_id(), std::move(compositor_frame), nullptr, 0);
+      child_local_surface_id_allocator_.GetCurrentLocalSurfaceId(),
+      std::move(compositor_frame), nullptr, 0);
   resource_provider_->ReleaseFrameResources();
 
   waiting_for_compositor_ack_ = true;
@@ -266,7 +270,8 @@ void VideoFrameSubmitter::SubmitEmptyFrame() {
   compositor_frame.render_pass_list.push_back(std::move(render_pass));
 
   compositor_frame_sink_->SubmitCompositorFrame(
-      surface_id_.local_surface_id(), std::move(compositor_frame), nullptr, 0);
+      child_local_surface_id_allocator_.GetCurrentLocalSurfaceId(),
+      std::move(compositor_frame), nullptr, 0);
   waiting_for_compositor_ack_ = true;
 }
 
@@ -376,4 +381,11 @@ void VideoFrameSubmitter::DidDeleteSharedBitmap(const viz::SharedBitmapId& id) {
       SharedBitmapIdToGpuMailboxPtr(id));
 }
 
+void VideoFrameSubmitter::SetSurfaceIdForTesting(
+    const viz::SurfaceId& surface_id) {
+  frame_sink_id_ = surface_id.frame_sink_id();
+  child_local_surface_id_allocator_.UpdateFromParent(
+      surface_id.local_surface_id());
+}
+
 }  // namespace blink

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

@@ -13,6 +13,7 @@
 #include "components/viz/client/shared_bitmap_reporter.h"
 #include "components/viz/common/gpu/context_provider.h"
 #include "components/viz/common/resources/shared_bitmap.h"
+#include "components/viz/common/surfaces/child_local_surface_id_allocator.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/system/buffer.h"
 #include "services/viz/public/interfaces/compositing/compositor_frame_sink.mojom-blink.h"
@@ -45,10 +46,6 @@ class PLATFORM_EXPORT VideoFrameSubmitter
   mojo::Binding<viz::mojom::blink::CompositorFrameSinkClient>* Binding() {
     return &binding_;
   }
-  void SetSink(viz::mojom::blink::CompositorFrameSinkPtr* sink) {
-    compositor_frame_sink_ = std::move(*sink);
-  }
-  void SetSurfaceId(viz::SurfaceId id) { surface_id_ = id; }
 
   void OnReceivedContextProvider(
       bool,
@@ -87,12 +84,21 @@ class PLATFORM_EXPORT VideoFrameSubmitter
                                const viz::SharedBitmapId&) override;
   void DidDeleteSharedBitmap(const viz::SharedBitmapId&) override;
 
+  void SetCompositorFrameSinkPtrForTesting(
+      viz::mojom::blink::CompositorFrameSinkPtr* sink) {
+    compositor_frame_sink_ = std::move(*sink);
+  }
+
+  void SetSurfaceIdForTesting(const viz::SurfaceId&);
+
  private:
   FRIEND_TEST_ALL_PREFIXES(VideoFrameSubmitterTest, ContextLostDuringSubmit);
   FRIEND_TEST_ALL_PREFIXES(VideoFrameSubmitterTest,
                            ShouldSubmitPreventsSubmission);
   FRIEND_TEST_ALL_PREFIXES(VideoFrameSubmitterTest,
                            SetForceSubmitForcesSubmission);
+  FRIEND_TEST_ALL_PREFIXES(VideoFrameSubmitterTest,
+                           FrameSizeChangeUpdatesLocalSurfaceId);
 
   void StartSubmitting();
   void UpdateSubmissionStateInternal();
@@ -116,7 +122,6 @@ class PLATFORM_EXPORT VideoFrameSubmitter
   WebContextProviderCallback context_provider_callback_;
   std::unique_ptr<VideoFrameResourceProvider> resource_provider_;
   WebFrameSinkDestroyedCallback frame_sink_destroyed_callback_;
-  viz::SurfaceId surface_id_;
   bool waiting_for_compositor_ack_ = false;
 
   bool is_rendering_;
@@ -127,11 +132,17 @@ class PLATFORM_EXPORT VideoFrameSubmitter
   media::VideoRotation rotation_;
   bool is_opaque_ = true;
 
+  viz::FrameSinkId frame_sink_id_;
+
   // Size of the video frame being submitted. It is set the first time a frame
-  // is submitted and is expected to never change aftewards. Used to send an
-  // empty frame when the video is out of view.
+  // 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_;
 
+  // Used to updated the LocalSurfaceId when detecting a change in video frame
+  // size.
+  viz::ChildLocalSurfaceIdAllocator child_local_surface_id_allocator_;
+
   THREAD_CHECKER(media_thread_checker_);
 
   base::WeakPtrFactory<VideoFrameSubmitter> weak_ptr_factory_;

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

@@ -161,8 +161,8 @@ class VideoFrameSubmitterTest : public testing::Test {
     // By setting the submission state before we set the sink, we can make
     // testing easier without having to worry about the first sent frame.
     submitter_->UpdateSubmissionState(true);
-    submitter_->SetSink(&submitter_sink);
-    submitter_->SetSurfaceId(viz::SurfaceId(
+    submitter_->SetCompositorFrameSinkPtrForTesting(&submitter_sink);
+    submitter_->SetSurfaceIdForTesting(viz::SurfaceId(
         viz::FrameSinkId(1, 1),
         viz::LocalSurfaceId(11,
                             base::UnguessableToken::Deserialize(0x111111, 0))));
@@ -769,4 +769,76 @@ TEST_F(VideoFrameSubmitterTest, ContextLostDuringSubmit) {
   scoped_task_environment_.RunUntilIdle();
 }
 
+// Test the behaviour of the ChildLocalSurfaceIdAllocator instance. It checks
+// that the LocalSurfaceId is propoerly set at creation and updated when the
+// video frames change.
+TEST_F(VideoFrameSubmitterTest, FrameSizeChangeUpdatesLocalSurfaceId) {
+  MakeSubmitter();
+  scoped_task_environment_.RunUntilIdle();
+
+  {
+    viz::LocalSurfaceId local_surface_id =
+        submitter_->child_local_surface_id_allocator_
+            .GetCurrentLocalSurfaceId();
+    EXPECT_TRUE(local_surface_id.is_valid());
+    EXPECT_EQ(11u, local_surface_id.parent_sequence_number());
+    EXPECT_EQ(viz::kInitialChildSequenceNumber,
+              local_surface_id.child_sequence_number());
+    EXPECT_TRUE(submitter_->frame_size_.IsEmpty());
+  }
+
+  EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
+
+  submitter_->StartRendering();
+  scoped_task_environment_.RunUntilIdle();
+
+  EXPECT_CALL(*provider_, GetCurrentFrame())
+      .WillOnce(Return(media::VideoFrame::CreateFrame(
+          media::PIXEL_FORMAT_YV12, gfx::Size(8, 8), gfx::Rect(gfx::Size(8, 8)),
+          gfx::Size(8, 8), base::TimeDelta())));
+  EXPECT_CALL(*sink_, DoSubmitCompositorFrame(_, _));
+  EXPECT_CALL(*provider_, PutCurrentFrame());
+  EXPECT_CALL(*resource_provider_, AppendQuads(_, _, _, _));
+  EXPECT_CALL(*resource_provider_, PrepareSendToParent(_, _));
+  EXPECT_CALL(*resource_provider_, ReleaseFrameResources());
+
+  submitter_->SubmitSingleFrame();
+  scoped_task_environment_.RunUntilIdle();
+
+  {
+    viz::LocalSurfaceId local_surface_id =
+        submitter_->child_local_surface_id_allocator_
+            .GetCurrentLocalSurfaceId();
+    EXPECT_TRUE(local_surface_id.is_valid());
+    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_CALL(*provider_, GetCurrentFrame())
+      .WillOnce(Return(media::VideoFrame::CreateFrame(
+          media::PIXEL_FORMAT_YV12, gfx::Size(2, 2), gfx::Rect(gfx::Size(2, 2)),
+          gfx::Size(2, 2), base::TimeDelta())));
+  EXPECT_CALL(*sink_, DoSubmitCompositorFrame(_, _));
+  EXPECT_CALL(*provider_, PutCurrentFrame());
+  EXPECT_CALL(*resource_provider_, AppendQuads(_, _, _, _));
+  EXPECT_CALL(*resource_provider_, PrepareSendToParent(_, _));
+  EXPECT_CALL(*resource_provider_, ReleaseFrameResources());
+
+  submitter_->SubmitSingleFrame();
+  scoped_task_environment_.RunUntilIdle();
+
+  {
+    viz::LocalSurfaceId local_surface_id =
+        submitter_->child_local_surface_id_allocator_
+            .GetCurrentLocalSurfaceId();
+    EXPECT_TRUE(local_surface_id.is_valid());
+    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_);
+  }
+}
+
 }  // namespace blink