Move DisplayCompositor code to new component.

Move DisplayCompositor code to new component.

The name DisplayCompositor is overloaded right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for a "frame sink manager" is desired. Move the existing
"frame sink manager" code into a single component under src/components.

This is a pure refactoring with no code changes.

BUG=664547

[kylechar, 2017-03-21 17:56:33]

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

[kylechar, 2017-03-21 17:57:21]

Description was changed from

==========
Move DisplayCompositor code to new component.

The name DisplayCompositor is overload right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for FrameSinkManager is desired. Move the existing
DisplayCompositor code into a single component under mus-gpu.

This is a pure refatoring with no code changes.

BUG=664547
==========

to

==========
Move DisplayCompositor code to new component.

The name DisplayCompositor is overload right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for a "frame sink manager" is desired. Move the existing
DisplayCompositor code into a single component under mus-gpu.

This is a pure refatoring with no code changes.

BUG=664547
==========

[danakj, 2017-03-21 18:19:57]

I think fady had said services/ui/frame_sinks, not services/ui/gpu/frame_sinks,
but LGTM % fsamuel

[Fady Samuel, 2017-03-21 18:23:17]

On 2017/03/21 18:19:57, danakj wrote:
> I think fady had said services/ui/frame_sinks, not
services/ui/gpu/frame_sinks,
> but LGTM % fsamuel

Ohh, we chatted offline about this. We felt like services/ui/gpu makes more
sense for all mus-gpu things.

[Fady Samuel, 2017-03-21 18:23:23]

lgtm

[kylechar, 2017-03-21 18:23:43]

On 2017/03/21 18:23:23, Fady Samuel wrote:
> lgtm

Thanks!

[kylechar, 2017-03-21 18:24:31]

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

[commit-bot: I haz the power, 2017-03-21 18:25:15]

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-21 18:32:11]

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

[commit-bot: I haz the power, 2017-03-21 18:32:12]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kylechar, 2017-03-21 19:05:53]

Description was changed from

==========
Move DisplayCompositor code to new component.

The name DisplayCompositor is overload right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for a "frame sink manager" is desired. Move the existing
DisplayCompositor code into a single component under mus-gpu.

This is a pure refatoring with no code changes.

BUG=664547
==========

to

==========
Move DisplayCompositor code to new component.

The name DisplayCompositor is overload right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for a "frame sink manager" is desired. Move the existing
"frame sink manager" code into a single component under components.

This is a pure refatoring with no code changes.

BUG=664547
==========

[kylechar, 2017-03-21 19:11:35]

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

[kylechar, 2017-03-21 19:11:36]

Moved to components as per offline discussion.

+jam as top level owner to approve creating src/components/frame_sink/.

[kylechar, 2017-03-21 19:13:33]

On 2017/03/21 19:11:36, kylechar wrote:
> Moved to components as per offline discussion.
> 
> +jam as top level owner to approve creating src/components/frame_sink/.

I mean src/components/frame_sinks/.

[kylechar, 2017-03-21 19:29:33]

Description was changed from

==========
Move DisplayCompositor code to new component.

The name DisplayCompositor is overload right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for a "frame sink manager" is desired. Move the existing
"frame sink manager" code into a single component under components.

This is a pure refatoring with no code changes.

BUG=664547
==========

to

==========
Move DisplayCompositor code to new component.

The name DisplayCompositor is overloaded right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for a "frame sink manager" is desired. Move the existing
"frame sink manager" code into a single component under src/components.

This is a pure refatoring with no code changes.

BUG=664547
==========

[kylechar, 2017-03-21 19:30:12]

Description was changed from

==========
Move DisplayCompositor code to new component.

The name DisplayCompositor is overloaded right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for a "frame sink manager" is desired. Move the existing
"frame sink manager" code into a single component under src/components.

This is a pure refatoring with no code changes.

BUG=664547
==========

to

==========
Move DisplayCompositor code to new component.

The name DisplayCompositor is overloaded right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for a "frame sink manager" is desired. Move the existing
"frame sink manager" code into a single component under src/components.

This is a pure refactoring with no code changes.

BUG=664547
==========

[kylechar, 2017-03-21 20:05:15]

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

[commit-bot: I haz the power, 2017-03-21 20:05:53]

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

[jam, 2017-03-21 20:33:04]

On 2017/03/21 19:13:33, kylechar wrote:
> On 2017/03/21 19:11:36, kylechar wrote:
> > Moved to components as per offline discussion.
> > 
> > +jam as top level owner to approve creating src/components/frame_sink/.
> 
> I mean src/components/frame_sinks/.

I don't understand why this is a component? This doesn't seem to map to the
usage in
https://cs.chromium.org/chromium/src/components/README?q=components/readme+pa...

[kylechar, 2017-03-21 20:41:41]

On 2017/03/21 20:33:04, jam wrote:
> On 2017/03/21 19:13:33, kylechar wrote:
> > On 2017/03/21 19:11:36, kylechar wrote:
> > > Moved to components as per offline discussion.
> > > 
> > > +jam as top level owner to approve creating src/components/frame_sink/.
> > 
> > I mean src/components/frame_sinks/.
> 
> I don't understand why this is a component? This doesn't seem to map to the
> usage in
>
https://cs.chromium.org/chromium/src/components/README?q=components/readme+pa...

This would be code that is going to be shared between content/browser/ and
services/ui/gpu/. There was disagreement about putting it in services/ui/gpu/
and it was suggested to put it in components instead. Does that sounds
reasonable or?

[commit-bot: I haz the power, 2017-03-21 21:33:42]

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

[commit-bot: I haz the power, 2017-03-21 21:33:43]

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

[jam, 2017-03-21 22:03:13]

On 2017/03/21 20:41:41, kylechar wrote:
> On 2017/03/21 20:33:04, jam wrote:
> > On 2017/03/21 19:13:33, kylechar wrote:
> > > On 2017/03/21 19:11:36, kylechar wrote:
> > > > Moved to components as per offline discussion.
> > > > 
> > > > +jam as top level owner to approve creating src/components/frame_sink/.
> > > 
> > > I mean src/components/frame_sinks/.
> > 
> > I don't understand why this is a component? This doesn't seem to map to the
> > usage in
> >
>
https://cs.chromium.org/chromium/src/components/README?q=components/readme+pa...
> 
> This would be code that is going to be shared between content/browser/ and
> services/ui/gpu/. There was disagreement about putting it in services/ui/gpu/
> and it was suggested to put it in components instead. Does that sounds
> reasonable or?

what is the reason to not put it in services/ui/gpu?

[danakj, 2017-03-21 22:05:14]

On Tue, Mar 21, 2017 at 6:03 PM, <jam@chromium.org> wrote:

> On 2017/03/21 20:41:41, kylechar wrote:
> > On 2017/03/21 20:33:04, jam wrote:
> > > On 2017/03/21 19:13:33, kylechar wrote:
> > > > On 2017/03/21 19:11:36, kylechar wrote:
> > > > > Moved to components as per offline discussion.
> > > > >
> > > > > +jam as top level owner to approve creating
> src/components/frame_sink/.
> > > >
> > > > I mean src/components/frame_sinks/.
> > >
> > > I don't understand why this is a component? This doesn't seem to map
> to the
> > > usage in
> > >
> >
> https://cs.chromium.org/chromium/src/components/
> README?q=components/readme+package:%5Echromium$&dr
> >
> > This would be code that is going to be shared between content/browser/
> and
> > services/ui/gpu/. There was disagreement about putting it in
> services/ui/gpu/
> > and it was suggested to put it in components instead. Does that sounds
> > reasonable or?
>
> what is the reason to not put it in services/ui/gpu?
>

services/ui/gpu is the impl of the gpu process. That's like putting code in
content/gpu for content/browser to use. Shared code goes elsewhere.


>
> https://codereview.chromium.org/2768563002/
>

-- 
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.

[jam, 2017-03-22 23:13:33]

On 2017/03/21 22:05:14, danakj wrote:
> On Tue, Mar 21, 2017 at 6:03 PM, <mailto:jam@chromium.org> wrote:
> 
> > On 2017/03/21 20:41:41, kylechar wrote:
> > > On 2017/03/21 20:33:04, jam wrote:
> > > > On 2017/03/21 19:13:33, kylechar wrote:
> > > > > On 2017/03/21 19:11:36, kylechar wrote:
> > > > > > Moved to components as per offline discussion.
> > > > > >
> > > > > > +jam as top level owner to approve creating
> > src/components/frame_sink/.
> > > > >
> > > > > I mean src/components/frame_sinks/.
> > > >
> > > > I don't understand why this is a component? This doesn't seem to map
> > to the
> > > > usage in
> > > >
> > >
> > https://cs.chromium.org/chromium/src/components/
> > README?q=components/readme+package:%5Echromium$&dr
> > >
> > > This would be code that is going to be shared between content/browser/
> > and
> > > services/ui/gpu/. There was disagreement about putting it in
> > services/ui/gpu/
> > > and it was suggested to put it in components instead. Does that sounds
> > > reasonable or?
> >
> > what is the reason to not put it in services/ui/gpu?
> >
> 
> services/ui/gpu is the impl of the gpu process. That's like putting code in
> content/gpu for content/browser to use. Shared code goes elsewhere.

I see, so which code in content/browser is going to use it? I don't think that
code exists since I don't see updates in this cl? Is it considered client code
of services/ui or something else? And can you explain why the existing
display_compositor components directory isn't good for this?

[kylechar, 2017-03-23 14:36:16]

On 2017/03/22 23:13:33, jam wrote:
> On 2017/03/21 22:05:14, danakj wrote:
> > On Tue, Mar 21, 2017 at 6:03 PM, <mailto:jam@chromium.org> wrote:
> > 
> > > On 2017/03/21 20:41:41, kylechar wrote:
> > > > On 2017/03/21 20:33:04, jam wrote:
> > > > > On 2017/03/21 19:13:33, kylechar wrote:
> > > > > > On 2017/03/21 19:11:36, kylechar wrote:
> > > > > > > Moved to components as per offline discussion.
> > > > > > >
> > > > > > > +jam as top level owner to approve creating
> > > src/components/frame_sink/.
> > > > > >
> > > > > > I mean src/components/frame_sinks/.
> > > > >
> > > > > I don't understand why this is a component? This doesn't seem to map
> > > to the
> > > > > usage in
> > > > >
> > > >
> > > https://cs.chromium.org/chromium/src/components/
> > > README?q=components/readme+package:%5Echromium$&dr
> > > >
> > > > This would be code that is going to be shared between content/browser/
> > > and
> > > > services/ui/gpu/. There was disagreement about putting it in
> > > services/ui/gpu/
> > > > and it was suggested to put it in components instead. Does that sounds
> > > > reasonable or?
> > >
> > > what is the reason to not put it in services/ui/gpu?
> > >
> > 
> > services/ui/gpu is the impl of the gpu process. That's like putting code in
> > content/gpu for content/browser to use. Shared code goes elsewhere.
> 
> I see, so which code in content/browser is going to use it? I don't think that
> code exists since I don't see updates in this cl? Is it considered client code
> of services/ui or something else? And can you explain why the existing
> display_compositor components directory isn't good for this?

Sure, I can try to elaborate. This is part of the "DisplayCompositor" project
we've been working on for mus. The goal is that every process submits
CompositorsFrames directly to the GPU where we produce the output frame
(pixels). We have implemented the DisplayCompositor in the GPU process(/thread)
for mus already. This CL moves code used in mus, so we can start to unify
implementations between mus and non-mus Chrome.

In non-mus Chrome, what we were calling the "DisplayCompositor" all happens in
the browser process. Clients (usually a renderer) submit CompositorFrames to the
browser process. The goal is swap out parts of GpuProcessTransportFactory and
everything else involved in submitting CompositorFrames to use a similar
architecture as mus, switch the CompositorFrame submission to use Mojo and
eventually be able to move the DisplayCompositor to the GPU.

So this code will be used by services/ui/gpu/ and content/browser/. It's either
browser or GPU code, but will all be GPU code one day.

danakj expressed opposition over the project name "DisplayCompositor" for the
whole system because there is already a "display compositor" eg. cc::Display.
Maybe she elaborate further about her concerns?

[jam, 2017-03-23 15:03:19]

On 2017/03/23 14:36:16, kylechar wrote:
> On 2017/03/22 23:13:33, jam wrote:
> > On 2017/03/21 22:05:14, danakj wrote:
> > > On Tue, Mar 21, 2017 at 6:03 PM, <mailto:jam@chromium.org> wrote:
> > > 
> > > > On 2017/03/21 20:41:41, kylechar wrote:
> > > > > On 2017/03/21 20:33:04, jam wrote:
> > > > > > On 2017/03/21 19:13:33, kylechar wrote:
> > > > > > > On 2017/03/21 19:11:36, kylechar wrote:
> > > > > > > > Moved to components as per offline discussion.
> > > > > > > >
> > > > > > > > +jam as top level owner to approve creating
> > > > src/components/frame_sink/.
> > > > > > >
> > > > > > > I mean src/components/frame_sinks/.
> > > > > >
> > > > > > I don't understand why this is a component? This doesn't seem to map
> > > > to the
> > > > > > usage in
> > > > > >
> > > > >
> > > > https://cs.chromium.org/chromium/src/components/
> > > > README?q=components/readme+package:%5Echromium$&dr
> > > > >
> > > > > This would be code that is going to be shared between content/browser/
> > > > and
> > > > > services/ui/gpu/. There was disagreement about putting it in
> > > > services/ui/gpu/
> > > > > and it was suggested to put it in components instead. Does that sounds
> > > > > reasonable or?
> > > >
> > > > what is the reason to not put it in services/ui/gpu?
> > > >
> > > 
> > > services/ui/gpu is the impl of the gpu process. That's like putting code
in
> > > content/gpu for content/browser to use. Shared code goes elsewhere.
> > 
> > I see, so which code in content/browser is going to use it? I don't think
that
> > code exists since I don't see updates in this cl? Is it considered client
code
> > of services/ui or something else? And can you explain why the existing
> > display_compositor components directory isn't good for this?
> 
> Sure, I can try to elaborate. This is part of the "DisplayCompositor" project
> we've been working on for mus. The goal is that every process submits
> CompositorsFrames directly to the GPU where we produce the output frame
> (pixels). We have implemented the DisplayCompositor in the GPU
process(/thread)
> for mus already. This CL moves code used in mus, so we can start to unify
> implementations between mus and non-mus Chrome.
> 
> In non-mus Chrome, what we were calling the "DisplayCompositor" all happens in
> the browser process. Clients (usually a renderer) submit CompositorFrames to
the
> browser process. The goal is swap out parts of GpuProcessTransportFactory and
> everything else involved in submitting CompositorFrames to use a similar
> architecture as mus, switch the CompositorFrame submission to use Mojo and
> eventually be able to move the DisplayCompositor to the GPU.
> 
> So this code will be used by services/ui/gpu/ and content/browser/. It's
either
> browser or GPU code, but will all be GPU code one day.
> 
> danakj expressed opposition over the project name "DisplayCompositor" for the
> whole system because there is already a "display compositor" eg. cc::Display.
> Maybe she elaborate further about her concerns?

Thanks. What I still don't understand is:
-why is there still a components/display_compositor after this cl?
-if there's still a components/display_compositor and components/frame_sinks, do
they really need to be in 2 separate components or can they be merged into one
with a more general name?
-i'm sure you already thought about this, but for me to understand, curious why
src/gpu isn't a fit for this?

[kylechar, 2017-03-23 15:15:13]

> Thanks. What I still don't understand is:
> -why is there still a components/display_compositor after this cl?
> -if there's still a components/display_compositor and components/frame_sinks,
do
> they really need to be in 2 separate components or can they be merged into one
> with a more general name?
> -i'm sure you already thought about this, but for me to understand, curious
why
> src/gpu isn't a fit for this?

I'm not very familiar with the other code in components/display_compositor/, so
I can't give a great answer, but I think it's more along the lines of the per
display compositor rather than the system that combines CompositorFrames.
Hopefully someone else can chime in.

[danakj, 2017-03-23 15:16:28]

On Thu, Mar 23, 2017 at 11:03 AM, <jam@chromium.org> wrote:

> On 2017/03/23 14:36:16, kylechar wrote:
> > On 2017/03/22 23:13:33, jam wrote:
> > > On 2017/03/21 22:05:14, danakj wrote:
> > > > On Tue, Mar 21, 2017 at 6:03 PM, <mailto:jam@chromium.org> wrote:
> > > >
> > > > > On 2017/03/21 20:41:41, kylechar wrote:
> > > > > > On 2017/03/21 20:33:04, jam wrote:
> > > > > > > On 2017/03/21 19:13:33, kylechar wrote:
> > > > > > > > On 2017/03/21 19:11:36, kylechar wrote:
> > > > > > > > > Moved to components as per offline discussion.
> > > > > > > > >
> > > > > > > > > +jam as top level owner to approve creating
> > > > > src/components/frame_sink/.
> > > > > > > >
> > > > > > > > I mean src/components/frame_sinks/.
> > > > > > >
> > > > > > > I don't understand why this is a component? This doesn't seem
> to map
> > > > > to the
> > > > > > > usage in
> > > > > > >
> > > > > >
> > > > > https://cs.chromium.org/chromium/src/components/
> > > > > README?q=components/readme+package:%5Echromium$&dr
> > > > > >
> > > > > > This would be code that is going to be shared between
> content/browser/
> > > > > and
> > > > > > services/ui/gpu/. There was disagreement about putting it in
> > > > > services/ui/gpu/
> > > > > > and it was suggested to put it in components instead. Does that
> sounds
> > > > > > reasonable or?
> > > > >
> > > > > what is the reason to not put it in services/ui/gpu?
> > > > >
> > > >
> > > > services/ui/gpu is the impl of the gpu process. That's like putting
> code
> in
> > > > content/gpu for content/browser to use. Shared code goes elsewhere.
> > >
> > > I see, so which code in content/browser is going to use it? I don't
> think
> that
> > > code exists since I don't see updates in this cl? Is it considered
> client
> code
> > > of services/ui or something else? And can you explain why the existing
> > > display_compositor components directory isn't good for this?
> >
> > Sure, I can try to elaborate. This is part of the "DisplayCompositor"
> project
> > we've been working on for mus. The goal is that every process submits
> > CompositorsFrames directly to the GPU where we produce the output frame
> > (pixels). We have implemented the DisplayCompositor in the GPU
> process(/thread)
> > for mus already. This CL moves code used in mus, so we can start to unify
> > implementations between mus and non-mus Chrome.
> >
> > In non-mus Chrome, what we were calling the "DisplayCompositor" all
> happens in
> > the browser process. Clients (usually a renderer) submit
> CompositorFrames to
> the
> > browser process. The goal is swap out parts of
> GpuProcessTransportFactory and
> > everything else involved in submitting CompositorFrames to use a similar
> > architecture as mus, switch the CompositorFrame submission to use Mojo
> and
> > eventually be able to move the DisplayCompositor to the GPU.
> >
> > So this code will be used by services/ui/gpu/ and content/browser/. It's
> either
> > browser or GPU code, but will all be GPU code one day.
> >
> > danakj expressed opposition over the project name "DisplayCompositor"
> for the
> > whole system because there is already a "display compositor" eg.
> cc::Display.
> > Maybe she elaborate further about her concerns?
>
> Thanks. What I still don't understand is:
>

I'll try to help out.


> -why is there still a components/display_compositor after this cl?
>

This is a problem of naming conflicts we will have to resolve.
components/display_compositor is embedding code for cc::Display. Whereas
this interface is for giving out frame sinks. I think of it as a mojo
equivalent for DelegatedFrameHost, pieces of RWHVA and
GpuProcessTransportFactory. Moving forward we're going to rename this API
so that a "display compsositor" is a single cc::Display and the pieces that
make it work. This API uses display compositor, and was named
"DisplayCompositor" but makes a naming conflict.


> -if there's still a components/display_compositor and
> components/frame_sinks, do
> they really need to be in 2 separate components or can they be merged into
> one
> with a more general name?
>

There is one set of code that receives compositor frames (from frame sinks)
and manages lifetimes, embeds them in each other. This is
DelegatedFrameHost/RWHVAura/ui::Compositor today. That is
components/frame_sinks. Behind that, the compositor frames are submitted
for components/display_compositor (there's one such display compositor per
physical screen on chromeos, one per window on desktop).

I hope this shows how the components/display_compositor is a lower level
concept, that takes a scene (via compositor frames) and draws it to a
physical output. Whereas components/frame_sinks is about cross process
collection of compositor frames, lifetime management, and organizing
hierarchies between them.


> -i'm sure you already thought about this, but for me to understand,
> curious why
> src/gpu isn't a fit for this?
>

gpu/ is even lower level code, used by components/display_compositor to
implement access to the current GPU process services ie GL contexts, as
well as implementation of those primitives.


>
>
> https://codereview.chromium.org/2768563002/
>

-- 
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.

[jam, 2017-03-23 22:46:33]

On 2017/03/23 15:16:28, danakj wrote:
> On Thu, Mar 23, 2017 at 11:03 AM, <mailto:jam@chromium.org> wrote:
> 
> > On 2017/03/23 14:36:16, kylechar wrote:
> > > On 2017/03/22 23:13:33, jam wrote:
> > > > On 2017/03/21 22:05:14, danakj wrote:
> > > > > On Tue, Mar 21, 2017 at 6:03 PM, <mailto:jam@chromium.org> wrote:
> > > > >
> > > > > > On 2017/03/21 20:41:41, kylechar wrote:
> > > > > > > On 2017/03/21 20:33:04, jam wrote:
> > > > > > > > On 2017/03/21 19:13:33, kylechar wrote:
> > > > > > > > > On 2017/03/21 19:11:36, kylechar wrote:
> > > > > > > > > > Moved to components as per offline discussion.
> > > > > > > > > >
> > > > > > > > > > +jam as top level owner to approve creating
> > > > > > src/components/frame_sink/.
> > > > > > > > >
> > > > > > > > > I mean src/components/frame_sinks/.
> > > > > > > >
> > > > > > > > I don't understand why this is a component? This doesn't seem
> > to map
> > > > > > to the
> > > > > > > > usage in
> > > > > > > >
> > > > > > >
> > > > > > https://cs.chromium.org/chromium/src/components/
> > > > > > README?q=components/readme+package:%5Echromium$&dr
> > > > > > >
> > > > > > > This would be code that is going to be shared between
> > content/browser/
> > > > > > and
> > > > > > > services/ui/gpu/. There was disagreement about putting it in
> > > > > > services/ui/gpu/
> > > > > > > and it was suggested to put it in components instead. Does that
> > sounds
> > > > > > > reasonable or?
> > > > > >
> > > > > > what is the reason to not put it in services/ui/gpu?
> > > > > >
> > > > >
> > > > > services/ui/gpu is the impl of the gpu process. That's like putting
> > code
> > in
> > > > > content/gpu for content/browser to use. Shared code goes elsewhere.
> > > >
> > > > I see, so which code in content/browser is going to use it? I don't
> > think
> > that
> > > > code exists since I don't see updates in this cl? Is it considered
> > client
> > code
> > > > of services/ui or something else? And can you explain why the existing
> > > > display_compositor components directory isn't good for this?
> > >
> > > Sure, I can try to elaborate. This is part of the "DisplayCompositor"
> > project
> > > we've been working on for mus. The goal is that every process submits
> > > CompositorsFrames directly to the GPU where we produce the output frame
> > > (pixels). We have implemented the DisplayCompositor in the GPU
> > process(/thread)
> > > for mus already. This CL moves code used in mus, so we can start to unify
> > > implementations between mus and non-mus Chrome.
> > >
> > > In non-mus Chrome, what we were calling the "DisplayCompositor" all
> > happens in
> > > the browser process. Clients (usually a renderer) submit
> > CompositorFrames to
> > the
> > > browser process. The goal is swap out parts of
> > GpuProcessTransportFactory and
> > > everything else involved in submitting CompositorFrames to use a similar
> > > architecture as mus, switch the CompositorFrame submission to use Mojo
> > and
> > > eventually be able to move the DisplayCompositor to the GPU.
> > >
> > > So this code will be used by services/ui/gpu/ and content/browser/. It's
> > either
> > > browser or GPU code, but will all be GPU code one day.
> > >
> > > danakj expressed opposition over the project name "DisplayCompositor"
> > for the
> > > whole system because there is already a "display compositor" eg.
> > cc::Display.
> > > Maybe she elaborate further about her concerns?
> >
> > Thanks. What I still don't understand is:
> >
> 
> I'll try to help out.
> 
> 
> > -why is there still a components/display_compositor after this cl?
> >
> 
> This is a problem of naming conflicts we will have to resolve.
> components/display_compositor is embedding code for cc::Display. Whereas
> this interface is for giving out frame sinks. I think of it as a mojo
> equivalent for DelegatedFrameHost, pieces of RWHVA and
> GpuProcessTransportFactory. Moving forward we're going to rename this API
> so that a "display compsositor" is a single cc::Display and the pieces that
> make it work. This API uses display compositor, and was named
> "DisplayCompositor" but makes a naming conflict.
> 
> 
> > -if there's still a components/display_compositor and
> > components/frame_sinks, do
> > they really need to be in 2 separate components or can they be merged into
> > one
> > with a more general name?
> >
> 
> There is one set of code that receives compositor frames (from frame sinks)
> and manages lifetimes, embeds them in each other. This is
> DelegatedFrameHost/RWHVAura/ui::Compositor today. That is
> components/frame_sinks. Behind that, the compositor frames are submitted
> for components/display_compositor (there's one such display compositor per
> physical screen on chromeos, one per window on desktop).
> 
> I hope this shows how the components/display_compositor is a lower level
> concept, that takes a scene (via compositor frames) and draws it to a
> physical output. Whereas components/frame_sinks is about cross process
> collection of compositor frames, lifetime management, and organizing
> hierarchies between them.

Thanks for the explanation, this is very helpful.

My last question is: should we have components/compositor, and have
subdirectories there? This is driven by the motivation to avoid having hundreds
of component directories with each just having a few files.

> 
> 
> > -i'm sure you already thought about this, but for me to understand,
> > curious why
> > src/gpu isn't a fit for this?
> >
> 
> gpu/ is even lower level code, used by components/display_compositor to
> implement access to the current GPU process services ie GL contexts, as
> well as implementation of those primitives.
> 
> 
> >
> >
> > https://codereview.chromium.org/2768563002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-03-24 15:13:28]

On Thu, Mar 23, 2017 at 6:46 PM, <jam@chromium.org> wrote:

> On 2017/03/23 15:16:28, danakj wrote:
>
> > On Thu, Mar 23, 2017 at 11:03 AM, <mailto:jam@chromium.org> wrote:
> >
> > > On 2017/03/23 14:36:16, kylechar wrote:
> > > > On 2017/03/22 23:13:33, jam wrote:
> > > > > On 2017/03/21 22:05:14, danakj wrote:
> > > > > > On Tue, Mar 21, 2017 at 6:03 PM, <mailto:jam@chromium.org>
> wrote:
> > > > > >
> > > > > > > On 2017/03/21 20:41:41, kylechar wrote:
> > > > > > > > On 2017/03/21 20:33:04, jam wrote:
> > > > > > > > > On 2017/03/21 19:13:33, kylechar wrote:
> > > > > > > > > > On 2017/03/21 19:11:36, kylechar wrote:
> > > > > > > > > > > Moved to components as per offline discussion.
> > > > > > > > > > >
> > > > > > > > > > > +jam as top level owner to approve creating
> > > > > > > src/components/frame_sink/.
> > > > > > > > > >
> > > > > > > > > > I mean src/components/frame_sinks/.
> > > > > > > > >
> > > > > > > > > I don't understand why this is a component? This doesn't
> seem
> > > to map
> > > > > > > to the
> > > > > > > > > usage in
> > > > > > > > >
> > > > > > > >
> > > > > > > https://cs.chromium.org/chromium/src/components/
> > > > > > > README?q=components/readme+package:%5Echromium$&dr
> > > > > > > >
> > > > > > > > This would be code that is going to be shared between
> > > content/browser/
> > > > > > > and
> > > > > > > > services/ui/gpu/. There was disagreement about putting it in
> > > > > > > services/ui/gpu/
> > > > > > > > and it was suggested to put it in components instead. Does
> that
> > > sounds
> > > > > > > > reasonable or?
> > > > > > >
> > > > > > > what is the reason to not put it in services/ui/gpu?
> > > > > > >
> > > > > >
> > > > > > services/ui/gpu is the impl of the gpu process. That's like
> putting
> > > code
> > > in
> > > > > > content/gpu for content/browser to use. Shared code goes
> elsewhere.
> > > > >
> > > > > I see, so which code in content/browser is going to use it? I don't
> > > think
> > > that
> > > > > code exists since I don't see updates in this cl? Is it considered
> > > client
> > > code
> > > > > of services/ui or something else? And can you explain why the
> existing
> > > > > display_compositor components directory isn't good for this?
> > > >
> > > > Sure, I can try to elaborate. This is part of the "DisplayCompositor"
> > > project
> > > > we've been working on for mus. The goal is that every process submits
> > > > CompositorsFrames directly to the GPU where we produce the output
> frame
> > > > (pixels). We have implemented the DisplayCompositor in the GPU
> > > process(/thread)
> > > > for mus already. This CL moves code used in mus, so we can start to
> unify
> > > > implementations between mus and non-mus Chrome.
> > > >
> > > > In non-mus Chrome, what we were calling the "DisplayCompositor" all
> > > happens in
> > > > the browser process. Clients (usually a renderer) submit
> > > CompositorFrames to
> > > the
> > > > browser process. The goal is swap out parts of
> > > GpuProcessTransportFactory and
> > > > everything else involved in submitting CompositorFrames to use a
> similar
> > > > architecture as mus, switch the CompositorFrame submission to use
> Mojo
> > > and
> > > > eventually be able to move the DisplayCompositor to the GPU.
> > > >
> > > > So this code will be used by services/ui/gpu/ and content/browser/.
> It's
> > > either
> > > > browser or GPU code, but will all be GPU code one day.
> > > >
> > > > danakj expressed opposition over the project name "DisplayCompositor"
> > > for the
> > > > whole system because there is already a "display compositor" eg.
> > > cc::Display.
> > > > Maybe she elaborate further about her concerns?
> > >
> > > Thanks. What I still don't understand is:
> > >
> >
> > I'll try to help out.
> >
> >
> > > -why is there still a components/display_compositor after this cl?
> > >
> >
> > This is a problem of naming conflicts we will have to resolve.
> > components/display_compositor is embedding code for cc::Display. Whereas
> > this interface is for giving out frame sinks. I think of it as a mojo
> > equivalent for DelegatedFrameHost, pieces of RWHVA and
> > GpuProcessTransportFactory. Moving forward we're going to rename this API
> > so that a "display compsositor" is a single cc::Display and the pieces
> that
> > make it work. This API uses display compositor, and was named
> > "DisplayCompositor" but makes a naming conflict.
> >
> >
> > > -if there's still a components/display_compositor and
> > > components/frame_sinks, do
> > > they really need to be in 2 separate components or can they be merged
> into
> > > one
> > > with a more general name?
> > >
> >
> > There is one set of code that receives compositor frames (from frame
> sinks)
> > and manages lifetimes, embeds them in each other. This is
> > DelegatedFrameHost/RWHVAura/ui::Compositor today. That is
> > components/frame_sinks. Behind that, the compositor frames are submitted
> > for components/display_compositor (there's one such display compositor
> per
> > physical screen on chromeos, one per window on desktop).
> >
> > I hope this shows how the components/display_compositor is a lower level
> > concept, that takes a scene (via compositor frames) and draws it to a
> > physical output. Whereas components/frame_sinks is about cross process
> > collection of compositor frames, lifetime management, and organizing
> > hierarchies between them.
>
> Thanks for the explanation, this is very helpful.
>
> My last question is: should we have components/compositor, and have
> subdirectories there? This is driven by the motivation to avoid having
> hundreds
> of component directories with each just having a few files.
>

My problem with naming this "compositor" is the same with the name
"DisplayCompositor" on the API, which we're going to change. We have a
bunch of things in chrome called "compositors" because they *used to be*
compositors like 4 years ago. This would add yet another thing called
compositor that isn't one. I'm getting very tired of explaining to people
"you're talking about the 'layer compositor' (which isn't a compositor)"
and so on, and don't want another thing where "that compositor isn't a
compositor but it has some display compositor (ie
components/display_compositor not 'DisplayCompositor') inside it that it
uses". We don't name components by the components they are built on, but by
the things that they do, and I think calling this compositor would poorly
explain it's role.

frame_sinks at least describes the public APIs that this component exposes.
I don't know what to actually call it, cuz historically this has always
just been called "content" and the closest I've gotten is "glue layer
between ui/blink/video/etc and compositing".

I get the wish to group things under a single directory. I don't have a
name that is specific enough to be meaningful and doesn't conflict with
other parts. Things like graphics_service are just so vague as to maybe
lose meaning. Putting "gpu" in it is misleading at best because this is
used for software compositing modes as well. gpu/ code is all actually for
real GPU. The "gpu process" is going to have to become a "compositing"
process or something as we move toward mus.

Naming is hard and I'm not happy with where we are right now. I just want
us to not have 3+ things called compositor (and 2 things called display
compositor) of which only one is, cuz it's impossible to talk, and makes it
hard to layer things correctly.

If you have some higher level name to put frame_sinks and
display_compositor inside that doesn't try to pass itself off as a
compositor itself, I'm all ears. TBH //cc should probably be part of it
too, but that's probably not moving any time soon, just hoping for
something to get us in a better place than where we are now.


>
>
> >
> >
> > > -i'm sure you already thought about this, but for me to understand,
> > > curious why
> > > src/gpu isn't a fit for this?
> > >
> >
> > gpu/ is even lower level code, used by components/display_compositor to
> > implement access to the current GPU process services ie GL contexts, as
> > well as implementation of those primitives.
> >
> >
> > >
> > >
> > > https://codereview.chromium.org/2768563002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2768563002/
>

-- 
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.

[kylechar, 2017-03-27 21:07:54]

Doesn't look like this CL is landable, so I'm going to close this.

[kylechar, 2017-03-27 21:08:01]

Description was changed from

==========
Move DisplayCompositor code to new component.

The name DisplayCompositor is overloaded right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for a "frame sink manager" is desired. Move the existing
"frame sink manager" code into a single component under src/components.

This is a pure refactoring with no code changes.

BUG=664547
==========

to

==========
Move DisplayCompositor code to new component.

The name DisplayCompositor is overloaded right now for both the per
display compositor and the display compositor service which is the
collection of per display compositors. From offline discussion, a new
component for a "frame sink manager" is desired. Move the existing
"frame sink manager" code into a single component under src/components.

This is a pure refactoring with no code changes.

BUG=664547
==========

[jam, 2017-03-28 15:35:10]

On 2017/03/27 21:07:54, kylechar wrote:
> Doesn't look like this CL is landable, so I'm going to close this.

Sorry, I missed this yesterday. Ok if you can't come up with a generic name in
components/, and I don't have a better suggestion, then lgtm for now to unlblock
you.

(in the future, if I don't respond quickly please IM me)

[kylechar, 2017-03-30 17:16:57]

On 2017/03/28 15:35:10, jam wrote:
> On 2017/03/27 21:07:54, kylechar wrote:
> > Doesn't look like this CL is landable, so I'm going to close this.
> 
> Sorry, I missed this yesterday. Ok if you can't come up with a generic name in
> components/, and I don't have a better suggestion, then lgtm for now to
unlblock
> you.
> 
> (in the future, if I don't respond quickly please IM me)

Thanks jam! Although I think we've concluded offline that this is not the
correct location, so I'll redo this in the correct location.