Implemented telemetry benchmark that loads page and outputs screenshot.

tools/perf/measurements/pixel_diff.py

+# Copyright 2017 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+import os

[wkorman, 2017/06/07 20:15:11]

+1 space above

+import time
+import py_utils
+
+from telemetry.page import legacy_page_test
+from telemetry.value import scalar
+from telemetry.util import image_util
+
+class PixelDiff(legacy_page_test.LegacyPageTest):

[wkorman, 2017/06/07 20:15:11]

Add doc comment describing purpose. See power.py which has a brief one as an
example. Perhaps note file output/naming similar to my comment on the other
script.

I also considered whether we should instead name this script 'screenshot' since
that's more self documenting. Could make a case either way. Let's see opinion of
our other reviewers.

Note there is already as we've discussed in email a telemetry/util/screenshot.py
script so a benefit of naming as we have here is lack of ambiguity. Also,
perhaps we'd do more/different things for pixel diff down the line, so having
that specific naming is better.

[rmistry, 2017/06/08 13:08:56]

I agree that 'screenshot' might be a better name here. This measurement is not
actually calculating any diffs, it's just outputting a screenshot, "pixel diff"
will be the name used in CT because it will be the one actually doing the
diff'ing.

+
+  def __init__(self, png_outdir, wait_time=0, disable_javascript=False):
+    super(PixelDiff, self).__init__()
+    self._png_outdir = png_outdir
+    self._wait_time = wait_time
+    self._disable_javascript = disable_javascript

[wkorman, 2017/06/07 20:15:11]

Same as previous: remove for now or comment to note not yet implemented.

+
+  def ValidateAndMeasurePage(self, page, tab, results):

[wkorman, 2017/06/07 20:15:11]

Do we have sufficient useful log output when we run this script with '-v' to
make scrutiny if we have to debug later useful? Or should we add some
logging.info calls to make it easier to see what the input params were, what
path we try to write to, etc.

+    if not tab.screenshot_supported:
+        raise legacy_page_test.MeasurementFailure(
+          'Screenshotting not supported on this platform')
+
+    try:
+      tab.WaitForDocumentReadyStateToBeComplete()
+    except py_utils.TimeoutException:
+      pass

[wkorman, 2017/06/07 20:15:11]

I see this is what rasterize_and_record_micro does as well. Two thoughts:

(1) I would think we'd at least want to log something here.

Two places where tools/perf logs timeouts currently for example:

page_sets/system_health/multi_tab_stories.py
contrib/cros_benchmarks/tab_switching_stories.py

(2) seeking feedback from other reviewers: if we time out waiting for document
ready, do we really still want to go ahead and try to take a screenshot? Or
should we just report an error? Is it sufficient to just output results dict
with 'saved_picture_count' of 0? How does Cluster Telemetry logic that calls
into this script want to handle failure/error like this? Concern here is to
ensure we (a) always check error results, and (b) make sure we do what we can to
make debugging in telemetry/cluster-telemetry logs easy for inevitable failures.

[nednguyen, 2017/06/07 20:32:12]

I think if we timeout here, it's better to report error or something like "I
can't process this" gracefully.

[rmistry, 2017/06/08 13:08:56]

Agreed, we should report a descriptive error. In CT whenever a
measurement/benchmark reports an error it either retries it or moves on to the
next webpage. Since CT deals with thousands of webpages, I tend to give up
quickly and move on to the next page so that each run does not take too long.

+    time.sleep(self._wait_time)
+
+    if not os.path.exists(self._png_outdir):
+        os.makedirs(self._png_outdir)

[wkorman, 2017/06/07 20:15:11]

If we fail to makedir (and probably some other os calls here) it will raise an
exception. Is it ok for that case to propagate directly and cause script to just
fail w/ stack trace? See my question (2) above re: how CT wants to deal with
error handling.

[rmistry, 2017/06/08 13:08:56]

I think it's ok to propagate the exception in cases like this. If it cannot
create the output dir there are problems going on that we want to know about.
Would be useful to add logging saying "Creating xyz dir".

+
+    outpath = os.path.abspath(
+        os.path.join(self._png_outdir, page.file_safe_name)) + '.png'
+    # Replace win32 path separator char '\' with '\\'.
+    outpath = outpath.replace('\\', '\\\\')
+
+    screenshot = tab.Screenshot()

[wkorman, 2017/06/07 20:15:12]

This can raise a few exceptions, should we catch and handle in any manner?
Again, need to hear from other reviewers on (2) above.

https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

[rmistry, 2017/06/08 13:08:56]

Hopefully the exception from Screenshot() will be descriptive. If it is not then
we can look into making it more descriptive after we do a few test runs.

+
+    last_mod_time = 0
+    if os.path.exists(outpath):
+        last_mod_time = os.path.getmtime(outpath)
+
+    image_util.WritePngFile(screenshot, outpath)

[wkorman, 2017/06/07 20:15:11]

Error handling -- it looks like this can throw some exceptions. It also looks
like one code path calls into cv2 python lib which actually has a retval bool
which presumably reports success/failure and is ignored in the image_util lib
but the docs are poor, see:

http://docs.opencv.org/2.4/modules/highgui/doc/reading_and_writing_images_and...

The other code path uses built in png lib which also has unclear docs on error
situation:

https://pythonhosted.org/pypng/png.html#png.Writer.write_array

I'm not sure what code path we are actually using. Could add logging to
image_util.py to see:

https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

In any case I would say it's ok to add a TODO here and potentially file bug to
enhance error handling of image_util.WritePngFile with unit tests separately.

+
+    saved_picture_count = 0
+    if os.path.exists(outpath) and os.path.getmtime(outpath) > last_mod_time:
+        saved_picture_count = 1
+    results.AddValue(scalar.ScalarValue(
+            results.current_page, 'saved_picture_count', 'count',
+            saved_picture_count))
+
+
+