process gm expectation violations

tools/skpdiff/skpdiff_server.py

Index: tools/skpdiff/skpdiff_server.py
diff --git a/tools/skpdiff/skpdiff_server.py b/tools/skpdiff/skpdiff_server.py
index 07f0e53af3633eca14c35dadb15d764bdc43e422..c84f0573f6069f030d814ba5c9bbe30921af225c 100644
--- a/tools/skpdiff/skpdiff_server.py
+++ b/tools/skpdiff/skpdiff_server.py
@@ -2,9 +2,36 @@
 # -*- coding: utf-8 -*-

[epoger, 2013/07/26 16:15:57]

Be sure to mark this file as executable, if you didn't already.  If you don't,
this will be the result:

$ tools/skpdiff/skpdiff_server.py
bash: tools/skpdiff/skpdiff_server.py: Permission denied

$ python tools/skpdiff/skpdiff_server.py
script dir         :
/usr/local/google/home/epoger/src/skia/red/trunk/tools/skpdiff
tools dir          : /usr/local/google/home/epoger/src/skia/red/trunk/tools
root dir           : /usr/local/google/home/epoger/src/skia/red/trunk
expectations dir   :
/usr/local/google/home/epoger/src/skia/red/trunk/expectations/gm
skpdiff path       :
/usr/local/google/home/epoger/src/skia/red/trunk/out/Debug/skpdiff
tmp dir            : /tmp/tmpfOkU2Dskpdiff
expected image dir : /tmp/tmpfOkU2Dskpdiff/expected
actual image dir   : /tmp/tmpfOkU2Dskpdiff/actual
Non-default runtime configuration options:
Navigate thine browser to: 127.0.0.1:8080

[Zach Reizner, 2013/07/26 18:00:40]

Done.

[epoger, 2013/07/26 16:15:57]

Here's an alternate set of steps to test this code, so you actually get some
diffs to view:

$ make tools
$ rm expectations/gm/base-*/updated-results.json

$ for PLATFORM in expectations/gm/base-* ; do if [ -e
$PLATFORM/updated-results.json ] ; then diff $PLATFORM/expected-results.json
$PLATFORM/updated-results.json; fi; done

$ tools/rebaseline.py --expectations-filename-output updated-results.json
--actuals-base-url
https://skia-autogen.googlecode.com/svn-history/r2400/gm-actual

$ for PLATFORM in expectations/gm/base-* ; do if [ -e
$PLATFORM/updated-results.json ] ; then diff $PLATFORM/expected-results.json
$PLATFORM/updated-results.json; fi; done
2623c2623
<           15666965705698152905
---
2632c2632
<           297568835278809227
---
[...]
2560c2560
<           11156208867963674446
---
940c940
<           304572795717583895
---
$ python tools/skpdiff/skpdiff_server.py

 from __future__ import print_function

[epoger, 2013/07/26 16:15:57]

I guess this is more of a viewer.html issue, but when I tried this out myself I
was confused by the UI.  When I hovered over an image, I saw dotted lines around
it but I didn't see any changes.  When I clicked on an image (in either expected
or actual column), I saw it change to match the OTHER one... I wasn't clear on
what that meant.

[Zach Reizner, 2013/07/26 18:00:40]

Weird. Clicking is not supposed to do anything. Hovering over is supposed to do
the switch of the image you're hovering over. Which browser version are you on?

+import argparse
 import BaseHTTPServer
+import json
 import os
 import os.path
+import re
+import sys
+import tempfile
+import urllib2
+
+# Grab the script path because that is where all the static assets are
+SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
+
+# Find the tools directory for python imports
+TOOLS_DIR = os.path.dirname(SCRIPT_DIR)
+
+# Find the root of the skia trunk for finding skpdiff binary
+SKIA_ROOT_DIR = os.path.dirname(TOOLS_DIR)
+
+# Find the default location of gm expectations
+DEFAULT_GM_EXPECTATIONS_DIR = os.path.join(SKIA_ROOT_DIR, 'expectations', 'gm')
+
+# Imports from within Skia
+if TOOLS_DIR not in sys.path:
+    sys.path.append(TOOLS_DIR)
+GM_DIR = os.path.join(SKIA_ROOT_DIR, 'gm')
+if GM_DIR not in sys.path:
+    sys.path.append(GM_DIR)
+import gm_json
+import jsondiff
 
 # A simple dictionary of file name extensions to MIME types. The empty string
 # entry is used as the default when no extension was given or if the extension
@@ -13,10 +40,157 @@ MIME_TYPE_MAP = {'': 'application/octet-stream',
                  'html': 'text/html',
                  'css': 'text/css',
                  'png': 'image/png',
-                 'js': 'application/javascript'
+                 'js': 'application/javascript',
+                 'json': 'application/json'
                  }
 
 
+IMAGE_FILENAME_RE = re.compile(gm_json.IMAGE_FILENAME_PATTERN)
+
+SKPDIFF_INVOKE_FORMAT = '{} --jsonp=false -o {} -f {} {}'
+
+
+def get_skpdiff_path(user_path=None):
+    """Find the skpdiff binary.
+
+    @param user_path If none, searches in Release and Debug out directories of
+           the skia root. If set, checks that the path is a real file and
+           returns it.
+    """
+    skpdiff_path = None
+    possible_paths = []
+
+    # Use the user given path, or try out some good default paths.
+    if user_path:
+        possible_paths.append(user_path)
+    else:
+        possible_paths.append(os.path.join(SKIA_ROOT_DIR, 'out',
+                                           'Release', 'skpdiff'))
+        possible_paths.append(os.path.join(SKIA_ROOT_DIR, 'out',
+                                           'Debug', 'skpdiff'))
+    # Use the first path that actually points to the binary
+    for possible_path in possible_paths:
+        if os.path.isfile(possible_path):
+            skpdiff_path = possible_path
+            break
+
+    # If skpdiff was not found, print out diagnostic info for the user.
+    if skpdiff_path is None:
+        print('Could not find skpdiff binary. Either build it into the ' +
+              'default directory, or specify the path on the command line.')
+        print('skpdiff paths tried:')
+        for possible_path in possible_paths:
+            print('   ', possible_path)
+    return skpdiff_path
+
+
+def download_file(url, output_path):
+    """Download the file at url and place it in output_path"""
+    reader = urllib2.urlopen(url)
+    with open(output_path, 'wb') as writer:
+        writer.write(reader.read())
+
+
+def download_gm_images(image_name, expected_image_path, actual_image_path,
+                       old_hash, new_hash):

[epoger, 2013/07/26 16:15:57]

I think it would be clearer if you made this function download exactly one image
(pass in image_name and hash, write the downloaded file to image_path)

[Zach Reizner, 2013/07/26 18:00:40]

Done.

+    """Download the expected and actual results into the given paths.
+
+    @param image_name          The GM file name, for example imageblur_gpu.png.
+    @param expected_image_path Path to place the expected image.
+    @param actual_image_path   Path to place the actual image.
+    @param old_hash            The hash value of the expected image.
+    @param new_hash            The hash value of the actual image.
+    """
+
+    # Seperate the image name into a GM name and syffix

[epoger, 2013/07/26 16:15:57]

More like...

# Extract the test name (e.g. "imageblur") from the image name (e.g.
# "imageblur_gpu.png")

[Zach Reizner, 2013/07/26 18:00:40]

Done.

+    image_match = IMAGE_FILENAME_RE.match(image_name)
+    gm_name = image_match.group(1)
+
+    # Calculate the URLs of the requested images
+    expected_image_url = gm_json.CreateGmActualUrl(
+        gm_name, gm_json.JSONKEY_HASHTYPE_BITMAP_64BITMD5, old_hash)
+
+    actual_image_url = gm_json.CreateGmActualUrl(
+        gm_name, gm_json.JSONKEY_HASHTYPE_BITMAP_64BITMD5, new_hash)
+
+    # Download the images as requested
+    download_file(expected_image_url, expected_image_path)
+    download_file(actual_image_url, actual_image_path)
+
+
+def download_actuals(expectations_dir, expected_name, updated_name,

[epoger, 2013/07/26 16:15:57]

It's downloading both expected and actual images, so I think this function
should be called something else (maybe download_all_images ?)

[Zach Reizner, 2013/07/26 18:00:40]

Done.

[epoger, 2013/07/26 18:36:57]

Ah, I get the "updated" part of the name (it's a bit confusing, because the
function downloads images referred to from both expected_name and updated_name).

Maybe rename this download_changed_images, so we have distinct meanings for
"updated" and "changed"?

+                     expected_image_dir, actual_image_dir):
+
+    """Download the expected and actual GMs that changed into the given paths.
+    Determining what changed will be done by comparing each expected_name JSON
+    results file to its corresponding updated_name JSON results file if it
+    exists.
+
+    @param expectations_dir   The directory to traverse for results files. This
+           should resmble expectations/gm in the Skia trunk.
+    @param expected_name      The name of the expected result files. These are
+           in the format of expected-results.json.
+    @param updated_name       The name of the updated expected result files.
+           Normally this matches --expectations-filename-output for the
+           rebaseline.py tool.
+    @param expected_image_dir The directory to place downloaded expected images
+           into.
+    @param actual_image_dir   The directory to place downloaded actual images
+           into.
+    """
+
+    differ = jsondiff.GMDiffer()
+
+    # Look through expectations for hashes that changed
+    for root, dirs, files in os.walk(expectations_dir):
+        for expectation_file in files:
+            # There are many files in the expectations directory. We only care
+            # about expected results.
+            if expectation_file != expected_name:
+                continue
+
+            # Get the name of the results file, and be sure there is an updated
+            # result to compare against. If there is not, there is no point in
+            # diffing this device.
+            expected_file_path = os.path.join(root, expected_name)
+            updated_file_path = os.path.join(root, updated_name)
+            if not os.path.isfile(updated_file_path):
+                continue
+
+            # Find all expectations that did not match.
+            expected_diff = differ.GenerateDiffDict(expected_file_path,
+                                                    updated_file_path)
+
+            # The name of the device corresponds to the name of the folder we
+            # are in.
+            device_name = os.path.basename(root)
+
+            # Create name prefixes to store the devices old and new GM results
+            expected_image_prefix = os.path.join(expected_image_dir,
+                                                 device_name) + '-'
+
+            actual_image_prefix = os.path.join(actual_image_dir,
+                                               device_name) + '-'
+
+            # Download each image that had a differing result
+            for image_name, hashes in expected_diff.iteritems():
+                print('Downloading', image_name, 'for device', device_name)
+                download_gm_images(image_name,
+                                   expected_image_prefix + image_name,
+                                   actual_image_prefix + image_name,
+                                   hashes['old'], hashes['new'])
+
+
+def get_image_set_from_skpdiff(skpdiff_records):
+    """Get the set of all images references in the given records.
+
+    @param skpdiff_records An array of records, which are dictionary objects.
+    """
+    expected_set = frozenset([r['baselinePath'] for r in skpdiff_records])
+    actual_set = frozenset([r['testPath'] for r in skpdiff_records])
+    return expected_set | actual_set
+
+
 class SkPDiffHandler(BaseHTTPServer.BaseHTTPRequestHandler):
     def send_file(self, file_path):
         # Grab the extension if there is one
@@ -28,12 +202,14 @@ class SkPDiffHandler(BaseHTTPServer.BaseHTTPRequestHandler):
         mime_type = MIME_TYPE_MAP.get(extension, MIME_TYPE_MAP[''])
 
         # Open the file and send it over HTTP
-        sending_file = open(file_path, 'rb')
-        self.send_response(200)
-        self.send_header('Content-type', mime_type)
-        self.end_headers()
-        self.wfile.write(sending_file.read())
-        sending_file.close()
+        if os.path.isfile(file_path):
+            with open(file_path, 'rb') as sending_file:
+                self.send_response(200)
+                self.send_header('Content-type', mime_type)
+                self.end_headers()
+                self.wfile.write(sending_file.read())
+        else:
+            self.send_error(404)
 
     def serve_if_in_dir(self, dir_path, file_path):
         # Determine if the file exists relative to the given dir_path AND exists
@@ -48,9 +224,6 @@ class SkPDiffHandler(BaseHTTPServer.BaseHTTPRequestHandler):
         return False
 
     def do_GET(self):
-        # Grab the script path because that is where all the static assets are
-        script_dir = os.path.dirname(os.path.abspath(__file__))
-
         # Simple rewrite rule of the root path to 'viewer.html'
         if self.path == '' or self.path == '/':
             self.path = '/viewer.html'
@@ -58,30 +231,130 @@ class SkPDiffHandler(BaseHTTPServer.BaseHTTPRequestHandler):
         # The [1:] chops off the leading '/'
         file_path = self.path[1:]
 
-        # Attempt to send static asset files first
-        if self.serve_if_in_dir(script_dir, file_path):
+        # Handle skpdiff_output.json manually because it is loaded manually into

[epoger, 2013/07/26 16:15:57]

Is it really necessary to handle this specially?  Seems like it would be simpler
to just handle it like any other file.

Maybe you do so because the web page requires the skpdiff_output.json file to be
JSONP-formatted (i.e. JSON with padding), as mentioned below?  If so, maybe
handle it specially, but change the comment:

# Handle skpdiff_output.json differently, because the web viewer requires it to
# be padded (JSONP format), and skpdiff's output is not padded.

P.S. *Why* does the web viewer require this?

+        # memory.
+        if file_path == 'skpdiff_output.json':
+            self.send_response(200)
+            self.send_header('Content-type', MIME_TYPE_MAP['json'])
+            self.end_headers()
+            self.wfile.write(self.server.skpdiff_output_json)
+            return
+
+        # Attempt to send static asset files first.
+        if self.serve_if_in_dir(SCRIPT_DIR, file_path):
             return
 
-        # WARNING: Using the root is a big ol' hack. Incredibly insecure. Only
-        # allow serving to localhost unless you want the network to be able to
-        # see ALL of the files you can.
-        # Attempt to send gm image files
-        if self.serve_if_in_dir('/', file_path):
+        # WARNING: Serving any file the user wants is incredibly insecure. Its
+        # redeeming quality is that we only serve gm files on a white list.
+        if self.path in self.server.image_set:
+            self.send_file(self.path)
             return
 
         # If no file to send was found, just give the standard 404
         self.send_error(404)
 
 
-def main():
+def run_server(skpdiff_output_path, port=8080):
+    # Preload the skpdiff results file.
+    skpdiff_output_json = ''
+    with open(skpdiff_output_path, 'rb') as records_file:
+        skpdiff_output_json = records_file.read()
+
+    # It's important to parse the results file so that we can make a set of
+    # images that the web page might request.
+    skpdiff_records = json.loads(skpdiff_output_json)['records']
+    image_set = get_image_set_from_skpdiff(skpdiff_records)
+
+    # Add JSONP padding to the JSON because the web page expects it.
+    skpdiff_output_json = 'var SkPDiffRecords = ' + skpdiff_output_json
+
     # Do not bind to interfaces other than localhost because the server will
     # attempt to serve files relative to the root directory as a last resort
     # before 404ing. This means all of your files can be accessed from this
     # server, so DO NOT let this server listen to anything but localhost.
-    server_address = ('127.0.0.1', 8080)
+    server_address = ('127.0.0.1', port)
     http_server = BaseHTTPServer.HTTPServer(server_address, SkPDiffHandler)
+    http_server.image_set = image_set
+    http_server.skpdiff_output_json = skpdiff_output_json
     print('Navigate thine browser to: {}:{}'.format(*server_address))

[epoger, 2013/07/26 16:15:57]

maybe change this line as follows, so the user can just click on the link:

    print('Navigate thine browser to: http://{}:{}'.format(*server_address))

     http_server.serve_forever()
 
+
+def main():
+    parser = argparse.ArgumentParser()
+    parser.add_argument('--port', '-p', metavar='PORT',
+                        type=int,
+                        default=8080,
+                        help='port to bind the server to; ' +
+                        'defaults to %(default)s',
+                        )
+
+    parser.add_argument('--expectations-dir', metavar='EXPECTATIONS_DIR',
+                        default=DEFAULT_GM_EXPECTATIONS_DIR,
+                        help='path to the gm expectations; ' +
+                        'defaults to %(default)s'
+                        )
+
+    parser.add_argument('--expected',
+                        metavar='EXPECTATIONS_FILE_NAME',
+                        default='expected-results.json',
+                        help='the file name of the expectations JSON; ' +
+                        'defaults to %(default)s'
+                        )
+
+    parser.add_argument('--updated',
+                        metavar='UPDATED_FILE_NAME',
+                        default='updated-results.json',
+                        help='the file name of the updated expectations JSON;' +
+                        ' defaults to %(default)s'
+                        )
+
+    parser.add_argument('--skpdiff-path', metavar='SKPDIFF_PATH',
+                        default=None,
+                        help='the path to the skpdiff binary to use; ' +
+                        'defaults to out/Release/skpdiff or out/Default/skpdiff'
+                        )
+
+    args = vars(parser.parse_args())  # Convert args into a python dict
+
+    # Make sure we have access to an skpdiff binary
+    skpdiff_path = get_skpdiff_path(args['skpdiff_path'])
+    if skpdiff_path is None:
+        sys.exit(1)
+
+    # Create a temporary file tree that makes sense for skpdiff.to operate on
+    image_output_dir = tempfile.mkdtemp('skpdiff')
+    expected_image_dir = os.path.join(image_output_dir, 'expected')
+    actual_image_dir = os.path.join(image_output_dir, 'actual')
+    os.mkdir(expected_image_dir)
+    os.mkdir(actual_image_dir)
+
+    # Print out the paths of things for easier debugging
+    print('script dir         :', SCRIPT_DIR)
+    print('tools dir          :', TOOLS_DIR)
+    print('root dir           :', SKIA_ROOT_DIR)
+    print('expectations dir   :', args['expectations_dir'])
+    print('skpdiff path       :', skpdiff_path)
+    print('tmp dir            :', image_output_dir)
+    print('expected image dir :', expected_image_dir)
+    print('actual image dir   :', actual_image_dir)
+
+    # Download expected and actual images that differed into the temporary file
+    # tree.
+    download_actuals(args['expectations_dir'],
+                     args['expected'],   args['updated'],
+                     expected_image_dir, actual_image_dir)
+
+    # Invoke skpdiff with our downloaded images and place its results in the
+    # temporary directory.
+    skpdiff_output_path = os.path.join(image_output_dir, 'skpdiff_output.json')
+    skpdiff_cmd = SKPDIFF_INVOKE_FORMAT.format(skpdiff_path,
+                                               skpdiff_output_path,
+                                               expected_image_dir,
+                                               actual_image_dir)
+    os.system(skpdiff_cmd)
+
+    run_server(skpdiff_output_path, port=args['port'])
+
 if __name__ == '__main__':
     main()