viz: Use a simple GpuMemoryBufferManager implementation.

viz: Use a simple GpuMemoryBufferManager implementation.

Introduce InProcessGpuMemoryBufferManager, which directly interacts
with a GpuMemoryBufferFactory to allocate memory used by the display
compositor.

Follow on steps: relocate ServerGpuMemoryBufferManager into viz/host.

BUG=733482
Review-Url: https://codereview.chromium.org/2942283002
Cr-Commit-Position: refs/heads/master@{#481231}
Committed: https://chromium.googlesource.com/chromium/src/+/1f51373d28edbd397234c9ee7d60198f0a5e0189

[sadrul, 2017-06-16 17:46:06]

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

[commit-bot: I haz the power, 2017-06-16 17:46:29]

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-06-16 18:41:54]

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

[commit-bot: I haz the power, 2017-06-16 18:41:56]

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

[sadrul, 2017-06-16 22:15:05]

Description was changed from

==========
viz: Use a simple GpuMemoryBufferManager implementation.

...

BUG=...
==========

to

==========
viz: Use a simple GpuMemoryBufferManager implementation.

Introduce InProcessGpuMemoryBufferManager, which directly interacts
with a GpuMemoryBufferFactory to allocate memory used by the display
compositor.

Follow on steps: relocate ServerGpuMemoryBufferManager into viz/host.

BUG=733482
==========

[sadrul, 2017-06-16 22:20:20]

sadrul@chromium.org changed reviewers:
+ danakj@chromium.org, fsamuel@chromium.org, reveman@chromium.org

[sadrul, 2017-06-16 22:20:21]

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
File components/viz/service/BUILD.gn (right):

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
components/viz/service/BUILD.gn:58: "//gpu/ipc/client",
This dependency is because the InProcessGMBManager needs to crete
GpuMemoryBufferImpl. It felt a little weird to add this (because it is also
depending on //gpu/ipc/service). But then:

 . Display compositor *is* kind of a client of the gpu service, in the sense
that it allocates memory just like clients do.
 . There is already dependency on //gpu/command_buffer/client.

So this is probably OK?

The alternative would be to move GpuMemoryBufferImpl into //gpu/ipc/common.

Let me know what you folks prefer.

[sadrul, 2017-06-16 22:20:56]

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

[commit-bot: I haz the power, 2017-06-16 22:21:19]

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-06-16 22:33:18]

lgtm

https://codereview.chromium.org/2942283002/diff/1/services/ui/gpu/gpu_main.cc
File services/ui/gpu/gpu_main.cc (right):

https://codereview.chromium.org/2942283002/diff/1/services/ui/gpu/gpu_main.cc...
services/ui/gpu/gpu_main.cc:48: class InProcessGpuMemoryBufferManager : public
gpu::GpuMemoryBufferManager {
Move this to a separate file please

[commit-bot: I haz the power, 2017-06-17 00:35:49]

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

[commit-bot: I haz the power, 2017-06-17 00:35:51]

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

[sadrul, 2017-06-17 03:28:43]

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

[commit-bot: I haz the power, 2017-06-17 03:28:49]

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-06-17 04:43:26]

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

[commit-bot: I haz the power, 2017-06-17 04:43:27]

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

[sadrul, 2017-06-19 20:08:58]

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

[commit-bot: I haz the power, 2017-06-19 20:09: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-06-19 21:32:54]

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

[commit-bot: I haz the power, 2017-06-19 21:32:55]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-06-20 14:52:41]

LGTM % reveman

https://codereview.chromium.org/2942283002/diff/60001/components/viz/service/...
File components/viz/service/BUILD.gn (right):

https://codereview.chromium.org/2942283002/diff/60001/components/viz/service/...
components/viz/service/BUILD.gn:58: "//gpu/ipc/client",
i didn't see this used while reading through?

https://codereview.chromium.org/2942283002/diff/60001/components/viz/service/...
File components/viz/service/display_compositor/gpu_display_provider.cc (right):

https://codereview.chromium.org/2942283002/diff/60001/components/viz/service/...
components/viz/service/display_compositor/gpu_display_provider.cc:34: return
buffer_factory ? buffer_factory->AsImageFactory() : nullptr;
nit: why not use channel_manager->gpu_image_factory()? This is reimplementing it
no?

[danakj, 2017-06-20 14:59:50]

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
File components/viz/service/BUILD.gn (right):

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
components/viz/service/BUILD.gn:58: "//gpu/ipc/client",
On 2017/06/16 22:20:21, sadrul wrote:
> This dependency is because the InProcessGMBManager needs to crete
> GpuMemoryBufferImpl. It felt a little weird to add this (because it is also
> depending on //gpu/ipc/service). But then:
> 
>  . Display compositor *is* kind of a client of the gpu service, in the sense
> that it allocates memory just like clients do.
>  . There is already dependency on //gpu/command_buffer/client.
> 
> So this is probably OK?
> 
> The alternative would be to move GpuMemoryBufferImpl into //gpu/ipc/common.
> 
> Let me know what you folks prefer.

Oh I see. The GpuMemoryBufferImpl doesn't look client-specific or that it would
bring any other client deps with it, so in common/ would be okay.

As you say, this will be using command buffer client, since it is going to be
going thru the command buffer. I'm not sure that implies it's also right to
depend on gpu/ipc/client. If the majority of that target would be wrong to use
here, then preventing that by moving to common seems to be of value to me. If
not, then what you did seems good, and maybe add a comment to the BUILD.gn and
make it not a public_dep?

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
components/viz/service/BUILD.gn:60: "//gpu/ipc/service",
Also doesn't need to be a public_dep?

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
File
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.cc
(right):

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.cc:9:
#include "gpu/ipc/service/gpu_memory_buffer_factory.h"
tbh i don't see this type anywhere in this file, is this include used?

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
File
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.h
(right):

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.h:10:
#include "gpu/ipc/service/gpu_memory_buffer_factory.h"
don't see this used in the header?

[reveman, 2017-06-20 18:22:07]

lgtm with nit

I don't feel strongly about about GpuMemoryBufferImpl in client/ or common/.
It's part of the client side API for GMBs (the API that's exposed to users of
GMBs) and from that POV it belongs in client/. However, if the client/ vs
common/ directory division is more about the GPU/VIZ process relationship then
common/ makes more sense as VIZ is a user of the client side GMB API.

https://codereview.chromium.org/2942283002/diff/60001/components/viz/service/...
File
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.h
(right):

https://codereview.chromium.org/2942283002/diff/60001/components/viz/service/...
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.h:30:
std::unique_ptr<gfx::GpuMemoryBuffer> CreateGpuMemoryBuffer(
nit: why not public? it's public to anyone with a gpu::GpuMemoryBufferManager*
to this instance so making it private here is confusing imo

[sadrul, 2017-06-21 05:57:06]

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

[commit-bot: I haz the power, 2017-06-21 05:57:27]

Dry run: CQ is trying da patch.

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

[sadrul, 2017-06-21 05:57:46]

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
File components/viz/service/BUILD.gn (right):

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
components/viz/service/BUILD.gn:60: "//gpu/ipc/service",
On 2017/06/20 14:59:50, danakj wrote:
> Also doesn't need to be a public_dep?

I have moved these out of public_dep.

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
File
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.cc
(right):

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.cc:9:
#include "gpu/ipc/service/gpu_memory_buffer_factory.h"
On 2017/06/20 14:59:50, danakj wrote:
> tbh i don't see this type anywhere in this file, is this include used?

This is needed (in line ~35:37).

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
File
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.h
(right):

https://codereview.chromium.org/2942283002/diff/20001/components/viz/service/...
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.h:10:
#include "gpu/ipc/service/gpu_memory_buffer_factory.h"
On 2017/06/20 14:59:50, danakj wrote:
> don't see this used in the header?

Removed.

https://codereview.chromium.org/2942283002/diff/60001/components/viz/service/...
File components/viz/service/display_compositor/gpu_display_provider.cc (right):

https://codereview.chromium.org/2942283002/diff/60001/components/viz/service/...
components/viz/service/display_compositor/gpu_display_provider.cc:34: return
buffer_factory ? buffer_factory->AsImageFactory() : nullptr;
On 2017/06/20 14:52:41, danakj wrote:
> nit: why not use channel_manager->gpu_image_factory()? This is reimplementing
it
> no?

It doesn't look like GpuChannelManager actually has that. There's one in
ui::GpuService, but we are not using that here.

https://codereview.chromium.org/2942283002/diff/60001/components/viz/service/...
File
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.h
(right):

https://codereview.chromium.org/2942283002/diff/60001/components/viz/service/...
components/viz/service/display_compositor/in_process_gpu_memory_buffer_manager.h:30:
std::unique_ptr<gfx::GpuMemoryBuffer> CreateGpuMemoryBuffer(
On 2017/06/20 18:22:07, reveman wrote:
> nit: why not public? it's public to anyone with a gpu::GpuMemoryBufferManager*
> to this instance so making it private here is confusing imo

Made these public.

[commit-bot: I haz the power, 2017-06-21 07:25:22]

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

[commit-bot: I haz the power, 2017-06-21 07:25:23]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-06-21 17:19:53]

I am going to go ahead and put it in the CQ. If we decide that it makes more
sense to move GpuMemoryBufferImpl out of client, we can do that as a follow up.

I will put up a CL to move the ServerGpuMemoryBufferManager into viz/host
shortly. Thanks for the reviews!

[sadrul, 2017-06-21 17:19:57]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2017-06-21 17:19:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, danakj@chromium.org, reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2942283002/#ps80001
(title: ".")

[commit-bot: I haz the power, 2017-06-21 17:20:19]

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-06-21 17:25:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498065597162050,
"parent_rev": "9300b027733c50637cdf6bc3668f432c598c7a68", "commit_rev":
"1f51373d28edbd397234c9ee7d60198f0a5e0189"}

[commit-bot: I haz the power, 2017-06-21 17:25:12]

Description was changed from

==========
viz: Use a simple GpuMemoryBufferManager implementation.

Introduce InProcessGpuMemoryBufferManager, which directly interacts
with a GpuMemoryBufferFactory to allocate memory used by the display
compositor.

Follow on steps: relocate ServerGpuMemoryBufferManager into viz/host.

BUG=733482
==========

to

==========
viz: Use a simple GpuMemoryBufferManager implementation.

Introduce InProcessGpuMemoryBufferManager, which directly interacts
with a GpuMemoryBufferFactory to allocate memory used by the display
compositor.

Follow on steps: relocate ServerGpuMemoryBufferManager into viz/host.

BUG=733482

Review-Url: https://codereview.chromium.org/2942283002
Cr-Commit-Position: refs/heads/master@{#481231}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f51373d28edbd397234c9ee7d60...
==========

[commit-bot: I haz the power, 2017-06-21 17:25:14]

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