[cc] Track observer status in ExternalBeginFrameSource.

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 d05e2dfeb2fab0a12eb3dc1891356d2d97857608..37f70b248be5abfce4b9033e52a954aa08a0b609 100644
--- a/cc/scheduler/begin_frame_source.cc
+++ b/cc/scheduler/begin_frame_source.cc
@@ -233,6 +233,111 @@ void DelayBasedBeginFrameSource::OnTimerTick() {
   }
 }
 
+// BeginFrameObserverAckTracker -------------------------------------------
+BeginFrameObserverAckTracker::BeginFrameObserverAckTracker()
+    : current_source_id_(0),
+      current_sequence_number_(BeginFrameArgs::kStartingFrameNumber),
+      observers_had_damage_(false) {}
+
+BeginFrameObserverAckTracker::~BeginFrameObserverAckTracker() {}
+
+void BeginFrameObserverAckTracker::OnBeginFrame(const BeginFrameArgs& args) {
+  if (current_source_id_ != args.source_id)
+    SourceChanged(args);
+
+  DCHECK_GE(args.sequence_number, current_sequence_number_);
+  // Reset for new BeginFrame.
+  current_sequence_number_ = args.sequence_number;
+  finished_observers_.clear();
+  observers_had_damage_ = false;
+}
+
+void BeginFrameObserverAckTracker::OnObserverBeginFrame(
+    BeginFrameObserver* obs,
+    const BeginFrameArgs& args) {
+  if (current_source_id_ != args.source_id)
+    SourceChanged(args);
+
+  DCHECK_EQ(args.sequence_number, current_sequence_number_);
+  // Observer may choose to drop the BeginFrame. In this case, it has finished,
+  // but is not considered up-to-date.
+  if (obs->LastUsedBeginFrameArgs().sequence_number != current_sequence_number_)

[brianderson, 2017/02/15 20:50:47]

Why is this != instead of ==?

[Eric Seckler, 2017/02/17 19:10:01]

This checks if the observer actually uses the BeginFrame. If it doesn't,
LastUsedBeginFrameArgs will be an older BeginFrame. In that case, the observer
has already finished the current BeginFrame, because it didn't use it.

[brianderson, 2017/02/17 20:49:54]

Ah, I see. Instead of doing this, can we require Observers to *always* call
OnObserverFinishedFrame in response to a BeginFrame - even when it drops a
BeginFrame?

[Eric Seckler, 2017/02/17 22:49:35]

I think I'll make those changes and send out another patch for that next week :)

[Eric Seckler, 2017/02/21 12:10:57]

Turns out it doesn't require much changes at this point, since aside from
Scheduler/DisplayScheduler, no observers call DidFinishFrame currently. When I
add those calls later, I'll make sure to also add them where they drop a
BeginFrame. Added a call in cc::Scheduler. The DisplayScheduler doesn't seem to
ever drop a BeginFrame this way.

Also updated the tests to match.

+    finished_observers_.insert(obs);
+}
+
+void BeginFrameObserverAckTracker::SourceChanged(const BeginFrameArgs& args) {
+  current_source_id_ = args.source_id;
+  current_sequence_number_ = args.sequence_number;
+
+  // Mark all observers invalid: We report an invalid frame until every observer
+  // has confirmed the frame.
+  for (auto& entry : latest_confirmed_sequence_numbers_)
+    entry.second = BeginFrameArgs::kInvalidFrameNumber;
+}
+
+void BeginFrameObserverAckTracker::OnObserverFinishedFrame(
+    BeginFrameObserver* obs,
+    const BeginFrameAck& ack) {
+  if (ack.source_id != current_source_id_)
+    return;
+
+  DCHECK_LE(ack.sequence_number, current_sequence_number_);
+  if (ack.sequence_number != current_sequence_number_)
+    return;
+
+  finished_observers_.insert(obs);
+  observers_had_damage_ |= ack.has_damage;
+
+  // We max() with the current value in |latest_confirmed_sequence_numbers_| to
+  // handle situations where an observer just started observing (again) and may
+  // acknowledge with an ancient latest_confirmed_sequence_number.
+  latest_confirmed_sequence_numbers_[obs] =
+      std::max(ack.latest_confirmed_sequence_number,
+               latest_confirmed_sequence_numbers_[obs]);
+}
+
+void BeginFrameObserverAckTracker::OnObserverAdded(BeginFrameObserver* obs) {
+  observers_.insert(obs);
+
+  // Since the observer didn't want BeginFrames before, we consider it
+  // up-to-date up to the last BeginFrame, except if it already handled the
+  // current BeginFrame. In which case, we consider it up-to-date up to the
+  // current one.
+  DCHECK_LT(BeginFrameArgs::kInvalidFrameNumber, current_sequence_number_);
+  const BeginFrameArgs& last_args = obs->LastUsedBeginFrameArgs();
+  if (last_args.IsValid() &&
+      last_args.sequence_number == current_sequence_number_ &&
+      last_args.source_id == current_source_id_) {
+    latest_confirmed_sequence_numbers_[obs] = current_sequence_number_;
+    finished_observers_.insert(obs);
+  } else {
+    latest_confirmed_sequence_numbers_[obs] = current_sequence_number_ - 1;
+  }
+}
+
+void BeginFrameObserverAckTracker::OnObserverRemoved(BeginFrameObserver* obs) {
+  observers_.erase(obs);
+  finished_observers_.erase(obs);
+  latest_confirmed_sequence_numbers_.erase(obs);
+}
+
+bool BeginFrameObserverAckTracker::AllObserversFinishedFrame() const {
+  return base::STLIncludes(finished_observers_, observers_);

[brianderson, 2017/02/15 20:50:48]

It looks like STLIncludes depends on std::includes which takes generic
InputIterators where it can't assume std::distance will be constant time, so
it's unlikely std::include can do an efficient early out based on the sizes of
the two containers.

It might help to start this method with:
if (finished_obervers.size() < observers.size())
  return false;

Also, if we can guarantee that we only get OnObserverFinishedFrame from
observers that are currently observing, we might be able to just return
(finished_observers.size == observers.size()). Any idea if that's the case given
the current state of the code? I assume not given the request+response is async.

[Eric Seckler, 2017/02/17 19:10:01]

Added the early out. I don't think we can guarantee that we never receive an ack
from an already-inactive BFO, given that some of these ACKs can come
asynchronously from across process boundaries.

+}
+
+bool BeginFrameObserverAckTracker::AnyObserversHadDamage() const {
+  return observers_had_damage_;
+}
+
+uint64_t BeginFrameObserverAckTracker::LatestConfirmedSequenceNumber() const {
+  uint64_t latest_confirmed_sequence_number = current_sequence_number_;
+  for (const auto& entry : latest_confirmed_sequence_numbers_) {
+    latest_confirmed_sequence_number =
+        std::min(latest_confirmed_sequence_number, entry.second);
+  }
+  return latest_confirmed_sequence_number;
+}
+
 // ExternalBeginFrameSource -----------------------------------------------
 ExternalBeginFrameSource::ExternalBeginFrameSource(
     ExternalBeginFrameSourceClient* client)
@@ -248,6 +353,7 @@ void ExternalBeginFrameSource::AddObserver(BeginFrameObserver* obs) {
 
   bool observers_was_empty = observers_.empty();
   observers_.insert(obs);
+  ack_tracker_.OnObserverAdded(obs);
   obs->OnBeginFrameSourcePausedChanged(paused_);
   if (observers_was_empty)
     client_->OnNeedsBeginFrames(true);
@@ -261,6 +367,7 @@ void ExternalBeginFrameSource::AddObserver(BeginFrameObserver* obs) {
              (missed_begin_frame_args_.sequence_number >
               last_args.sequence_number));
       obs->OnBeginFrame(missed_begin_frame_args_);
+      ack_tracker_.OnObserverBeginFrame(obs, missed_begin_frame_args_);
     }
   }
 }
@@ -270,12 +377,20 @@ void ExternalBeginFrameSource::RemoveObserver(BeginFrameObserver* obs) {
   DCHECK(observers_.find(obs) != observers_.end());
 
   observers_.erase(obs);
+  ack_tracker_.OnObserverRemoved(obs);
+  MaybeFinishFrame();
   if (observers_.empty()) {
     missed_begin_frame_args_ = BeginFrameArgs();
     client_->OnNeedsBeginFrames(false);
   }
 }
 
+void ExternalBeginFrameSource::DidFinishFrame(BeginFrameObserver* obs,
+                                              const BeginFrameAck& ack) {
+  ack_tracker_.OnObserverFinishedFrame(obs, ack);
+  MaybeFinishFrame();
+}
+
 bool ExternalBeginFrameSource::IsThrottled() const {
   return true;
 }
@@ -290,11 +405,35 @@ void ExternalBeginFrameSource::OnSetBeginFrameSourcePaused(bool paused) {
 }
 
 void ExternalBeginFrameSource::OnBeginFrame(const BeginFrameArgs& args) {
+  if (frame_active_)
+    FinishFrame();
+
+  frame_active_ = true;
   missed_begin_frame_args_ = args;
   missed_begin_frame_args_.type = BeginFrameArgs::MISSED;
+  ack_tracker_.OnBeginFrame(args);
   std::unordered_set<BeginFrameObserver*> observers(observers_);
-  for (auto* obs : observers)
+  for (auto* obs : observers) {
     obs->OnBeginFrame(args);
+    ack_tracker_.OnObserverBeginFrame(obs, args);
+  }
+  MaybeFinishFrame();
+}
+
+void ExternalBeginFrameSource::MaybeFinishFrame() {
+  if (!frame_active_ || !ack_tracker_.AllObserversFinishedFrame())
+    return;
+  FinishFrame();
+}
+
+void ExternalBeginFrameSource::FinishFrame() {
+  frame_active_ = false;
+
+  BeginFrameAck ack(missed_begin_frame_args_.source_id,
+                    missed_begin_frame_args_.sequence_number,
+                    ack_tracker_.LatestConfirmedSequenceNumber(), 0,
+                    ack_tracker_.AnyObserversHadDamage());
+  client_->OnDidFinishFrame(ack);
 }
 
 }  // namespace cc