Adding additional optional parameters to imagediffdb to make calling from Cluster Telemetry possibl…

gm/rebaseline_server/imagediffdb.py

 #!/usr/bin/python
 
 """

[epoger, 2013/12/20 19:46:37]

Have you confirmed that its self-test still passes?  (That self-test is not run
automatically yet, so we need to check it manually)

[rmistry, 2013/12/20 20:00:04]

Yes I ran it before and after my changes.

 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 logging
 import os
 import shutil
 import urllib
 try:
   from PIL import Image, ImageChops
 except ImportError:
   raise ImportError('Requires PIL to be installed; see '
                     + 'http://www.pythonware.com/products/pil/')
 
-IMAGE_SUFFIX = '.png'
+DEFAULT_IMAGE_SUFFIX = '.png'
+DEFAULT_IMAGES_SUBDIR = 'images'
 
-IMAGES_SUBDIR = 'images'
 DIFFS_SUBDIR = 'diffs'
 WHITEDIFFS_SUBDIR = 'whitediffs'
 
 VALUES_PER_BAND = 256
 
 
 class DiffRecord(object):
   """ Record of differences between two images. """
 
   def __init__(self, storage_root,
                expected_image_url, expected_image_locator,
-               actual_image_url, actual_image_locator):
+               actual_image_url, actual_image_locator,
+               images_subdir=DEFAULT_IMAGES_SUBDIR,
+               actual_images_subdir=DEFAULT_IMAGES_SUBDIR,
+               use_diff_image_locator=True,
+               image_suffix=DEFAULT_IMAGE_SUFFIX):
     """Download this pair of images (unless we already have them on local disk),
     and prepare a DiffRecord for them.
 
     TODO(epoger): Make this asynchronously download images, rather than blocking
     until the images have been downloaded and processed.
 
     Args:
       storage_root: root directory on local disk within which we store all
           images
       expected_image_url: file or HTTP url from which we will download the
           expected image
       expected_image_locator: a unique ID string under which we will store the
           expected image within storage_root (probably including a checksum to
           guarantee uniqueness)
       actual_image_url: file or HTTP url from which we will download the
           actual image
       actual_image_locator: a unique ID string under which we will store the
           actual image within storage_root (probably including a checksum to
           guarantee uniqueness)
+      images_subdir: the subdirectory images are stored in. Optional parameter,

[epoger, 2013/12/20 19:46:37]

what are "images" as opposed to "actual images"?  perhaps expected_images_subdir
?

Tell me if I have this right: in my existing code, the assumption is that it's
fine to store both expected and actual images in the same dir, because they are
named using checksums and thus we don't have to worry about name collisions. 
But you are using filenames that do NOT include checksums, so you expect to have
two different images with the same filename (and thus must store them in
different dirs).  Right?

[rmistry, 2013/12/20 20:00:04]

Ah right. Done.
Exactly.

+          the default value is in DEFAULT_IMAGES_SUBDIR.

[epoger, 2013/12/20 19:46:37]

Thank you for being descriptive, but I think we're actually better off *without*
the "Optional parameter, ..." bit throughout.  That is clearly expressed in the
method signature; let's not duplicate that documentation.

[rmistry, 2013/12/20 20:00:04]

Done.

+      actual_images_subdir: the subdirectory actual images are stored in.
+          Optional parameter, the default value is in DEFAULT_IMAGES_SUBDIR.
+      use_diff_image_locator: If True outputs differences as
+          'expected_image_locator-vs-actual_image_locator' else outputs
+          differences using the expected_image_locator if expected_image_locator

[epoger, 2013/12/20 19:46:37]

Confusing description... so, if this is False, but expected_image_locator !=
actual_image_locator, what then?

Proposal: eliminate this new parameter, and always name the differences
'expected_image_locator-vs-actual_image_locator'.  OK, so the filename will be
repetitive ('youtube-vs-youtube.png'), but that seems like less of a worry than
varying rules on filename construction.

[rmistry, 2013/12/20 20:00:04]

Agreed. Done.

+          is equal to the actual_image_locator. Optional parameter, the default
+          value is True.
+      image_suffix: the suffix of images. Optional parameter, the default
+          value is in DEFAULT_IMAGE_SUFFIX.
     """
     # Download the expected/actual images, if we don't have them already.
     expected_image = _download_and_open_image(
-        os.path.join(storage_root, IMAGES_SUBDIR,
-                     str(expected_image_locator) + IMAGE_SUFFIX),
+        os.path.join(storage_root, images_subdir,
+                     str(expected_image_locator) + image_suffix),

[epoger, 2013/12/20 19:46:37]

Warning: if you are not using checksums to generate the image locators,
download_and_open_image() may say "oh, you already have 'youtube.png' in this
directory, I don't need to download it" and then you'll get old images that will
confuse the heck out of you.

Workarounds:
1. use checksums within your image locators (so a different bitmap always means
a different locator)
2. clear out the directory between runs  (downside: you will have to download
every image every time, you won't benefit from cacheing)

[rmistry, 2013/12/20 20:00:04]

I tested this through the python interpreter and it seemed to work ok. But I
will keep this in mind incase I see strange behavior, in which case I can clear
out the directory between runs.

Also about the downloading part: all my files are available locally, when I call
this util I will actually pass in 'None' urls for download to verify that it
does not try to go get something.

[epoger, 2013/12/20 20:05:08]

Sounds like a good idea.  More than anything, I wanted to make sure you knew
about this possibility.

You might actually consider adding a parameter to the object, telling it whether
to download images or to assume that they will already be available on local
disk (and raise an Exception otherwise).  As a TODO.

[rmistry, 2013/12/20 20:53:27]

Added a TODO.

         expected_image_url)
     actual_image = _download_and_open_image(
-        os.path.join(storage_root, IMAGES_SUBDIR,
-                     str(actual_image_locator) + IMAGE_SUFFIX),
+        os.path.join(storage_root, actual_images_subdir,
+                     str(actual_image_locator) + image_suffix),
         actual_image_url)
 
     # 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._weighted_diff_measure = _calculate_weighted_diff_metric(
         diff_histogram, diff_width * diff_height)
     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))
 
     # 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
+    if not use_diff_image_locator and (
+        expected_image_locator == actual_image_locator):
+      basename = expected_image_locator + image_suffix
+    else:
+      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])
 
@@ skipping 212 matching lines @@
   and actual_image.
 
   Args:
     expected_image_locator: locator string pointing at expected image
     actual_image_locator: locator string pointing at actual image
 
   Returns: locator where the diffs between expected and actual images can be
       found
   """
   return "%s-vs-%s" % (expected_image_locator, actual_image_locator)