Queue latency info swap promises in a separate already-active queue.

Support swap promises that are pinned to a particular layer tree.

A pinned swap promise queued on a pending tree will only see
DidActivate() or DidNotSwap(ACTIVATION_FAILS). It is not passed
to the active tree, so it will not swap.

A pinned swap promise on the active tree will either see only
DidSwap() or DidNotSwap(SWAP_FAILS). No DidActivate() will be
seen because that has already happened prior to queueing of the
swap promise.

Conversely, an unpinned swap promise (queued by
QueueSwapPromise()) travels with the layer information currently
associated with the tree. For example, when a pending tree is
activated, the swap promise is passed to the active tree along
with the layer information. Similarly, when a new activation
overwrites layer information on the active tree, queued swap
promises are broken.

This allows us to break unpinned swap promises on the active tree
at the point at which we activate the pending tree without
changing latency measurement.

BUG=498824
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2e02f73e17f4fc3f611a08582b7944bb32362a2f
Cr-Commit-Position: refs/heads/master@{#346894}

[Tobias Sargeant, 2015-08-14 17:33:21]

tobiasjs@chromium.org changed reviewers:
+ skyostil@chromium.org

[Tobias Sargeant, 2015-08-14 17:33:22]

Ptal. The bug didn't generate any discussion, so I've just gone with the simple
approach, which should work to allow us to break swap promises early without
hurting latency measurements.

[danakj, 2015-08-14 17:47:38]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-08-14 17:47:39]

are there some unit tests you can add for this?

https://codereview.chromium.org/1292773003/diff/20001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1292773003/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1124: BreakSwapPromises(SwapPromise::SWAP_FAILS);
add a comment explaining what's going on? doesn't this break active tree ones
too which was your goal to avoid?

https://codereview.chromium.org/1292773003/diff/20001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1292773003/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:289: void
QueueActiveTreeSwapPromise(scoped_ptr<SwapPromise> swap_promise);
Explain this in a comment?

[Sami, 2015-08-18 11:19:18]

Seems reasonable to me. Dumb question though: do we actually need unpinned swap
promises for anything or could we just make everything pinned? Or do you need
them for the visual state listener?

https://codereview.chromium.org/1292773003/diff/60001/cc/output/swap_promise.h
File cc/output/swap_promise.h (right):

https://codereview.chromium.org/1292773003/diff/60001/cc/output/swap_promise....
cc/output/swap_promise.h:65: static std::string
DidNotSwapReasonToString(DidNotSwapReason reason) {
Is this used anywhere?

nit: this could just be a const char*

https://codereview.chromium.org/1292773003/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1292773003/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3898: scoped_ptr<SwapPromise>(
nit: make_scoped_ptr

[Tobias Sargeant, 2015-08-18 11:22:15]

https://codereview.chromium.org/1292773003/diff/20001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1292773003/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1124: BreakSwapPromises(SwapPromise::SWAP_FAILS);
On 2015/08/14 17:47:39, danakj wrote:
> add a comment explaining what's going on? doesn't this break active tree ones
> too which was your goal to avoid?

Yes, you're right. Corrected now.

https://codereview.chromium.org/1292773003/diff/20001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1292773003/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:289: void
QueueActiveTreeSwapPromise(scoped_ptr<SwapPromise> swap_promise);
On 2015/08/14 17:47:39, danakj wrote:
> Explain this in a comment?

Done.

[Tobias Sargeant, 2015-08-18 12:35:15]

On 2015/08/18 11:19:18, Sami wrote:
> Seems reasonable to me. Dumb question though: do we actually need unpinned
swap
> promises for anything or could we just make everything pinned? Or do you need
> them for the visual state listener?

Interesting question. VSL now only cares about activations now, and this could
be done with pinned promises (but that would require LTH changes). In the linked
bug there are several other instances (2b-e) that I can't comment upon.
SurfaceLayer::SatisfyDestroySequence() looks like a strong candidate for the
necessity of unpinned swap promises.

[Tobias Sargeant, 2015-08-18 13:39:53]

https://codereview.chromium.org/1292773003/diff/60001/cc/output/swap_promise.h
File cc/output/swap_promise.h (right):

https://codereview.chromium.org/1292773003/diff/60001/cc/output/swap_promise....
cc/output/swap_promise.h:65: static std::string
DidNotSwapReasonToString(DidNotSwapReason reason) {
On 2015/08/18 11:19:18, Sami wrote:
> Is this used anywhere?
> 
> nit: this could just be a const char*

It isn't. It was useful during debugging the unittest, and I figured it was
useful to keep around. Upon reflection, I've removed it.

https://codereview.chromium.org/1292773003/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1292773003/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:3898: scoped_ptr<SwapPromise>(
On 2015/08/18 11:19:18, Sami wrote:
> nit: make_scoped_ptr

Done.

[danakj, 2015-08-18 17:50:06]

danakj@chromium.org changed reviewers:
+ jbauman@chromium.org

[danakj, 2015-08-18 17:50:06]

+jbauman for surfaces

[danakj, 2015-08-18 17:50:29]

On 2015/08/18 17:50:06, danakj wrote:
> +jbauman for surfaces

John please see https://codereview.chromium.org/1292773003/#msg7

[jbauman, 2015-08-19 00:48:46]

On 2015/08/18 17:50:29, danakj wrote:
> On 2015/08/18 17:50:06, danakj wrote:
> > +jbauman for surfaces
> 
> John please see https://codereview.chromium.org/1292773003/#msg7

Is there any way now to get a swap promise that will pass from the pending tree
to the active tree and won't be broken if a new pending tree is activated
(they'll stay on the new active tree)? That's what Surfaces need. Pinned swap
promises seem to be broken during the first activation, while unpinned seem to
be broken during the second activation.

[Tobias Sargeant, 2015-08-19 11:49:03]

On 2015/08/19 00:48:46, jbauman wrote:
> On 2015/08/18 17:50:29, danakj wrote:
> > On 2015/08/18 17:50:06, danakj wrote:
> > > +jbauman for surfaces
> > 
> > John please see https://codereview.chromium.org/1292773003/#msg7
> 
> Is there any way now to get a swap promise that will pass from the pending
tree
> to the active tree and won't be broken if a new pending tree is activated
> (they'll stay on the new active tree)? That's what Surfaces need. Pinned swap
> promises seem to be broken during the first activation, while unpinned seem to
> be broken during the second activation.

Yes, there's no way after this to queue a swap promise on the LTH that will be
guaranteed to survive until the next swap (or swap failure).

Unpinned promises are broken if a second activation overwrites the active tree
before the active tree has a chance to draw and swap. Is that not ok in this
case? I would presume that at that point, the there isn't a destruction
dependency between the Surface and the SurfaceLayer any more (the subsequent
pending tree may have created a new dependency?).

[jbauman, 2015-08-19 22:58:16]

On 2015/08/19 11:49:03, Tobias Sargeant wrote:
> On 2015/08/19 00:48:46, jbauman wrote:
> > On 2015/08/18 17:50:29, danakj wrote:
> > > On 2015/08/18 17:50:06, danakj wrote:
> > > > +jbauman for surfaces
> > > 
> > > John please see https://codereview.chromium.org/1292773003/#msg7
> > 
> > Is there any way now to get a swap promise that will pass from the pending
> tree
> > to the active tree and won't be broken if a new pending tree is activated
> > (they'll stay on the new active tree)? That's what Surfaces need. Pinned
swap
> > promises seem to be broken during the first activation, while unpinned seem
to
> > be broken during the second activation.
> 
> Yes, there's no way after this to queue a swap promise on the LTH that will be
> guaranteed to survive until the next swap (or swap failure).
> 
> Unpinned promises are broken if a second activation overwrites the active tree
> before the active tree has a chance to draw and swap. Is that not ok in this
> case? I would presume that at that point, the there isn't a destruction
> dependency between the Surface and the SurfaceLayer any more (the subsequent
> pending tree may have created a new dependency?).

If a new dependency is created, the SurfaceLayer should create a new
SurfaceSequence that needs to be satisfied, but the previous dependency still
needs to be satisfied to ensure that it'll be destroyed eventually.

I'm currently working on thinking of ways to get rid of this use of swap
promises to track this, so hopefully we'll at least be able to get rid of this
soon.

[Tobias Sargeant, 2015-08-20 14:10:05]

On 2015/08/19 22:58:16, jbauman wrote:
> If a new dependency is created, the SurfaceLayer should create a new
> SurfaceSequence that needs to be satisfied, but the previous dependency still
> needs to be satisfied to ensure that it'll be destroyed eventually.
> 
> I'm currently working on thinking of ways to get rid of this use of swap
> promises to track this, so hopefully we'll at least be able to get rid of this
> soon.

I'm still not sure why this patch will introduce incorrect behaviour. It looks
to me like if SatisfySwapPromise receives DidNotSwap then it removes the
dependency that Surface has on that layer immediately, whereas if it receives
DidSwap, then the dependency is added to the frame metadata, and removed when
the frame is complete.

If we get two activations in a row (without an intervening swap) then the
SurfaceLayers in the active tree that are overwritten by the second activation
won't be drawn. This seems like a good time to remove the dependency, which is
what will happen as a result of the patch.

How would I go about creating a unittest to ensure that in the case of multiple
activations without a swap, that the dependencies are managed properly?

[jbauman, 2015-08-25 00:30:24]

On 2015/08/20 14:10:05, Tobias Sargeant wrote:
> On 2015/08/19 22:58:16, jbauman wrote:
> > If a new dependency is created, the SurfaceLayer should create a new
> > SurfaceSequence that needs to be satisfied, but the previous dependency
still
> > needs to be satisfied to ensure that it'll be destroyed eventually.
> > 
> > I'm currently working on thinking of ways to get rid of this use of swap
> > promises to track this, so hopefully we'll at least be able to get rid of
this
> > soon.
> 
> I'm still not sure why this patch will introduce incorrect behaviour. It looks
> to me like if SatisfySwapPromise receives DidNotSwap then it removes the
> dependency that Surface has on that layer immediately, whereas if it receives
> DidSwap, then the dependency is added to the frame metadata, and removed when
> the frame is complete.
> 
> If we get two activations in a row (without an intervening swap) then the
> SurfaceLayers in the active tree that are overwritten by the second activation
> won't be drawn. This seems like a good time to remove the dependency, which is
> what will happen as a result of the patch.

That does seem reasonable, good point. This patch should work, then.
> 
> How would I go about creating a unittest to ensure that in the case of
multiple
> activations without a swap, that the dependencies are managed properly?

SurfaceLayerSwapPromise tests the overall surface dependencies, but I'm not sure
how to modify it to activate twice in a row without swapping. Maybe subclass the
output surface to only do the swap buffers complete callback on demand (so we
can delay the next swap, and trigger ChangeTree on activate as well in one case.

[Tobias Sargeant, 2015-08-25 15:13:09]

> SurfaceLayerSwapPromise tests the overall surface dependencies, but I'm not
sure
> how to modify it to activate twice in a row without swapping. Maybe subclass
the
> output surface to only do the swap buffers complete callback on demand (so we
> can delay the next swap, and trigger ChangeTree on activate as well in one
case.

I've added a unittest that I think covers this case. I've verified that it
passes both before and after the early breaking introduced by this CL.

John, could you please take a look and tell me what you think? I think its
validity revolves around the Satisfy callback resolving the surface dependency
correctly.

[jbauman, 2015-08-25 21:01:02]

All the surfaces stuff lgtm. Thanks for adding that test.

[danakj, 2015-08-25 21:37:31]

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/latency_info_...
File cc/trees/latency_info_swap_promise_monitor.cc (right):

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/latency_info_...
cc/trees/latency_info_swap_promise_monitor.cc:63: // measurement of the time to
the next SwapBuffers().The swap
space after period

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:3878: PinnedLayerTreeSwapPromise() {}
don't need this

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:3891: .Pass());
you don't need .Pass() here. make_scoped_ptr's return value is an rvalue, it
doesn't have a name yet

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:3895: .Pass());
and here, etc

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:3933: TestSwapPromiseResult
pending_swap_promise_result_,
please define one variable per statement.

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:227: for (auto* swap_promise :
pinned_swap_promise_list_)
Why is this here and not in DidBecomeActive like for the other list?

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:811: 
please don't add whitespace randomly

[Tobias Sargeant, 2015-08-26 11:06:49]

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/latency_info_...
File cc/trees/latency_info_swap_promise_monitor.cc (right):

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/latency_info_...
cc/trees/latency_info_swap_promise_monitor.cc:63: // measurement of the time to
the next SwapBuffers().The swap
On 2015/08/25 21:37:30, danakj wrote:
> space after period

Done.

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:3878: PinnedLayerTreeSwapPromise() {}
On 2015/08/25 21:37:30, danakj wrote:
> don't need this

Done.

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:3891: .Pass());
On 2015/08/25 21:37:30, danakj wrote:
> you don't need .Pass() here. make_scoped_ptr's return value is an rvalue, it
> doesn't have a name yet

Done.

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:3895: .Pass());
On 2015/08/25 21:37:30, danakj wrote:
> and here, etc

Done.

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:3933: TestSwapPromiseResult
pending_swap_promise_result_,
On 2015/08/25 21:37:30, danakj wrote:
> please define one variable per statement.

Done.

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:227: for (auto* swap_promise :
pinned_swap_promise_list_)
On 2015/08/25 21:37:30, danakj wrote:
> Why is this here and not in DidBecomeActive like for the other list?

I've added a comment describing why this is here, but the problem is that
DidBecomeActive is called on the active tree (after unpinned promises are passed
to it), whereas pinned promises are retained by the pending tree and then fail
to swap with ACTIVATION_FAILED when the pending tree is deleted. To get around
that, we need to DidActivate the pinned swap promises and clear the pinned list
before the deletion occurs.

The other alternative would have been a new method on LTI that does the same
thing directly after PushPropertiesTo().

As it stands, I don't think that swap promise callbacks ever expect to be able
to access the pending/active tree, but maybe this should be pushed to as late as
possible in PushPropertiesTo?

[danakj, 2015-08-26 18:16:16]

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1292773003/diff/110001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:227: for (auto* swap_promise :
pinned_swap_promise_list_)
On 2015/08/26 11:06:49, Tobias Sargeant wrote:
> On 2015/08/25 21:37:30, danakj wrote:
> > Why is this here and not in DidBecomeActive like for the other list?
> 
> I've added a comment describing why this is here, but the problem is that
> DidBecomeActive is called on the active tree (after unpinned promises are
passed
> to it), whereas pinned promises are retained by the pending tree and then fail
> to swap with ACTIVATION_FAILED when the pending tree is deleted. To get around
> that, we need to DidActivate the pinned swap promises and clear the pinned
list
> before the deletion occurs.
> 
> The other alternative would have been a new method on LTI that does the same
> thing directly after PushPropertiesTo().
> 
> As it stands, I don't think that swap promise callbacks ever expect to be able
> to access the pending/active tree, but maybe this should be pushed to as late
as
> possible in PushPropertiesTo? 

Is there a use case for pinned swap promises on the pending tree? Maybe we just
DCHECK IsActiveTree() somewhere and call it a day?

[Tobias Sargeant, 2015-08-27 13:07:34]

On 2015/08/26 18:16:16, danakj wrote:

> Is there a use case for pinned swap promises on the pending tree? Maybe we
just
> DCHECK IsActiveTree() somewhere and call it a day?

The visual state listener could at some point be simplified to make use of
pending tree pinned promises (we really only care about activations - in order
to make the promise that what we will draw next is at least as new as frame X).
For now, though, I've removed the relevant code and added a DCHECK to
QueuePinnedSwapPromise. This can be revisited if the need ever arises.

[danakj, 2015-08-27 17:52:54]

Thanks LGTM

https://codereview.chromium.org/1292773003/diff/150001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1292773003/diff/150001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1125: for (auto* swap_promise : swap_promise_list_)
{
nit: don't need {} for 1 line

https://codereview.chromium.org/1292773003/diff/150001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1292773003/diff/150001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.h:302: // Thus, a pinned swap promise on a pending tree
will only see
Update this comment then about the pending tree please

[Tobias Sargeant, 2015-08-28 10:57:58]

The CQ bit was checked by tobiasjs@chromium.org

[Tobias Sargeant, 2015-08-28 10:57:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1292773003/#ps170001
(title: "Update comment, remove extraneous braces.")

[commit-bot: I haz the power, 2015-08-28 10:58:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292773003/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292773003/170001

[Tobias Sargeant, 2015-08-28 10:58:10]

https://codereview.chromium.org/1292773003/diff/150001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1292773003/diff/150001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1125: for (auto* swap_promise : swap_promise_list_)
{
On 2015/08/27 17:52:54, danakj wrote:
> nit: don't need {} for 1 line

Done.

https://codereview.chromium.org/1292773003/diff/150001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1292773003/diff/150001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.h:302: // Thus, a pinned swap promise on a pending tree
will only see
On 2015/08/27 17:52:54, danakj wrote:
> Update this comment then about the pending tree please

Done.

[commit-bot: I haz the power, 2015-08-28 11:33:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-28 11:33:54]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Tobias Sargeant, 2015-08-28 13:50:59]

Could you please take another look at PS11? The NOTREACHED() in
SchedulerStateMachine::DidDrawIfPossibleCompleted if DRAW_ABORTED_CANT_DRAW is a
little inconvenient (sorry I didn't pick it up locally). Using
DRAW_ABORTED_MISSING_HIGH_RES_CONTENT seems hacky, but it was the choice that
seemed to have minimal additional side-effects.

[Tobias Sargeant, 2015-09-02 11:00:00]

The CQ bit was checked by tobiasjs@chromium.org

[Tobias Sargeant, 2015-09-02 11:00:00]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/1292773003/#ps190001
(title: "Use different return value for failed draw in test, to avoid
NOTREACHED()")

[commit-bot: I haz the power, 2015-09-02 11:00:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292773003/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292773003/190001

[commit-bot: I haz the power, 2015-09-02 12:08:09]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2015-09-02 12:08:47]

Patchset 11 (id:??) landed as
https://crrev.com/2e02f73e17f4fc3f611a08582b7944bb32362a2f
Cr-Commit-Position: refs/heads/master@{#346894}