As discussed with junov@ on Hangouts, we're going to attempt to get the GPU pixel tests running against android_webview_instrumentation_apk. If this works then we can at least get pixel_test running on the GPU bots in that mode.

Talking with boliu@ and the DevTools folks about this. We're trying to figure out whether it's expected that this call to SynchronousCopyContents should work:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc?rcl=54b2402cbdf82422ad9ae5b798acfc6f4b751a23&l=812

and in particular, allow for example GPU pixel tests that contain WebGL-rendered canvases to work.

fsamuel, sadrul, any feedback you have on this would be appreciated.

chrome (on android) uses this:
https://cs.chromium.org/chromium/src/components/viz/service/frame_sinks/compositor_frame_sink_support.h?rcl=b517c095662472ec36a323dd842a1ff46b3c603d&l=135

I think that does a composite of that viz::Surface into its own render surface and reads it back? And I think it does that as part of a viz::Display real composite?


For real webview gpu pixel tests, webview has to do that as well. With the caveat that webview's viz::Display isn't not on the UI thread, and isn't always accessible. Need to implement quite a bit of plumbing. And need to use some heuristic to decide between using the existing software path and the gpu readback path.

This uses DemandDrawSw under the good. Perhaps we can switch to DemandDrawHw to capture gpu-rendered stuff?

Actually I think that's the wrong entry point. I'm pretty sure we come in via DevTools here, to PageHandler::CaptureScreenshot:
https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/page_handler.cc?sq=package:chromium&dr=CSs&l=516

and from there go to RenderWidgetHostImpl::GetSnapshotFromBrowser:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_impl.cc?sq=package:chromium&dr=CSs&l=1632

Still digging to see what's at the bottom of that.

Just to document what we're observing today:

Teied running gpu pixel tests with this command:
 ./content/test/gpu/run_gpu_integration_test.py pixel --browser=android-webview-instrumentation

I see all the tests running on the phone, but they all fail in the test harness due to lack of a functional telemetry readback path for android webview.

RenderWidgetHostImpl::GetSnapshotFromBrowser ends up calling RWHView::CopyFromSurface as well: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_impl.cc?rcl=60f840e7e212c79d76e5e7fa32667214e050e20b&l=2568

All of the desktop platforms, and Chromium on Android, go through RenderWidgetHostImpl::GetSnapshotFromBrowser and RWHView::CopyFromSurface. I don't see where RWHView::CopyFromSurface was called in the other code path (SynchronousCopyContents, in render_widget_host_view_android.cc) pointed out above.

Other way around. RenderWidgetHostViewAndroid::CopyFromSurface calls SynchronousCopyContents: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc?rcl=54b2402cbdf82422ad9ae5b798acfc6f4b751a23&l=812

Raising the priority to 1 because the current situation exposes a whole bunch of important apps (e.g. Google Search App) to unacceptable risks of regression due to the absence of automated rendering tests.  

Also, now that we have a clearer idea of what needs to be done, this issue needs to be reassigned to someone who is familiar with the right parts of the graphics stack.  Tentatively assigning to boliu@. If you're not the right person, you probably know who is.

Bo, if necessary, can we plumb a different readback path to RenderWidgetHostImpl::GetSnapshotFromBrowser on Android so that we just read back the GPU-rendered results? It doesn't have to be pretty or efficient, it just has to work.

Chrome on android already does a GPU readback for CopyFromSurface (afaik that's how viz::CopyOutputRequest works?). It's only webview that does weird software draw thing.

My proposal is webview should use CopyOutputRequest too. Does that match your expectation? It's not trivial to hook up CopyOutputRequest, and I need to come up with a concrete design first before handing it off to someone else to do the work.

Yes, having WebView use CopyOutputRequest in RenderWidgetHostViewAndroid::CopyFromSurface sounds good for this use case.

I'm not sure of the design decisions which led to the current code so would request you sync up with compositor and viz folks like danakj@, sadrul@ and kylechar@ for a sanity check that this will work for WebView.

Out of curiosity, what's different about webview that makes CopyOutputRequests difficult. Certainly we should do that if possible.

Webview may or may not have a GL viz::Display, and the viz::Display may or may not be accessible whenever whenever we want. And webview doesn't know these things upfront. So will need some sort of heuristic to decide which path to use.

Also the viz::Display is not on the UI thread. That just requires adding plumbing, though still not trivial work.

Was looking at this for a bit. Adding an API that's equivalent to DelegatedFrameHostAndroid::CopyFromCompositingSurface is probably not too difficult. The request can just live in a SynchronousCompositor::Frame. We need to be careful that when Frames are dropped, the request should be queued on the next frame. Then the result can be posted back to UI thread.

For deciding between doing readback vs a software draw, I think we can be pretty aggressive here in favouring readback. Chrome's implementation of readback doesn't always work either since readback depends on the actual surface being drawn and swapped, which doesn't happen if chrome or the tab being read back is not foreground.

Other notes:

* For webview, we are likely reading back from the root render pass, ie the actual frame buffer, which means it can readback content from other java views (if webview's background is not solid). I didn't see any code that avoids this (that we actually lift the root render pass into a separate texture first), though I was just glancing around the code; if anyone knows readback in detail and could answer this, that would be great. Also I'm not sure if this is important to avoid or not.

* Current chrome implementation for devtools relies on observing submit frame to add a copy request. The newly submitted frame is likely to be drawn immediately after, thus fulfilling the request. This doesn't work for webview, and won't work in the viz world either, because submit frame is not on the UI thread, which means devtools doesn't have a guarantee to catch the swap of a frame (in webview's case, it's guaranteed it won't). I'm not sure if we want to invalidate just so to fulfill a copy request; doesn't seem like a great idea. If we don't invalidate, then that means devtools will always be one frame behind, which is also not great. I don't have a great solution here.

* I'm really confused what these "readback layers" do on android chrome. afaict, they serve no purpose at all (except to keep track of whether there are pending readback requests), and probably left over from old implementations when we were reading from a separate layer. Would be good if someone could answer this definitively though..
https://cs.chromium.org/chromium/src/ui/android/delegated_frame_host_android.cc?rcl=7f3fe8cc08dcee50fe0fb993718c6fd11a78b5f5&l=122
https://cs.chromium.org/chromium/src/content/browser/renderer_host/compositor_impl_android.cc?rcl=2967489d082e4175e49956f5dd5b6d50468c5f73&l=549

Webview team doesn't really have full bandwidth to work on this, so if no one else picks this up, then I'll pass this to jamwalla as a P2 for webview.

boliu@: thanks for looking into this in depth. Re: #19:

> Current chrome implementation for devtools relies on observing submit frame to add a copy request.

I thought the fundamental mechanism that guaranteed that screenshots were satisfied was the addition of the BROWSER_SNAPSHOT_FRAME_NUMBER_COMPONENT here:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_impl.cc?type=cs&sq=package:chromium&l=1690

and the proper merging and transmission of this component down the chain to the GPU process. I thought then that RenderWidgetHostImpl::OnGpuSwapBuffersCompletedInternal was the callback responsible for completing those snapshot requests:

https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_impl.cc?type=cs&q=WindowSnapshotReachedScreen&l=1993

Is the swap promise in RenderViewImpl::OnForceRedraw at https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?type=cs&sq=package:chromium&l=1214 really integral to this mechanism?

I would really like to ask and encourage that this be kept as a P1 RFE. Right now there is a large gap in testing of Android WebView – rendering to the screen can be completely and silently broken – and by adding this mechanism, we can turn on the same pixel tests that prevent this from breaking on Chromium's other platforms and configurations (including Chrome on Android).

re #21.

OnForceRedraw forces the renderer compositor to submit a new frame if possible. But it doesn't force anything to happen viz::Display on chrome android, afaict. viz::Display still won't swap if app is in the background, or (I think) if tab is not foreground.

Actually, looks like RenderWidgetHostImpl::OnGpuSwapBuffersCompletedInternal is assuming that the readback implementation is reading an EGLSurface *after* a swap, so it's listening for a swap before inserting a copy request. That's not how viz::Display works. viz::Display copy requests allows reading back individual layers and surfaces, and copy requests are fulfilled *during* a DrawAndSwap.

So afaict, OnGpuSwapBuffersCompletedInternal is going to force an extra renderer compositor frame and a viz::Display::DrawAndSwap for each from it reads back, all for nothing (on platforms that implements ui::GrabViewSnapshotAsync with copy requests anyway)


Also, none of that was really relevant. I was only referring to devtools usage of copy request here:
https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/page_handler.cc?rcl=8e136a673dfb4edac3bdd870f587f7cef8d26261&l=860

which is called on renderer compositor frame submit: 
https://cs.chromium.org/chromium/src/content/browser/devtools/render_frame_devtools_agent_host.cc?rcl=0c939c0dd4092e0756bc0a35a5ed06f19e1a6d31&l=711

> * For webview, we are likely reading back from the root render pass, ie the
> actual frame buffer, which means it can readback content from other java views
> (if webview's background is not solid). I didn't see any code that avoids
> this (that we actually lift the root render pass into a separate texture first),
> though I was just glancing around the code; if anyone knows readback in detail
> and could answer this, that would be great. Also I'm not sure if this is
> important to avoid or not.

This is correct, and I think it seems fine for a test, so long as anything we pick up behind can be deterministic.

Readback will also be used for devtools, which can connect to webviews in random apps. Might be confusing if that starts showing things that's not drawn by webview :/

To get them out of the root surface, put the request on a non-root layer. Add an intermediate layer if needed as a copy-root for everything to hang off of instead of the root. Then it should get a renderpass of its own. https://cs.chromium.org/chromium/src/cc/trees/property_tree_builder.cc?rcl=2b75a6227027468a44de506e333f7820c46514ab&l=758

Umm, webview doesn't have a browser compositor. Does the same trick for non-root viz::surface?

Yeah, a non-root surface would have a separate renderpass already (each surface submits a separate renderpass) which would then not be merged if it had a copy request. https://cs.chromium.org/chromium/src/components/viz/service/display/surface_aggregator.cc?rcl=17471a72481450aea238e1db90918254a3a5a453&l=332