cc: Introduce BeginFrame sequence numbers and acknowledgements.

cc/output/begin_frame_args.h

Index: cc/output/begin_frame_args.h
diff --git a/cc/output/begin_frame_args.h b/cc/output/begin_frame_args.h
index 78ddce99ddfcde3c4508f1d706d4b1eb205ff89f..30455a42d3c6539efe0ae66e0c111bf8bf7f888b 100644
--- a/cc/output/begin_frame_args.h
+++ b/cc/output/begin_frame_args.h
@@ -5,6 +5,8 @@
 #ifndef CC_OUTPUT_BEGIN_FRAME_ARGS_H_
 #define CC_OUTPUT_BEGIN_FRAME_ARGS_H_
 
+#include <stdint.h>
+
 #include <memory>
 
 #include "base/location.h"
@@ -63,6 +65,8 @@ struct CC_EXPORT BeginFrameArgs {
   // created by searching for "BeginFrameArgs::Create".
   // The location argument should **always** be BEGINFRAME_FROM_HERE macro.
   static BeginFrameArgs Create(CreationLocation location,
+                               uint64_t source_id,
+                               uint64_t sequence_number,
                                base::TimeTicks frame_time,
                                base::TimeTicks deadline,
                                base::TimeDelta interval,
@@ -81,6 +85,8 @@ struct CC_EXPORT BeginFrameArgs {
   std::unique_ptr<base::trace_event::ConvertableToTraceFormat> AsValue() const;
   void AsValueInto(base::trace_event::TracedValue* dict) const;
 
+  uint64_t source_id;
+  uint64_t sequence_number;
   base::TimeTicks frame_time;
   base::TimeTicks deadline;
   base::TimeDelta interval;
@@ -88,12 +94,46 @@ struct CC_EXPORT BeginFrameArgs {
   bool on_critical_path;
 
  private:
-  BeginFrameArgs(base::TimeTicks frame_time,
+  BeginFrameArgs(uint64_t source_id,
+                 uint64_t sequence_number,
+                 base::TimeTicks frame_time,
                  base::TimeTicks deadline,
                  base::TimeDelta interval,
                  BeginFrameArgsType type);
 };
 
+// Sent by a BeginFrameObserver as acknowledgment of completing a BeginFrame.
+struct CC_EXPORT BeginFrameAck {
+  BeginFrameAck();
+  BeginFrameAck(uint64_t source_id,
+                uint64_t sequence_number,
+                bool has_updates,
+                uint64_t oldest_incorporated_frame,
+                uint32_t remaining_frames);
+
+  // Source identifier of the BeginFrame that is acknowledged.
+  uint64_t source_id;
+
+  // Sequence number of the BeginFrame that is acknowledged.
+  uint64_t sequence_number;
+
+  // |true| if the consumer has produced updates (e.g. sent a CompositorFrame or
+  // damaged a surface) as part of responding to the BeginFrame.
+  bool has_updates;

[Sami, 2016/12/06 12:41:06]

|oldest_incorporated_frame| is only valid if this is true, right?

[Eric Seckler, 2016/12/06 17:33:59]

No :) I updated the comment below with a note, but here a longer explanation:

|oldest_incorporated_frame| remains valid even if there were no updates. There
are two situations where |has_updates| is false:
a) The observer didn't need to produce any updates.
b) The observer has pending updates but didn't manage to produce them (e.g. took
to long).

For a), |oldest_incorporated_frame| is incremented to the current frame (since
the current frame contributed its updates -- there just weren't any).

For b), |oldest_incorporated_frame| remains at the latest frame that
successfully contributed its updates.

If |has_updates| is true, this just means that a surface was indeed damaged / a
CompositorFrame was sent (rather than the frame just "acked").

[Sami, 2016/12/07 16:59:40]

Sorry, I think the explanation is still a bit confusing. The text below says
"Sequence number of the oldest frame that contributed contents to the last
update the consumer sent", but does an ack with has_updates=false count as an
update or not? :) Maybe we need a different word for ack vs. damage?

I agree that distinguishing (a) and (b) feels important. Let's just make it
crystal clear how to do that :)

[Eric Seckler, 2016/12/08 17:54:28]

Renamed and updated the comment again after discussion with Sami: We're now
calling this the |latest_confirmed_frame|, which is the last BeginFrame for
which all pending updates successfully led to damages - or where no pending
updates existed (and thus, no damages needed to be produced). We also replaced
|has_updates| by |has_damages| to distinguish better between pending updates and
resulting damages.

+
+  // Sequence number of the oldest frame that contributed contents to the last
+  // update the consumer sent. It describes the "staleness" of the update.
+  //
+  // The compositor Scheduler indicates the latest frame that both an impl and
+  // main frame contributed to (or didn't have any updates). The
+  // DisplayScheduler indicates the minimum oldest_incorporated_frame that all
+  // its BeginFrameObservers acknowledged.
+  uint64_t oldest_incorporated_frame;

[Sami, 2016/12/06 12:41:06]

Do we need to define an invalid/unknown value for this -- say if the BF source
changes and it's no longer meaningful to compare sequence numbers?

[Eric Seckler, 2016/12/06 17:33:59]

My current approach to this is to consider the consumer/observer up to date up
to the last frame of the new source. That way, the observer can indicate that it
is not up to date, but also doesn't propagate an invalid value. I think if we
start sequence numbering at 1, this would naturally work for the first source as
well (|oldest_incorporated_frame| would be initialized to frame 0).

[Sami, 2016/12/07 16:59:40]

Makes sense. If we decide to do that, let's add a named constant for 0 too.

[Eric Seckler, 2016/12/08 17:54:28]

Done, also for 1 as starting frame number.

+
+  // Number of BeginFrames queued at the observer at time of acknowledgment.
+  uint32_t remaining_frames;

[Sami, 2016/12/06 12:41:06]

nit: might want to order these fields from largest to smallest to help with
packing a bit.

[Eric Seckler, 2016/12/06 17:33:59]

Done.

+};
+
 }  // namespace cc
 
 #endif  // CC_OUTPUT_BEGIN_FRAME_ARGS_H_