Modify skpdiff to write diffs directly to provided directories

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 1f939407a8a2e93b2af7d89031f0506a8b39f307..816b43f4857bfc98ec3f57419ca6359cb520b229 100644
--- a/gm/rebaseline_server/imagediffdb.py
+++ b/gm/rebaseline_server/imagediffdb.py
@@ -122,8 +122,10 @@ class DiffRecord(object):
     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.
@@ -134,6 +136,7 @@ class DiffRecord(object):
       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:

@@ -154,8 +157,6 @@ class DiffRecord(object):
       # 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']
@@ -172,22 +173,6 @@ class DiffRecord(object):
       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)