Surface Synchronization: Distinguish between dependencies and references

Surface Synchronization: Distinguish between dependencies and references

This CL simplifies the relationship between surface synchronization and surface
references.

With this CL, surface references represent the DAG of surfaces known to the display
compositor. In the context of surface synchronization, the reference DAG consists
of the set of active CompositorFrames' fallback surface IDs.

CompositorFrameMetadata's |embedded_surfaces| field represents surface 
*dependencies*. A surface dependency is a surface ID that a given CompositorFrame
will block on until it is available or a deadline hits. With this CL, surface dependencies
and surface references are tracked differently. |embedded_surfaces| represent
dependencies whereas |referenced_surfaces| represent surface references.

With surface synchronization, a side channel is assumed to exist (e.g. browser => 
renderer) that allocates a LocalSurfaceId for the child and passes that LocalSurfaceId
along to the child. If a child submits a CompositorFrame to the display compositor,
a temporary (root) reference, that is then passed along to the window server / 
browser and then to the parent. The parent can then claim ownership of that 
reference by embedding it as a fallback surface.

In other words, surface synchronization will now use the existing surface reference
control flow. It's just the time it takes for a temporary reference to be embedded by
a parent might take a little longer due to surface synchronization.

Tests have been updated to take into account this change.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2811813004
Cr-Commit-Position: refs/heads/master@{#463863}
Committed: https://chromium.googlesource.com/chromium/src/+/23724703cfbba977e4628436698c59bdba5c2b05

[Fady Samuel, 2017-04-11 02:52:38]

Description was changed from

==========
WIP Surface Synchronization: Distinguish between dependencies and references

BUG=
==========

to

==========
WIP Surface Synchronization: Distinguish between dependencies and references

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-04-11 02:53:19]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 02:54:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-04-11 03:23:29]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 03:23:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-04-11 03:42:45]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 03:43:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-04-11 03:59:08]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 03:59:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 04:30:14]

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

[commit-bot: I haz the power, 2017-04-11 04:30:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2017-04-11 05:03:56]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 05:04:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-04-11 05:14:59]

Description was changed from

==========
WIP Surface Synchronization: Distinguish between dependencies and references

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Surface Synchronization: Distinguish between dependencies and references

This CL simplifies the relationship between surface synchronization and surface
references.

With this CL, surface references represent the DAG of surfaces known to the
display
compositor. In the context of surface synchronization, the reference DAG
consists
of the set of active CompositorFrames' fallback surface IDs.

CompositorFrameMetadata's |embedded_clients| field represents surface 
*dependencies*. A surface dependency is a surface ID that a given
CompositorFrame
will block on until it is available or a deadline hits. With this CL, surface
dependencies
and surface references are tracked differently. |embedded_clients| represent
dependencies whereas |referenced_surfaces| represent surface references.

With surface synchronziation, a side channel is assumed to exist (e.g. browser
=> renderer) that allocates a LocalSurfaceId for the child and passes that
LocalSurfaceId
along to the child. If a child submits a CompositorFrame to the display
compositor,
a temporary (root) reference, that is then passed along to the window server /
browser and then to the parent. The parent can then claim ownership of that
reference by
embedding it as a fallback surface.

In other words, surface synchronization will now use the existing surface
reference
control flow. It's just the time it takes for a temporary reference to be
embedded by
a parent might take a little longer due to surface synchronization.

Tests have been updated to take into account this change.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-04-11 05:15:18]

Description was changed from

==========
Surface Synchronization: Distinguish between dependencies and references

This CL simplifies the relationship between surface synchronization and surface
references.

With this CL, surface references represent the DAG of surfaces known to the
display
compositor. In the context of surface synchronization, the reference DAG
consists
of the set of active CompositorFrames' fallback surface IDs.

CompositorFrameMetadata's |embedded_clients| field represents surface 
*dependencies*. A surface dependency is a surface ID that a given
CompositorFrame
will block on until it is available or a deadline hits. With this CL, surface
dependencies
and surface references are tracked differently. |embedded_clients| represent
dependencies whereas |referenced_surfaces| represent surface references.

With surface synchronziation, a side channel is assumed to exist (e.g. browser
=> renderer) that allocates a LocalSurfaceId for the child and passes that
LocalSurfaceId
along to the child. If a child submits a CompositorFrame to the display
compositor,
a temporary (root) reference, that is then passed along to the window server /
browser and then to the parent. The parent can then claim ownership of that
reference by
embedding it as a fallback surface.

In other words, surface synchronization will now use the existing surface
reference
control flow. It's just the time it takes for a temporary reference to be
embedded by
a parent might take a little longer due to surface synchronization.

Tests have been updated to take into account this change.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Surface Synchronization: Distinguish between dependencies and references

This CL simplifies the relationship between surface synchronization and surface
references.

With this CL, surface references represent the DAG of surfaces known to the
display
compositor. In the context of surface synchronization, the reference DAG
consists
of the set of active CompositorFrames' fallback surface IDs.

CompositorFrameMetadata's |embedded_clients| field represents surface 
*dependencies*. A surface dependency is a surface ID that a given
CompositorFrame
will block on until it is available or a deadline hits. With this CL, surface
dependencies
and surface references are tracked differently. |embedded_clients| represent
dependencies whereas |referenced_surfaces| represent surface references.

With surface synchronziation, a side channel is assumed to exist (e.g. browser
=> 
renderer) that allocates a LocalSurfaceId for the child and passes that
LocalSurfaceId
along to the child. If a child submits a CompositorFrame to the display
compositor,
a temporary (root) reference, that is then passed along to the window server / 
browser and then to the parent. The parent can then claim ownership of that 
reference by embedding it as a fallback surface.

In other words, surface synchronization will now use the existing surface
reference
control flow. It's just the time it takes for a temporary reference to be
embedded by
a parent might take a little longer due to surface synchronization.

Tests have been updated to take into account this change.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-04-11 05:15:37]

fsamuel@chromium.org changed reviewers:
+ vmpstr@chromium.org

[Fady Samuel, 2017-04-11 05:15:37]

PTAL Vlad! Thanks!

[commit-bot: I haz the power, 2017-04-11 05:58:09]

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

[commit-bot: I haz the power, 2017-04-11 05:58:10]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-04-11 18:35:18]

Looks good, in your description when you say |embedded_clients|, do you mean
|embedded_surfaces| in the code?

Also, could you add a second reviewer for sanity :P (someone who is more
familiar with this code)

https://codereview.chromium.org/2811813004/diff/100001/cc/layers/surface_laye...
File cc/layers/surface_layer_impl.cc (right):

https://codereview.chromium.org/2811813004/diff/100001/cc/layers/surface_laye...
cc/layers/surface_layer_impl.cc:76: fallback_surface_info_, nullptr);
nit: comment like 

nullptr /* embedded_surfaces */);

https://codereview.chromium.org/2811813004/diff/100001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2811813004/diff/100001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:53: const std::vector<SurfaceId>*
active_referenced_surfaces) override;
nit: rename to referenced_surfaces or do you want to keep the distinction clear?

https://codereview.chromium.org/2811813004/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2811813004/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1627: if
(!settings_.enable_surface_synchronization) {
nit: can you flip this to the positive and exchange if/else blocks

https://codereview.chromium.org/2811813004/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1633:
surface_layer_impl->fallback_surface_info().id());
hmm why do we not reference primary surface info if we have surface
synchronization enabled?

[Fady Samuel, 2017-04-11 19:24:39]

Description was changed from

==========
Surface Synchronization: Distinguish between dependencies and references

This CL simplifies the relationship between surface synchronization and surface
references.

With this CL, surface references represent the DAG of surfaces known to the
display
compositor. In the context of surface synchronization, the reference DAG
consists
of the set of active CompositorFrames' fallback surface IDs.

CompositorFrameMetadata's |embedded_clients| field represents surface 
*dependencies*. A surface dependency is a surface ID that a given
CompositorFrame
will block on until it is available or a deadline hits. With this CL, surface
dependencies
and surface references are tracked differently. |embedded_clients| represent
dependencies whereas |referenced_surfaces| represent surface references.

With surface synchronziation, a side channel is assumed to exist (e.g. browser
=> 
renderer) that allocates a LocalSurfaceId for the child and passes that
LocalSurfaceId
along to the child. If a child submits a CompositorFrame to the display
compositor,
a temporary (root) reference, that is then passed along to the window server / 
browser and then to the parent. The parent can then claim ownership of that 
reference by embedding it as a fallback surface.

In other words, surface synchronization will now use the existing surface
reference
control flow. It's just the time it takes for a temporary reference to be
embedded by
a parent might take a little longer due to surface synchronization.

Tests have been updated to take into account this change.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Surface Synchronization: Distinguish between dependencies and references

This CL simplifies the relationship between surface synchronization and surface
references.

With this CL, surface references represent the DAG of surfaces known to the
display
compositor. In the context of surface synchronization, the reference DAG
consists
of the set of active CompositorFrames' fallback surface IDs.

CompositorFrameMetadata's |embedded_surfaces| field represents surface 
*dependencies*. A surface dependency is a surface ID that a given
CompositorFrame
will block on until it is available or a deadline hits. With this CL, surface
dependencies
and surface references are tracked differently. |embedded_surfaces| represent
dependencies whereas |referenced_surfaces| represent surface references.

With surface synchronization, a side channel is assumed to exist (e.g. browser
=> 
renderer) that allocates a LocalSurfaceId for the child and passes that
LocalSurfaceId
along to the child. If a child submits a CompositorFrame to the display
compositor,
a temporary (root) reference, that is then passed along to the window server / 
browser and then to the parent. The parent can then claim ownership of that 
reference by embedding it as a fallback surface.

In other words, surface synchronization will now use the existing surface
reference
control flow. It's just the time it takes for a temporary reference to be
embedded by
a parent might take a little longer due to surface synchronization.

Tests have been updated to take into account this change.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-04-11 19:39:23]

fsamuel@chromium.org changed reviewers:
+ piman@chromium.org

[Fady Samuel, 2017-04-11 19:39:24]

Yea, I meant embedded_surfaces not embedded_clients. I've updated the
description. Adding piman@...Antoine, please let me know if you want more
background about this.

https://codereview.chromium.org/2811813004/diff/100001/cc/layers/surface_laye...
File cc/layers/surface_layer_impl.cc (right):

https://codereview.chromium.org/2811813004/diff/100001/cc/layers/surface_laye...
cc/layers/surface_layer_impl.cc:76: fallback_surface_info_, nullptr);
On 2017/04/11 18:35:17, vmpstr wrote:
> nit: comment like 
> 
> nullptr /* embedded_surfaces */);

Done.

https://codereview.chromium.org/2811813004/diff/100001/cc/surfaces/compositor...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2811813004/diff/100001/cc/surfaces/compositor...
cc/surfaces/compositor_frame_sink_support.h:53: const std::vector<SurfaceId>*
active_referenced_surfaces) override;
On 2017/04/11 18:35:17, vmpstr wrote:
> nit: rename to referenced_surfaces or do you want to keep the distinction
clear?

I was thinking about that but I think it's useful to make it clear that these
belong to the active. CompositorFrame.

https://codereview.chromium.org/2811813004/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2811813004/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1627: if
(!settings_.enable_surface_synchronization) {
On 2017/04/11 18:35:17, vmpstr wrote:
> nit: can you flip this to the positive and exchange if/else blocks

Done.

https://codereview.chromium.org/2811813004/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1633:
surface_layer_impl->fallback_surface_info().id());
On 2017/04/11 18:35:17, vmpstr wrote:
> hmm why do we not reference primary surface info if we have surface
> synchronization enabled?

As the description says, fallbacks are references and primaries are
dependencies. Dependencies eventually become references when the CompositorFrame
activates AND the parent refers to the child in a fallback.

[Fady Samuel, 2017-04-11 19:53:39]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 19:54:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 20:55:18]

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

[commit-bot: I haz the power, 2017-04-11 20:55:19]

Dry run: This issue passed the CQ dry run.

[piman, 2017-04-12 00:09:34]

https://codereview.chromium.org/2811813004/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2811813004/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1631: }
Should we add a reference to the primary when we don't have a fallback (because
we don't need one, we're embedding an already known surface)?

[Fady Samuel, 2017-04-12 00:19:45]

https://codereview.chromium.org/2811813004/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2811813004/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1631: }
On 2017/04/12 00:09:34, piman wrote:
> Should we add a reference to the primary when we don't have a fallback
(because
> we don't need one, we're embedding an already known surface)?
Currently we have both primary and fallback SurfaceInfos but we'll only emit a
single SurfaceDrawQuad:

https://cs.chromium.org/chromium/src/cc/layers/surface_layer_impl.cc?q=Surfac...

I don't want to add only the primary if a fallback doesn't exist because the
display compositor may not know about that primary yet, and I don't want to add
a reference to a thing that doesn't yet exist.

In Aura-Mus, we generate a big gutter that fills the window on the first frame
if it arrives late. Other OSes like Windows 10, I noticed does something
similar.

[piman, 2017-04-12 00:33:21]

On Tue, Apr 11, 2017 at 5:19 PM, <fsamuel@chromium.org> wrote:

>
> https://codereview.chromium.org/2811813004/diff/120001/cc/
> trees/layer_tree_host_impl.cc
> File cc/trees/layer_tree_host_impl.cc (right):
>
> https://codereview.chromium.org/2811813004/diff/120001/cc/
> trees/layer_tree_host_impl.cc#newcode1631
> cc/trees/layer_tree_host_impl.cc:1631: }
> On 2017/04/12 00:09:34, piman wrote:
> > Should we add a reference to the primary when we don't have a fallback
> (because
> > we don't need one, we're embedding an already known surface)?
> Currently we have both primary and fallback SurfaceInfos but we'll only
> emit a single SurfaceDrawQuad:
>
> https://cs.chromium.org/chromium/src/cc/layers/surface_layer_impl.cc?q=
> SurfaceLayerImpl+package:%5Echromium$&l=73
>
> I don't want to add only the primary if a fallback doesn't exist because
> the display compositor may not know about that primary yet, and I don't
> want to add a reference to a thing that doesn't yet exist.
>

But, how does this work in steady state then (no fallback)? What keeps
references to child surfaces?


>
> In Aura-Mus, we generate a big gutter that fills the window on the first
> frame if it arrives late. Other OSes like Windows 10, I noticed does
> something similar.
>
> https://codereview.chromium.org/2811813004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2017-04-12 00:40:08]

Talked in person, I got it now. LGTM.

[Fady Samuel, 2017-04-12 00:40:46]

On 2017/04/12 00:40:08, piman wrote:
> Talked in person, I got it now. LGTM.

Thanks! CQ'ing!

[Fady Samuel, 2017-04-12 00:40:50]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2017-04-12 00:41:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 00:55:45]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491957650982770,
"parent_rev": "eb3eb6c7fc2ef28b4fbf1633b32afea7f2be27e2", "commit_rev":
"23724703cfbba977e4628436698c59bdba5c2b05"}

[commit-bot: I haz the power, 2017-04-12 00:57:03]

Description was changed from

==========
Surface Synchronization: Distinguish between dependencies and references

This CL simplifies the relationship between surface synchronization and surface
references.

With this CL, surface references represent the DAG of surfaces known to the
display
compositor. In the context of surface synchronization, the reference DAG
consists
of the set of active CompositorFrames' fallback surface IDs.

CompositorFrameMetadata's |embedded_surfaces| field represents surface 
*dependencies*. A surface dependency is a surface ID that a given
CompositorFrame
will block on until it is available or a deadline hits. With this CL, surface
dependencies
and surface references are tracked differently. |embedded_surfaces| represent
dependencies whereas |referenced_surfaces| represent surface references.

With surface synchronization, a side channel is assumed to exist (e.g. browser
=> 
renderer) that allocates a LocalSurfaceId for the child and passes that
LocalSurfaceId
along to the child. If a child submits a CompositorFrame to the display
compositor,
a temporary (root) reference, that is then passed along to the window server / 
browser and then to the parent. The parent can then claim ownership of that 
reference by embedding it as a fallback surface.

In other words, surface synchronization will now use the existing surface
reference
control flow. It's just the time it takes for a temporary reference to be
embedded by
a parent might take a little longer due to surface synchronization.

Tests have been updated to take into account this change.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Surface Synchronization: Distinguish between dependencies and references

This CL simplifies the relationship between surface synchronization and surface
references.

With this CL, surface references represent the DAG of surfaces known to the
display
compositor. In the context of surface synchronization, the reference DAG
consists
of the set of active CompositorFrames' fallback surface IDs.

CompositorFrameMetadata's |embedded_surfaces| field represents surface 
*dependencies*. A surface dependency is a surface ID that a given
CompositorFrame
will block on until it is available or a deadline hits. With this CL, surface
dependencies
and surface references are tracked differently. |embedded_surfaces| represent
dependencies whereas |referenced_surfaces| represent surface references.

With surface synchronization, a side channel is assumed to exist (e.g. browser
=> 
renderer) that allocates a LocalSurfaceId for the child and passes that
LocalSurfaceId
along to the child. If a child submits a CompositorFrame to the display
compositor,
a temporary (root) reference, that is then passed along to the window server / 
browser and then to the parent. The parent can then claim ownership of that 
reference by embedding it as a fallback surface.

In other words, surface synchronization will now use the existing surface
reference
control flow. It's just the time it takes for a temporary reference to be
embedded by
a parent might take a little longer due to surface synchronization.

Tests have been updated to take into account this change.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2811813004
Cr-Commit-Position: refs/heads/master@{#463863}
Committed:
https://chromium.googlesource.com/chromium/src/+/23724703cfbba977e4628436698c...
==========

[commit-bot: I haz the power, 2017-04-12 00:57:04]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/23724703cfbba977e4628436698c...