rebaseline_server: use just skpdiff, not Python Image Library

gm/rebaseline_server/imagediffdb.py

 #!/usr/bin/python

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

Toplevel comments:

This CL is not ready for line-by-line review yet.  Instead, I'd like to use it
as fodder for discussion.

Before this CL, rebaseline_server uses two subsystems to generate diffs:
1. Python Image Library to generate diff images
2. skpdiff to calculate perceptual diff measurements

My guess was that this was wasteful, and it would be faster to just call skpdiff
to do *everything* rather than perform any image-diffing operations in
Python-land.

However, it seems like it is actually SLIGHTLY SLOWER after this CL.
Before this CL: took 128 seconds to prepare diffs for all 55 builders
After this CL: took 130 seconds

I tried to make sure that all of the source images were already downloaded to
local disk before launching those tests, so I think all that time is being spent
on performing the diffs, not downloading files.  (But I should make absolutely
sure of that.)

I have these hopes for improving performance in an skpdiff world, which we could
not do in a PIL world:
1. Send image pairs into skpdiff in batches, rather than one pair at a time
2. Tweak performance of skpdiff binary (including building it in Release mode,
not Debug)

I hope we get a chance to discuss this live sometime soon.

Also- right now, I have 2 rebaseline_server instances running, one with the old
code (before this CL) and one with the new code.  Feel free to check 'em out...
I think their results are identical.
Before: http://172.24.179.183:1111
After: http://172.24.179.183:2222

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

Building in Release should get a speed up in that the compiler should optimize
more, but looking through the code I didn't see any ASSERTS in the tight loops. 
The other promising thing to do would be to use the multithreading option.

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

Right.  I guess to get the most out of multithreading (within skpdiff, I guess
you mean), I'll need to pass a whole batch of image pairs into skpdiff.  Which
will present its own problems.  But I should be able to take advantage of that
in a future CL.

I also want to re-evaluate performance of this at a high level-- I want to
figure out where most of the time is going.  I am beginning to suspect that the
perceptual diff metric is our performance bottleneck, and I don't think it's
providing all that much value (compared to simpler metrics like the old
weightedDiffMeasure).

Also, I'm sure there's lots of fat to trim from the "first run" case (which
downloads all the needed image files), which is probably most painful for
developers doing rebaselines.

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

My main concern is that we are passing the same image pairs to TWO places (PIL
and skpdiff) and thus duplicating much of the work (looping over all the pixels
and comparing them).

I think it's also possible that doing everything in PIL and kicking skpdiff to
the curb would work reasonably well.  Another way of thinking about that: making
our own contributions to the PIL native code to perform the particular
operations we want.
No, I haven't been, and I agree that would be good to do.  Thank you for the
offer.  Let's see where this goes--after my vacation I may well take you up on
it.

 
 """
 Copyright 2013 Google Inc.
 
 Use of this source code is governed by a BSD-style license that can be
 found in the LICENSE file.
 
 Calulate differences between image pairs, and store them in a database.
 """
 
 import contextlib
-import csv
+import json
 import logging
 import os
 import re
 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(
     os.path.join(
         os.path.dirname(os.path.realpath(__file__)), os.pardir, os.pardir,
                         'tools'))
 import find_run_binary
 
 SKPDIFF_BINARY = find_run_binary.find_path_to_program('skpdiff')
 
 DEFAULT_IMAGE_SUFFIX = '.png'
 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'
 KEY__DIFFERENCES__NUM_DIFF_PIXELS = 'numDifferingPixels'
 KEY__DIFFERENCES__PERCENT_DIFF_PIXELS = 'percentDifferingPixels'
 KEY__DIFFERENCES__PERCEPTUAL_DIFF = 'perceptualDifference'
 
 
 class DiffRecord(object):
   """ Record of differences between two images. """
@@ skipping 24 matching lines @@
           actual image within storage_root (probably including a checksum to
           guarantee uniqueness)
       expected_images_subdir: the subdirectory expected images are stored in.
       actual_images_subdir: the subdirectory actual images are stored in.
       image_suffix: the suffix of images.
     """
     expected_image_locator = _sanitize_locator(expected_image_locator)
     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)
     actual_image_file = os.path.join(
         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)
+      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...

 
-    # 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])
-
+  # 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
 
   def get_percent_pixels_differing(self):
     """Returns the percentage of pixels that differ, as a float between
     0 and 100 (inclusive)."""
     return ((float(self._num_pixels_differing) * 100) /
             (self._width * self._height))
 
@@ skipping 113 matching lines @@
     min_index = band * VALUES_PER_BAND
     index = min_index + VALUES_PER_BAND
     while index > min_index:
       index -= 1
       if histogram[index] > 0:
         max_per_band.append(index - min_index)
         break
   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.
+def _find_single_file_in_dir(dirpath):
+  """Returns the full path of the lone file within this directory.
+
+  If the directory does not exist, contains no files, or contains multiple
+  files, this will raise an exception.
 
   Args:
-    filepath: path on local disk to load image from
-
-  Returns: a PIL image object
+    dirpath: path to a directory on local disk
   """
-  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 _save_image(image, filepath, format='PNG'):
-  """Write an image to disk, creating any intermediate directories as needed.
-
-  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
-  """
-  _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):
   """Unless path refers to an already-existing directory, create it.
 
   Args:
     path: path on local disk
   """
   if not os.path.isdir(path):
     os.makedirs(path)
@@ skipping 18 matching lines @@
 
   Args:
     expected_image_locator: locator string pointing at expected image
     actual_image_locator: locator string pointing at actual image
 
   Returns: already-sanitized locator where the diffs between expected and
       actual images can be found
   """
   return "%s-vs-%s" % (_sanitize_locator(expected_image_locator),
                        _sanitize_locator(actual_image_locator))