[cc] Acknowledge BeginFrames in SurfaceDependencyTracker.

[cc] Acknowledge BeginFrames in SurfaceDependencyTracker.

SurfaceDependencyTracker now observes the BeginFrameSource for two
purposes: First, to enforce a deadline for forced pending frame
activation. Second, to acknowledge BeginFrames on behalf of the pending
CompositorFrames:

While a CompositorFrame is pending, the tracker prevents the
confirmation of the CompositorFrame's updates to the BeginFrameSource.
The tracker accomplishes this by observing, but not confirming
BeginFrames while the frame is pending.

It also does not acknowledge BeginFrames while a CompositorFrame is
pending to indicate to the BeginFrameSource that it is still waiting for
blocking surfaces to be resolved.

DO_NOT_SUBMIT.
superseded by https://codereview.chromium.org/2854163003/.

BUG=703079, 697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[Eric Seckler, 2017-04-03 14:59:14]

Description was changed from

==========
[cc] Acknowledge BeginFrames in SurfaceDependencyTracker.

SurfaceDependencyTracker now observes the BeginFrameSource for two
purposes: First, to enforce a deadline for forced pending frame
activation. Second, to acknowledge BeginFrames on behalf of the pending
CompositorFrames:

While a CompositorFrame is pending, the tracker prevents the
confirmation of the CompositorFrame's updates to the BeginFrameSource.
The tracker accomplishes this by observing, but not confirming
BeginFrames while the frame is pending.

It also does not acknowledge BeginFrames while a CompositorFrame is
pending to indicate to the BeginFrameSource that it is still waiting for
blocking surfaces to be resolved.

BUG=703079, 697086, 646774
==========

to

==========
[cc] Acknowledge BeginFrames in SurfaceDependencyTracker.

SurfaceDependencyTracker now observes the BeginFrameSource for two
purposes: First, to enforce a deadline for forced pending frame
activation. Second, to acknowledge BeginFrames on behalf of the pending
CompositorFrames:

While a CompositorFrame is pending, the tracker prevents the
confirmation of the CompositorFrame's updates to the BeginFrameSource.
The tracker accomplishes this by observing, but not confirming
BeginFrames while the frame is pending.

It also does not acknowledge BeginFrames while a CompositorFrame is
pending to indicate to the BeginFrameSource that it is still waiting for
blocking surfaces to be resolved.

BUG=703079, 697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-04-03 15:28:30]

Patchset #1 (id:1) has been deleted

[Eric Seckler, 2017-04-03 15:30:05]

eseckler@chromium.org changed reviewers:
+ brianderson@chromium.org, fsamuel@chromium.org

[Eric Seckler, 2017-04-03 15:30:06]

Hey Fady + Brian,

This is an alternative to https://codereview.chromium.org/2785103003/.

It has the advantage of being rather simple - at least for now. (This works in
conjunction with CompositorFrameSinkSupport simply acknowledging as it does
right now.)

I'm taking a few shortcuts here, though, namely:
(1) Not acknowledging BeginFrames while pending frames exist .
(2) Possibly receiving the same BeginFrame multiple times in
SurfaceDependencyTracker.
(3) Possibly confirming sequence numbers that we shouldn't be confirming.

To explain (3): Say we have a client that has previously confirmed sequence
number 100, then needs three BeginFrames to build a CompositorFrame that is
blocked on surface 12, which includes updates up to and including BeginFrame
103. When it submits this CompositorFrame to the CFSSupport for surface 11, the
CFSSupport will submit the frame to SurfaceFactory, and afterwards confirm
BeginFrame 103. The SurfaceDependencyTracker will notice the submitted pending
CompositorFrame and start observing BeginFrames. Because it was not observing
BeginFrames before, it will be considered (by BeginFrameObserverAckTracker) to
have confirmed the last BeginFrame, i.e. 102. So we end up confirming 102, even
though the updates for 102 haven't yet made it to the display (since they're in
the pending CompositorFrame).

The only way to solve this, I believe, is by tracking the BeginFrameAcks sent by
the CFSSupports in SurfaceDependencyTracker, to extract the correct
latest_confirmed_sequence_number that we should be confirming for each
BeginFrame (which probably won't be very beautiful).

But if we did that, we could also try to integrate what I proposed on
crbug.com/707513#c2 : SurfaceDependencyTracker could then acknowledge (but not
confirm) a BeginFrame when all CFSSupports have acknowledged the BeginFrame.

Sorry for the rambling, and thank you! :)

[Fady Samuel, 2017-04-03 19:03:02]

This is certainly much simpler, and while it might not be ideal in the short
term, I can live it with it for a while, I suppose.

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
File cc/surfaces/surface_dependency_tracker.cc (right):

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:121: static BeginFrameArgs args;
This seems kinda icky... What's wrong with always observing a BeginFrameSource?

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:129:
DCHECK(pending_surfaces_.empty());
This makes me kind of sad...but maybe it's OK for now... Maybe a TODO to see if
there's a better way?

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:180: UpdateNeedsBeginFrames();  //
|pending_surfaces_| changed.
nit: put this comment above.

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:194: UpdateNeedsBeginFrames();  //
|pending_surfaces_| changed.
nit: put this comment above.

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:195: 
nit: remove.

[brianderson, 2017-04-04 00:29:19]

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
File cc/surfaces/surface_dependency_tracker.cc (right):

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:120: // acknowledge it again on behalf
of any newly added pending surfaces.
I'm not familiar with SurfaceDependencyTracker. When could you get a newly added
pending surface after having already acked a BeginFrame. Seems weird to double
ack.

[Eric Seckler, 2017-04-04 08:36:45]

Thanks guys! I'll add some CFSS tests for this after playing around with the
CFSS tracking alternative mentioned below.

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
File cc/surfaces/surface_dependency_tracker.cc (right):

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:120: // acknowledge it again on behalf
of any newly added pending surfaces.
On 2017/04/04 00:29:19, brianderson wrote:
> I'm not familiar with SurfaceDependencyTracker. When could you get a newly
added
> pending surface after having already acked a BeginFrame. Seems weird to double
> ack.

I was erring on the side of caution, i.e. not assuming anything about when
during a BeginFrame a Surface can become pending / unblocked.

Consider this example:
 (1) Surface A becomes blocked on B. We start observing.
 (2) Surface B submits, unblocks A. We ack BeginFrame and stop observing.
 (3) Surface C becomes blocked on D. We start observing.
 (4) Surface D submits, unblocks C. We ack BeginFrame and stop observing.

If (1)-(3) happen within the same BeginFrame, we need to make sure that we
receive the same BeginFrame in (3), which we then have to re-ack
(BeginFrameObserverAckTracker supports that).

If we indicate (via LastUsedBeginFrameArgs) that we already used (and thus
received + acked) this BeginFrame, we would be indicating to the
BeginFrameSource (and thus, to DisplayScheduler) that the updates in the
CompositorFrame for Surface C have already propagated.

What I'm not sure about is whether it is currently possible for (1)-
(3) to happen during one BeginFrame - Fady? If it isn't, maybe we can somehow
DCHECK that we don't start observing twice during the same BeginFrame... Yet,
I'm not sure if we even what to restrict ourselves to that.

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:121: static BeginFrameArgs args;
On 2017/04/03 19:03:02, Fady Samuel wrote:
> This seems kinda icky... What's wrong with always observing a
BeginFrameSource?

If we always observe, we have the problem that we don't quite know when to ack a
BeginFrame if we don't have any pending surfaces.

Consider this example (3a and 3b being alternatives):
 (1) no pending surfaces.
 (2) BeginFrame.
 (3a) CFSS for surface A submits a non-blocking frame. All other CFSS report no
damage.
 (3b) CFSS for surface A submits a frame that is blocked on B. All other CFSS
report no damage.

For (3a), SurfaceDependencyTracker should have acked in (2) immediately. For
(3b), SurfaceDependencyTracker shouldn't ack at all b/c a pending frame exist.

But at (2), it doesn't know whether (3a) or (3b) will happen, so can't decide
whether to ack or wait for some surface/frame to become pending.

By starting to observe only when a surface becomes pending, we still receive the
BeginFrame from (2) as missed BeginFrame during (3b). That is, we receive it
only once we know that we shouldn't ack it immediately.

https://codereview.chromium.org/2789163002/diff/20001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:129:
DCHECK(pending_surfaces_.empty());
On 2017/04/03 19:03:02, Fady Samuel wrote:
> This makes me kind of sad...but maybe it's OK for now... Maybe a TODO to see
if
> there's a better way?

Yeah. Added one. FWIW, I'm going to try out tracking the CFSS acks in
SurfaceDependencyTracker. If only to confirm that it would be ugly... :P

[Eric Seckler, 2017-05-26 11:11:11]

Description was changed from

==========
[cc] Acknowledge BeginFrames in SurfaceDependencyTracker.

SurfaceDependencyTracker now observes the BeginFrameSource for two
purposes: First, to enforce a deadline for forced pending frame
activation. Second, to acknowledge BeginFrames on behalf of the pending
CompositorFrames:

While a CompositorFrame is pending, the tracker prevents the
confirmation of the CompositorFrame's updates to the BeginFrameSource.
The tracker accomplishes this by observing, but not confirming
BeginFrames while the frame is pending.

It also does not acknowledge BeginFrames while a CompositorFrame is
pending to indicate to the BeginFrameSource that it is still waiting for
blocking surfaces to be resolved.

BUG=703079, 697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Acknowledge BeginFrames in SurfaceDependencyTracker.

SurfaceDependencyTracker now observes the BeginFrameSource for two
purposes: First, to enforce a deadline for forced pending frame
activation. Second, to acknowledge BeginFrames on behalf of the pending
CompositorFrames:

While a CompositorFrame is pending, the tracker prevents the
confirmation of the CompositorFrame's updates to the BeginFrameSource.
The tracker accomplishes this by observing, but not confirming
BeginFrames while the frame is pending.

It also does not acknowledge BeginFrames while a CompositorFrame is
pending to indicate to the BeginFrameSource that it is still waiting for
blocking surfaces to be resolved.

DO_NOT_SUBMIT.
superseded by https://codereview.chromium.org/2854163003/.

BUG=703079, 697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========