exo: Directly create GpuMemoryBufferImpl when a handle is available.

exo: Directly create GpuMemoryBufferImpl when a handle is available.

Instead of going through the GpuMemoryBufferManager, directly allocate
the right kind of GpuMemoryBuffer. One of the changes with the existing
code is that the buffer is currently always created on the IO thread.
With this change, that is no longer enforced. This thus avoids thread
hopping during buffer allocation, and some complex code in
BrowserGpuMemoryBufferManager. This is also the only use of
GpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle(). So this CL
removes that (and the corresponding various implementations).

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=jam@ for top-level owner for updating for API change.

Committed: https://crrev.com/00a4065140b523047346d4b2a593cb6fad1a4409
Cr-Commit-Position: refs/heads/master@{#438435}

[sadrul, 2016-12-13 18:52:08]

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

[commit-bot: I haz the power, 2016-12-13 18:52:39]

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

[sadrul, 2016-12-13 19:04:38]

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

[commit-bot: I haz the power, 2016-12-13 19:05:02]

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

[sadrul, 2016-12-13 19:15:49]

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

[sadrul, 2016-12-13 19:15:50]

Hi! This changes how exo allocates some of the gpu memory buffer: instead of
going through the GpuMemoryBufferManager, it directly allocates the
GpuMemoryBufferImpl. One of the changes with the existing code is that the
GMBImpl instance currently is always created on the IO thread. With this change,
that is no longer enforced. This thus avoids thread-hopping, and some complex
code in BrowserGpuMemoryBufferManager, but I don't know if this is going to
break something.

The motivation is to simplify the code a little bit (and remove
GpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle()) if possible. PTAL.
Thanks!

[commit-bot: I haz the power, 2016-12-13 20:22:11]

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

[commit-bot: I haz the power, 2016-12-13 20:22:11]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-12-13 21:30:53]

https://codereview.chromium.org/2572703004/diff/20001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/2572703004/diff/20001/components/exo/display....
components/exo/display.cc:86: auto gpu_memory_buffer =
gpu::GpuMemoryBufferImpl::CreateFromHandle(
Can you use gpu::GpuMemoryBufferImplOzoneNativePixmap here instead?

nit: what's the return type now? can you remove auto keep having that be clear?

https://codereview.chromium.org/2572703004/diff/20001/components/exo/shared_m...
File components/exo/shared_memory.cc (right):

https://codereview.chromium.org/2572703004/diff/20001/components/exo/shared_m...
components/exo/shared_memory.cc:73: auto gpu_memory_buffer =
gpu::GpuMemoryBufferImpl::CreateFromHandle(
ditto. avoid auto and use gpu::GpuMemoryBufferImplSharedMemory directly

[sadrul, 2016-12-14 01:29:45]

Description was changed from

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

...

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

to

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

...

BUG=...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sadrul, 2016-12-14 01:29:45]

Description was changed from

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

...

BUG=...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

...

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

[sadrul, 2016-12-14 01:29:57]

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

[commit-bot: I haz the power, 2016-12-14 01:30:49]

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

[sadrul, 2016-12-14 01:32:54]

Description was changed from

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

...

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

to

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

Instead of going through the GpuMemoryBufferManager, directly allocate
the right kind of GpuMemoryBuffer. One of the changes with the existing
code is that the buffer is currently always created on the IO thread.
With this change, that is no longer enforced. This thus avoids thread
hopping during buffer allocation, and some complex code in
BrowserGpuMemoryBufferManager. This is also the only use of
GpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle(). So this CL
removes that (and the corresponding various implementations).

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

[sadrul, 2016-12-14 01:41:30]

https://codereview.chromium.org/2572703004/diff/20001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/2572703004/diff/20001/components/exo/display....
components/exo/display.cc:86: auto gpu_memory_buffer =
gpu::GpuMemoryBufferImpl::CreateFromHandle(
On 2016/12/13 21:30:53, reveman wrote:
> Can you use gpu::GpuMemoryBufferImplOzoneNativePixmap here instead?
> 
> nit: what's the return type now? can you remove auto keep having that be
clear?

Done.

https://codereview.chromium.org/2572703004/diff/20001/components/exo/shared_m...
File components/exo/shared_memory.cc (right):

https://codereview.chromium.org/2572703004/diff/20001/components/exo/shared_m...
components/exo/shared_memory.cc:73: auto gpu_memory_buffer =
gpu::GpuMemoryBufferImpl::CreateFromHandle(
On 2016/12/13 21:30:53, reveman wrote:
> ditto. avoid auto and use gpu::GpuMemoryBufferImplSharedMemory directly

Done.

[reveman, 2016-12-14 02:21:00]

lgtm % null callback nit

https://codereview.chromium.org/2572703004/diff/40001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/2572703004/diff/40001/components/exo/display....
components/exo/display.cc:35: void DoNothing(const gpu::SyncToken& sync) {}
As this callback is going away completely in the near future. How about we allow
it to be null for now and add an if-statement to the GpuMemoryBufferImpl dtor to
avoid these noops?

[commit-bot: I haz the power, 2016-12-14 02:48:05]

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

[commit-bot: I haz the power, 2016-12-14 02:48:05]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-12-14 03:01:09]

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

[commit-bot: I haz the power, 2016-12-14 03:02:26]

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

[sadrul, 2016-12-14 03:06:00]

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

[sadrul, 2016-12-14 03:06:01]

+jam@ as top-level owner to stamp (will TBR since API owner has lgtm'ed, and the
rest of the code is to update for the API change)

https://codereview.chromium.org/2572703004/diff/40001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/2572703004/diff/40001/components/exo/display....
components/exo/display.cc:35: void DoNothing(const gpu::SyncToken& sync) {}
On 2016/12/14 02:20:59, reveman wrote:
> As this callback is going away completely in the near future. How about we
allow
> it to be null for now and add an if-statement to the GpuMemoryBufferImpl dtor
to
> avoid these noops?

Yep, done.

[commit-bot: I haz the power, 2016-12-14 04:11:03]

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

[commit-bot: I haz the power, 2016-12-14 04:11:05]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-12-14 04:53:48]

Description was changed from

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

Instead of going through the GpuMemoryBufferManager, directly allocate
the right kind of GpuMemoryBuffer. One of the changes with the existing
code is that the buffer is currently always created on the IO thread.
With this change, that is no longer enforced. This thus avoids thread
hopping during buffer allocation, and some complex code in
BrowserGpuMemoryBufferManager. This is also the only use of
GpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle(). So this CL
removes that (and the corresponding various implementations).

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

to

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

Instead of going through the GpuMemoryBufferManager, directly allocate
the right kind of GpuMemoryBuffer. One of the changes with the existing
code is that the buffer is currently always created on the IO thread.
With this change, that is no longer enforced. This thus avoids thread
hopping during buffer allocation, and some complex code in
BrowserGpuMemoryBufferManager. This is also the only use of
GpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle(). So this CL
removes that (and the corresponding various implementations).

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=jam@ for top-level owner for updating for API change.
==========

[sadrul, 2016-12-14 04:54:20]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-12-14 04:54:20]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2572703004/#ps60001
(title: "null callback")

[commit-bot: I haz the power, 2016-12-14 04:54:37]

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-12-14 04:59:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481691260228630,
"parent_rev": "3cc1a660f81a89f2401a65df6fe167b8d5a13804", "commit_rev":
"8b48d7ef55252b7f165a95c57faa7a20ee6c90b1"}

[commit-bot: I haz the power, 2016-12-14 04:59:34]

Description was changed from

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

Instead of going through the GpuMemoryBufferManager, directly allocate
the right kind of GpuMemoryBuffer. One of the changes with the existing
code is that the buffer is currently always created on the IO thread.
With this change, that is no longer enforced. This thus avoids thread
hopping during buffer allocation, and some complex code in
BrowserGpuMemoryBufferManager. This is also the only use of
GpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle(). So this CL
removes that (and the corresponding various implementations).

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=jam@ for top-level owner for updating for API change.
==========

to

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

Instead of going through the GpuMemoryBufferManager, directly allocate
the right kind of GpuMemoryBuffer. One of the changes with the existing
code is that the buffer is currently always created on the IO thread.
With this change, that is no longer enforced. This thus avoids thread
hopping during buffer allocation, and some complex code in
BrowserGpuMemoryBufferManager. This is also the only use of
GpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle(). So this CL
removes that (and the corresponding various implementations).

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=jam@ for top-level owner for updating for API change.

Review-Url: https://codereview.chromium.org/2572703004
==========

[commit-bot: I haz the power, 2016-12-14 04:59:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-14 05:02:22]

Description was changed from

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

Instead of going through the GpuMemoryBufferManager, directly allocate
the right kind of GpuMemoryBuffer. One of the changes with the existing
code is that the buffer is currently always created on the IO thread.
With this change, that is no longer enforced. This thus avoids thread
hopping during buffer allocation, and some complex code in
BrowserGpuMemoryBufferManager. This is also the only use of
GpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle(). So this CL
removes that (and the corresponding various implementations).

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=jam@ for top-level owner for updating for API change.

Review-Url: https://codereview.chromium.org/2572703004
==========

to

==========
exo: Directly create GpuMemoryBufferImpl when a handle is available.

Instead of going through the GpuMemoryBufferManager, directly allocate
the right kind of GpuMemoryBuffer. One of the changes with the existing
code is that the buffer is currently always created on the IO thread.
With this change, that is no longer enforced. This thus avoids thread
hopping during buffer allocation, and some complex code in
BrowserGpuMemoryBufferManager. This is also the only use of
GpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle(). So this CL
removes that (and the corresponding various implementations).

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=jam@ for top-level owner for updating for API change.

Committed: https://crrev.com/00a4065140b523047346d4b2a593cb6fad1a4409
Cr-Commit-Position: refs/heads/master@{#438435}
==========

[commit-bot: I haz the power, 2016-12-14 05:02:23]

Patchset 4 (id:??) landed as
https://crrev.com/00a4065140b523047346d4b2a593cb6fad1a4409
Cr-Commit-Position: refs/heads/master@{#438435}