Fix ui::Compositor::refresh_rate() with OOP-D.

With OOP-D enabled Compositor::SetDisplayVSyncParameters() won't
necessarily be called anymore as most of that logic moves to the GPU
process. This means that |refresh_rate_| won't be updated ever and would
be incorrect.

The vsync interval in received BeginFrameArgs is equivalent to the
refresh rate, so plumb that information out of cc::Scheduler and back
into ui::Compositor. This should always work correctly.

This CL also removes ContextFactory::GetRefreshRate(). This set the
initial value for |refresh_rate_| in tests. This no longer serves any
purpose, as |refresh_rate_| is only used for metric reporting that
doesn't impact tests. Tests that want to run faster already setup a
BeginFrameSource with the appropriate shorter interval.

Bug: 899300
Change-Id: I8f7dcbb984a4cd804712d39a571e43ad627c9d4b
Reviewed-on: https://chromium-review.googlesource.com/c/1311077Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604783}

cc/scheduler/scheduler.cc

@@ -306,6 +306,16 @@ void Scheduler::OnBeginFrameSourcePausedChanged(bool paused) {
 bool Scheduler::OnBeginFrameDerivedImpl(const viz::BeginFrameArgs& args) {
   TRACE_EVENT1("cc,benchmark", "Scheduler::BeginFrame", "args", args.AsValue());
 
+  // If the begin frame interval is different than last frame and bigger than
+  // zero then let |client_| know about the new interval for animations. In
+  // theory the interval should always be bigger than zero but the value is
+  // provided by APIs outside our control.
+  if (args.interval != last_frame_interval_ &&
+      args.interval > base::TimeDelta()) {
+    last_frame_interval_ = args.interval;
+    client_->FrameIntervalUpdated(last_frame_interval_);
+  }
+
   if (ShouldDropBeginFrame(args)) {
     TRACE_EVENT_INSTANT0("cc", "Scheduler::BeginFrameDropped",
                          TRACE_EVENT_SCOPE_THREAD);

cc/scheduler/scheduler.h

@@ -52,8 +52,9 @@ class SchedulerClient {
   virtual void SendBeginMainFrameNotExpectedSoon() = 0;
   virtual void ScheduledActionBeginMainFrameNotExpectedUntil(
       base::TimeTicks time) = 0;
+  virtual void FrameIntervalUpdated(base::TimeDelta interval) = 0;
 
-  // Functions used for reporting anmation targeting UMA, crbug.com/758439.
+  // Functions used for reporting animation targeting UMA, crbug.com/758439.
   virtual size_t CompositedAnimationsCount() const = 0;
   virtual size_t MainThreadAnimationsCount() const = 0;
   virtual bool CurrentFrameHadRAF() const = 0;
@@ -223,6 +224,10 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
 
   bool stopped_ = false;
 
+  // Keeps track of the begin frame interval from the last BeginFrameArgs to
+  // arrive so that |client_| can be informed about changes.
+  base::TimeDelta last_frame_interval_;
+
  private:
   // Posts the deadline task if needed by checking
   // SchedulerStateMachine::CurrentBeginImplFrameDeadlineMode(). This only

cc/scheduler/scheduler_unittest.cc

@@ -73,6 +73,8 @@ class FakeSchedulerClient : public SchedulerClient,
     return posted_begin_impl_frame_deadline_;
   }
 
+  base::TimeDelta frame_interval() const { return frame_interval_; }
+
   int ActionIndex(const char* action) const {
     for (size_t i = 0; i < actions_.size(); i++)
       if (!strcmp(actions_[i], action))
@@ -135,6 +137,9 @@ class FakeSchedulerClient : public SchedulerClient,
     PushAction("ScheduledActionSendBeginMainFrame");
     last_begin_main_frame_args_ = args;
   }
+  void FrameIntervalUpdated(base::TimeDelta interval) override {
+    frame_interval_ = interval;
+  }
 
   const viz::BeginFrameArgs& last_begin_main_frame_args() {
     return last_begin_main_frame_args_;
@@ -275,6 +280,7 @@ class FakeSchedulerClient : public SchedulerClient,
   std::vector<std::unique_ptr<base::trace_event::ConvertableToTraceFormat>>
       states_;
   TestScheduler* scheduler_ = nullptr;
+  base::TimeDelta frame_interval_;
 };
 
 enum BeginFrameSourceType {
@@ -1492,6 +1498,63 @@ TEST_F(SchedulerTest, MainFrameNotSkippedAfterLateBeginFrame) {
                  "ScheduledActionDrawIfPossible");
 }
 
+TEST_F(SchedulerTest, FrameIntervalUpdated) {
+  // Verify that the SchedulerClient gets updates when the begin frame interval
+  // changes.
+  SetUpScheduler(EXTERNAL_BFS);
+  constexpr uint64_t kSourceId = viz::BeginFrameArgs::kStartingSourceId;
+  uint64_t sequence_number = viz::BeginFrameArgs::kStartingFrameNumber;
+
+  base::TimeDelta interval = base::TimeDelta::FromMicroseconds(
+      base::Time::kMicrosecondsPerSecond / 120.0);
+
+  // Send BeginFrameArgs with 120hz refresh rate and confirm client gets update.
+  scheduler_->SetNeedsRedraw();
+  task_runner_->AdvanceMockTickClock(interval);
+  viz::BeginFrameArgs args1 = viz::BeginFrameArgs::Create(
+      BEGINFRAME_FROM_HERE, kSourceId, sequence_number++,
+      task_runner_->NowTicks(), task_runner_->NowTicks() + interval, interval,
+      viz::BeginFrameArgs::NORMAL);
+  fake_external_begin_frame_source_->TestOnBeginFrame(args1);
+  EXPECT_EQ(client_->frame_interval(), interval);
+
+  // Send another BeginFrameArgs with 120hz refresh rate that arrives late. Even
+  // though the interval between begin frames arriving is bigger than |interval|
+  // the client only hears the interval specified in BeginFrameArgs.
+  scheduler_->SetNeedsRedraw();
+  const base::TimeDelta late_delta = base::TimeDelta::FromMilliseconds(4);
+  task_runner_->AdvanceMockTickClock(interval + late_delta);
+  viz::BeginFrameArgs args2 = viz::BeginFrameArgs::Create(
+      BEGINFRAME_FROM_HERE, kSourceId, sequence_number++, args1.deadline,
+      args1.deadline + interval, interval, viz::BeginFrameArgs::NORMAL);
+  fake_external_begin_frame_source_->TestOnBeginFrame(args2);
+  EXPECT_EQ(client_->frame_interval(), interval);
+
+  // Change the interval for 90hz refresh rate.
+  interval = base::TimeDelta::FromMicroseconds(
+      base::Time::kMicrosecondsPerSecond / 90.0);
+
+  // Send BeginFrameArgs with 90hz refresh rate and confirm client gets update.
+  scheduler_->SetNeedsRedraw();
+  task_runner_->AdvanceMockTickClock(args2.deadline - task_runner_->NowTicks());
+  viz::BeginFrameArgs args3 = viz::BeginFrameArgs::Create(
+      BEGINFRAME_FROM_HERE, kSourceId, sequence_number++, args2.deadline,
+      args2.deadline + interval, interval, viz::BeginFrameArgs::NORMAL);
+  fake_external_begin_frame_source_->TestOnBeginFrame(args3);
+  EXPECT_EQ(client_->frame_interval(), interval);
+
+  // Send BeginFrameArgs with zero interval. This isn't a valid interval and
+  // client shouldn't find out about it.
+  scheduler_->SetNeedsRedraw();
+  task_runner_->AdvanceMockTickClock(interval);
+  viz::BeginFrameArgs args4 = viz::BeginFrameArgs::Create(
+      BEGINFRAME_FROM_HERE, kSourceId, sequence_number++, args3.deadline,
+      args3.deadline + interval, base::TimeDelta(),
+      viz::BeginFrameArgs::NORMAL);
+  fake_external_begin_frame_source_->TestOnBeginFrame(args4);
+  EXPECT_EQ(client_->frame_interval(), interval);
+}
+
 TEST_F(SchedulerTest, MainFrameSkippedAfterLateCommit) {
   SetUpScheduler(EXTERNAL_BFS);
   fake_compositor_timing_history_->SetAllEstimatesTo(kFastDuration);

cc/trees/layer_tree_host_single_thread_client.h

@@ -5,6 +5,8 @@
 #ifndef CC_TREES_LAYER_TREE_HOST_SINGLE_THREAD_CLIENT_H_
 #define CC_TREES_LAYER_TREE_HOST_SINGLE_THREAD_CLIENT_H_
 
+#include "base/time/time.h"
+
 namespace cc {
 
 class LayerTreeHostSingleThreadClient {
@@ -15,6 +17,10 @@ class LayerTreeHostSingleThreadClient {
   // delay for potential future frame.
   virtual void RequestScheduleAnimation() {}
 
+  // Called whenever the begin frame interval changes. This interval can be used
+  // for animations.
+  virtual void FrameIntervalUpdated(base::TimeDelta interval) {}
+
   // Called whenever the compositor submits a CompositorFrame. Afterward,
   // LayerTreeHostClient::DidReceiveCompositorFrameAck() will be called once the
   // display compositor/ finishes processing the frame. So these functions can

cc/trees/proxy_impl.h

@@ -122,6 +122,7 @@ class CC_EXPORT ProxyImpl : public LayerTreeHostImplClient,
   void SendBeginMainFrameNotExpectedSoon() override;
   void ScheduledActionBeginMainFrameNotExpectedUntil(
       base::TimeTicks time) override;
+  void FrameIntervalUpdated(base::TimeDelta interval) override {}
   size_t CompositedAnimationsCount() const override;
   size_t MainThreadAnimationsCount() const override;
   bool CurrentFrameHadRAF() const override;

cc/trees/single_thread_proxy.cc

@@ -704,6 +704,11 @@ void SingleThreadProxy::ScheduledActionSendBeginMainFrame(
   host_impl_->DidSendBeginMainFrame();
 }
 
+void SingleThreadProxy::FrameIntervalUpdated(base::TimeDelta interval) {
+  DebugScopedSetMainThread main(task_runner_provider_);
+  single_thread_client_->FrameIntervalUpdated(interval);
+}
+
 void SingleThreadProxy::SendBeginMainFrameNotExpectedSoon() {
   layer_tree_host_->BeginMainFrameNotExpectedSoon();
 }

cc/trees/single_thread_proxy.h

@@ -88,6 +88,7 @@ class CC_EXPORT SingleThreadProxy : public Proxy,
   void SendBeginMainFrameNotExpectedSoon() override;
   void ScheduledActionBeginMainFrameNotExpectedUntil(
       base::TimeTicks time) override;
+  void FrameIntervalUpdated(base::TimeDelta interval) override;
   size_t CompositedAnimationsCount() const override;
   size_t MainThreadAnimationsCount() const override;
   bool CurrentFrameHadRAF() const override;

content/browser/compositor/gpu_process_transport_factory.cc

@@ -737,10 +737,6 @@ void GpuProcessTransportFactory::RemoveCompositor(ui::Compositor* compositor) {
 #endif
 }
 
-double GpuProcessTransportFactory::GetRefreshRate() const {
-  return 60.0;
-}
-
 gpu::GpuMemoryBufferManager*
 GpuProcessTransportFactory::GetGpuMemoryBufferManager() {
   return gpu_channel_factory_->GetGpuMemoryBufferManager();

content/browser/compositor/gpu_process_transport_factory.h

@@ -73,7 +73,6 @@ class GpuProcessTransportFactory : public ui::ContextFactory,
       base::WeakPtr<ui::Compositor> compositor) override;
   scoped_refptr<viz::ContextProvider> SharedMainThreadContextProvider()
       override;
-  double GetRefreshRate() const override;
   gpu::GpuMemoryBufferManager* GetGpuMemoryBufferManager() override;
   cc::TaskGraphRunner* GetTaskGraphRunner() override;
   void AddObserver(ui::ContextFactoryObserver* observer) override;

content/browser/compositor/test/test_image_transport_factory.cc

@@ -96,12 +96,6 @@ TestImageTransportFactory::SharedMainThreadContextProvider() {
   return shared_main_context_provider_;
 }
 
-double TestImageTransportFactory::GetRefreshRate() const {
-  // The context factory created here is for unit tests, thus using a higher
-  // refresh rate to spend less time waiting for BeginFrames.
-  return 200.0;
-}
-
 gpu::GpuMemoryBufferManager*
 TestImageTransportFactory::GetGpuMemoryBufferManager() {
   return &gpu_memory_buffer_manager_;

content/browser/compositor/test/test_image_transport_factory.h

@@ -48,7 +48,6 @@ class TestImageTransportFactory : public ui::ContextFactory,
   scoped_refptr<viz::ContextProvider> SharedMainThreadContextProvider()
       override;
   void RemoveCompositor(ui::Compositor* compositor) override {}
-  double GetRefreshRate() const override;
   gpu::GpuMemoryBufferManager* GetGpuMemoryBufferManager() override;
   cc::TaskGraphRunner* GetTaskGraphRunner() override;
   void AddObserver(ui::ContextFactoryObserver* observer) override;

content/browser/compositor/viz_process_transport_factory.cc

@@ -250,11 +250,6 @@ void VizProcessTransportFactory::RemoveCompositor(ui::Compositor* compositor) {
   UnconfigureCompositor(compositor);
 }
 
-double VizProcessTransportFactory::GetRefreshRate() const {
-  // TODO(kylechar): Delete this function from ContextFactoryPrivate.
-  return 60.0;
-}
-
 gpu::GpuMemoryBufferManager*
 VizProcessTransportFactory::GetGpuMemoryBufferManager() {
   return gpu_channel_establish_factory_->GetGpuMemoryBufferManager();

content/browser/compositor/viz_process_transport_factory.h

@@ -66,7 +66,6 @@ class VizProcessTransportFactory : public ui::ContextFactory,
   scoped_refptr<viz::ContextProvider> SharedMainThreadContextProvider()
       override;
   void RemoveCompositor(ui::Compositor* compositor) override;
-  double GetRefreshRate() const override;
   gpu::GpuMemoryBufferManager* GetGpuMemoryBufferManager() override;
   cc::TaskGraphRunner* GetTaskGraphRunner() override;
   void AddObserver(ui::ContextFactoryObserver* observer) override;

services/ws/host_context_factory.cc

@@ -74,10 +74,6 @@ void HostContextFactory::RemoveCompositor(ui::Compositor* compositor) {
   context_factory_private_->UnconfigureCompositor(compositor);
 }
 
-double HostContextFactory::GetRefreshRate() const {
-  return 60.0;
-}
-
 gpu::GpuMemoryBufferManager* HostContextFactory::GetGpuMemoryBufferManager() {
   return gpu_->gpu_memory_buffer_manager();
 }

services/ws/host_context_factory.h

@@ -51,7 +51,6 @@ class HostContextFactory : public ui::ContextFactory {
   scoped_refptr<viz::ContextProvider> SharedMainThreadContextProvider()
       override;
   void RemoveCompositor(ui::Compositor* compositor) override;
-  double GetRefreshRate() const override;
   gpu::GpuMemoryBufferManager* GetGpuMemoryBufferManager() override;
   cc::TaskGraphRunner* GetTaskGraphRunner() override;
   void AddObserver(ui::ContextFactoryObserver* observer) override {}

ui/aura/mus/mus_context_factory.cc

@@ -73,10 +73,6 @@ void MusContextFactory::RemoveCompositor(ui::Compositor* compositor) {
   // NOTIMPLEMENTED();
 }
 
-double MusContextFactory::GetRefreshRate() const {
-  return 60.0;
-}
-
 gpu::GpuMemoryBufferManager* MusContextFactory::GetGpuMemoryBufferManager() {
   return gpu_->gpu_memory_buffer_manager();
 }

ui/aura/mus/mus_context_factory.h

@@ -44,7 +44,6 @@ class AURA_EXPORT MusContextFactory : public ui::ContextFactory {
   scoped_refptr<viz::ContextProvider> SharedMainThreadContextProvider()
       override;
   void RemoveCompositor(ui::Compositor* compositor) override;
-  double GetRefreshRate() const override;
   gpu::GpuMemoryBufferManager* GetGpuMemoryBufferManager() override;
   cc::TaskGraphRunner* GetTaskGraphRunner() override;
   void AddObserver(ui::ContextFactoryObserver* observer) override {}

ui/aura/test/aura_test_context_factory.cc

@@ -54,7 +54,7 @@ void AuraTestContextFactory::CreateLayerTreeFrameSink(
       std::make_unique<FrameSinkClient>(context_provider);
   constexpr bool synchronous_composite = false;
   constexpr bool disable_display_vsync = false;
-  const double refresh_rate = GetRefreshRate();
+  const double refresh_rate = 200.0;
   auto frame_sink = std::make_unique<viz::TestLayerTreeFrameSink>(
       context_provider, viz::TestContextProvider::CreateWorker(),
       GetGpuMemoryBufferManager(), renderer_settings(),

ui/compositor/compositor.cc

@@ -106,7 +106,6 @@ Compositor::Compositor(const viz::FrameSinkId& frame_sink_id,
   settings.layers_always_allowed_lcd_text = true;
   // Use occlusion to allow more overlapping windows to take less memory.
   settings.use_occlusion_for_tile_prioritization = true;
-  refresh_rate_ = context_factory_->GetRefreshRate();
   settings.main_frame_before_activation_enabled = false;
   settings.delegated_sync_points_required =
       context_factory_->SyncTokensRequiredForDisplayCompositor();
@@ -472,8 +471,6 @@ void Compositor::SetDisplayVSyncParameters(base::TimeTicks timebase,
 
   vsync_timebase_ = timebase;
   vsync_interval_ = interval;
-  refresh_rate_ =
-      base::Time::kMillisecondsPerSecond / interval.InMillisecondsF();
   if (context_factory_private_) {
     context_factory_private_->SetDisplayVSyncParameters(this, timebase,
                                                         interval);
@@ -636,6 +633,11 @@ void Compositor::DidSubmitCompositorFrame() {
     observer.OnCompositingStarted(this, start_time);
 }
 
+void Compositor::FrameIntervalUpdated(base::TimeDelta interval) {
+  refresh_rate_ =
+      base::Time::kMicrosecondsPerSecond / interval.InMicrosecondsF();
+}
+
 void Compositor::OnFirstSurfaceActivation(
     const viz::SurfaceInfo& surface_info) {
   NOTREACHED();

ui/compositor/compositor.h

@@ -182,9 +182,6 @@ class COMPOSITOR_EXPORT ContextFactory {
   // Destroys per-compositor data.
   virtual void RemoveCompositor(Compositor* compositor) = 0;
 
-  // Returns refresh rate. Tests may return higher values.
-  virtual double GetRefreshRate() const = 0;
-
   // Gets the GPU memory buffer manager.
   virtual gpu::GpuMemoryBufferManager* GetGpuMemoryBufferManager() = 0;
 
@@ -415,6 +412,7 @@ class COMPOSITOR_EXPORT Compositor : public cc::LayerTreeHostClient,
   // cc::LayerTreeHostSingleThreadClient implementation.
   void DidSubmitCompositorFrame() override;
   void DidLoseLayerTreeFrameSink() override {}
+  void FrameIntervalUpdated(base::TimeDelta interval) override;
 
   // viz::HostFrameSinkClient implementation.
   void OnFirstSurfaceActivation(const viz::SurfaceInfo& surface_info) override;
@@ -465,8 +463,9 @@ class COMPOSITOR_EXPORT Compositor : public cc::LayerTreeHostClient,
   // A sequence number of a current compositor frame for use with metrics.
   int activated_frame_count_ = 0;
 
-  // Current vsync refresh rate per second.
-  float refresh_rate_ = 0.f;
+  // Current vsync refresh rate per second. Initialized to 60hz as a reasonable
+  // value until first begin frame arrives with the real refresh rate.
+  float refresh_rate_ = 60.f;
 
   // A map from child id to parent id.
   std::unordered_set<viz::FrameSinkId, viz::FrameSinkIdHash> child_frame_sinks_;

ui/compositor/test/fake_context_factory.cc

@@ -55,10 +55,6 @@ void FakeContextFactory::RemoveCompositor(ui::Compositor* compositor) {
   frame_sink_ = nullptr;
 }
 
-double FakeContextFactory::GetRefreshRate() const {
-  return 200.0;
-}
-
 gpu::GpuMemoryBufferManager* FakeContextFactory::GetGpuMemoryBufferManager() {
   return &gpu_memory_buffer_manager_;
 }

ui/compositor/test/fake_context_factory.h

@@ -36,7 +36,6 @@ class FakeContextFactory : public ui::ContextFactory {
   scoped_refptr<viz::ContextProvider> SharedMainThreadContextProvider()
       override;
   void RemoveCompositor(ui::Compositor* compositor) override;
-  double GetRefreshRate() const override;
   gpu::GpuMemoryBufferManager* GetGpuMemoryBufferManager() override;
   cc::TaskGraphRunner* GetTaskGraphRunner() override;
   void AddObserver(ui::ContextFactoryObserver* observer) override {}

ui/compositor/test/in_process_context_factory.cc

@@ -330,10 +330,6 @@ void InProcessContextFactory::RemoveCompositor(Compositor* compositor) {
   per_compositor_data_.erase(it);
 }
 
-double InProcessContextFactory::GetRefreshRate() const {
-  return refresh_rate_;
-}
-
 gpu::GpuMemoryBufferManager*
 InProcessContextFactory::GetGpuMemoryBufferManager() {
   return &gpu_memory_buffer_manager_;

ui/compositor/test/in_process_context_factory.h

@@ -65,7 +65,6 @@ class InProcessContextFactory : public ContextFactory,
   scoped_refptr<viz::ContextProvider> SharedMainThreadContextProvider()
       override;
   void RemoveCompositor(Compositor* compositor) override;
-  double GetRefreshRate() const override;
   gpu::GpuMemoryBufferManager* GetGpuMemoryBufferManager() override;
   cc::TaskGraphRunner* GetTaskGraphRunner() override;
   viz::FrameSinkId AllocateFrameSinkId() override;