Modify skpdiff to write diffs directly to provided directories

gm/rebaseline_server/imagediffdb.py

 #!/usr/bin/python
 
 """
 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.
 """
@@ skipping 107 matching lines @@
     # Return early if we do not need to generate diffs.
     if (expected_image_url == actual_image_url or
         not expected_image_url or not actual_image_url):
       return
 
     # Get all diff images and values using the skpdiff binary.
     skpdiff_output_dir = tempfile.mkdtemp()
     try:
       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')
+      skpdiff_rgbdiff_dir = os.path.join(storage_root, RGBDIFFS_SUBDIR)
+      skpdiff_whitediff_dir = os.path.join(storage_root, WHITEDIFFS_SUBDIR)
+      _mkdir_unless_exists(skpdiff_rgbdiff_dir)
+      _mkdir_unless_exists(skpdiff_rgbdiff_dir)
 
       # TODO(epoger): Consider calling skpdiff ONCE for all image pairs,
       # instead of calling it separately for each image pair.
       # Pro: we'll incur less overhead from making repeated system calls,
       # spinning up the skpdiff binary, etc.
       # Con: we would have to wait until all image pairs were loaded before
       # generating any of the diffs?
+      # Note(stephana): '--longnames' was added to allow for this 
+      # case (multiple files at once) versus specifying output diffs 
+      # directly.
       find_run_binary.run_command(
           [SKPDIFF_BINARY, '-p', expected_image_file, actual_image_file,
            '--jsonp', 'false',
+           '--longnames', 'true',
            '--output', skpdiff_summary_file,
            '--differs', 'perceptual', 'different_pixels',
            '--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']
       # TODO: make max_diff_per_channel a tuple instead of a list, because the
       # structure is meaningful (first element is red, second is green, etc.)
       # See http://stackoverflow.com/a/626871
       self._max_diff_per_channel = [
           record['maxRedDiff'], record['maxGreenDiff'], record['maxBlueDiff']]
-      rgb_diff_path = record['rgbDiffPath']
-      white_diff_path = record['whiteDiffPath']
       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: Modify skpdiff's behavior so we can tell it exactly where to
-      # write the image files into, rather than having to move them around
-      # after skpdiff writes them out.
-      shutil.copyfile(rgb_diff_path,
-                      os.path.join(storage_root, RGBDIFFS_SUBDIR, basename))
-      shutil.copyfile(white_diff_path,
-                      os.path.join(storage_root, WHITEDIFFS_SUBDIR, basename))
-
     finally:
       shutil.rmtree(skpdiff_output_dir)
 
+
+
+  @staticmethod 
+  def get_diff_records(gs, storage_root,
+                       images_to_compare, 
+                       expected_images_subdir=DEFAULT_IMAGES_SUBDIR,
+                       actual_images_subdir=DEFAULT_IMAGES_SUBDIR,
+                       image_suffix=DEFAULT_IMAGE_SUFFIX):
+    # TODO: stub method to be filled in 

[epoger, 2014/08/12 18:03:40]

Do you intend to fill in this method before committing the CL?

[stephana, 2014/08/12 19:25:13]

No. That's an artifact. Will remove it. 

On 2014/08/12 18:03:40, epoger wrote:

+    diff_records = []
+    return diff_records
+
+  
   # TODO(epoger): Use properties instead of getters throughout.
   # See http://stackoverflow.com/a/6618176
   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) /
@@ skipping 282 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))