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 104 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?
       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,

[epoger, 2014/08/11 19:33:27]

Instead of adding the --longnames parameter and allowing skpdiff to construct
the filenames, maybe we could do this instead?


diff_image_locator = _get_difference_locator(blah blah blah)
basename = str(diff_image_locator) + image_suffix
rgb_diff_path = os.path.join(storage_root, RGBDIFFS_SUBDIR, basename)
white_diff_path = os.path.join(storage_root, WHITEDIFFS_SUBDIR, basename)

find_run_binary.run_command(
    [SKPDIFF_BINARY, '-p', expected_image_file, actual_image_file,
     '--jsonp', 'false',
     '--output', skpdiff_summary_file,
     '--differs', 'perceptual', 'different_pixels',
     '--rgbDiffPath', rgb_diff_path,
     '--whiteDiffPath', white_diff_path,
     ])

[stephana, 2014/08/11 20:03:57]

That was going to be my first approach (namely to specify the output files
directly).
It's fine when there are only two pairs of images, but if we want to compare
many in a single call to skpdiff I would have to provide a list of output files
and I thought it would become unwieldy and error prone. 


On 2014/08/11 19:33:27, epoger wrote:

            '--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(

[epoger, 2014/08/11 19:33:27]

I *think* this is the only call to _get_difference_locator(), so if we get rid
of it, I think we should delete _get_difference_locator() as well.

BUT, as my note within SkDiffContext.cpp says, I'm worried that moving the
filename-construction logic into skpdiff is not the right direction.

[stephana, 2014/08/11 20:03:57]

Agreed.

On 2014/08/11 19:33:27, epoger wrote:

-          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)
 
   # 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):
@@ skipping 256 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))