Add GpuMemoryBuffer support for DXGI handles.

Add GpuMemoryBuffer support for DXGI handles.

These can only be presented using overlays for now - GL isn't supported.
Also, there's no code to create them. They can only be passed in from
other components (e.g. the CDM).

BUG=527890, 729238
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

[jbauman, 2017-07-06 23:33:33]

Description was changed from

==========
Add GpuMemoryBuffer support for DXGI handles.

These can only be presented using overlays for now - GL isn't supported.
Also, there's no code to create them. They can only be passed in from
other components (e.g. the CDM).


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

to

==========
Add GpuMemoryBuffer support for DXGI handles.

These can only be presented using overlays for now - GL isn't supported.
Also, there's no code to create them. They can only be passed in from
other components (e.g. the CDM).


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[jbauman, 2017-07-06 23:33:39]

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

[commit-bot: I haz the power, 2017-07-06 23:33:46]

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-07-06 23:44:57]

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

[commit-bot: I haz the power, 2017-07-06 23:44:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbauman, 2017-07-06 23:53:10]

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

[commit-bot: I haz the power, 2017-07-06 23:53:20]

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-07-07 00:33:49]

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

[commit-bot: I haz the power, 2017-07-07 00:33:50]

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

[jbauman, 2017-07-08 01:00:06]

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

[commit-bot: I haz the power, 2017-07-08 01:00:24]

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-07-08 03:18:15]

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

[commit-bot: I haz the power, 2017-07-08 03:18:16]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-07-12 00:26:03]

Description was changed from

==========
Add GpuMemoryBuffer support for DXGI handles.

These can only be presented using overlays for now - GL isn't supported.
Also, there's no code to create them. They can only be passed in from
other components (e.g. the CDM).


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Add GpuMemoryBuffer support for DXGI handles.

These can only be presented using overlays for now - GL isn't supported.
Also, there's no code to create them. They can only be passed in from
other components (e.g. the CDM).

BUG=527890,729238
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[jbauman, 2017-07-12 00:27:19]

jbauman@chromium.org changed reviewers:
+ dcheng@chromium.org, reveman@chromium.org

[dcheng, 2017-07-12 08:46:17]

dcheng@chromium.org changed reviewers:
+ yzshen@chromium.org

[dcheng, 2017-07-12 08:46:20]

+yzshen for the shared memory question

https://codereview.chromium.org/2970353002/diff/20001/gpu/ipc/service/gpu_mem...
File gpu/ipc/service/gpu_memory_buffer_factory_win.cc (right):

https://codereview.chromium.org/2970353002/diff/20001/gpu/ipc/service/gpu_mem...
gpu/ipc/service/gpu_memory_buffer_factory_win.cc:48:
scoped_refptr<gl::GLImageDXGIHandle> image(new gl::GLImageDXGIHandle(
Nit: auto image = base::MakeRefCounted<gl::GLImageDXGIHandle>(...);

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
File ui/gfx/mojo/buffer_types.mojom (right):

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
ui/gfx/mojo/buffer_types.mojom:47: DXGI_HANDLE,
Do you mind adding a TODO to make these Windows only? It can point to
https://crbug.com/676224.

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
File ui/gfx/mojo/buffer_types_struct_traits.cc (right):

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
ui/gfx/mojo/buffer_types_struct_traits.cc:91: return
mojo::MakeScopedHandle(mojo_handle);
Maybe this can just use WrapSharedMemoryHandle?

One thing I'm not sure about though... it's not clear to me if this is safe. The
documentation for ScopedHandle says it takes ownership, and it certainly /looks/
like ownership is transferred here. However, Mojo currently calls each
serializer 2x (this is https://crbug.com/681080) and it's not clear to me what
happens if we transfer ownership twice...

That being said, the input parameter is const T& which would imply that
ownership is not actually transferred... so I'm wondering how this actually
works.

(there is some other code that seems to have this issue: for example,
https://cs.chromium.org/chromium/src/media/gpu/mojo/jpeg_decoder_typemap_trai...)

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
File ui/gfx/mojo/buffer_types_struct_traits.h (right):

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
ui/gfx/mojo/buffer_types_struct_traits.h:171: return
gfx::mojom::GpuMemoryBufferType::DXGI_HANDLE;
Should non-Windows platforms NOTREACHED() this case?

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
ui/gfx/mojo/buffer_types_struct_traits.h:193: *out =
gfx::GpuMemoryBufferType::DXGI_HANDLE;
Though this can't be notreached, since this received over ipc

[reveman, 2017-07-12 15:04:14]

lg, mostly nits

https://codereview.chromium.org/2970353002/diff/20001/gpu/ipc/service/gpu_mem...
File gpu/ipc/service/gpu_memory_buffer_factory_win.cc (right):

https://codereview.chromium.org/2970353002/diff/20001/gpu/ipc/service/gpu_mem...
gpu/ipc/service/gpu_memory_buffer_factory_win.cc:45: if (handle.type !=
gfx::DXGI_HANDLE)
DCHECK_EQ(handle.type, gfx::DXGI_HANDLE)?

https://codereview.chromium.org/2970353002/diff/20001/gpu/ipc/service/gpu_mem...
gpu/ipc/service/gpu_memory_buffer_factory_win.cc:47: base::win::ScopedHandle
raw_handle(handle.handle.GetHandle());
optional nit: s/raw_handle/scoped_handle/ or maybe just avoid the temporary
variable and base::win::ScopedHandle(handle.handle.GetHandle()) below in ctor
call

https://codereview.chromium.org/2970353002/diff/20001/gpu/ipc/service/gpu_mem...
File gpu/ipc/service/gpu_memory_buffer_factory_win.h (right):

https://codereview.chromium.org/2970353002/diff/20001/gpu/ipc/service/gpu_mem...
gpu/ipc/service/gpu_memory_buffer_factory_win.h:14: class GPU_EXPORT
GpuMemoryBufferFactoryWin : public GpuMemoryBufferFactory,
s/GpuMemoryBufferFactoryWin/GpuMemoryBufferFactoryDXGI/ as this should be named
after the buffer type and not the platform to be consistent with other buffer
types. This is so it's not too confusing to have multiple buffer types per
platform

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/gpu_memory_buffe...
ui/gfx/gpu_memory_buffer.h:36: DXGI_HANDLE,
DXGI_BUFFER? as this should represent the storage and not what is used for cross
process sharing.

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
File ui/gfx/mojo/buffer_types_struct_traits.h (right):

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
ui/gfx/mojo/buffer_types_struct_traits.h:171: return
gfx::mojom::GpuMemoryBufferType::DXGI_HANDLE;
On 2017/07/12 at 08:46:19, dcheng wrote:
> Should non-Windows platforms NOTREACHED() this case?

Then similar NOTREACHED() for other buffer types too to be consistent. As is is
fine with me.

[dcheng, 2017-07-12 21:58:46]

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
File ui/gfx/mojo/buffer_types_struct_traits.cc (right):

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
ui/gfx/mojo/buffer_types_struct_traits.cc:91: return
mojo::MakeScopedHandle(mojo_handle);
On 2017/07/12 08:46:19, dcheng wrote:
> Maybe this can just use WrapSharedMemoryHandle?
> 
> One thing I'm not sure about though... it's not clear to me if this is safe.
The
> documentation for ScopedHandle says it takes ownership, and it certainly
/looks/
> like ownership is transferred here. However, Mojo currently calls each
> serializer 2x (this is https://crbug.com/681080) and it's not clear to me what
> happens if we transfer ownership twice...
> 
> That being said, the input parameter is const T& which would imply that
> ownership is not actually transferred... so I'm wondering how this actually
> works.
> 
> (there is some other code that seems to have this issue: for example,
>
https://cs.chromium.org/chromium/src/media/gpu/mojo/jpeg_decoder_typemap_trai...)

OK I asked rockot@ and it turns out that Mojo special-cases handle serializers
to only be invoked once.

Meh.

[yzshen1, 2017-07-12 23:38:49]

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
File ui/gfx/mojo/buffer_types_struct_traits.cc (right):

https://codereview.chromium.org/2970353002/diff/20001/ui/gfx/mojo/buffer_type...
ui/gfx/mojo/buffer_types_struct_traits.cc:91: return
mojo::MakeScopedHandle(mojo_handle);
On 2017/07/12 21:58:46, dcheng wrote:
> On 2017/07/12 08:46:19, dcheng wrote:
> > Maybe this can just use WrapSharedMemoryHandle?
> > 
> > One thing I'm not sure about though... it's not clear to me if this is safe.
> The
> > documentation for ScopedHandle says it takes ownership, and it certainly
> /looks/
> > like ownership is transferred here. However, Mojo currently calls each
> > serializer 2x (this is https://crbug.com/681080) and it's not clear to me
what
> > happens if we transfer ownership twice...
> > 
> > That being said, the input parameter is const T& which would imply that
> > ownership is not actually transferred... so I'm wondering how this actually
> > works.
> > 
> > (there is some other code that seems to have this issue: for example,
> >
>
https://cs.chromium.org/chromium/src/media/gpu/mojo/jpeg_decoder_typemap_trai...)
> 
> OK I asked rockot@ and it turns out that Mojo special-cases handle serializers
> to only be invoked once.
> 
> Meh.

Sorry I didn't see this comment earlier.
Yes, getters of object types (struct/union/map/string/array) are called twice
because their size needs to be calculated. Primitive types / handles are only
called once.

Btw, Ken and I chatted about the calculate-before-allocate v.s.
grow-if-necessary issue again. We both think it is good to try out the
grow-if-necessary approach. (All credits go to Ken because he plans to do the
work). If that finally happens, we could make sure each getter is called once,
and a lot of custom context usage could be removed.

[jbauman, 2017-08-11 22:59:37]

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

[commit-bot: I haz the power, 2017-08-11 22:59:45]

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-08-11 23:31:34]

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

[commit-bot: I haz the power, 2017-08-11 23:31:35]

Dry run: Try jobs failed on following builders:
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)