Aura-Mus: Enable Surface Synchronization behind flag

Aura-Mus: Enable Surface Synchronization behind flag

This CL turns on surface synchronization in Aura-Mus behind a flag.
Iff surface synchronization is enabled, then we can embed a child client
if both the FrameSinkId and LocalSurfaceId are known to the parent.

The FrameSinkId is allocated by the window server and so the parent
is informed of it asynchronously.

Once both pieces of data are known, the parent manufactures a SurfaceInfo
with the desired size, and device scale factor and embeds the child.

The display compositor will block the parent's frame UNTIL the child
has submitted a CompositorFrame or a deadline has passed.

There are several known issues to be fixed in subsequent CLs:

1. Guttering should be done in the display compositor if the background
   color of the CompositorFrame is opaque.
2. Auto-resizing where the child asks the parent for a size (Blink in
   particular) does not yet work correctly.
3. Resize is currently propagated from the browser ot the renderer but
   the LocalSurfaceId is coming from the window server. This results
   in a race where the renderer may use the wrong SurfaceId to submit
   a frame. This could result in more guttering than necessary.
4. We do not throttle input events or BeginFrames (AFAICT) on clients that
   have blocked CompostiorFrames. This will further improve performance.
5. We do not yet scope deadlines to a single surface subtree.

BUG=672962
Review-Url: https://codereview.chromium.org/2790673003
Cr-Commit-Position: refs/heads/master@{#461170}
Committed: https://chromium.googlesource.com/chromium/src/+/5eda1f753751a5c38367cd4f3db8fbf38c180d40

[Fady Samuel, 2017-03-30 18:40:30]

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

[Fady Samuel, 2017-03-30 18:40:52]

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

[Fady Samuel, 2017-03-30 18:40:53]

PTAL Sadrul. Working on unit test(s) in parallel.

[commit-bot: I haz the power, 2017-03-30 18:40:59]

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-03-30 19:12:24]

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

[commit-bot: I haz the power, 2017-03-30 19:12:35]

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-03-30 19:48:53]

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

[Fady Samuel, 2017-03-30 19:49:02]

PTAL Sadrul! I've added a unit test.

[commit-bot: I haz the power, 2017-03-30 19:49:59]

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-03-30 21:06:22]

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

[commit-bot: I haz the power, 2017-03-30 21:06:23]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-03-31 01:26:20]

Looks good. I have a question about what happens to the fallback surface with
the visibility change of a window.

For ui/compositor, can you add an owner from there?

https://codereview.chromium.org/2790673003/diff/40001/ui/aura/mus/client_surf...
File ui/aura/mus/client_surface_embedder.h (right):

https://codereview.chromium.org/2790673003/diff/40001/ui/aura/mus/client_surf...
ui/aura/mus/client_surface_embedder.h:33: void SetFallbackSurfaceInfo(const
cc::SurfaceInfo& surface_info);
Does the clip-layer apply here? Are there restrictions on the fallback surface?
(e.g. does its size have to match the size of the primary surface, and so on)

https://codereview.chromium.org/2790673003/diff/40001/ui/aura/mus/window_port...
File ui/aura/mus/window_port_mus.cc (right):

https://codereview.chromium.org/2790673003/diff/40001/ui/aura/mus/window_port...
ui/aura/mus/window_port_mus.cc:331: }
Can we have a method that does this? e.g.

  void UpdatePrimarySurface() {
    bool embeds_surface =
        window_mus_type() == WindowMusType::TOP_LEVEL_IN_WM ||
        window_mus_type() == WindowMusType::EMBED_IN_OWNER;
    if (!embeds_surface)
      return;
    if (!local_surface_id_.is_valid() || !frame_sink_id_.is_valid())
      return;
    ...
  }

This makes it clear when reading the code that (1) the reason for ignoring the
other window types is that they don't actually embed surface (as opposed to,
say, they embed surface in a different manner, somewhere else), and (2) avoids
some code dup (... although like 3 lines, but I think that still makes it easier
to read)

https://codereview.chromium.org/2790673003/diff/40001/ui/aura/mus/window_port...
ui/aura/mus/window_port_mus.cc:344:
client_surface_embedder_->SetFallbackSurfaceInfo(surface_info);
What happens if the window is hidden + shown after this fallback surface is set?

https://codereview.chromium.org/2790673003/diff/40001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2790673003/diff/40001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:686: // insert gutter layers.
Maybe just comment that we need to fix for guttering. Exactly how we gutter may
be something different?

[Fady Samuel, 2017-03-31 01:57:42]

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

[Fady Samuel, 2017-03-31 01:57:43]

PTAL sadrul@
+piman@ for ui/compositor

https://codereview.chromium.org/2790673003/diff/40001/ui/aura/mus/client_surf...
File ui/aura/mus/client_surface_embedder.h (right):

https://codereview.chromium.org/2790673003/diff/40001/ui/aura/mus/client_surf...
ui/aura/mus/client_surface_embedder.h:33: void SetFallbackSurfaceInfo(const
cc::SurfaceInfo& surface_info);
On 2017/03/31 01:26:20, sadrul wrote:
> Does the clip-layer apply here? Are there restrictions on the fallback
surface?
> (e.g. does its size have to match the size of the primary surface, and so on)

The clip layer is explicitly NOT updated. I've explained as much in a comment.

https://codereview.chromium.org/2790673003/diff/40001/ui/aura/mus/window_port...
File ui/aura/mus/window_port_mus.cc (right):

https://codereview.chromium.org/2790673003/diff/40001/ui/aura/mus/window_port...
ui/aura/mus/window_port_mus.cc:331: }
On 2017/03/31 01:26:20, sadrul wrote:
> Can we have a method that does this? e.g.
> 
>   void UpdatePrimarySurface() {
>     bool embeds_surface =
>         window_mus_type() == WindowMusType::TOP_LEVEL_IN_WM ||
>         window_mus_type() == WindowMusType::EMBED_IN_OWNER;
>     if (!embeds_surface)
>       return;
>     if (!local_surface_id_.is_valid() || !frame_sink_id_.is_valid())
>       return;
>     ...
>   }
> 
> This makes it clear when reading the code that (1) the reason for ignoring the
> other window types is that they don't actually embed surface (as opposed to,
> say, they embed surface in a different manner, somewhere else), and (2) avoids
> some code dup (... although like 3 lines, but I think that still makes it
easier
> to read)

Done.

https://codereview.chromium.org/2790673003/diff/40001/ui/aura/mus/window_port...
ui/aura/mus/window_port_mus.cc:344:
client_surface_embedder_->SetFallbackSurfaceInfo(surface_info);
On 2017/03/31 01:26:20, sadrul wrote:
> What happens if the window is hidden + shown after this fallback surface is
set?

The fallback surface is lost (it is no longer referenced by the embedder and may
have been garbage collected since. If the primary surface is available, we will
use it, if not the parent will block on visibility change.

https://codereview.chromium.org/2790673003/diff/40001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2790673003/diff/40001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:686: // insert gutter layers.
On 2017/03/31 01:26:20, sadrul wrote:
> Maybe just comment that we need to fix for guttering. Exactly how we gutter
may
> be something different?

I've gotten rid of the second sentence. We should compute the gutter in the
display compositor is something we agreed on.

[Fady Samuel, 2017-03-31 02:05:11]

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

[commit-bot: I haz the power, 2017-03-31 02:06:02]

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-03-31 03:11:41]

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

[commit-bot: I haz the power, 2017-03-31 03:11:42]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-03-31 05:23:00]

I left a couple of questions to clarify when/where the primary-surface is set
when surface-sync is turned on.

aura parts lgtm

https://codereview.chromium.org/2790673003/diff/60001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2790673003/diff/60001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1307:
window->SetFallbackSurfaceInfo(surface_info);
Can you comment that the PrimarySurfaceInfo is set in WindowPortMus in this
case?

Shouldn't |surface_info| be set as the primary here too, though? Can you comment
on why not?

https://codereview.chromium.org/2790673003/diff/60001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2790673003/diff/60001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:680: scoped_refptr<cc::SurfaceReferenceFactory>
ref_factory) {
It doesn't look like |ref_factory| is needed?

[Fady Samuel, 2017-03-31 12:49:27]

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

[commit-bot: I haz the power, 2017-03-31 12:49:42]

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-03-31 12:51:27]

Thanks Sadrul! PTAL Antoine! This is trivial plumbing in ui::Layer at this point
:-)

As mentioned in the TODO though, guttering is not right yet. That will be fixed
in a subsequent patch. This is also why this code is behind a flag :-)

My plan is to turn this on by default when --mash is turned on once guttering is
fixed.

https://codereview.chromium.org/2790673003/diff/60001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2790673003/diff/60001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1307:
window->SetFallbackSurfaceInfo(surface_info);
On 2017/03/31 05:23:00, sadrul wrote:
> Can you comment that the PrimarySurfaceInfo is set in WindowPortMus in this
> case?
> 
> Shouldn't |surface_info| be set as the primary here too, though? Can you
comment
> on why not?

The parent allocates the LocalSurfaceId for the child and it manufactures the
primary SurfaceInfo. I've updated the comment to mention this. Thanks!

https://codereview.chromium.org/2790673003/diff/60001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2790673003/diff/60001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:680: scoped_refptr<cc::SurfaceReferenceFactory>
ref_factory) {
On 2017/03/31 05:23:00, sadrul wrote:
> It doesn't look like |ref_factory| is needed?

Heh, you're right. It's a remnant of an old version of this function and I
missed removing it. Done.

[commit-bot: I haz the power, 2017-03-31 14:19:16]

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

[commit-bot: I haz the power, 2017-03-31 14:19:17]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-03-31 17:43:49]

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

[Fady Samuel, 2017-03-31 17:43:50]

I just realized Antoine is OOO today. Adding danakj@ PTAL

[danakj, 2017-03-31 17:55:38]

LGTM

[Fady Samuel, 2017-03-31 17:59:51]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-03-31 17:59:51]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2790673003/#ps80001
(title: "Addressed Sadrul's comments")

[commit-bot: I haz the power, 2017-03-31 18:00:43]

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-03-31 18:06:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490983191483840,
"parent_rev": "81f7c216e1ebbcda921597130789d349dae9f8bf", "commit_rev":
"5eda1f753751a5c38367cd4f3db8fbf38c180d40"}

[commit-bot: I haz the power, 2017-03-31 18:07:20]

Description was changed from

==========
Aura-Mus: Enable Surface Synchronization behind flag

This CL turns on surface synchronization in Aura-Mus behind a flag.
Iff surface synchronization is enabled, then we can embed a child client
if both the FrameSinkId and LocalSurfaceId are known to the parent.

The FrameSinkId is allocated by the window server and so the parent
is informed of it asynchronously.

Once both pieces of data are known, the parent manufactures a SurfaceInfo
with the desired size, and device scale factor and embeds the child.

The display compositor will block the parent's frame UNTIL the child
has submitted a CompositorFrame or a deadline has passed.

There are several known issues to be fixed in subsequent CLs:

1. Guttering should be done in the display compositor if the background
   color of the CompositorFrame is opaque.
2. Auto-resizing where the child asks the parent for a size (Blink in
   particular) does not yet work correctly.
3. Resize is currently propagated from the browser ot the renderer but
   the LocalSurfaceId is coming from the window server. This results
   in a race where the renderer may use the wrong SurfaceId to submit
   a frame. This could result in more guttering than necessary.
4. We do not throttle input events or BeginFrames (AFAICT) on clients that
   have blocked CompostiorFrames. This will further improve performance.
5. We do not yet scope deadlines to a single surface subtree.

BUG=672962
==========

to

==========
Aura-Mus: Enable Surface Synchronization behind flag

This CL turns on surface synchronization in Aura-Mus behind a flag.
Iff surface synchronization is enabled, then we can embed a child client
if both the FrameSinkId and LocalSurfaceId are known to the parent.

The FrameSinkId is allocated by the window server and so the parent
is informed of it asynchronously.

Once both pieces of data are known, the parent manufactures a SurfaceInfo
with the desired size, and device scale factor and embeds the child.

The display compositor will block the parent's frame UNTIL the child
has submitted a CompositorFrame or a deadline has passed.

There are several known issues to be fixed in subsequent CLs:

1. Guttering should be done in the display compositor if the background
   color of the CompositorFrame is opaque.
2. Auto-resizing where the child asks the parent for a size (Blink in
   particular) does not yet work correctly.
3. Resize is currently propagated from the browser ot the renderer but
   the LocalSurfaceId is coming from the window server. This results
   in a race where the renderer may use the wrong SurfaceId to submit
   a frame. This could result in more guttering than necessary.
4. We do not throttle input events or BeginFrames (AFAICT) on clients that
   have blocked CompostiorFrames. This will further improve performance.
5. We do not yet scope deadlines to a single surface subtree.

BUG=672962

Review-Url: https://codereview.chromium.org/2790673003
Cr-Commit-Position: refs/heads/master@{#461170}
Committed:
https://chromium.googlesource.com/chromium/src/+/5eda1f753751a5c38367cd4f3db8...
==========

[commit-bot: I haz the power, 2017-03-31 18:07:22]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5eda1f753751a5c38367cd4f3db8...