[cc] Add and use BeginFrameAck for DidFinishFrame.

cc/output/begin_frame_args.h

 // Copyright 2013 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.
 
 #ifndef CC_OUTPUT_BEGIN_FRAME_ARGS_H_
 #define CC_OUTPUT_BEGIN_FRAME_ARGS_H_
 
 #include <stdint.h>
 #include <memory>
 
@@ skipping 92 matching lines @@
 
  private:
   BeginFrameArgs(uint32_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,
+                uint64_t latest_confirmed_frame,
+                uint32_t remaining_frames,
+                bool has_damage);
+
+  // Source identifier of the BeginFrame that is acknowledged. The
+  // BeginFrameSource that receives the acknowledgment uses this to discard
+  // BeginFrameAcks for BeginFrames sent by a different source. Such a situation
+  // may occur when the BeginFrameSource of the observer changes while a
+  // BeginFrame from the old source is still in flight.
+  uint64_t source_id;

[brianderson, 2016/12/21 20:15:06]

uint32_t here or uint64_t elsewhere. Don't care which, as long as they are
consistent.

[Eric Seckler, 2016/12/22 11:11:48]

thanks, this should be uint32_t :)

+
+  // Sequence number of the BeginFrame that is acknowledged.
+  uint64_t sequence_number;
+
+  // Sequence number of the latest BeginFrame that was positively acknowledged
+  // (confirmed) by the observer.
+  //
+  // There are two scenarios for a positive acknowledgment:
+  //  a) All of the observer's pending updates led to successful damage (e.g. a
+  //     CompositorFrame or a damaged surface).
+  //  b) The observer did not have any updates and thus did not need to
+  //     produce damage.
+  // A negative acknowledgment, in contrast, describes a situation in which the
+  // observer had pending updates, but was unable to successfully produce
+  // corresponding damage for all its updates in time.
+  //
+  // As a result, |latest_confirmed_frame| describes the "staleness" of the last
+  // damage that was produced by the observer. Note that even if
+  // |has_damage == true|, the damage produced as a result of the acknowledged
+  // BeginFrame may be stale (|latest_confirmed_frame < sequence_number|). In
+  // such a case, the damage that was produced may contain updates from previous
+  // BeginFrames or only part of this BeginFrame's updates.
+  //
+  // Observers aggregate the |latest_confirmed_frame| of their children: The
+  // compositor Scheduler indicates the latest BeginFrame that both impl and
+  // main thread confirmed. Likewise, the DisplayScheduler indicates the minimum
+  // |latest_confirmed_frame| that all its BeginFrameObservers acknowledged.
+  // TODO(eseckler): Set this in Scheduler / DisplayScheduler and other
+  // observers according to above comment.
+  uint64_t latest_confirmed_frame;

[brianderson, 2016/12/21 20:15:06]

If this is a sequence number, would it be clearer if it had "sequence_number" in
the name?

[Eric Seckler, 2016/12/22 11:11:48]

I guess so :) Changed to "latest_confirmed_sequence_number".

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

[brianderson, 2016/12/21 20:15:06]

Is this useful for anything anymore? If not, feel free to remove as part of this
patch or a separate one.

I'm not sure we even use it, currently. A quick code search reveals all existing
hard coded calls to DidFinishFrame have a remaining_frames of 0:
https://cs.chromium.org/search/?q=DidFinishFrame&sq=package:chromium&type=cs

Now that this patch adds an ack, I think the BeginFrameSource will have a better
idea of how many BeginFrames are outstanding anyway, since there could be some
in flight that aren't yet visible to the Observer.

[Eric Seckler, 2016/12/22 11:11:48]

Yeah, I think we can get rid of it. Let's do that separately though, since it'll
require changes to BackToBackBFS and its uses, particularly in tests.

Added a TODO here.

[brianderson, 2017/01/03 19:30:16]

sg.

+
+  // |true| if the observer has produced damage (e.g. sent a CompositorFrame or
+  // damaged a surface) as part of responding to the BeginFrame.
+  bool has_damage;

[brianderson, 2016/12/21 20:15:06]

Even if there isn't damage, the compositor might request a draw for a readback.
A better name for this might be has_update.

[Eric Seckler, 2016/12/22 11:11:48]

Hehe, that was the field's original name :)

I think what we're referring to is that the observer has caused damage visible
to its owner. Thinking of the scenario you describe, wouldn't the draw cause
damage on whatever is drawn on? (Irrespective of whether the draw itself was
caused by damage or not.) But maybe damage is a term specific to stuff happening
before the draw (surfaces)?

Btw, we changed it to "has_damage" from "has_updates" to clarify that this
references the "out-of-band" side effect that the observer may have caused
before sending the ack (rather than the ack itself). Another reason for using
"damage" was picking a term that distinguishes this sort of "resulting update"
from the "pending updates" mentioned in the description of
|latest_confirmed_frame| above.

... would "did_draw" maybe capture this better? or is that too specific in a
different way? :)

[brianderson, 2017/01/03 19:30:16]

I see. Don't want to bikeshed this, has_damage is good. The comment for the
field explains enough.

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