[cc] Adds source_id and sequence_number to BeginFrameArgs.

cc/scheduler/begin_frame_source.cc

Index: cc/scheduler/begin_frame_source.cc
diff --git a/cc/scheduler/begin_frame_source.cc b/cc/scheduler/begin_frame_source.cc
index 30916948cec360b751b158e69316c4e6617f8b3b..caf11b3e21e3ed1b14f895ecb930e82f6cb05c58 100644
--- a/cc/scheduler/begin_frame_source.cc
+++ b/cc/scheduler/begin_frame_source.cc
@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include "base/atomic_sequence_num.h"
 #include "base/auto_reset.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ -25,7 +26,7 @@ namespace {
 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) {
 }
@@ -33,9 +34,12 @@ BeginFrameObserverBase::BeginFrameObserverBase()
 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;
@@ -44,17 +48,31 @@ void BeginFrameObserverBase::OnBeginFrame(const BeginFrameArgs& 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.
@@ -77,7 +95,7 @@ void BackToBackBeginFrameSource::RemoveObserver(BeginFrameObserver* 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);
 }
 
@@ -97,8 +115,8 @@ 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);
@@ -139,11 +157,12 @@ void DelayBasedBeginFrameSource::SetAuthoritativeVSyncInterval(
 }
 
 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) {
@@ -153,14 +172,31 @@ void DelayBasedBeginFrameSource::AddObserver(BeginFrameObserver* obs) {
   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_);
   }
 }
 
@@ -178,18 +214,22 @@ bool DelayBasedBeginFrameSource::IsThrottled() const {
 }
 
 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) {
@@ -212,7 +252,9 @@ void ExternalBeginFrameSource::AddObserver(BeginFrameObserver* obs) {
   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_);
     }
   }