Mus+Ash: Unify CompositorFrameSinks

Mus+Ash: Unify CompositorFrameSinks

This CL moves towards enabling unified BeginFrame in Mus and splitting
the display compositor out of the window server.

1. FrameGenerator now submits CompositorFrames to the root window's
CompositorFrameSink instead of a special "DisplayCompositorFrameSink".
2. ServerWindowCompositorFrameSinks that have a valid AcceleratedWidget
will get a cc::Display (this should become SurfaceHandle when ready).

In an upcoming CL, FrmaeGenerator will no longer need to be aware of
GpuChannelHost. Instead it will simply use the root window's
CompositorFrameSink as is.

BUG=658988
Committed: https://crrev.com/d2f1931ecdde26742abf7cc23f79b38d1dfaf7fb
Cr-Commit-Position: refs/heads/master@{#429286}

[Fady Samuel, 2016-11-01 20:23:42]

Description was changed from

==========
WIP: Mus+Ash: Unify CompositorFrameSinks

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

to

==========
Mus+Ash: Unify CompositorFrameSinks

This CL moves towards enabling unified BeginFrame in Mus and splitting 
the display compositor out of the window server.

1. FrameGenerator now submits CompositorFrames to the root window's
CompositorFrameSink instead of a special "DisplayCompositorFrameSink".
2. ServerWindowCompositorFrameSinks that have a valid AcceleratedWidget
will get a cc::Display (this should become SurfaceHandle when ready).

In an upcoming CL, FrmaeGenerator will no longer need to be aware of
GpuChannelHost. Instead it will simply use the root window's 
CompositorFrameSink as is.

BUG=658988
==========

[Fady Samuel, 2016-11-01 20:23:54]

Description was changed from

==========
Mus+Ash: Unify CompositorFrameSinks

This CL moves towards enabling unified BeginFrame in Mus and splitting 
the display compositor out of the window server.

1. FrameGenerator now submits CompositorFrames to the root window's
CompositorFrameSink instead of a special "DisplayCompositorFrameSink".
2. ServerWindowCompositorFrameSinks that have a valid AcceleratedWidget
will get a cc::Display (this should become SurfaceHandle when ready).

In an upcoming CL, FrmaeGenerator will no longer need to be aware of
GpuChannelHost. Instead it will simply use the root window's 
CompositorFrameSink as is.

BUG=658988
==========

to

==========
Mus+Ash: Unify CompositorFrameSinks

This CL moves towards enabling unified BeginFrame in Mus and splitting 
the display compositor out of the window server.

1. FrameGenerator now submits CompositorFrames to the root window's
CompositorFrameSink instead of a special "DisplayCompositorFrameSink".
2. ServerWindowCompositorFrameSinks that have a valid AcceleratedWidget
will get a cc::Display (this should become SurfaceHandle when ready).

In an upcoming CL, FrmaeGenerator will no longer need to be aware of 
GpuChannelHost. Instead it will simply use the root window's 
CompositorFrameSink as is.

BUG=658988
==========

[Fady Samuel, 2016-11-01 20:24:08]

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

[Fady Samuel, 2016-11-01 20:24:08]

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

[Fady Samuel, 2016-11-01 20:24:36]

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

[commit-bot: I haz the power, 2016-11-01 20:25:14]

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

[Fady Samuel, 2016-11-01 20:25:52]

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

[Fady Samuel, 2016-11-01 20:25:53]

+sky@ once kylechar@ is happy. Thanks!

[commit-bot: I haz the power, 2016-11-01 20:30:33]

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

[commit-bot: I haz the power, 2016-11-01 20:30:34]

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

[Fady Samuel, 2016-11-01 20:43:33]

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

[commit-bot: I haz the power, 2016-11-01 20:43: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, 2016-11-01 22:47:57]

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

[commit-bot: I haz the power, 2016-11-01 22:47:58]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kylechar, 2016-11-02 03:37:24]

https://codereview.chromium.org/2471503002/diff/40001/services/ui/surfaces/di...
File services/ui/surfaces/direct_output_surface.cc (right):

https://codereview.chromium.org/2471503002/diff/40001/services/ui/surfaces/di...
services/ui/surfaces/direct_output_surface.cc:28:
static_cast<SurfacesContextProvider*>(context_provider.get())
Is this cast back to the actual type a temporary thing that will go away with
the final architecture? Add a TODO if so.

https://codereview.chromium.org/2471503002/diff/40001/services/ui/surfaces/di...
File services/ui/surfaces/direct_output_surface_ozone.cc (right):

https://codereview.chromium.org/2471503002/diff/40001/services/ui/surfaces/di...
services/ui/surfaces/direct_output_surface_ozone.cc:55:
static_cast<SurfacesContextProvider*>(context_provider.get())
Ditto.

[Fady Samuel, 2016-11-02 04:31:22]

PTAL Kyle, Scott.

https://codereview.chromium.org/2471503002/diff/40001/services/ui/surfaces/di...
File services/ui/surfaces/direct_output_surface.cc (right):

https://codereview.chromium.org/2471503002/diff/40001/services/ui/surfaces/di...
services/ui/surfaces/direct_output_surface.cc:28:
static_cast<SurfacesContextProvider*>(context_provider.get())
On 2016/11/02 03:37:23, kylechar wrote:
> Is this cast back to the actual type a temporary thing that will go away with
> the final architecture? Add a TODO if so. 

I'm passing around a SurfacesContextProvider directly. Really we shouldn't be
passing this through ServerWindow::CreateCompositorFrameSink but rather should
be an implementation detail of mus-gpu. I've sprinkled comments to that effect.

https://codereview.chromium.org/2471503002/diff/40001/services/ui/surfaces/di...
File services/ui/surfaces/direct_output_surface_ozone.cc (right):

https://codereview.chromium.org/2471503002/diff/40001/services/ui/surfaces/di...
services/ui/surfaces/direct_output_surface_ozone.cc:55:
static_cast<SurfacesContextProvider*>(context_provider.get())
On 2016/11/02 03:37:24, kylechar wrote:
> Ditto.

Done.

[Fady Samuel, 2016-11-02 04:31:32]

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

[commit-bot: I haz the power, 2016-11-02 04:31:40]

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, 2016-11-02 05:39:35]

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

[commit-bot: I haz the power, 2016-11-02 05:39:36]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-11-02 10:06:51]

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

[commit-bot: I haz the power, 2016-11-02 10:07:03]

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, 2016-11-02 10:57:12]

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

[commit-bot: I haz the power, 2016-11-02 10:57:13]

Dry run: This issue passed the CQ dry run.

[kylechar, 2016-11-02 13:22:36]

https://codereview.chromium.org/2471503002/diff/80001/services/ui/ws/server_w...
File services/ui/ws/server_window_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2471503002/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink.cc:39:
task_runner_(base::ThreadTaskRunnerHandle::Get()),
Does task_runner_ need to be a class variable? It's only used in InitDisplay().
(Unless that is expected to be a different thread?)

https://codereview.chromium.org/2471503002/diff/80001/services/ui/ws/server_w...
File services/ui/ws/server_window_compositor_frame_sink.h (right):

https://codereview.chromium.org/2471503002/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink.h:83:
std::unique_ptr<cc::Display> display_;
Can you add a comment for when SWCFS owns a cc::Display vs doesn't own.

[Fady Samuel, 2016-11-02 13:47:23]

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

[commit-bot: I haz the power, 2016-11-02 13:47:50]

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

[Fady Samuel, 2016-11-02 13:48:18]

PTAL Kyle, Scott!

https://codereview.chromium.org/2471503002/diff/80001/services/ui/ws/server_w...
File services/ui/ws/server_window_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2471503002/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink.cc:39:
task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2016/11/02 13:22:36, kylechar wrote:
> Does task_runner_ need to be a class variable? It's only used in
InitDisplay().
> (Unless that is expected to be a different thread?)

I'm just refactoring in this CL, but if I recall correctly, we store it in a
class variable here to be sure that we retain a reference to it for the entire
lifetime of this object. cc::Display holds a raw pointer to it.

https://codereview.chromium.org/2471503002/diff/80001/services/ui/ws/server_w...
File services/ui/ws/server_window_compositor_frame_sink.h (right):

https://codereview.chromium.org/2471503002/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window_compositor_frame_sink.h:83:
std::unique_ptr<cc::Display> display_;
On 2016/11/02 13:22:36, kylechar wrote:
> Can you add a comment for when SWCFS owns a cc::Display vs doesn't own.

Done.

[kylechar, 2016-11-02 14:12:41]

lgtm

[commit-bot: I haz the power, 2016-11-02 14:34:40]

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

[commit-bot: I haz the power, 2016-11-02 14:34:43]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-02 15:42:55]

LGTM

[Fady Samuel, 2016-11-02 15:46:08]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-11-02 15:46:25]

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, 2016-11-02 15:51:21]

Description was changed from

==========
Mus+Ash: Unify CompositorFrameSinks

This CL moves towards enabling unified BeginFrame in Mus and splitting 
the display compositor out of the window server.

1. FrameGenerator now submits CompositorFrames to the root window's
CompositorFrameSink instead of a special "DisplayCompositorFrameSink".
2. ServerWindowCompositorFrameSinks that have a valid AcceleratedWidget
will get a cc::Display (this should become SurfaceHandle when ready).

In an upcoming CL, FrmaeGenerator will no longer need to be aware of 
GpuChannelHost. Instead it will simply use the root window's 
CompositorFrameSink as is.

BUG=658988
==========

to

==========
Mus+Ash: Unify CompositorFrameSinks

This CL moves towards enabling unified BeginFrame in Mus and splitting 
the display compositor out of the window server.

1. FrameGenerator now submits CompositorFrames to the root window's
CompositorFrameSink instead of a special "DisplayCompositorFrameSink".
2. ServerWindowCompositorFrameSinks that have a valid AcceleratedWidget
will get a cc::Display (this should become SurfaceHandle when ready).

In an upcoming CL, FrmaeGenerator will no longer need to be aware of 
GpuChannelHost. Instead it will simply use the root window's 
CompositorFrameSink as is.

BUG=658988
==========

[commit-bot: I haz the power, 2016-11-02 15:51:22]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-02 16:20:35]

Description was changed from

==========
Mus+Ash: Unify CompositorFrameSinks

This CL moves towards enabling unified BeginFrame in Mus and splitting 
the display compositor out of the window server.

1. FrameGenerator now submits CompositorFrames to the root window's
CompositorFrameSink instead of a special "DisplayCompositorFrameSink".
2. ServerWindowCompositorFrameSinks that have a valid AcceleratedWidget
will get a cc::Display (this should become SurfaceHandle when ready).

In an upcoming CL, FrmaeGenerator will no longer need to be aware of 
GpuChannelHost. Instead it will simply use the root window's 
CompositorFrameSink as is.

BUG=658988
==========

to

==========
Mus+Ash: Unify CompositorFrameSinks

This CL moves towards enabling unified BeginFrame in Mus and splitting
the display compositor out of the window server.

1. FrameGenerator now submits CompositorFrames to the root window's
CompositorFrameSink instead of a special "DisplayCompositorFrameSink".
2. ServerWindowCompositorFrameSinks that have a valid AcceleratedWidget
will get a cc::Display (this should become SurfaceHandle when ready).

In an upcoming CL, FrmaeGenerator will no longer need to be aware of
GpuChannelHost. Instead it will simply use the root window's
CompositorFrameSink as is.

BUG=658988

Committed: https://crrev.com/d2f1931ecdde26742abf7cc23f79b38d1dfaf7fb
Cr-Commit-Position: refs/heads/master@{#429286}
==========

[commit-bot: I haz the power, 2016-11-02 16:20:36]

Patchset 7 (id:??) landed as
https://crrev.com/d2f1931ecdde26742abf7cc23f79b38d1dfaf7fb
Cr-Commit-Position: refs/heads/master@{#429286}

[gab, 2016-11-02 18:07:32]

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

[gab, 2016-11-02 18:07:33]

Careful with IWYU, comments below addressed in PS14 of
https://codereview.chromium.org/2443103003

https://codereview.chromium.org/2471503002/diff/120001/services/ui/ws/server_...
File services/ui/ws/server_window_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2471503002/diff/120001/services/ui/ws/server_...
services/ui/ws/server_window_compositor_frame_sink.cc:39:
task_runner_(base::ThreadTaskRunnerHandle::Get()),
#include "base/threading/thread_task_runner_handle.h"

Also,

#include "base/single_thread_task_runner.h"

(since it was fwd-decl'd in header and member needs to be fully defined in .cc)

https://codereview.chromium.org/2471503002/diff/120001/services/ui/ws/server_...
File services/ui/ws/server_window_compositor_frame_sink.h (right):

https://codereview.chromium.org/2471503002/diff/120001/services/ui/ws/server_...
services/ui/ws/server_window_compositor_frame_sink.h:50:
scoped_refptr<SurfacesContextProvider> context_provider,
#include ref_counted.h

https://codereview.chromium.org/2471503002/diff/120001/services/ui/ws/server_...
services/ui/ws/server_window_compositor_frame_sink.h:78:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
Fwd-decl SingleThreadTaskRunner;

https://codereview.chromium.org/2471503002/diff/120001/services/ui/ws/server_...
services/ui/ws/server_window_compositor_frame_sink.h:86:
std::unique_ptr<cc::Display> display_;
#include <memory>

[gab, 2016-11-02 18:07:51]

Description was changed from

==========
Mus+Ash: Unify CompositorFrameSinks

This CL moves towards enabling unified BeginFrame in Mus and splitting
the display compositor out of the window server.

1. FrameGenerator now submits CompositorFrames to the root window's
CompositorFrameSink instead of a special "DisplayCompositorFrameSink".
2. ServerWindowCompositorFrameSinks that have a valid AcceleratedWidget
will get a cc::Display (this should become SurfaceHandle when ready).

In an upcoming CL, FrmaeGenerator will no longer need to be aware of
GpuChannelHost. Instead it will simply use the root window's
CompositorFrameSink as is.

BUG=658988

Committed: https://crrev.com/d2f1931ecdde26742abf7cc23f79b38d1dfaf7fb
Cr-Commit-Position: refs/heads/master@{#429286}
==========

to

==========
Mus+Ash: Unify CompositorFrameSinks

This CL moves towards enabling unified BeginFrame in Mus and splitting
the display compositor out of the window server.

1. FrameGenerator now submits CompositorFrames to the root window's
CompositorFrameSink instead of a special "DisplayCompositorFrameSink".
2. ServerWindowCompositorFrameSinks that have a valid AcceleratedWidget
will get a cc::Display (this should become SurfaceHandle when ready).

In an upcoming CL, FrmaeGenerator will no longer need to be aware of
GpuChannelHost. Instead it will simply use the root window's
CompositorFrameSink as is.

BUG=658988

Committed: https://crrev.com/d2f1931ecdde26742abf7cc23f79b38d1dfaf7fb
Cr-Commit-Position: refs/heads/master@{#429286}
==========

[gab, 2016-11-02 18:07:52]

gab@chromium.org changed reviewers:
- gab@chromium.org