[cc] Adds source_id and sequence_number to BeginFrameArgs.

cc/scheduler/begin_frame_source.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/scheduler/begin_frame_source.h"
 
 #include <stddef.h>
 
+#include "base/atomic_sequence_num.h"
 #include "base/auto_reset.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/trace_event_argument.h"
 #include "cc/scheduler/delay_based_time_source.h"
 #include "cc/scheduler/scheduler.h"
 
 namespace cc {
 
 namespace {
 // kDoubleTickDivisor prevents the SyntheticBFS from sending BeginFrames too
 // often to an observer.
 static const double kDoubleTickDivisor = 2.0;
 }
 
-// BeginFrameObserverBase -----------------------------------------------
+// BeginFrameObserverBase -------------------------------------------------

[Sami, 2016/12/15 13:53:00]

Very detail-oriented :)

[Eric Seckler, 2016/12/15 14:59:38]

.. and maybe a touch of ocd :)

 BeginFrameObserverBase::BeginFrameObserverBase()
     : last_begin_frame_args_(), dropped_begin_frame_args_(0) {
 }
 
 const BeginFrameArgs& BeginFrameObserverBase::LastUsedBeginFrameArgs() const {
   return last_begin_frame_args_;
 }
+
 void BeginFrameObserverBase::OnBeginFrame(const BeginFrameArgs& args) {
   DCHECK(args.IsValid());
   DCHECK(args.frame_time >= last_begin_frame_args_.frame_time);
+  DCHECK(args.sequence_number > last_begin_frame_args_.sequence_number ||
+         args.source_id != last_begin_frame_args_.source_id);
   bool used = OnBeginFrameDerivedImpl(args);
   if (used) {
     last_begin_frame_args_ = args;
   } else {
     ++dropped_begin_frame_args_;
   }
 }
 
+// BeginFrameSource -------------------------------------------------------
+namespace {
+static base::StaticAtomicSequenceNumber next_source_id_;

[Sami, 2016/12/15 13:53:00]

nit: g_next_source_id since this isn't a member.

[Eric Seckler, 2016/12/15 14:59:38]

Done.

+}  // namespace
+
+BeginFrameSource::BeginFrameSource() : source_id_(next_source_id_.GetNext()) {}
+
+uint32_t BeginFrameSource::source_id() const {
+  return source_id_;
+}
+
+// StubBeginFrameSource ---------------------------------------------------
 bool StubBeginFrameSource::IsThrottled() const {
   return true;
 }
 
-// SyntheticBeginFrameSource ---------------------------------------------
+// SyntheticBeginFrameSource ----------------------------------------------
 SyntheticBeginFrameSource::~SyntheticBeginFrameSource() = default;
 
-// BackToBackBeginFrameSource --------------------------------------------
+// BackToBackBeginFrameSource ---------------------------------------------
 BackToBackBeginFrameSource::BackToBackBeginFrameSource(
     std::unique_ptr<DelayBasedTimeSource> time_source)
-    : time_source_(std::move(time_source)), weak_factory_(this) {
+    : time_source_(std::move(time_source)),
+      next_sequence_number_(BeginFrameArgs::kStartingFrameNumber),
+      weak_factory_(this) {
   time_source_->SetClient(this);
   // The time_source_ ticks immediately, so we SetActive(true) for a single
   // tick when we need it, and keep it as SetActive(false) otherwise.
   time_source_->SetTimebaseAndInterval(base::TimeTicks(), base::TimeDelta());
 }
 
 BackToBackBeginFrameSource::~BackToBackBeginFrameSource() = default;
 
 void BackToBackBeginFrameSource::AddObserver(BeginFrameObserver* obs) {
   DCHECK(obs);
   DCHECK(observers_.find(obs) == observers_.end());
   observers_.insert(obs);
   pending_begin_frame_observers_.insert(obs);
   obs->OnBeginFrameSourcePausedChanged(false);
   time_source_->SetActive(true);
 }
 
 void BackToBackBeginFrameSource::RemoveObserver(BeginFrameObserver* obs) {
   DCHECK(obs);
   DCHECK(observers_.find(obs) != observers_.end());
   observers_.erase(obs);
   pending_begin_frame_observers_.erase(obs);
-  if (observers_.empty())
+  if (pending_begin_frame_observers_.empty())

[brianderson, 2016/12/15 20:30:36]

Is this change related? If it fixes a bug, please create a separate patch along
with a unit test.

[Eric Seckler, 2016/12/16 17:25:57]

ok, will do.

     time_source_->SetActive(false);
 }
 
 void BackToBackBeginFrameSource::DidFinishFrame(BeginFrameObserver* obs,
                                                 size_t remaining_frames) {
   if (remaining_frames == 0 && observers_.find(obs) != observers_.end()) {
     pending_begin_frame_observers_.insert(obs);
     time_source_->SetActive(true);
   }
 }
 
 bool BackToBackBeginFrameSource::IsThrottled() const {
   return false;
 }
 
 void BackToBackBeginFrameSource::OnTimerTick() {
   base::TimeTicks frame_time = time_source_->LastTickTime();
   base::TimeDelta default_interval = BeginFrameArgs::DefaultInterval();
   BeginFrameArgs args = BeginFrameArgs::Create(
-      BEGINFRAME_FROM_HERE, frame_time, frame_time + default_interval,
-      default_interval, BeginFrameArgs::NORMAL);
+      BEGINFRAME_FROM_HERE, source_id(), next_sequence_number_++, frame_time,

[brianderson, 2016/12/15 20:30:35]

Please move the next_sequence_number_++ after the call for readability.

[Eric Seckler, 2016/12/16 17:25:57]

Done.

+      frame_time + default_interval, default_interval, BeginFrameArgs::NORMAL);
 
   // This must happen after getting the LastTickTime() from the time source.
   time_source_->SetActive(false);
 
   std::unordered_set<BeginFrameObserver*> pending_observers;
   pending_observers.swap(pending_begin_frame_observers_);
   for (BeginFrameObserver* obs : pending_observers)
     obs->OnBeginFrame(args);
 }
 
@@ skipping 20 matching lines @@
   time_source_->SetTimebaseAndInterval(timebase, interval);
 }
 
 void DelayBasedBeginFrameSource::SetAuthoritativeVSyncInterval(
     base::TimeDelta interval) {
   authoritative_interval_ = interval;
   OnUpdateVSyncParameters(last_timebase_, interval);
 }
 
 BeginFrameArgs DelayBasedBeginFrameSource::CreateBeginFrameArgs(
+    uint64_t sequence_number,
     base::TimeTicks frame_time,
     BeginFrameArgs::BeginFrameArgsType type) {
-  return BeginFrameArgs::Create(BEGINFRAME_FROM_HERE, frame_time,
-                                time_source_->NextTickTime(),
-                                time_source_->Interval(), type);
+  return BeginFrameArgs::Create(
+      BEGINFRAME_FROM_HERE, source_id(), sequence_number, frame_time,
+      time_source_->NextTickTime(), time_source_->Interval(), type);
 }
 
 void DelayBasedBeginFrameSource::AddObserver(BeginFrameObserver* obs) {
   DCHECK(obs);
   DCHECK(observers_.find(obs) == observers_.end());
 
   observers_.insert(obs);
   obs->OnBeginFrameSourcePausedChanged(false);
   time_source_->SetActive(true);
-  BeginFrameArgs args = CreateBeginFrameArgs(
-      time_source_->NextTickTime() - time_source_->Interval(),
-      BeginFrameArgs::MISSED);
+
+  // Check if |current_begin_frame_args_| are still valid. They may not be valid

[brianderson, 2016/12/15 20:30:36]

"valid" is an overloaded term in this context. Can you talk about sequence
number specifically?

[Eric Seckler, 2016/12/16 17:25:58]

Done.

+  // if OnTimerTick() has never run yet, the time source was inactive before
+  // AddObserver() was called, or the interval changed.
+  base::TimeTicks last_or_missed_tick_time =
+      time_source_->NextTickTime() - time_source_->Interval();
+  if (current_begin_frame_args_.IsValid() &&
+      current_begin_frame_args_.frame_time == last_or_missed_tick_time &&
+      current_begin_frame_args_.interval == time_source_->Interval()) {
+    // Ensure that the args have the right type.
+    current_begin_frame_args_.type = BeginFrameArgs::MISSED;
+  } else {
+    // The args are invalid and we need to create new ones with the missed
+    // tick's time and a new sequence number.
+    current_begin_frame_args_ =
+        CreateBeginFrameArgs(current_begin_frame_args_.sequence_number + 1,

[brianderson, 2016/12/15 20:30:36]

"current_begin_frame_args_.sequence_number + 1" relies on the fact that the
invalid value is 0 and initial value is 1, which is brittle and difficult to
decode that's how this works for the first frame.

Can you do what you did in BackToBackBeginFrameSource instead? That might also
help avoid having a sequence number as an argument to CreateBegiFrameArgs.

[Eric Seckler, 2016/12/16 17:25:57]

Done.

+                             last_or_missed_tick_time, BeginFrameArgs::MISSED);
+  }
+
   BeginFrameArgs last_args = obs->LastUsedBeginFrameArgs();
   if (!last_args.IsValid() ||
-      (args.frame_time >
-       last_args.frame_time + args.interval / kDoubleTickDivisor)) {
-    obs->OnBeginFrame(args);
+      (current_begin_frame_args_.frame_time >
+       last_args.frame_time +
+           current_begin_frame_args_.interval / kDoubleTickDivisor)) {
+    obs->OnBeginFrame(current_begin_frame_args_);
   }
 }
 
 void DelayBasedBeginFrameSource::RemoveObserver(BeginFrameObserver* obs) {
   DCHECK(obs);
   DCHECK(observers_.find(obs) != observers_.end());
 
   observers_.erase(obs);
   if (observers_.empty())
     time_source_->SetActive(false);
 }
 
 bool DelayBasedBeginFrameSource::IsThrottled() const {
   return true;
 }
 
 void DelayBasedBeginFrameSource::OnTimerTick() {
-  BeginFrameArgs args = CreateBeginFrameArgs(time_source_->LastTickTime(),
-                                             BeginFrameArgs::NORMAL);
+  current_begin_frame_args_ = CreateBeginFrameArgs(
+      current_begin_frame_args_.sequence_number + 1,
+      time_source_->LastTickTime(), BeginFrameArgs::NORMAL);
   std::unordered_set<BeginFrameObserver*> observers(observers_);
   for (auto* obs : observers) {
     BeginFrameArgs last_args = obs->LastUsedBeginFrameArgs();
     if (!last_args.IsValid() ||
-        (args.frame_time >
-         last_args.frame_time + args.interval / kDoubleTickDivisor))
-      obs->OnBeginFrame(args);
+        (current_begin_frame_args_.frame_time >
+         last_args.frame_time +
+             current_begin_frame_args_.interval / kDoubleTickDivisor)) {
+      obs->OnBeginFrame(current_begin_frame_args_);
+    }
   }
 }
 
+// ExternalBeginFrameSource -----------------------------------------------
 ExternalBeginFrameSource::ExternalBeginFrameSource(
     ExternalBeginFrameSourceClient* client)
     : client_(client) {
   DCHECK(client_);
 }
 
 ExternalBeginFrameSource::~ExternalBeginFrameSource() = default;
 
 void ExternalBeginFrameSource::AddObserver(BeginFrameObserver* obs) {
   DCHECK(obs);
   DCHECK(observers_.find(obs) == observers_.end());
 
   bool observers_was_empty = observers_.empty();
   observers_.insert(obs);
   obs->OnBeginFrameSourcePausedChanged(paused_);
   if (observers_was_empty)
     client_->OnNeedsBeginFrames(true);
 
   // Send a MISSED begin frame if necessary.
   if (missed_begin_frame_args_.IsValid()) {
     BeginFrameArgs last_args = obs->LastUsedBeginFrameArgs();
     if (!last_args.IsValid() ||
-        (missed_begin_frame_args_.frame_time > last_args.frame_time)) {
+        (missed_begin_frame_args_.source_id != last_args.source_id) ||
+        (missed_begin_frame_args_.sequence_number >
+         last_args.sequence_number)) {

[brianderson, 2016/12/15 20:30:36]

I think this if statement should stay the way it was before since monotonic
timestamps is the contract we have with the animation system.

The lines you added here should probably turn into a DCHECK instead.

[Eric Seckler, 2016/12/16 17:25:57]

Hm, okay. Will do. I was thinking ahead, considering situations where we might
want to retry a frame with the same timestamp. That'd be useful for headless
when a frame fails for some reason but we still want a deterministic result.
Anyway, seems like this requires some more thinking on its own :)

[brianderson, 2016/12/16 17:53:38]

I see. Maybe the animation system is okay with monotonic rather than strictly
monotonic timestamps - but I imagine there could be weird corner cases to debug.
It definitely didn't work when we ticked backwards in time!

       obs->OnBeginFrame(missed_begin_frame_args_);
     }
   }
 }
 
 void ExternalBeginFrameSource::RemoveObserver(BeginFrameObserver* obs) {
   DCHECK(obs);
   DCHECK(observers_.find(obs) != observers_.end());
 
   observers_.erase(obs);
@@ skipping 18 matching lines @@
 
 void ExternalBeginFrameSource::OnBeginFrame(const BeginFrameArgs& args) {
   missed_begin_frame_args_ = args;
   missed_begin_frame_args_.type = BeginFrameArgs::MISSED;
   std::unordered_set<BeginFrameObserver*> observers(observers_);
   for (auto* obs : observers)
     obs->OnBeginFrame(args);
 }
 
 }  // namespace cc