cc: Introduce BeginFrame sequence numbers and acknowledgements.

cc/scheduler/begin_frame_source.h

Index: cc/scheduler/begin_frame_source.h
diff --git a/cc/scheduler/begin_frame_source.h b/cc/scheduler/begin_frame_source.h
index bde01afd79ec6bd72a1d525c85b9d6d49bafe9d0..1208d2bc81368cfe74baff3edaf90a2820f65c43 100644
--- a/cc/scheduler/begin_frame_source.h
+++ b/cc/scheduler/begin_frame_source.h
@@ -10,6 +10,7 @@
 
 #include <set>
 #include <string>
+#include <unordered_set>
 
 #include "base/logging.h"
 #include "base/macros.h"
@@ -26,8 +27,10 @@ class CC_EXPORT BeginFrameObserver {
   virtual ~BeginFrameObserver() {}
 
   // The |args| given to OnBeginFrame is guaranteed to have
-  // |args|.IsValid()==true and have |args|.frame_time
-  // field be strictly greater than the previous call.
+  // |args|.IsValid()==true. If |args|.source_id did not change between
+  // invocations, |args|.sequence_number is guaranteed to be be strictly greater
+  // than the previous call. Further, |args|.frame_time is guaranteed to be
+  // greater than or equal to the previous call.

[brianderson, 2016/12/09 01:08:11]

To "Further, |args|.frame_time is guaranteed to be greater than or equal to the
previous call", can you add: "even if the source_id changes.".

I'm assuming that's still true after this patch?

[Eric Seckler, 2016/12/09 16:47:16]

Yes, done!

   //
   // Side effects: This function can (and most of the time *will*) change the
   // return value of the LastUsedBeginFrameArgs method. See the documentation
@@ -99,14 +102,21 @@ class CC_EXPORT BeginFrameObserverBase : public BeginFrameObserver {
 // all BeginFrameSources *must* provide.
 class CC_EXPORT BeginFrameSource {
  public:
+  BeginFrameSource();
   virtual ~BeginFrameSource() {}
 
-  // DidFinishFrame provides back pressure to a frame source about frame
-  // processing (rather than toggling SetNeedsBeginFrames every frame). It is
-  // used by systems like the BackToBackFrameSource to make sure only one frame
-  // is pending at a time.
+  // BeginFrameObservers use DidFinishFrame to acknowledge that they have
+  // completed handling a BeginFrame.
+  //
+  // The DisplayScheduler uses these acknowledgments to trigger an early
+  // deadline once all BeginFrameObservers have completed a frame.
+  //
+  // They also provide back pressure to a frame source about frame processing
+  // (rather than toggling SetNeedsBeginFrames every frame). For example, the
+  // BackToBackFrameSource uses them to make sure only one frame is pending at a
+  // time.
   virtual void DidFinishFrame(BeginFrameObserver* obs,
-                              size_t remaining_frames) = 0;
+                              const BeginFrameAck& ack) = 0;
 
   // Add/Remove an observer from the source. When no observers are added the BFS
   // should shut down its timers, disable vsync, etc.
@@ -116,13 +126,22 @@ class CC_EXPORT BeginFrameSource {
   // Returns false if the begin frame source will just continue to produce
   // begin frames without waiting.
   virtual bool IsThrottled() const = 0;
+
+  // Returns an identifier for this BeginFrameSource. Guaranteed unique within a
+  // process, and very likely unique across processes. This is used to create

[brianderson, 2016/12/09 01:08:11]

"Guaranteed unique within a process, but not across processes." since we don't
want to rely on that.

[Eric Seckler, 2016/12/09 16:47:17]

Okay. I guess the question is whether there may be a situation where the
"origin" BeginFrameSource (the one that initiates the BeginFrames) of an
observer might be changed to a different one in a different process. But this
shouldn't happen, since those BeginFrameSources typically only live in the
browser process (or the window server in Mus), right?

If we agree that we don't need cross-process identities, I'll get rid of the
process id mangling in CreateSourceId().

+  // BeginFrames that originate at this source. Note that BeginFrameSources may
+  // pass on BeginFrames created by other sources, with different IDs.
+  uint64_t source_id() const;
+
+ private:
+  uint64_t source_id_;
 };
 
 // A BeginFrameSource that does nothing.
 class CC_EXPORT StubBeginFrameSource : public BeginFrameSource {
  public:
   void DidFinishFrame(BeginFrameObserver* obs,
-                      size_t remaining_frames) override {}
+                      const BeginFrameAck& ack) override {}
   void AddObserver(BeginFrameObserver* obs) override {}
   void RemoveObserver(BeginFrameObserver* obs) override {}
   bool IsThrottled() const override;
@@ -152,7 +171,7 @@ class CC_EXPORT BackToBackBeginFrameSource : public SyntheticBeginFrameSource,
   void AddObserver(BeginFrameObserver* obs) override;
   void RemoveObserver(BeginFrameObserver* obs) override;
   void DidFinishFrame(BeginFrameObserver* obs,
-                      size_t remaining_frames) override;
+                      const BeginFrameAck& ack) override;
   bool IsThrottled() const override;
 
   // SyntheticBeginFrameSource implementation.
@@ -167,6 +186,7 @@ class CC_EXPORT BackToBackBeginFrameSource : public SyntheticBeginFrameSource,
   std::unique_ptr<DelayBasedTimeSource> time_source_;
   std::unordered_set<BeginFrameObserver*> observers_;
   std::unordered_set<BeginFrameObserver*> pending_begin_frame_observers_;
+  uint64_t next_sequence_number_;
   base::WeakPtrFactory<BackToBackBeginFrameSource> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(BackToBackBeginFrameSource);
@@ -185,7 +205,7 @@ class CC_EXPORT DelayBasedBeginFrameSource : public SyntheticBeginFrameSource,
   void AddObserver(BeginFrameObserver* obs) override;
   void RemoveObserver(BeginFrameObserver* obs) override;
   void DidFinishFrame(BeginFrameObserver* obs,
-                      size_t remaining_frames) override {}
+                      const BeginFrameAck& ack) override {}
   bool IsThrottled() const override;
 
   // SyntheticBeginFrameSource implementation.
@@ -197,21 +217,64 @@ class CC_EXPORT DelayBasedBeginFrameSource : public SyntheticBeginFrameSource,
   void OnTimerTick() override;
 
  private:
-  BeginFrameArgs CreateBeginFrameArgs(base::TimeTicks frame_time,
+  BeginFrameArgs CreateBeginFrameArgs(uint64_t sequence_number,
+                                      base::TimeTicks frame_time,
                                       BeginFrameArgs::BeginFrameArgsType type);
 
   std::unique_ptr<DelayBasedTimeSource> time_source_;
   std::unordered_set<BeginFrameObserver*> observers_;
   base::TimeTicks last_timebase_;
   base::TimeDelta authoritative_interval_;
+  BeginFrameArgs current_begin_frame_args_;
 
   DISALLOW_COPY_AND_ASSIGN(DelayBasedBeginFrameSource);
 };
 
+// Helper class that tracks outstanding acknowledgments from
+// BeginFrameObservers.
+class CC_EXPORT BeginFrameObserverAckTracker {
+ public:
+  BeginFrameObserverAckTracker();
+  virtual ~BeginFrameObserverAckTracker();
+
+  // The BeginFrameSource uses these methods to notify us when a BeginFrame was
+  // started and sent to a specific observer, an observer finished a frame, or
+  // an observer was added/removed.
+  void OnBeginFrame(const BeginFrameArgs& args);
+  void OnObserverBeginFrame(BeginFrameObserver* obs,
+                            const BeginFrameArgs& args);
+  void OnObserverFinishedFrame(BeginFrameObserver* obs,
+                               const BeginFrameAck& ack);
+  void OnObserverAdded(BeginFrameObserver* obs);
+  void OnObserverRemoved(BeginFrameObserver* obs);
+
+  // Returns |true| if all the source's observers completed the current frame.
+  bool AllObserversFinishedFrame() const;
+
+  // Returns |true| if any observer had damages during the current frame.
+  bool AnyObserversHadDamages() const;
+
+  // Returns the sequence number of the latest_confirmed_frame that any of
+  // the observers have contributed for the latest frame.
+  uint64_t LatestConfirmedFrame() const;
+
+ private:
+  void SourceChanged(const BeginFrameArgs& args);
+
+  uint64_t current_source_id_;
+  uint64_t current_sequence_number_;
+  std::set<BeginFrameObserver*> observers_;
+  std::set<BeginFrameObserver*> finished_observers_;
+  bool observers_had_damages_;

[brianderson, 2016/12/09 01:08:11]

observers_had_damage_

[Eric Seckler, 2016/12/09 16:47:17]

Done.

[Sami, 2016/12/09 18:36:37]

Should we do a s/damages/damage/g overall?

+  std::unordered_map<BeginFrameObserver*, uint64_t> latest_confirmed_frames_;
+};
+
 class CC_EXPORT ExternalBeginFrameSourceClient {
  public:
   // Only called when changed.  Assumed false by default.
   virtual void OnNeedsBeginFrames(bool needs_begin_frames) = 0;
+  // Called when all observers have completed a frame.
+  virtual void OnDidFinishFrame(const BeginFrameAck& ack) = 0;
 };
 
 // A BeginFrameSource that is only ticked manually.  Usually the endpoint
@@ -228,22 +291,45 @@ class CC_EXPORT ExternalBeginFrameSource : public BeginFrameSource {
   void AddObserver(BeginFrameObserver* obs) override;
   void RemoveObserver(BeginFrameObserver* obs) override;
   void DidFinishFrame(BeginFrameObserver* obs,
-                      size_t remaining_frames) override {}
+                      const BeginFrameAck& ack) override;
   bool IsThrottled() const override;
 
   void OnSetBeginFrameSourcePaused(bool paused);
   void OnBeginFrame(const BeginFrameArgs& args);
 
  protected:
+  void MaybeFinishFrame();
+  void FinishFrame();
+
   BeginFrameArgs missed_begin_frame_args_;
   std::unordered_set<BeginFrameObserver*> observers_;
   ExternalBeginFrameSourceClient* client_;
   bool paused_ = false;
+  bool frame_active_ = false;
+  BeginFrameObserverAckTracker ack_tracker_;

[brianderson, 2016/12/09 01:08:11]

For my education, is this something that is actually used by this class at ToT?
i.e. do we ever have more than one observer for an ExternalBeginFrameSource?

You don't have to remove this. I'm just wondering if anything has changed since
I last looked at the code.

If it's dead code, maybe we should revisit how soon it'll become alive.

[Eric Seckler, 2016/12/09 16:47:17]

Mus uses ExternalBeginFrameSources in WindowCompositorFrameSink, which (aren't
currently but) should be hooked up to a Display. So I'd say, yeah, there'll be
such a use, at least until Mus get's rid of the additional Display in the
browser.

 
  private:
   DISALLOW_COPY_AND_ASSIGN(ExternalBeginFrameSource);
 };
 
+// A BeginFrameSource that delegates to another given source.

[brianderson, 2016/12/09 01:08:11]

Can you add a comment explaining why this class is needed as opposed to using
the delegate directly.

[Eric Seckler, 2016/12/09 16:47:16]

I think moving the ownership of the BeginFrameSource out of Display would render
the DelegatingBFS unnecessary. I'll get rid of it then.

+class CC_EXPORT DelegatingBeginFrameSource : public BeginFrameSource {
+ public:
+  // |delegate| lifetime must be preserved past the lifetime of this class.
+  explicit DelegatingBeginFrameSource(BeginFrameSource* delegate);
+  ~DelegatingBeginFrameSource() override {}
+
+  // BeginFrameSource implementation.
+  void DidFinishFrame(BeginFrameObserver* obs,
+                      const BeginFrameAck& ack) override;
+  void AddObserver(BeginFrameObserver* obs) override;
+  void RemoveObserver(BeginFrameObserver* obs) override;
+  bool IsThrottled() const override;
+
+ private:
+  BeginFrameSource* delegate_;  // Not owned.
+};
+
 }  // namespace cc
 
 #endif  // CC_SCHEDULER_BEGIN_FRAME_SOURCE_H_