Do glFinish before sending snapshot latency info to browser

Do glFinish before sending snapshot latency info to browser

On some GPU bots the swap was completing long before the contents got on
screen, so PrintWindow didn't get the correct window contents and some
tests were flaky. Triggering a glFinish before sending the latency info
for a snapshot request to the browser should be enough to ensure DWM
executes the Present before the snapshot is executed.

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

Review-Url: https://codereview.chromium.org/2888993002
Cr-Commit-Position: refs/heads/master@{#472749}
Committed: https://chromium.googlesource.com/chromium/src/+/5cfb0199ae5af577ca239feccd2d3c3f976ddac6

[jbauman, 2017-05-17 21:41:03]

Description was changed from

==========
Do glFinish before sending snapshot latency info to browser

On some GPU bots the swap was completing long before the contents got on
screen, so PrintWindow didn't get the correct window contents and some
tests were flaky. Triggering a glFinish before sending the latency info
for a snapshot request to the browser should be enough to

BUG=670098
==========

to

==========
Do glFinish before sending snapshot latency info to browser

On some GPU bots the swap was completing long before the contents got on
screen, so PrintWindow didn't get the correct window contents and some
tests were flaky. Triggering a glFinish before sending the latency info
for a snapshot request to the browser should be enough to

BUG=670098
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-05-17 21:41:09]

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

[commit-bot: I haz the power, 2017-05-17 21:42:14]

Dry run: CQ is trying da patch.

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

[jbauman, 2017-05-17 22:12:11]

Description was changed from

==========
Do glFinish before sending snapshot latency info to browser

On some GPU bots the swap was completing long before the contents got on
screen, so PrintWindow didn't get the correct window contents and some
tests were flaky. Triggering a glFinish before sending the latency info
for a snapshot request to the browser should be enough to

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

==========
Do glFinish before sending snapshot latency info to browser

On some GPU bots the swap was completing long before the contents got on
screen, so PrintWindow didn't get the correct window contents and some
tests were flaky. Triggering a glFinish before sending the latency info
for a snapshot request to the browser should be enough to ensure DWM
executes the Present before the snapshot is executed.

BUG=670098
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-05-18 00:14:01]

jbauman@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2017-05-18 01:02:11]

Awesome work tracking this down John!

Is it confirmed that this has (to some degree of confidence) decreased or
eliminated the flakiness of these tests?

LGTM; couple of comments.

https://codereview.chromium.org/2888993002/diff/1/gpu/ipc/service/child_windo...
File gpu/ipc/service/child_window_surface_win.cc (right):

https://codereview.chromium.org/2888993002/diff/1/gpu/ipc/service/child_windo...
gpu/ipc/service/child_window_surface_win.cc:169: glFinish();
Should there be a DCHECK that there's an OpenGL context current?

https://codereview.chromium.org/2888993002/diff/1/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/2888993002/diff/1/ui/gl/gl_surface.h#newcode224
ui/gl/gl_surface.h:224: // will capture the last swapped contents.
Should this indicate that a context must be current against this surface?

[jbauman, 2017-05-18 01:27:34]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2017-05-18 01:27:34]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2888993002/#ps20001
(title: "add current tests")

[jbauman, 2017-05-18 01:28:23]

On 2017/05/18 01:02:11, Ken Russell wrote:
> Awesome work tracking this down John!
> 
> Is it confirmed that this has (to some degree of confidence) decreased or
> eliminated the flakiness of these tests?

Yeah, I ran with this a number of times on the bots and I haven't seen issues,
whereas without it flaked maybe 25% of the time.
> 
> LGTM; couple of comments.
> 
>
https://codereview.chromium.org/2888993002/diff/1/gpu/ipc/service/child_windo...
> File gpu/ipc/service/child_window_surface_win.cc (right):
> 
>
https://codereview.chromium.org/2888993002/diff/1/gpu/ipc/service/child_windo...
> gpu/ipc/service/child_window_surface_win.cc:169: glFinish();
> Should there be a DCHECK that there's an OpenGL context current?
> 
> https://codereview.chromium.org/2888993002/diff/1/ui/gl/gl_surface.h
> File ui/gl/gl_surface.h (right):
> 
>
https://codereview.chromium.org/2888993002/diff/1/ui/gl/gl_surface.h#newcode224
> ui/gl/gl_surface.h:224: // will capture the last swapped contents.
> Should this indicate that a context must be current against this surface?

[commit-bot: I haz the power, 2017-05-18 01:28:24]

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-05-18 07:06:12]

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

[commit-bot: I haz the power, 2017-05-18 07:06:13]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbauman, 2017-05-18 07:08:18]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2017-05-18 07:08:53]

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-05-18 09:13:44]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495091298727400,
"parent_rev": "e268ff50e685b07e22e4c1d2e59d70e93c36a1a0", "commit_rev":
"5cfb0199ae5af577ca239feccd2d3c3f976ddac6"}

[commit-bot: I haz the power, 2017-05-18 09:13:57]

Description was changed from

==========
Do glFinish before sending snapshot latency info to browser

On some GPU bots the swap was completing long before the contents got on
screen, so PrintWindow didn't get the correct window contents and some
tests were flaky. Triggering a glFinish before sending the latency info
for a snapshot request to the browser should be enough to ensure DWM
executes the Present before the snapshot is executed.

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

==========
Do glFinish before sending snapshot latency info to browser

On some GPU bots the swap was completing long before the contents got on
screen, so PrintWindow didn't get the correct window contents and some
tests were flaky. Triggering a glFinish before sending the latency info
for a snapshot request to the browser should be enough to ensure DWM
executes the Present before the snapshot is executed.

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

Review-Url: https://codereview.chromium.org/2888993002
Cr-Commit-Position: refs/heads/master@{#472749}
Committed:
https://chromium.googlesource.com/chromium/src/+/5cfb0199ae5af577ca239feccd2d...
==========

[commit-bot: I haz the power, 2017-05-18 09:14:00]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5cfb0199ae5af577ca239feccd2d...