Allow VideoFrameProvider::Client to avoid driving frame updates.

Previously, VideoFrameCompositor assumed that, if it had a client,
that the client would drive VideoFrame updates based on the vsync
clock.  If an external consumer (e.g., WebGL) painted the frame,
VFC would still not update the current frame, since the client would
take care of it.

VideoFrameSubmitter is a client that sometimes elides frame updates,
if it believes that no consumer is present (e.g., if the
corresponding SurfaceLayer is not in the layer tree, or is not
visible).  However, this would break external consumers, since they
would no longer get current VideoFrames.

This CL allows a VideoFrameProvider::Client to signal that it isn't
doing frame updates.  In this case, VFC will still try to move the
frame forward to match the wall clock, so that external consumers
get current frames.

Additionally, this CL causes SurfaceLayerImpl to signal that it's
no longer visible upon destruction.  This catches cases where a
video element is removed from the DOM.

Bug: 869277
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ia53a349af4add6d4792ef16e0306aa7c139ab70c
Reviewed-on: https://chromium-review.googlesource.com/1159274Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581605}

cc/layers/surface_layer.h

@@ -18,7 +18,7 @@ namespace cc {
 
 // If given true, we should submit frames, as we are unoccluded on screen.
 // If given false, we should not submit compositor frames.
-using UpdateSubmissionStateCB = base::RepeatingCallback<void(bool)>;
+using UpdateSubmissionStateCB = base::RepeatingCallback<void(bool is_visible)>;
 
 // A layer that renders a surface referencing the output of another compositor
 // instance or client.

cc/layers/surface_layer_impl.cc

@@ -24,7 +24,10 @@ SurfaceLayerImpl::SurfaceLayerImpl(
       update_submission_state_callback_(
           std::move(update_submission_state_callback)) {}
 
-SurfaceLayerImpl::~SurfaceLayerImpl() = default;
+SurfaceLayerImpl::~SurfaceLayerImpl() {
+  if (update_submission_state_callback_)
+    update_submission_state_callback_.Run(false);
+}
 
 std::unique_ptr<LayerImpl> SurfaceLayerImpl::CreateLayerImpl(
     LayerTreeImpl* tree_impl) {

cc/layers/surface_layer_impl.h

@@ -20,7 +20,7 @@
 namespace cc {
 
 // This must match SurfaceLayer::UpdateSubmissionStateCB.
-using UpdateSubmissionStateCB = base::RepeatingCallback<void(bool)>;
+using UpdateSubmissionStateCB = base::RepeatingCallback<void(bool is_visible)>;
 
 class CC_EXPORT SurfaceLayerImpl : public LayerImpl {
  public:

cc/layers/video_frame_provider.h

@@ -39,12 +39,23 @@ class CC_EXPORT VideoFrameProvider {
     // Callers should use these methods to indicate when it expects and no
     // longer expects (respectively) to have new frames for the client. Clients
     // may use this information for power conservation.
+    //
+    // Note that the client may also choose to stop driving frame updates, such
+    // as if it believes that the frames are not visible.  In this case, the
+    // client should report this via IsDrivingFrameUpdates().
     virtual void StartRendering() = 0;
     virtual void StopRendering() = 0;
 
     // Notifies the client that GetCurrentFrame() will return new data.
     virtual void DidReceiveFrame() = 0;
 
+    // Should return true if and only if the client is actively driving frame
+    // updates.  Note that this implies that the client has been told to
+    // StartRendering by the VideoFrameProvider.  However, it's okay if the
+    // client chooses to elide these calls, for example to save power when the
+    // client knows that the frames are not visible.
+    virtual bool IsDrivingFrameUpdates() const = 0;
+
    protected:
     virtual ~Client() {}
   };

cc/layers/video_frame_provider_client_impl.cc

@@ -177,4 +177,9 @@ void VideoFrameProviderClientImpl::DidDrawFrame() {
   needs_put_current_frame_ = false;
 }
 
+bool VideoFrameProviderClientImpl::IsDrivingFrameUpdates() const {
+  // We drive frame updates any time we're rendering, even if we're off-screen.
+  return rendering_;
+}
+
 }  // namespace cc

cc/layers/video_frame_provider_client_impl.h

@@ -56,6 +56,7 @@ class CC_EXPORT VideoFrameProviderClientImpl
   void StartRendering() override;
   void StopRendering() override;
   void DidReceiveFrame() override;
+  bool IsDrivingFrameUpdates() const override;
 
   const VideoFrameProvider* get_provider_for_testing() const {
     return provider_;

content/renderer/media/stream/webmediaplayer_ms_unittest.cc

@@ -573,6 +573,7 @@ class WebMediaPlayerMSTest
   void StartRendering() override;
   void StopRendering() override;
   void DidReceiveFrame() override;
+  bool IsDrivingFrameUpdates() const override { return true; }
 
   // For test use
   void SetBackgroundRendering(bool background_rendering) {

media/blink/video_frame_compositor.cc

@@ -59,9 +59,9 @@ VideoFrameCompositor::GetUpdateSubmissionStateCallback() {
   return update_submission_state_callback_;
 }
 
-void VideoFrameCompositor::UpdateSubmissionState(bool state) {
+void VideoFrameCompositor::UpdateSubmissionState(bool is_visible) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  submitter_->UpdateSubmissionState(state);
+  submitter_->UpdateSubmissionState(is_visible);
 }
 
 void VideoFrameCompositor::InitializeSubmitter() {
@@ -220,9 +220,18 @@ void VideoFrameCompositor::PaintSingleFrame(
 void VideoFrameCompositor::UpdateCurrentFrameIfStale() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  if (IsClientSinkAvailable() || !rendering_ || !is_background_rendering_)
+  // If we're not rendering, then the frame can't be stale.
+  if (!rendering_ || !is_background_rendering_)
     return;
 
+  // If we have a client, and it is currently rendering, then it's not stale
+  // since the client is driving the frame updates at the proper rate.
+  if (IsClientSinkAvailable() && client_->IsDrivingFrameUpdates())
+    return;
+
+  // We're rendering, but the client isn't driving the updates.  See if the
+  // frame is stale, and update it.
+
   DCHECK(!last_background_render_.is_null());
 
   const base::TimeTicks now = tick_clock_->NowTicks();

media/blink/video_frame_compositor.h

@@ -162,8 +162,9 @@ class MEDIA_BLINK_EXPORT VideoFrameCompositor : public VideoRendererSink,
   // Ran on the |task_runner_| to initalize |submitter_|;
   void InitializeSubmitter();
 
-  // Signals the VideoFrameSubmitter to stop submitting frames.
-  void UpdateSubmissionState(bool);
+  // Signals the VideoFrameSubmitter to stop submitting frames.  |is_visible|
+  // indicates whether or not the consumer of the frames is (probably) visible.
+  void UpdateSubmissionState(bool is_visible);
 
   // Indicates whether the endpoint for the VideoFrame exists.
   bool IsClientSinkAvailable();

media/blink/video_frame_compositor_unittest.cc

@@ -16,6 +16,7 @@
 #include "third_party/blink/public/platform/web_video_frame_submitter.h"
 
 using testing::_;
+using testing::AnyNumber;
 using testing::DoAll;
 using testing::Eq;
 using testing::Return;
@@ -31,6 +32,7 @@ class MockWebVideoFrameSubmitter : public blink::WebVideoFrameSubmitter {
                void(viz::SurfaceId, blink::WebFrameSinkDestroyedCallback));
   MOCK_METHOD0(StartRendering, void());
   MOCK_METHOD0(StopRendering, void());
+  MOCK_CONST_METHOD0(IsDrivingFrameUpdates, bool(void));
   MOCK_METHOD1(Initialize, void(cc::VideoFrameProvider*));
   MOCK_METHOD1(SetRotation, void(media::VideoRotation));
   MOCK_METHOD1(SetIsOpaque, void(bool));
@@ -261,6 +263,10 @@ TEST_P(VideoFrameCompositorTest, UpdateCurrentFrameIfStale) {
   scoped_refptr<VideoFrame> opaque_frame_2 = CreateOpaqueFrame();
   compositor_->set_background_rendering_for_testing(true);
 
+  EXPECT_CALL(*submitter_, IsDrivingFrameUpdates)
+      .Times(AnyNumber())
+      .WillRepeatedly(Return(true));
+
   // Starting the video renderer should return a single frame.
   EXPECT_CALL(*this, Render(_, _, true)).WillOnce(Return(opaque_frame_1));
   StartVideoRendererSink();
@@ -272,8 +278,11 @@ TEST_P(VideoFrameCompositorTest, UpdateCurrentFrameIfStale) {
   EXPECT_CALL(*this, Render(_, _, _)).Times(0);
   compositor()->UpdateCurrentFrameIfStale();
 
-  // Clear our client, which means no mock function calls for Client.
-  compositor()->SetVideoFrameProviderClient(nullptr);
+  // Have the client signal that it will not drive the frame clock, so that
+  // calling UpdateCurrentFrameIfStale may update the frame.
+  EXPECT_CALL(*submitter_, IsDrivingFrameUpdates)
+      .Times(AnyNumber())
+      .WillRepeatedly(Return(false));
 
   // Wait for background rendering to tick.
   base::RunLoop run_loop;
@@ -294,6 +303,16 @@ TEST_P(VideoFrameCompositorTest, UpdateCurrentFrameIfStale) {
   compositor()->UpdateCurrentFrameIfStale();
   EXPECT_EQ(opaque_frame_1, compositor()->GetCurrentFrame());
 
+  // Clear our client, which means no mock function calls for Client.  It will
+  // also permit UpdateCurrentFrameIfStale to update the frame.
+  compositor()->SetVideoFrameProviderClient(nullptr);
+
+  // Advancing the tick clock should allow a new frame to be requested.
+  tick_clock_.Advance(base::TimeDelta::FromMilliseconds(10));
+  EXPECT_CALL(*this, Render(_, _, true)).WillOnce(Return(opaque_frame_2));
+  compositor()->UpdateCurrentFrameIfStale();
+  EXPECT_EQ(opaque_frame_2, compositor()->GetCurrentFrame());
+
   // Background rendering should tick another render callback.
   StopVideoRendererSink(false);
 }

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

@@ -82,7 +82,7 @@ void VideoFrameSubmitter::SetForceSubmit(bool force_submit) {
 
 void VideoFrameSubmitter::UpdateSubmissionStateInternal() {
   if (compositor_frame_sink_) {
-    compositor_frame_sink_->SetNeedsBeginFrame(is_rendering_ && ShouldSubmit());
+    compositor_frame_sink_->SetNeedsBeginFrame(IsDrivingFrameUpdates());
     if (ShouldSubmit())
       SubmitSingleFrame();
     else if (!frame_size_.IsEmpty())
@@ -117,9 +117,10 @@ void VideoFrameSubmitter::SubmitSingleFrame() {
   scoped_refptr<media::VideoFrame> video_frame = provider_->GetCurrentFrame();
   if (video_frame) {
     base::SequencedTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(&VideoFrameSubmitter::SubmitFrame,
-                                  weak_ptr_factory_.GetWeakPtr(),
-                                  current_begin_frame_ack, video_frame));
+        FROM_HERE,
+        base::BindOnce(base::IgnoreResult(&VideoFrameSubmitter::SubmitFrame),
+                       weak_ptr_factory_.GetWeakPtr(), current_begin_frame_ack,
+                       video_frame));
     provider_->PutCurrentFrame();
   }
 }
@@ -128,6 +129,13 @@ bool VideoFrameSubmitter::ShouldSubmit() const {
   return should_submit_internal_ || force_submit_;
 }
 
+bool VideoFrameSubmitter::IsDrivingFrameUpdates() const {
+  // We drive frame updates only when we believe that something is consuming
+  // them.  This is different than VideoLayer, which drives updates any time
+  // they're in the layer tree.
+  return is_rendering_ && ShouldSubmit();
+}
+
 void VideoFrameSubmitter::DidReceiveFrame() {
   DCHECK_CALLED_ON_VALID_THREAD(media_thread_checker_);
   DCHECK(provider_);
@@ -206,13 +214,14 @@ void VideoFrameSubmitter::StartSubmitting() {
   UpdateSubmissionStateInternal();
 }
 
-void VideoFrameSubmitter::SubmitFrame(
+bool VideoFrameSubmitter::SubmitFrame(
     const viz::BeginFrameAck& begin_frame_ack,
     scoped_refptr<media::VideoFrame> video_frame) {
   TRACE_EVENT0("media", "VideoFrameSubmitter::SubmitFrame");
+  DCHECK(video_frame);
   DCHECK_CALLED_ON_VALID_THREAD(media_thread_checker_);
   if (!compositor_frame_sink_ || !ShouldSubmit())
-    return;
+    return false;
 
   if (frame_size_ != gfx::Rect(video_frame->coded_size())) {
     if (!frame_size_.IsEmpty())
@@ -228,6 +237,9 @@ void VideoFrameSubmitter::SubmitFrame(
   resource_provider_->AppendQuads(render_pass.get(), video_frame, rotation_,
                                   is_opaque_);
   compositor_frame.metadata.begin_frame_ack = begin_frame_ack;
+  // We don't assume that the ack is marked as having damage.  However, we're
+  // definitely emitting a CompositorFrame that damages the entire surface.
+  compositor_frame.metadata.begin_frame_ack.has_damage = true;
   compositor_frame.metadata.device_scale_factor = 1;
   compositor_frame.metadata.may_contain_video = true;
 
@@ -250,6 +262,7 @@ void VideoFrameSubmitter::SubmitFrame(
   resource_provider_->ReleaseFrameResources();
 
   waiting_for_compositor_ack_ = true;
+  return true;
 }
 
 void VideoFrameSubmitter::SubmitEmptyFrame() {
@@ -296,18 +309,17 @@ void VideoFrameSubmitter::OnBeginFrame(const viz::BeginFrameArgs& args) {
     return;
   }
 
+  scoped_refptr<media::VideoFrame> video_frame = provider_->GetCurrentFrame();
+
   // We do have a new frame that we could display.  See if we're supposed to
-  // actually submit a frame or not.
-  if (!is_rendering_ || waiting_for_compositor_ack_) {
+  // actually submit a frame or not, and try to submit one.
+  if (!is_rendering_ || waiting_for_compositor_ack_ ||
+      !SubmitFrame(current_begin_frame_ack, std::move(video_frame))) {
     compositor_frame_sink_->DidNotProduceFrame(current_begin_frame_ack);
     return;
   }
 
-  current_begin_frame_ack.has_damage = true;
-
-  scoped_refptr<media::VideoFrame> video_frame = provider_->GetCurrentFrame();
-
-  SubmitFrame(current_begin_frame_ack, video_frame);
+  // We submitted a frame!
 
   // We still signal PutCurrentFrame here, rather than on the ack, so that it
   // lines up with the correct frame.  Otherwise, any intervening calls to

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

@@ -56,13 +56,14 @@ class PLATFORM_EXPORT VideoFrameSubmitter
   void StartRendering() override;
   void StopRendering() override;
   void DidReceiveFrame() override;
+  bool IsDrivingFrameUpdates() const override;
 
   // WebVideoFrameSubmitter implementation.
   void Initialize(cc::VideoFrameProvider*) override;
   void SetRotation(media::VideoRotation) override;
   void SetIsOpaque(bool) override;
   void EnableSubmission(viz::SurfaceId, WebFrameSinkDestroyedCallback) override;
-  void UpdateSubmissionState(bool) override;
+  void UpdateSubmissionState(bool is_visible) override;
   void SetForceSubmit(bool) override;
 
   // viz::ContextLostObserver implementation.
@@ -102,7 +103,8 @@ class PLATFORM_EXPORT VideoFrameSubmitter
 
   void StartSubmitting();
   void UpdateSubmissionStateInternal();
-  void SubmitFrame(const viz::BeginFrameAck&, scoped_refptr<media::VideoFrame>);
+  // Returns whether a frame was submitted.
+  bool SubmitFrame(const viz::BeginFrameAck&, scoped_refptr<media::VideoFrame>);
   void SubmitEmptyFrame();
 
   // Pulls frame and submits it to compositor.
@@ -127,7 +129,7 @@ class PLATFORM_EXPORT VideoFrameSubmitter
   bool is_rendering_;
   // If we are not on screen, we should not submit.
   bool should_submit_internal_ = false;
-  // Whether frames should always be submitted.
+  // Whether frames should always be submitted, even if we're not visible.
   bool force_submit_ = false;
   media::VideoRotation rotation_;
   bool is_opaque_ = true;