Changed GpuVSyncProvider to implement gfx::VSyncProvider

Changed GpuVSyncProvider to implement gfx::VSyncProvider

GpuVSyncProvider was introduced recently to generate D3D VSync
signals in the GPU process. This change makes GpuVSyncProvider an actual VSync provider that derives from gfx::VSyncProvider.
The class name is also changed to GpuVSyncProviderWin.

This will make it easy to integrate GpuVSyncProviderWin into
existing code by replacing the current version of the provider with
this new one without having to change much of the existing code.

GpuVSyncProviderWin ignores the existing mechanism for requesting
VSync parameter updates and instead uses IPC::MessageFilter to 
send and receive VSync related IPC messages directly. That bypasses
the main GPU thread and should allow us to achieve the best possible
latency.

BUG=467617
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

Review-Url: https://codereview.chromium.org/2681033011
Cr-Commit-Position: refs/heads/master@{#450784}
Committed: https://chromium.googlesource.com/chromium/src/+/eed2187ba7e9acb812754997400e56ff9107d8a0

[stanisc, 2017-02-10 01:49:50]

Description was changed from

==========
Changed GpuVSyncProvider to implement gfx::VSyncProvider

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

to

==========
Changed GpuVSyncProvider to implement gfx::VSyncProvider

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

[stanisc, 2017-02-10 01:53:33]

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

[commit-bot: I haz the power, 2017-02-10 01:54:05]

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

[stanisc, 2017-02-10 02:04:04]

Description was changed from

==========
Changed GpuVSyncProvider to implement gfx::VSyncProvider

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

==========
Changed GpuVSyncProvider to implement gfx::VSyncProvider

GpuVSyncProvider was introduced recently to generate D3D VSync
signals in the GPU process. This change makes GpuVSyncProvider an actual VSync
provider that derives from gfx::VSyncProvider.
The class name is also changed to GpuVSyncProviderWin.

This will make it easy to integrate GpuVSyncProviderWin into
existing code by replacing the current version of the provider with
this new one without having to change much of the existing code.

GpuVSyncProviderWin ignores the existing mechanism for requesting
VSync parameter updates and instead uses IPC::MessageFilter to 
send and receive VSync related IPC messages directly. That bypasses
the main GPU thread and should allow us to achieve the best possible
latency.

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

[commit-bot: I haz the power, 2017-02-10 02:04:23]

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

[commit-bot: I haz the power, 2017-02-10 02:04:24]

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

[stanisc, 2017-02-10 21:50:11]

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

[commit-bot: I haz the power, 2017-02-10 21:51:07]

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

[stanisc, 2017-02-10 22:02:08]

stanisc@chromium.org changed reviewers:
+ dcheng@chromium.org, jbauman@chromium.org, sunnyps@chromium.org

[stanisc, 2017-02-10 22:02:09]

This is partial change from https://codereview.chromium.org/2626413002/ that
addresses only GpuVSyncProvider on GPU side. The rest will be addressed in a
separate CL.

jbauman@ - please review most of the change. You've probably already seen most
of this as a part of the CL above.

dcheng@ - added as IPC security owner for gpu/ipc/common/gpu_messages.h

I added sunnyps@ as an optional reviewer because he suggested implementing
GpuVSyncProvider as a regular vsync provider to avoid the confusion between two
similar but differently implemented and handled types.

[commit-bot: I haz the power, 2017-02-10 22:47:56]

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

[commit-bot: I haz the power, 2017-02-10 22:47:57]

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

[stanisc, 2017-02-10 23:58:43]

Description was changed from

==========
Changed GpuVSyncProvider to implement gfx::VSyncProvider

GpuVSyncProvider was introduced recently to generate D3D VSync
signals in the GPU process. This change makes GpuVSyncProvider an actual VSync
provider that derives from gfx::VSyncProvider.
The class name is also changed to GpuVSyncProviderWin.

This will make it easy to integrate GpuVSyncProviderWin into
existing code by replacing the current version of the provider with
this new one without having to change much of the existing code.

GpuVSyncProviderWin ignores the existing mechanism for requesting
VSync parameter updates and instead uses IPC::MessageFilter to 
send and receive VSync related IPC messages directly. That bypasses
the main GPU thread and should allow us to achieve the best possible
latency.

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

==========
Changed GpuVSyncProvider to implement gfx::VSyncProvider

GpuVSyncProvider was introduced recently to generate D3D VSync
signals in the GPU process. This change makes GpuVSyncProvider an actual VSync
provider that derives from gfx::VSyncProvider.
The class name is also changed to GpuVSyncProviderWin.

This will make it easy to integrate GpuVSyncProviderWin into
existing code by replacing the current version of the provider with
this new one without having to change much of the existing code.

GpuVSyncProviderWin ignores the existing mechanism for requesting
VSync parameter updates and instead uses IPC::MessageFilter to 
send and receive VSync related IPC messages directly. That bypasses
the main GPU thread and should allow us to achieve the best possible
latency.

BUG=467617
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
==========

[stanisc, 2017-02-13 19:24:44]

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

[commit-bot: I haz the power, 2017-02-13 19:25:31]

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

[dcheng, 2017-02-13 21:10:39]

One thought: does it make sense to use Mojo here, since we're bypassing the gpu
thread anyway?

https://codereview.chromium.org/2681033011/diff/40001/gpu/ipc/service/gpu_vsy...
File gpu/ipc/service/gpu_vsync_provider_win.cc (right):

https://codereview.chromium.org/2681033011/diff/40001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:387:
message_filter_->Send(std::unique_ptr<IPC::Message>(
Nit: base::MakeUnique

[commit-bot: I haz the power, 2017-02-13 21:25:29]

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

[commit-bot: I haz the power, 2017-02-13 21:25:29]

Dry run: This issue passed the CQ dry run.

[stanisc, 2017-02-13 22:36:17]

On 2017/02/13 21:10:39, dcheng wrote:
> One thought: does it make sense to use Mojo here, since we're bypassing the
gpu
> thread anyway?
> 

I don't know how easy that would be. This plugs into GpuChannel via
GpuCommandBufferStub
and also uses GpuChannel to send/receive both messages on the client side. I
think it would
be better to convert entire GpuChannel / GpuCommandBuffer together.

>
https://codereview.chromium.org/2681033011/diff/40001/gpu/ipc/service/gpu_vsy...
> File gpu/ipc/service/gpu_vsync_provider_win.cc (right):
> 
>
https://codereview.chromium.org/2681033011/diff/40001/gpu/ipc/service/gpu_vsy...
> gpu/ipc/service/gpu_vsync_provider_win.cc:387:
> message_filter_->Send(std::unique_ptr<IPC::Message>(
> Nit: base::MakeUnique

Done.

[stanisc, 2017-02-14 00:42:35]

jbauman@ and sunnyps@, could you take a look when you have time?
You've seen most of this code already as a part of
https://codereview.chromium.org/2626413002/.

https://codereview.chromium.org/2681033011/diff/40001/gpu/ipc/service/gpu_vsy...
File gpu/ipc/service/gpu_vsync_provider_win.cc (right):

https://codereview.chromium.org/2681033011/diff/40001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:387:
message_filter_->Send(std::unique_ptr<IPC::Message>(
On 2017/02/13 21:10:39, dcheng wrote:
> Nit: base::MakeUnique

Done.

[jbauman, 2017-02-14 02:59:22]

lgtm with a couple of nits (and a question).

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
File gpu/ipc/service/gpu_vsync_provider_win.cc (right):

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:212: ((now - timestamp + interval / 8)
% interval + interval) % interval -
Can interval be 0 here, due to buggy drivers?

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:313: int32_t route_id_;
nit: const int32_t route_id_;

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:348: DCHECK(!message->is_sync());
Nit: move this dcheck up and remove the "|| message->is_sync()" from the if.

[dcheng, 2017-02-14 09:03:17]

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
File gpu/ipc/service/gpu_vsync_provider_unittest_win.cc (right):

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_unittest_win.cc:154: provider_.reset();
Are these resets() needed to ensure a particular destruction order? It would be
nice to add a comment, since this is a bit unusual.

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
File gpu/ipc/service/gpu_vsync_provider_win.cc (right):

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:339: CHECK(io_task_runner_->PostTask(
Is it necessary to CHECK() this? CHECK() should be pretty rare, per the logging
guidance in
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:355:
base::WeakPtr<ImageTransportSurfaceDelegate> delegate,
I'm curious why this needs to be a WeakPtr: we dereference it without checking,
and we don't store it. Maybe it can just be a raw pointer?

[Fady Samuel, 2017-02-14 09:09:37]

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

[Fady Samuel, 2017-02-14 09:09:38]

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/in_process_comm...
File gpu/ipc/in_process_command_buffer.cc (right):

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:1069: NOTREACHED();
This is used by Mus. Any reason not to update this too? Thanks!

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:1073: NOTREACHED();
This is used by Mus. Any reason not to update this too? Thanks!

[stanisc, 2017-02-14 19:55:25]

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

[commit-bot: I haz the power, 2017-02-14 19:56:01]

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

[stanisc, 2017-02-14 19:56:13]

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/in_process_comm...
File gpu/ipc/in_process_command_buffer.cc (right):

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:1069: NOTREACHED();
On 2017/02/14 09:09:38, Fady Samuel wrote:
> This is used by Mus. Any reason not to update this too? Thanks!

I am not sure GPU VSyncProvider could be used with mus. Take a look at
https://codereview.chromium.org/2626413002/diff/60001/content/browser/composi...
which is from a pending change that shows integration of this feature on the
compositor side. It specifically avoids turning GPU v-sync in the case of mus.
Whether or not this could be used with mus we should discuss.

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:1073: NOTREACHED();
On 2017/02/14 09:09:38, Fady Samuel wrote:
> This is used by Mus. Any reason not to update this too? Thanks!

See above.

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
File gpu/ipc/service/gpu_vsync_provider_unittest_win.cc (right):

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_unittest_win.cc:154: provider_.reset();
On 2017/02/14 09:03:17, dcheng wrote:
> Are these resets() needed to ensure a particular destruction order? It would
be
> nice to add a comment, since this is a bit unusual.

No, it should be OK to not destruct these objects here. I added these mostly for
symmetry with SetUp().
And apparently removing this code revealed a bug in
GpuVSyncWorker::CleanupAndStop() where it wouldn't stop if it was still enabled.

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
File gpu/ipc/service/gpu_vsync_provider_win.cc (right):

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:212: ((now - timestamp + interval / 8)
% interval + interval) % interval -
On 2017/02/14 02:59:22, jbauman wrote:
> Can interval be 0 here, due to buggy drivers?

Good question! Compositor code deals with zero vsync interval replacing it with
default 16.7 ms interval which means it is possible to receive zero interval
here. I am going to treat that the same as zero timestamp and just use "now"
time which in my experience is usually within 0.03-0.1 ms of the timestamp. That
should probably be good enough for that corner case.

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:313: int32_t route_id_;
On 2017/02/14 02:59:22, jbauman wrote:
> nit: const int32_t route_id_;

Done.

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:339: CHECK(io_task_runner_->PostTask(
On 2017/02/14 09:03:17, dcheng wrote:
> Is it necessary to CHECK() this? CHECK() should be pretty rare, per the
logging
> guidance in
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Not necessary. I've copied this as an example from other IPC filter
implementation. I've remove the CHECK.

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:348: DCHECK(!message->is_sync());
On 2017/02/14 02:59:22, jbauman wrote:
> Nit: move this dcheck up and remove the "|| message->is_sync()" from the if.

Done.

https://codereview.chromium.org/2681033011/diff/60001/gpu/ipc/service/gpu_vsy...
gpu/ipc/service/gpu_vsync_provider_win.cc:355:
base::WeakPtr<ImageTransportSurfaceDelegate> delegate,
On 2017/02/14 09:03:17, dcheng wrote:
> I'm curious why this needs to be a WeakPtr: we dereference it without
checking,
> and we don't store it. Maybe it can just be a raw pointer?

Good question. This pointer already comes from GpuCommandBufferStub as WeakPtr -
see ImageTransportSurface::CreateNativeSurface. Here is how I plan to integrate
GpuVSyncProviderWin -
https://codereview.chromium.org/2626413002/diff/60001/gpu/ipc/service/image_t....

It is true that CreateNativeSurface could resolve the weak pointer and pass a
raw pointer here. I decided to keep the weak pointer in case a future iteration
of this code would need to store the delegate. I feels more future-proof this
way.

[commit-bot: I haz the power, 2017-02-14 21:18:01]

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

[commit-bot: I haz the power, 2017-02-14 21:18:02]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-15 08:31:17]

ipc lgtm

[stanisc, 2017-02-15 19:28:14]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2017-02-15 19:28:15]

The patchset sent to the CQ was uploaded after l-g-t-m from jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2681033011/#ps80001
(title: "Addressed CR feedback")

[commit-bot: I haz the power, 2017-02-15 19:29:32]

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-02-15 19:56:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487186894952590,
"parent_rev": "5a91761d5cf889ee6990230b3add84cc48f1022a", "commit_rev":
"eed2187ba7e9acb812754997400e56ff9107d8a0"}

[commit-bot: I haz the power, 2017-02-15 19:56:51]

Description was changed from

==========
Changed GpuVSyncProvider to implement gfx::VSyncProvider

GpuVSyncProvider was introduced recently to generate D3D VSync
signals in the GPU process. This change makes GpuVSyncProvider an actual VSync
provider that derives from gfx::VSyncProvider.
The class name is also changed to GpuVSyncProviderWin.

This will make it easy to integrate GpuVSyncProviderWin into
existing code by replacing the current version of the provider with
this new one without having to change much of the existing code.

GpuVSyncProviderWin ignores the existing mechanism for requesting
VSync parameter updates and instead uses IPC::MessageFilter to 
send and receive VSync related IPC messages directly. That bypasses
the main GPU thread and should allow us to achieve the best possible
latency.

BUG=467617
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

==========
Changed GpuVSyncProvider to implement gfx::VSyncProvider

GpuVSyncProvider was introduced recently to generate D3D VSync
signals in the GPU process. This change makes GpuVSyncProvider an actual VSync
provider that derives from gfx::VSyncProvider.
The class name is also changed to GpuVSyncProviderWin.

This will make it easy to integrate GpuVSyncProviderWin into
existing code by replacing the current version of the provider with
this new one without having to change much of the existing code.

GpuVSyncProviderWin ignores the existing mechanism for requesting
VSync parameter updates and instead uses IPC::MessageFilter to 
send and receive VSync related IPC messages directly. That bypasses
the main GPU thread and should allow us to achieve the best possible
latency.

BUG=467617
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

Review-Url: https://codereview.chromium.org/2681033011
Cr-Commit-Position: refs/heads/master@{#450784}
Committed:
https://chromium.googlesource.com/chromium/src/+/eed2187ba7e9acb812754997400e...
==========

[commit-bot: I haz the power, 2017-02-15 19:56:52]

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