Migrate Telemetry from beginWindowSnapshotPNG to Page.captureScreenshot

Migrate Telemetry from beginWindowSnapshotPNG to Page.captureScreenshot

This patch:
1. Changes Page.captureScreenshot implementation to  synchronize with the renderer properly.

2. Migrates Telemetry test to using Page.captureScreenshot, including screenshot_sync test.

3. Removes window.chrome.gpuBenchmarking.beginWindowSnapshotPNG as obsolete.

BUG=242405

[pfeldman, 2014-03-07 12:21:22]

https://chromiumcodereview.appspot.com/190693002/diff/1/content/browser/rende...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://chromiumcodereview.appspot.com/190693002/diff/1/content/browser/rende...
content/browser/renderer_host/render_widget_host_impl.h:180: virtual void
GetSnapshotFromRenderer(
It looks like this method is the wrong way of doing what you are trying to do
the right way. Why not to fix it instead? And reuse exising IPC message
ViewMsg_Snapshot that is supposed to do the same thing as ForcePaint that you
introduce.

https://chromiumcodereview.appspot.com/190693002/diff/1/content/browser/rende...
content/browser/renderer_host/render_widget_host_impl.h:559: void
WindowSnapshotAsyncCallback(
TODO(kaznacheev): retire.

[Vladislav Kaznacheev, 2014-03-11 13:08:56]

https://chromiumcodereview.appspot.com/190693002/diff/1/content/browser/rende...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://chromiumcodereview.appspot.com/190693002/diff/1/content/browser/rende...
content/browser/renderer_host/render_widget_host_impl.h:180: virtual void
GetSnapshotFromRenderer(
This method is used when the compositor is not available, so it cannot be
replaced with what I am adding.

On 2014/03/07 12:21:22, pfeldman wrote:
> It looks like this method is the wrong way of doing what you are trying to do
> the right way. Why not to fix it instead? And reuse exising IPC message
> ViewMsg_Snapshot that is supposed to do the same thing as ForcePaint that you
> introduce.

https://chromiumcodereview.appspot.com/190693002/diff/1/content/browser/rende...
content/browser/renderer_host/render_widget_host_impl.h:559: void
WindowSnapshotAsyncCallback(
On 2014/03/07 12:21:22, pfeldman wrote:
> TODO(kaznacheev): retire.

Done.

[Vladislav Kaznacheev, 2014-03-11 13:14:30]

Please review for the general sanity. Will add OWNERs for final approvals when
you are satisfied.

[Ken Russell (switch to Gerrit), 2014-03-11 18:22:29]

+bajones for review

This CL leaves the code in a strange intermediate state. As much as I agree with
the general principle of keeping CLs small, I think you should just push ahead
and switch over the GPU pixel tests to using your new code path, and delete the
GPU benchmarking extension's beginWindowSnapshotPNG code now. This will make it
easier to reason about the correctness of your code, and also allow it to be
sent to the GPU try servers, which will be able to test it for correctness.

Other opinions?

https://chromiumcodereview.appspot.com/190693002/diff/40001/content/browser/d...
File content/browser/devtools/renderer_overrides_handler.h (right):

https://chromiumcodereview.appspot.com/190693002/diff/40001/content/browser/d...
content/browser/devtools/renderer_overrides_handler.h:81: void
ScreencastFrameCaptured(
What is the difference between ScreenshotCaptured and ScreencastFrameCaptured?
It seems to me that only one of these should exist.

https://chromiumcodereview.appspot.com/190693002/diff/40001/content/browser/r...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/190693002/diff/40001/content/browser/r...
content/browser/renderer_host/render_widget_host_impl.cc:2400: // of the two
methods should be used when testing a specific renderer.
That's a non-intuitive restriction that leaves the code in a weird state until
the old GPU benchmarking extension entry point is removed.

[Vladislav Kaznacheev, 2014-03-12 09:24:23]

https://chromiumcodereview.appspot.com/190693002/diff/40001/content/browser/d...
File content/browser/devtools/renderer_overrides_handler.h (right):

https://chromiumcodereview.appspot.com/190693002/diff/40001/content/browser/d...
content/browser/devtools/renderer_overrides_handler.h:81: void
ScreencastFrameCaptured(
These are two different things.
Screencast is a special mode when DevTools captures compositor frames as they
are generated and sends them to DevTools frontend. In this case there is no need
to force synchronization with the renderer.

On 2014/03/11 18:22:29, Ken Russell wrote:
> What is the difference between ScreenshotCaptured and ScreencastFrameCaptured?
> It seems to me that only one of these should exist.

https://chromiumcodereview.appspot.com/190693002/diff/40001/content/browser/r...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/190693002/diff/40001/content/browser/r...
content/browser/renderer_host/render_widget_host_impl.cc:2400: // of the two
methods should be used when testing a specific renderer.
Fair enough. I will try and remove the old GPU benchmarking extension.

On 2014/03/11 18:22:29, Ken Russell wrote:
> That's a non-intuitive restriction that leaves the code in a weird state until
> the old GPU benchmarking extension entry point is removed.

[Vladislav Kaznacheev, 2014-03-12 15:28:53]

PTAL. Included the changes to Telemetry and window.chrome.gpuBenchmarking.

[bajones, 2014-03-12 17:03:46]

This CL changes the intent of gpu_tests/screenshot_sync.html. The previous
version was a stress test. It ran 100 times to ensure that the screenshot
capture was properly synced every time. The repetition was necessary, as
failures were extremely intermittent. We need to preserve the synchronized
behavior of screen captures to ensure our testing is reliable, and as such we
need to have a way of stressing that behavior. Its not entirely clear to me how
that behavior should be tested with the new API (I don't know if there's a way
to bounce back and forth from page logic to Telemetry), but we can't lose this
coverage.

[Ken Russell (switch to Gerrit), 2014-03-12 17:47:51]

On 2014/03/12 17:03:46, bajones wrote:
> This CL changes the intent of gpu_tests/screenshot_sync.html. The previous
> version was a stress test. It ran 100 times to ensure that the screenshot
> capture was properly synced every time. The repetition was necessary, as
> failures were extremely intermittent. We need to preserve the synchronized
> behavior of screen captures to ensure our testing is reliable, and as such we
> need to have a way of stressing that behavior. Its not entirely clear to me
how
> that behavior should be tested with the new API (I don't know if there's a way
> to bounce back and forth from page logic to Telemetry), but we can't lose this
> coverage.

Yes, it is essential to not lose coverage in this area and Brandon, thanks for
pointing this out.

I didn't think this through carefully enough, but now realize that the removal
of beginWindowSnapshotPNG will prevent JavaScript code in tests from taking
screen snapshots of themselves. They'll only be able to initiate screenshots
through Telemetry's Python API.

This might not be a showstopper. The only test which does this is this
particular stress test. Perhaps it can be rewritten so that it's driven more by
the Python harness.

The JavaScript code would draw a frame and communicate to screenshot_sync.py
that a new frame is ready, perhaps by setting a flag which the Python code is
waiting for. The Python code would grab a screenshot and either the JS or Python
code would validate the grabbed color against the one which the JS code thought
it drew. Repeat ~50 times. Then set another flag which the Python code is also
waiting for (perhaps it's waiting on the "or" of two booleans). If the
"__testComplete" flag is set, the Python code should exit the test.

Vladislav, could you restructure this test to work this way?

[Vladislav Kaznacheev, 2014-03-12 19:44:06]

On 2014/03/12 17:47:51, Ken Russell wrote:
> On 2014/03/12 17:03:46, bajones wrote:
> > This CL changes the intent of gpu_tests/screenshot_sync.html. The previous
> > version was a stress test. It ran 100 times to ensure that the screenshot
> > capture was properly synced every time. The repetition was necessary, as
> > failures were extremely intermittent. We need to preserve the synchronized
> > behavior of screen captures to ensure our testing is reliable, and as such
we
> > need to have a way of stressing that behavior. Its not entirely clear to me
> how
> > that behavior should be tested with the new API (I don't know if there's a
way
> > to bounce back and forth from page logic to Telemetry), but we can't lose
this
> > coverage.
> 
> Yes, it is essential to not lose coverage in this area and Brandon, thanks for
> pointing this out.
> 
> I didn't think this through carefully enough, but now realize that the removal
> of beginWindowSnapshotPNG will prevent JavaScript code in tests from taking
> screen snapshots of themselves. They'll only be able to initiate screenshots
> through Telemetry's Python API.
> 
> This might not be a showstopper. The only test which does this is this
> particular stress test. Perhaps it can be rewritten so that it's driven more
by
> the Python harness.
> 
> The JavaScript code would draw a frame and communicate to screenshot_sync.py
> that a new frame is ready, perhaps by setting a flag which the Python code is
> waiting for. The Python code would grab a screenshot and either the JS or
Python
> code would validate the grabbed color against the one which the JS code
thought
> it drew. Repeat ~50 times. Then set another flag which the Python code is also
> waiting for (perhaps it's waiting on the "or" of two booleans). If the
> "__testComplete" flag is set, the Python code should exit the test.
> 
> Vladislav, could you restructure this test to work this way?

I could restructure the test, but may I ask you to point out what exactly is
wrong with the way I have written it? One good thing about it is that it fails
reliably (not intermittently) if I roll back the changes to
Page.captureScreenshot implementation (the synchronization with the renderer
part). This means it does in fact test the synchronization.

[Ken Russell (switch to Gerrit), 2014-03-12 20:23:47]

On 2014/03/12 19:44:06, Vladislav Kaznacheev wrote:
> On 2014/03/12 17:47:51, Ken Russell wrote:
> > On 2014/03/12 17:03:46, bajones wrote:
> > > This CL changes the intent of gpu_tests/screenshot_sync.html. The previous
> > > version was a stress test. It ran 100 times to ensure that the screenshot
> > > capture was properly synced every time. The repetition was necessary, as
> > > failures were extremely intermittent. We need to preserve the synchronized
> > > behavior of screen captures to ensure our testing is reliable, and as such
> we
> > > need to have a way of stressing that behavior. Its not entirely clear to
me
> > how
> > > that behavior should be tested with the new API (I don't know if there's a
> way
> > > to bounce back and forth from page logic to Telemetry), but we can't lose
> this
> > > coverage.
> > 
> > Yes, it is essential to not lose coverage in this area and Brandon, thanks
for
> > pointing this out.
> > 
> > I didn't think this through carefully enough, but now realize that the
removal
> > of beginWindowSnapshotPNG will prevent JavaScript code in tests from taking
> > screen snapshots of themselves. They'll only be able to initiate screenshots
> > through Telemetry's Python API.
> > 
> > This might not be a showstopper. The only test which does this is this
> > particular stress test. Perhaps it can be rewritten so that it's driven more
> by
> > the Python harness.
> > 
> > The JavaScript code would draw a frame and communicate to screenshot_sync.py
> > that a new frame is ready, perhaps by setting a flag which the Python code
is
> > waiting for. The Python code would grab a screenshot and either the JS or
> Python
> > code would validate the grabbed color against the one which the JS code
> thought
> > it drew. Repeat ~50 times. Then set another flag which the Python code is
also
> > waiting for (perhaps it's waiting on the "or" of two booleans). If the
> > "__testComplete" flag is set, the Python code should exit the test.
> > 
> > Vladislav, could you restructure this test to work this way?
> 
> I could restructure the test, but may I ask you to point out what exactly is
> wrong with the way I have written it? One good thing about it is that it fails
> reliably (not intermittently) if I roll back the changes to
> Page.captureScreenshot implementation (the synchronization with the renderer
> part). This means it does in fact test the synchronization.

As Brandon already pointed out, this test is specifically designed as a stress
test. Its behavior shouldn't be changed just as a side-effect of this CL.

It is good that it fails reliably if the needed synchronization in
Page.captureScreenshot is removed. I hope that reintroducing the ~50 iterations
will make it test the synchronization even more reliably.

[Vladislav Kaznacheev, 2014-03-13 14:01:01]

On 2014/03/12 20:23:47, Ken Russell wrote:
> On 2014/03/12 19:44:06, Vladislav Kaznacheev wrote:
> > On 2014/03/12 17:47:51, Ken Russell wrote:
> > > On 2014/03/12 17:03:46, bajones wrote:
> > > > This CL changes the intent of gpu_tests/screenshot_sync.html. The
previous
> > > > version was a stress test. It ran 100 times to ensure that the
screenshot
> > > > capture was properly synced every time. The repetition was necessary, as
> > > > failures were extremely intermittent. We need to preserve the
synchronized
> > > > behavior of screen captures to ensure our testing is reliable, and as
such
> > we
> > > > need to have a way of stressing that behavior. Its not entirely clear to
> me
> > > how
> > > > that behavior should be tested with the new API (I don't know if there's
a
> > way
> > > > to bounce back and forth from page logic to Telemetry), but we can't
lose
> > this
> > > > coverage.
> > > 
> > > Yes, it is essential to not lose coverage in this area and Brandon, thanks
> for
> > > pointing this out.
> > > 
> > > I didn't think this through carefully enough, but now realize that the
> removal
> > > of beginWindowSnapshotPNG will prevent JavaScript code in tests from
taking
> > > screen snapshots of themselves. They'll only be able to initiate
screenshots
> > > through Telemetry's Python API.
> > > 
> > > This might not be a showstopper. The only test which does this is this
> > > particular stress test. Perhaps it can be rewritten so that it's driven
more
> > by
> > > the Python harness.
> > > 
> > > The JavaScript code would draw a frame and communicate to
screenshot_sync.py
> > > that a new frame is ready, perhaps by setting a flag which the Python code
> is
> > > waiting for. The Python code would grab a screenshot and either the JS or
> > Python
> > > code would validate the grabbed color against the one which the JS code
> > thought
> > > it drew. Repeat ~50 times. Then set another flag which the Python code is
> also
> > > waiting for (perhaps it's waiting on the "or" of two booleans). If the
> > > "__testComplete" flag is set, the Python code should exit the test.
> > > 
> > > Vladislav, could you restructure this test to work this way?
> > 
> > I could restructure the test, but may I ask you to point out what exactly is
> > wrong with the way I have written it? One good thing about it is that it
fails
> > reliably (not intermittently) if I roll back the changes to
> > Page.captureScreenshot implementation (the synchronization with the renderer
> > part). This means it does in fact test the synchronization.
> 
> As Brandon already pointed out, this test is specifically designed as a stress
> test. Its behavior shouldn't be changed just as a side-effect of this CL.
> 
> It is good that it fails reliably if the needed synchronization in
> Page.captureScreenshot is removed. I hope that reintroducing the ~50
iterations
> will make it test the synchronization even more reliably.

I have increased snapshot repetitions to 50. I do believe that this code
structure suits the purpose of the test. However if you strongly believe that
the test structure should stay close to what it was before I can do that as well
(the page would call 'draw' from requestAnimationFrame, and Telemetry would do
an independent loop capturing snapshots and comparing the pixels).

I also had to increase the number of requestAnimationFrame repetitions to 15
(similar to what pixel.py is doing) to prevent Page.captureSnapshot from
hanging.
I wonder if there is a better way.

[bajones, 2014-03-13 16:34:55]

On 2014/03/13 14:01:01, Vladislav Kaznacheev wrote:
> I have increased snapshot repetitions to 50. I do believe that this code
> structure suits the purpose of the test. However if you strongly believe that
> the test structure should stay close to what it was before I can do that as
well
> (the page would call 'draw' from requestAnimationFrame, and Telemetry would do
> an independent loop capturing snapshots and comparing the pixels).
> 
> I also had to increase the number of requestAnimationFrame repetitions to 15
> (similar to what pixel.py is doing) to prevent Page.captureSnapshot from
> hanging.
> I wonder if there is a better way.

The new code does indeed look like it emulates the old behavior better. Thanks.
content/test LGTM

I am curious about the 15 rAF cycle on test start, though I don't think it's
necessarily a problem for this CL. It's surprising to hear that it hangs if we
don't do that. Probably worth investigating as part of a separate ticket.

[Ken Russell (switch to Gerrit), 2014-03-13 18:11:46]

This looks good overall.

Could you please send this CL to the linux_gpu, win_gpu and mac_gpu try bots
with "git cl try -b [bot]"? I tried to do this with the "try more bots" UI, but
I think that's broken right now with the tryserver restructuring.

[Ken Russell (switch to Gerrit), 2014-03-13 18:12:24]

phajdan.jr@ points out that you may also need an explicit -m argument to git cl
try, e.g. -m tryserver.chromium.

[nduca, 2014-03-17 20:06:49]

so exciting, thank you for contributing this patch. minor issue.

https://codereview.chromium.org/190693002/diff/110001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py
(right):

https://codereview.chromium.org/190693002/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py:120:
response = self.SyncRequest({'method': 'Page.captureScreenshot'}, timeout)
telemetry needs to work against old chrome revs up to the stable channel, so
you'll want to check the version of chrome [look in this file for examples] and
then use the new approach only when possible

http://www.chromium.org/developers/telemetry/telemetry-feature-guidelines

[Ken Russell (switch to Gerrit), 2014-03-17 21:30:18]

Thanks for submitting the try jobs; looks like they're working well. LGTM with
nduca's comment addressed.

[Vladislav Kaznacheev, 2014-03-19 12:31:09]

On 2014/03/17 21:30:18, Ken Russell wrote:
> Thanks for submitting the try jobs; looks like they're working well. LGTM with
> nduca's comment addressed.

Thanks for the comments, everyone!

I did not feel good about spinning the requestAnimationFrame loop, so I decided
to look into the reason for having to do this. Turns out requestAnimationFrame
is not really necessary (at least on my Linux box), a simple 300ms timeout
solves the problem.

When I reduce the timeout to 0 the Page.captureScreenshot call hangs more often
than not. After a few fun hours of tracing I found the cause: skipped frames. If
RenderWidgetHostViewAura::ShouldSkipFrame decides to skip the frame then
LatencyInfo is lost and RenderWidgetHostImpl::CompositorFrameDrawn fails to
detect the correct moment to take a screenshot.

I tried adding "&& !latency_info.size()" to the condition at
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
and it seems to fix the problem, but I strongly suspect that a correct fix is
much more complex (this needs to be addressed on Aura/Android/Mac x
software/accelerated).

To me this is a blocker. While it might be OK to require some preparatory
actions in a test, Page.captureScreenshot is a user-facing API which should just
work. Should I file a bug for this (skipped frames causing LatencyInfo loss) or
is there already one?

[bajones, 2014-03-19 15:47:54]

Thanks for investigating!

This definitely sounds like a problem, and presumably one that could occur at
times other than page startup. I'd be surprised if there was already a ticket on
this, so please file one!

[Vladislav Kaznacheev, 2014-03-20 07:41:05]

On 2014/03/19 15:47:54, bajones wrote:
> Thanks for investigating!
> 
> This definitely sounds like a problem, and presumably one that could occur at
> times other than page startup. I'd be surprised if there was already a ticket
on
> this, so please file one!

Filed http://crbug.com/354339. Please help me find the correct owner.

[bajones, 2014-05-22 23:09:01]

http://crbug.com/354339 is fixed now, and I've verified that this with that fix
in place this patch runs correctly with it even when you take out the delay at
the test start.

Vladislav: Rebasing was a bit ugly, so I'd like to have you verify that it was
done correctly. I'm not able to upload my code into this CL, however. (It
complains I'm not an owner.) WOuld it be preferable for me to start another CL
with the rebased code or send my current patch to you directly so you can upload
it here?

[Vladislav Kaznacheev, 2014-05-23 11:48:19]

On 2014/05/22 23:09:01, bajones wrote:
> http://crbug.com/354339 is fixed now, and I've verified that this with that
fix
> in place this patch runs correctly with it even when you take out the delay at
> the test start.
> 
> Vladislav: Rebasing was a bit ugly, so I'd like to have you verify that it was
> done correctly. I'm not able to upload my code into this CL, however. (It
> complains I'm not an owner.) WOuld it be preferable for me to start another CL
> with the rebased code or send my current patch to you directly so you can
upload
> it here?

Thanks for taking care of the rebase! I have uploaded the patch and looking at
trybots now.

[Vladislav Kaznacheev, 2014-05-28 09:15:54]

Hello OWNERS,

pfeldman@, please review:
content/browser/devtools/renderer_overrides_handler.cc 
content/browser/devtools/renderer_overrides_handler.h

kbr@, please review:
content/browser/renderer_host/render_widget_host_impl.cc
content/browser/renderer_host/render_widget_host_impl.h
content/renderer/gpu/gpu_benchmarking_extension.cc
content/test/data/gpu/screenshot_sync.html
content/test/gpu/gpu_tests/screenshot_sync.py 

tsepez@, please review:
content/common/view_messages.h                    

jochen@, please review:
content/public/browser/render_widget_host.h       

jamesr@, please review:
content/renderer/render_view_impl.cc
content/renderer/render_view_impl.h

tonyg@, please review:
tools/telemetry/telemetry/core/backends/chrome/inspector_backend.py

Best regards,

  Vlad

[jochen (gone - plz use gerrit), 2014-05-28 12:10:29]

+enne/danakj who recently ripped out similar APIs

[tonyg, 2014-05-28 17:12:19]

Let's split the Telemetry change into a separate patch. Since Telemetry has to
work back to the current stable chrome, that verifies that there are no
dependencies on the chrome changes in the patch.

Also, I wonder whether this fixes crbug.com/358664 . Should we try re-enabling
the test?

[Tom Sepez, 2014-05-28 17:32:10]

Messages LGTM.

[Vladislav Kaznacheev, 2014-05-29 11:29:54]

On 2014/05/28 17:12:19, tonyg wrote:
> Let's split the Telemetry change into a separate patch. Since Telemetry has to
> work back to the current stable chrome, that verifies that there are no
> dependencies on the chrome changes in the patch.
> 
> Also, I wonder whether this fixes crbug.com/358664 . Should we try re-enabling
> the test?

Oh, I did not think of older Chrome versions. However, I cannot just split out
the telemetry part. The Chrome part of the patch is removing the old way of
capturing synchronized snapshots (beginWindowSnapshotPNG), so telemetry will
break as soon as I commit it.

Keeping the old way (beginWindowSnapshotPNG) proved to be messy (see comment
#5). How about I try and teach telemetry to support both the old way and the new
way?

[tonyg, 2014-05-30 17:12:25]

> How about I try and teach telemetry to support both the old way and the new
way?

sgtm

[Ken Russell (switch to Gerrit), 2014-06-05 01:43:24]

I'm really sorry for taking so long to get to this code review. It looks great
overall.

I'm not sure how many Telemetry consumers are really using the Screenshot
support. From the standpoint of the GPU bots, since they only test on top of
tree, it would be fine to require the new code path. However, it shouldn't be
hard to test for the existence of
window.chrome.gpuBenchmarking.beginWindowSnapshotPNG and just go down the old
code path if necessary.