telemetry: add screenshot measurement

telemetry: add screenshot measurement

R=tonyg@chromium.org,nduca@chromium.org
BUG=373045
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276623

[ernstm, 2014-05-13 23:39:26]

PTAL

[ernstm, 2014-05-20 18:30:45]

On 2014/05/13 23:39:26, ernstm wrote:
> PTAL

Ping

[tonyg, 2014-05-21 09:36:56]

Nat is probably a better reviewer than me for this. He has a lot of opinions
about how measurements should (or should not) be written these days.

https://codereview.chromium.org/271733008/diff/1/tools/perf/measurements/scre...
File tools/perf/measurements/screenshot.py (right):

https://codereview.chromium.org/271733008/diff/1/tools/perf/measurements/scre...
tools/perf/measurements/screenshot.py:32:
time.sleep(self.options.screenshot_delay)
This seems like it'll be really flaky.

[ernstm, 2014-05-22 17:48:01]

https://codereview.chromium.org/271733008/diff/1/tools/perf/measurements/scre...
File tools/perf/measurements/screenshot.py (right):

https://codereview.chromium.org/271733008/diff/1/tools/perf/measurements/scre...
tools/perf/measurements/screenshot.py:32:
time.sleep(self.options.screenshot_delay)
On 2014/05/21 09:36:57, tonyg wrote:
> This seems like it'll be really flaky.

This measurement will always be flaky on non-static pages, and that is okay. We
are not planning to run this measurement routinely on the bots. Instead, we want
to use it for one-off correctness tests of new features, e.g. GPU rasterization
in a semi-automated way. The plan is to use cluster telemetry to generate
screenshots for large page sets with and without the feature and then compare
them with e.g. Skia's image diff. I've done this locally on my machine and it
proved to be very useful. I was able to identify two correctness issues that
have not been detected by other tests.

[ernstm, 2014-05-27 18:12:13]

Nat, could you please review this CL?

[nduca, 2014-05-29 16:12:38]

The ratio of "this is urgent but not architecturally rationalized" patches is
getting a bit high so I'm going to start having to get more strict about reviews
coming in.

The sleep time etc stuff should be part of the page's action. The measurements
shouldn't have these timing parameters, or timing tweaking parameters. You
should probably be telling the constructor of the measurement to run the
'prepareForScreenshot' action and then figure out how to have a default
implementation that contains these parameters.

[ernstm, 2014-05-29 19:47:27]

On 2014/05/29 16:12:38, nduca wrote:
> The ratio of "this is urgent but not architecturally rationalized" patches is
> getting a bit high so I'm going to start having to get more strict about
reviews
> coming in.
> 
> The sleep time etc stuff should be part of the page's action. The measurements
> shouldn't have these timing parameters, or timing tweaking parameters. You
> should probably be telling the constructor of the measurement to run the
> 'prepareForScreenshot' action and then figure out how to have a default
> implementation that contains these parameters.

I wasn't aware that timers should now be on the page set; We had them on
measurements in the past. I agree that the new approach is cleaner. Please take
a look at the changes.

[ernstm, 2014-06-03 00:45:14]

Per offline discussion with Nat, I added action_name_to_run_to_prepare (better
name?) to PageTest, which is an optional action that is run before the
benchmarking actions. In the case of screenshot, the
action_name_to_run_to_prepare is 'PrepareForScreenshot'. It gets the page into a
state to take the screenshot. action_name_to_run is empty as nothing needs to be
done for the measurement; the screenshot is taken in in MeasurePage.
action_name_to_run_to_prepare is strictly optional, that means even when an
action is specified in the constructor of PageTest, pages without this action
will still run (this is not the case for action_name_to_run).

[nduca, 2014-06-06 16:00:40]

https://codereview.chromium.org/271733008/diff/30001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/271733008/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:41:
action_name_to_run_to_prepare=''):
why this than making the action_name_to_run optional?

the problem with 'prepare' is that it you're adding yet another stage into the
'going to a page' pipeline: already its navigate, then action_to_run. All i was
suggesting is that if the action_to_run is not present, then stop at navigate...

[ernstm, 2014-06-06 17:26:57]

https://codereview.chromium.org/271733008/diff/30001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/271733008/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:41:
action_name_to_run_to_prepare=''):
On 2014/06/06 16:00:40, nduca wrote:
> why this than making the action_name_to_run optional?
> 
> the problem with 'prepare' is that it you're adding yet another stage into the
> 'going to a page' pipeline: already its navigate, then action_to_run. All i
was
> suggesting is that if the action_to_run is not present, then stop at
navigate...

I was concerned that making action_name_to_run optional would remove a level of
error checking, because CanRunForPage would always return True. But this can be
solved by passing in a flag that tells us explicitly that action_name_to_run
should be treated as optional. I'll implement that approach.

[ernstm, 2014-06-06 18:06:40]

https://codereview.chromium.org/271733008/diff/30001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/271733008/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:41:
action_name_to_run_to_prepare=''):
On 2014/06/06 17:26:57, ernstm wrote:
> On 2014/06/06 16:00:40, nduca wrote:
> > why this than making the action_name_to_run optional?
> > 
> > the problem with 'prepare' is that it you're adding yet another stage into
the
> > 'going to a page' pipeline: already its navigate, then action_to_run. All i
> was
> > suggesting is that if the action_to_run is not present, then stop at
> navigate...
> 
> I was concerned that making action_name_to_run optional would remove a level
of
> error checking, because CanRunForPage would always return True. But this can
be
> solved by passing in a flag that tells us explicitly that action_name_to_run
> should be treated as optional. I'll implement that approach.

Done.

[nduca, 2014-06-09 20:23:26]

lgtm 

@nednguyen, this is yet another example of a test producing an artefact that
should be going into the results object but that is instead going into a file.
:(

https://codereview.chromium.org/271733008/diff/50001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_measurement.py (right):

https://codereview.chromium.org/271733008/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_measurement.py:39: def __init__(self,
can you clarify the semantics of this new variable in the docstring please? e.g.
if you pass True, what happens?

[nednguyen, 2014-06-09 20:33:41]

On 2014/06/09 20:23:26, nduca wrote:
> lgtm 
> 
> @nednguyen, this is yet another example of a test producing an artefact that
> should be going into the results object but that is instead going into a file.
> :(
> 
>
https://codereview.chromium.org/271733008/diff/50001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/page/page_measurement.py (right):
> 
>
https://codereview.chromium.org/271733008/diff/50001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/page_measurement.py:39: def __init__(self,
> can you clarify the semantics of this new variable in the docstring please?
e.g.
> if you pass True, what happens?

Why do we need a whole measurement just to be able to take screenshot?

[ernstm, 2014-06-09 20:47:27]

> Why do we need a whole measurement just to be able to take screenshot?

How else would you implement it?

[nednguyen, 2014-06-09 21:02:39]

On 2014/06/09 20:47:27, ernstm wrote:
> > Why do we need a whole measurement just to be able to take screenshot?
> 
> How else would you implement it?

Can you provide a little bit of context why we have this use case? 

I wonder whether we should have an action_runner.TakeScreenShot(...). If you
want to take two or three screenshots during the page interaction instead of
one, this won't be able to support that.

[ernstm, 2014-06-09 21:52:54]

On 2014/06/09 21:02:39, nednguyen wrote:
> On 2014/06/09 20:47:27, ernstm wrote:
> > > Why do we need a whole measurement just to be able to take screenshot?
> > 
> > How else would you implement it?
> 
> Can you provide a little bit of context why we have this use case? 
> 
> I wonder whether we should have an action_runner.TakeScreenShot(...). If you
> want to take two or three screenshots during the page interaction instead of
> one, this won't be able to support that.

This measurement will be used for correctness testing. We are planning to run
this in cluster telementry on a large set of pages to grab screenshots with CPU
and GPU rasterization and then compare the results.

[ernstm, 2014-06-09 22:59:28]

https://codereview.chromium.org/271733008/diff/50001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_measurement.py (right):

https://codereview.chromium.org/271733008/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_measurement.py:39: def __init__(self,
On 2014/06/09 20:23:26, nduca wrote:
> can you clarify the semantics of this new variable in the docstring please?
e.g.
> if you pass True, what happens?

Done.

[ernstm, 2014-06-10 17:47:14]

Per offline discussion with Ned, there is no obvious way how to implement this
differently in the current architecture. I'll proceed and commit.

[ernstm, 2014-06-10 17:47:18]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-06-10 17:52:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ernstm@chromium.org/271733008/70001

[commit-bot: I haz the power, 2014-06-11 02:32:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-06-11 02:54:11]

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

[commit-bot: I haz the power, 2014-06-11 02:54:12]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[ernstm, 2014-06-11 18:07:52]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-06-11 18:10:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ernstm@chromium.org/271733008/90001

[commit-bot: I haz the power, 2014-06-11 21:30:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:33:55]

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

[commit-bot: I haz the power, 2014-06-11 21:33:56]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[ernstm, 2014-06-11 23:24:30]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-06-11 23:25:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ernstm@chromium.org/271733008/90001

[commit-bot: I haz the power, 2014-06-12 00:06:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-12 00:10:24]

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

[commit-bot: I haz the power, 2014-06-12 00:10:25]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[ernstm, 2014-06-12 01:08:44]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-06-12 01:10:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ernstm@chromium.org/271733008/90001

[commit-bot: I haz the power, 2014-06-12 10:28:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-06-12 11:05:01]

Change committed as 276623