rebaseline_server: use just skpdiff, not Python Image Library

gm/rebaseline_server/imagediffdb.py

Index: gm/rebaseline_server/imagediffdb.py
diff --git a/gm/rebaseline_server/imagediffdb.py b/gm/rebaseline_server/imagediffdb.py
index 6b684143b03f256a6a4eee87d43447eabfe713f8..c7b9a75e2b8326f00cac7760f1879ada77e823e5 100644
--- a/gm/rebaseline_server/imagediffdb.py
+++ b/gm/rebaseline_server/imagediffdb.py
@@ -10,7 +10,7 @@ Calulate differences between image pairs, and store them in a database.
 """
 
 import contextlib
-import csv
+import json
 import logging
 import os
 import re
@@ -18,11 +18,6 @@ import shutil
 import sys
 import tempfile
 import urllib
-try:
-  from PIL import Image, ImageChops
-except ImportError:
-  raise ImportError('Requires PIL to be installed; see '
-                    + 'http://www.pythonware.com/products/pil/')
 
 # Set the PYTHONPATH to include the tools directory.
 sys.path.append(
@@ -38,11 +33,9 @@ DEFAULT_IMAGES_SUBDIR = 'images'
 
 DISALLOWED_FILEPATH_CHAR_REGEX = re.compile('[^\w\-]')
 
-DIFFS_SUBDIR = 'diffs'
+RGBDIFFS_SUBDIR = 'diffs'
 WHITEDIFFS_SUBDIR = 'whitediffs'
 
-VALUES_PER_BAND = 256
-
 # Keys used within DiffRecord dictionary representations.
 # NOTE: Keep these in sync with static/constants.js
 KEY__DIFFERENCES__MAX_DIFF_PER_CHANNEL = 'maxDiffPerChannel'
@@ -87,9 +80,8 @@ class DiffRecord(object):
     actual_image_locator = _sanitize_locator(actual_image_locator)
 
     # Download the expected/actual images, if we don't have them already.
-    # TODO(rmistry): Add a parameter that makes _download_and_open_image raise
-    # an exception if images are not found locally (instead of trying to
-    # download them).
+    # TODO(rmistry): Add a parameter that just tries to use already-present
+    # image files rather than downloading them.
     expected_image_file = os.path.join(
         storage_root, expected_images_subdir,
         str(expected_image_locator) + image_suffix)
@@ -97,81 +89,91 @@ class DiffRecord(object):
         storage_root, actual_images_subdir,
         str(actual_image_locator) + image_suffix)
     try:
-      expected_image = _download_and_open_image(
-          expected_image_file, expected_image_url)
+      expected_image = _download_file(expected_image_file, expected_image_url)
     except Exception:
       logging.exception('unable to download expected_image_url %s to file %s' %
                         (expected_image_url, expected_image_file))
       raise
     try:
-      actual_image = _download_and_open_image(
-          actual_image_file, actual_image_url)
+      actual_image = _download_file(actual_image_file, actual_image_url)
     except Exception:
       logging.exception('unable to download actual_image_url %s to file %s' %
                         (actual_image_url, actual_image_file))
       raise
 
-    # Generate the diff image (absolute diff at each pixel) and
-    # max_diff_per_channel.
-    diff_image = _generate_image_diff(actual_image, expected_image)
-    diff_histogram = diff_image.histogram()
-    (diff_width, diff_height) = diff_image.size
-    self._max_diff_per_channel = _max_per_band(diff_histogram)
-
-    # Generate the whitediff image (any differing pixels show as white).
-    # This is tricky, because when you convert color images to grayscale or
-    # black & white in PIL, it has its own ideas about thresholds.
-    # We have to force it: if a pixel has any color at all, it's a '1'.
-    bands = diff_image.split()
-    graydiff_image = ImageChops.lighter(ImageChops.lighter(
-        bands[0], bands[1]), bands[2])
-    whitediff_image = (graydiff_image.point(lambda p: p > 0 and VALUES_PER_BAND)
-                                     .convert('1', dither=Image.NONE))
-
-    # Calculate the perceptual difference percentage.
-    skpdiff_csv_dir = tempfile.mkdtemp()
+    # EPOGER: check for 80 columns throughout

[epoger, 2014/06/12 07:02:07]

These EPOGERs indicate changes that I *know* I want to make before committing
this CL.

+    # Get all diff images and values from skpdiff binary.
+    skpdiff_output_dir = tempfile.mkdtemp()
     try:
-      skpdiff_csv_output = os.path.join(skpdiff_csv_dir, 'skpdiff-output.csv')
+      skpdiff_summary_file = os.path.join(skpdiff_output_dir,
+                                          'skpdiff-output.json')
+      skpdiff_rgbdiff_dir = os.path.join(skpdiff_output_dir, 'rgbDiff')
+      skpdiff_whitediff_dir = os.path.join(skpdiff_output_dir, 'whiteDiff')
       expected_img = os.path.join(storage_root, expected_images_subdir,
                                   str(expected_image_locator) + image_suffix)
       actual_img = os.path.join(storage_root, actual_images_subdir,
                                 str(actual_image_locator) + image_suffix)
+
+      # TODO: Call skpdiff ONCE for all image pairs, instead of calling it
+      # repeatedly.
       find_run_binary.run_command(
           [SKPDIFF_BINARY, '-p', expected_img, actual_img,
-           '--csv', skpdiff_csv_output, '-d', 'perceptual'])
-      with contextlib.closing(open(skpdiff_csv_output)) as csv_file:
-        for row in csv.DictReader(csv_file):
-          perceptual_similarity = float(row[' perceptual'].strip())
-          if not 0 <= perceptual_similarity <= 1:
-            # skpdiff outputs -1 if the images are different sizes. Treat any
-            # output that does not lie in [0, 1] as having 0% perceptual
-            # similarity.
-            perceptual_similarity = 0
-          # skpdiff returns the perceptual similarity, convert it to get the
-          # perceptual difference percentage.
-          self._perceptual_difference = 100 - (perceptual_similarity * 100)
+           '--jsonp', 'false',
+           '--output', skpdiff_summary_file,
+           '--differs', 'perceptual', 'different_pixels',

[epoger, 2014/06/12 07:02:07]

This is one cause of the slowdown... we just to tell skpdiff to only run the
"perceptual" differ; now we tell it to also run the "different_pixels" differ.

[djsollen, 2014/06/12 13:27:00]

If you're not specifying the number of threads it should default to using as
many as it can.  Which should run those in diffs in parallel, if you detect it
isn't then we need to fix that.

This should be much slower than running them all in one shot as the perceptual
diff always takes longer and so some of the time it is running on a single core.
 If you do them all at once then the all cores should stay busier longer getting
you better throughput.

[epoger, 2014/06/12 14:02:34]

Good explanation, thanks.  I definitely want to explore batching multiple image
pairs.  I think we can leave that for a future CL.

+           '--rgbDiffDir', skpdiff_rgbdiff_dir,
+           '--whiteDiffDir', skpdiff_whitediff_dir,
+           ])
+
+      # Get information out of the skpdiff_summary_file.
+      with contextlib.closing(open(skpdiff_summary_file)) as fp:
+        data = json.load(fp)
+
+      # For now, we can assume there is only one record in the output summary,
+      # since we passed skpdiff only one pair of images.
+      record = data['records'][0]
+      self._width = record['width']
+      self._height = record['height']
+      self._max_diff_per_channel = [
+          record['maxRedDiff'], record['maxGreenDiff'], record['maxBlueDiff']]
+      per_differ_stats = record['diffs']
+      for stats in per_differ_stats:
+        differ_name = stats['differName']
+        if differ_name == 'different_pixels':
+          self._num_pixels_differing = stats['pointsOfInterest']
+        elif differ_name == 'perceptual':
+          perceptual_similarity = stats['result']
+
+      # skpdiff returns the perceptual similarity; convert it to get the
+      # perceptual difference percentage.
+      # skpdiff outputs -1 if the images are different sizes. Treat any
+      # output that does not lie in [0, 1] as having 0% perceptual
+      # similarity.
+      if not 0 <= perceptual_similarity <= 1:
+        perceptual_similarity = 0
+      self._perceptual_difference = 100 - (perceptual_similarity * 100)
+
+      # Store the rgbdiff and whitediff images generated above.
+      diff_image_locator = _get_difference_locator(
+          expected_image_locator=expected_image_locator,
+          actual_image_locator=actual_image_locator)
+      basename = str(diff_image_locator) + image_suffix
+      _mkdir_unless_exists(os.path.join(storage_root, RGBDIFFS_SUBDIR))
+      _mkdir_unless_exists(os.path.join(storage_root, WHITEDIFFS_SUBDIR))
+      # TODO: we could just os.rename() the file, if we knew that
+      # skpdiff_whitediff_dir was on the same filesystem.  But it's not.
+      #
+      # EPOGER: instead of _find_single_file_in_dir(), we can now use

[epoger, 2014/06/12 07:02:07]

If we could make skpdiff write its output directly into the desired directory,
that would make us a bit faster...

[djsollen, 2014/06/12 13:26:59]

That shouldn't be too hard.

[epoger, 2014/06/12 14:02:34]

The main problem is that right now you can tell skpdiff the DIRECTORY to write
its files into, but not the FILENAME to use.

+      # rgbDiffPath and whiteDiffPath within the skpdiff summary
+      shutil.copyfile(_find_single_file_in_dir(skpdiff_rgbdiff_dir),
+                      os.path.join(storage_root, RGBDIFFS_SUBDIR, basename))
+      shutil.copyfile(_find_single_file_in_dir(skpdiff_whitediff_dir),
+                      os.path.join(storage_root, WHITEDIFFS_SUBDIR, basename))
+
     finally:
-      shutil.rmtree(skpdiff_csv_dir)
-
-    # Final touches on diff_image: use whitediff_image as an alpha mask.
-    # Unchanged pixels are transparent; differing pixels are opaque.
-    diff_image.putalpha(whitediff_image)
-
-    # Store the diff and whitediff images generated above.
-    diff_image_locator = _get_difference_locator(
-        expected_image_locator=expected_image_locator,
-        actual_image_locator=actual_image_locator)
-    basename = str(diff_image_locator) + image_suffix
-    _save_image(diff_image, os.path.join(
-        storage_root, DIFFS_SUBDIR, basename))
-    _save_image(whitediff_image, os.path.join(
-        storage_root, WHITEDIFFS_SUBDIR, basename))
-
-    # Calculate difference metrics.
-    (self._width, self._height) = diff_image.size
-    self._num_pixels_differing = (
-        whitediff_image.histogram()[VALUES_PER_BAND - 1])
+      shutil.rmtree(skpdiff_output_dir)

[epoger, 2014/06/12 07:02:07]

All of these writing files and then deleting directories must be a waste of
time...

 
+  # EPOGER: how many of these methods are no longer used?
   def get_num_pixels_differing(self):
     """Returns the absolute number of pixels that differ."""
     return self._num_pixels_differing
@@ -305,75 +307,34 @@ def _max_per_band(histogram):
   return max_per_band
 
 
-def _generate_image_diff(image1, image2):
-  """Wrapper for ImageChops.difference(image1, image2) that will handle some
-  errors automatically, or at least yield more useful error messages.
-
-  TODO(epoger): Currently, some of the images generated by the bots are RGBA
-  and others are RGB.  I'm not sure why that is.  For now, to avoid confusion
-  within the UI, convert all to RGB when diffing.
-
-  Args:
-    image1: a PIL image object
-    image2: a PIL image object
-
-  Returns: per-pixel diffs between image1 and image2, as a PIL image object
-  """
-  try:
-    return ImageChops.difference(image1.convert('RGB'), image2.convert('RGB'))
-  except ValueError:
-    logging.error('Error diffing image1 [%s] and image2 [%s].' % (
-        repr(image1), repr(image2)))
-    raise
-
-
-def _download_and_open_image(local_filepath, url):
-  """Open the image at local_filepath; if there is no file at that path,
-  download it from url to that path and then open it.
+def _download_file(local_filepath, url):
+  """Download a file from url to local_filepath, unless it is already there.
 
   Args:
     local_filepath: path on local disk where the image should be stored
     url: URL from which we can download the image if we don't have it yet
-
-  Returns: a PIL image object
   """
   if not os.path.exists(local_filepath):
     _mkdir_unless_exists(os.path.dirname(local_filepath))
     with contextlib.closing(urllib.urlopen(url)) as url_handle:
       with open(local_filepath, 'wb') as file_handle:
         shutil.copyfileobj(fsrc=url_handle, fdst=file_handle)
-  return _open_image(local_filepath)
-
 
-def _open_image(filepath):
-  """Wrapper for Image.open(filepath) that yields more useful error messages.
-
-  Args:
-    filepath: path on local disk to load image from
-
-  Returns: a PIL image object
-  """
-  try:
-    return Image.open(filepath)
-  except IOError:
-    # If we are unable to load an image from the file, delete it from disk
-    # and we will try to fetch it again next time.  Fixes http://skbug.com/2247
-    logging.error('IOError loading image file %s ; deleting it.' % filepath)
-    os.remove(filepath)
-    raise
 
+def _find_single_file_in_dir(dirpath):
+  """Returns the full path of the lone file within this directory.
 
-def _save_image(image, filepath, format='PNG'):
-  """Write an image to disk, creating any intermediate directories as needed.
+  If the directory does not exist, contains no files, or contains multiple
+  files, this will raise an exception.
 
   Args:
-    image: a PIL image object
-    filepath: path on local disk to write image to
-    format: one of the PIL image formats, listed at
-            http://effbot.org/imagingbook/formats.htm
+    dirpath: path to a directory on local disk
   """
-  _mkdir_unless_exists(os.path.dirname(filepath))
-  image.save(filepath, format)
+  filenames = os.listdir(dirpath)
+  if len(filenames) != 1:
+    raise Exception('dir %s does not contain exactly one file: %s' % (
+        dirpath, filenames))
+  return os.path.join(dirpath, filenames[0])
 
 
 def _mkdir_unless_exists(path):