Of course one way to solve this is send that particular latency info back to the host, but I'm wondering why this latency info is needed at all. How come everywhere else that we want to capture screenshot we just do it but in this particular case we have to wait for some latency info? 

It looks like RWHI just needs to know when the GPU swap happens for a renderer CompositorFrame with a particular sequence_number. That information is available in LatencyInfo so the screenshot is piggybacked off it.

Why does it need to know this though?

If you have a fallback SurfaceId can't you just take a screenshot? I can't find anywhere else in the code base that we wait for that latency info before taking a screenshot.

It looks like the code was written to use OS screen capture, so it had to wait until the GPU swap so it knows the content is on screen. It looks like aura implementation is wrong because it CopyOutputRequests which it adds to a later frame, especially on Windows with that extra 165ms of delay. The mac implementation still uses OS screen capture though.

That's what I guessed too. So you also agree that this is unnecessary complexity, at least for aura platforms?

It seems like something is wrong with aura. We should also investigate  crbug.com/702023  and figure if we should be using OS screen capture on Windows after all?

I see. So I guess using OS snapshot is preferred for tests (Just in case the compositor generates the right output but it doesn't actually show up on the screen)? I think we can just make it work by forwarding the necessary latency info from viz to host.

 Issue 718288  has been merged into this issue.

Yes, for end-to-end pixel tests in Chrome it is absolutely necessary to use OS-level screenshot mechanisms, and to know when the requested frame has reached the screen.

Various Telemetry-based pixel tests that run on the GPU bots rely on this working. There is also a new SnapshotBrowserTest which attempts to stress test this mechanism. Simply deleting the sequence number from LatencyInfo causes one of these tests to reliably fail.

Is the OS-level screenshot used on Windows? I see that https://crrev.com/2752373002 added GrabHwndSnapshot() for Windows and updated RenderWidgetHostImpl::OnGpuSwapBuffersCompletedInternal() to add some delay before taking a screenshot on Windows. What I don't see is anything using GrabHwndSnapshot().

https://cs.chromium.org/search/?q=GrabHwndSnapshot&sq=package:chromium&type=cs

I'm not 100% sure. In recent code investigations I think we found that the on-screen snapshot was broken on Aura platforms. It's definitely working on macOS though, and Android I think too.

+miu are you working on this?

@kenrb: Is it possible to run tries on the gpu bots? Are the various _optional_gpu_tests_rel bots the ones I am looking for?

@sadrul did you mean me?

Yes, you can run tryjobs on the GPU bots.

linux_chromuim_rel_ng, mac_chromium_rel_ng and win7_chromium_rel_ng all run the base set of GPU tests, including pixel tests on real GPU bots.

linux_optional_gpu_tests_rel, mac_optional_gpu_tests_rel, win_optional_gpu_tests_rel, and android_optional_gpu_tests_rel run larger GPU test suites, like the WebGL 2.0 conformance tests, which are too large to run on every tryjob.

However, the SnapshotBrowserTest is a better stress test of the screenshot functionality -- it almost always hangs if the sequence number is removed from LatencyInfo, for example. That one runs on those trybots as part of browser_tests and doesn't run on the GPU -- it doesn't need to.

Please tell me if you need more help.

I meant kbr@, yes, sorry! (I don't know why I keep making that mistake!)

Thank you! I am trying to make this a little simpler if possible, and wanted to make sure I can run the tests. I will let you know if I get stuck.

Reply to #c15: No, I'm not working on this. Though, this is all loosely related to  bug 812059  and  bug 759310 .

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/478e85a9d6185aaa1f9d059ed889986be6a0a532

commit 478e85a9d6185aaa1f9d059ed889986be6a0a532
Author: Saman Sami <samans@chromium.org>
Date: Thu Apr 19 23:51:23 2018

Remove RenderWidgetHostLatencyTracker::OnSwapCompositorFrame

This method adds the right component id to snapshot-related latency
info, We can just send the right component id to the renderer so that
a correction browser-side is not necessary.

Test: Lots of pixel tests take screenshots. Also tested
Page.captureScreenshot manually.

Bug:  810037 ,  775030 
Change-Id: I61242ad63c5af8975331cb405fc629ce19ef92f8
Reviewed-on: https://chromium-review.googlesource.com/1019389
Commit-Queue: Saman Sami <samans@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552204}
[modify] https://crrev.com/478e85a9d6185aaa1f9d059ed889986be6a0a532/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
[modify] https://crrev.com/478e85a9d6185aaa1f9d059ed889986be6a0a532/content/browser/renderer_host/input/render_widget_host_latency_tracker.h
[modify] https://crrev.com/478e85a9d6185aaa1f9d059ed889986be6a0a532/content/browser/renderer_host/render_widget_host_impl.cc

 Issue 784941  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a02a568d368576d4cfda1056c600e73f33e2e320

commit a02a568d368576d4cfda1056c600e73f33e2e320
Author: Saman Sami <samans@chromium.org>
Date: Tue Apr 24 20:59:35 2018

viz: Make Page.captureScreenshot work

Page.captureScreenshot blocks on arrival of a LatencyInfo with
BROWSER_SNAPSHOT_FRAME_NUMBER_COMPONENT component. However, the
browse process does not receive latency info when Viz is enabled.
Plumb latency info with BROWSER_SNAPSHOT_FRAME_NUMBER_COMPONENT
component from viz to the browser process.

TBR=boliu@chromium.org

Bug:  810037 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ia81b0644f29a37059a7b3e0303074fb0db340757
Reviewed-on: https://chromium-review.googlesource.com/1020301
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553271}
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/android_webview/browser/surfaces_instance.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/cc/test/fake_output_surface_client.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/display/display.cc
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/display/display.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/display/display_client.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/display/display_unittest.cc
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/display/output_surface_client.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/display_embedder/gl_output_surface.cc
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/display_embedder/software_output_surface.cc
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/frame_sinks/direct_layer_tree_frame_sink.cc
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/frame_sinks/direct_layer_tree_frame_sink.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/service/frame_sinks/root_compositor_frame_sink_impl.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/test/BUILD.gn
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/test/mock_display_client.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/components/viz/test/test_layer_tree_frame_sink.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/content/browser/compositor/in_process_display_client.cc
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/content/browser/compositor/in_process_display_client.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/content/renderer/android/synchronous_layer_tree_frame_sink.h
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/services/viz/privileged/interfaces/compositing/BUILD.gn
[modify] https://crrev.com/a02a568d368576d4cfda1056c600e73f33e2e320/services/viz/privileged/interfaces/compositing/display_private.mojom

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fbab1626c83e8303db94fef2903c73aa6fb95be3

commit fbab1626c83e8303db94fef2903c73aa6fb95be3
Author: Saman Sami <samans@chromium.org>
Date: Thu Apr 26 20:45:37 2018

Decouple RenderWidgetHostLatencyTracker and ui::LatencyTracker

Currently RenderWidgetHostLatencyTracker derives from
ui::LatencyTracker, presumably because we want
RenderWidgetHostLatencyTracker to inherit OnGpuSwapBuffersCompleted.
However, this method should be called by viz. I can't think of any
other reason these two classes should be related, so decouple these,
two classes and make OutputSurfaces call OnGpuSwapBuffersCompleted.
Probably we should move ui::LatencyTracker to components/viz as well
and rename it.

Bug:  810037 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I4351da79dfb49c8e53f33f98a444aad248028c13
Reviewed-on: https://chromium-review.googlesource.com/1026459
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554141}
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/components/viz/service/display_embedder/gl_output_surface.cc
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/components/viz/service/display_embedder/software_output_surface.cc
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/compositor/gpu_browser_compositor_output_surface.cc
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/compositor/gpu_browser_compositor_output_surface.h
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/compositor/offscreen_browser_compositor_output_surface.cc
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/compositor/offscreen_browser_compositor_output_surface.h
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/compositor/software_browser_compositor_output_surface.cc
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/compositor/software_browser_compositor_output_surface.h
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/renderer_host/input/render_widget_host_latency_tracker.h
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/ui/latency/latency_tracker.cc
[modify] https://crrev.com/fbab1626c83e8303db94fef2903c73aa6fb95be3/ui/latency/latency_tracker.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b8aedbdc195d908871669fe9446ae20631a30227

commit b8aedbdc195d908871669fe9446ae20631a30227
Author: kylechar <kylechar@chromium.org>
Date: Thu Apr 26 20:53:58 2018

viz: Enable screenshot viz_content_browsertests.

Enable some disabled tests in viz_content_browsertests that relied on
DevTools Page.captureScreenshot. That call now works so the tests can be
re-enabled.

Bug:  785308 ,  810037 
Change-Id: Ia249682d8395ac18873b4c2b326e63ffc567d68f
Reviewed-on: https://chromium-review.googlesource.com/1030974
Reviewed-by: Saman Sami <samans@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554147}
[modify] https://crrev.com/b8aedbdc195d908871669fe9446ae20631a30227/testing/buildbot/filters/viz.content_browsertests.filter