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

gm/rebaseline_server/imagediffdb.py

Index: gm/rebaseline_server/imagediffdb.py
===================================================================
--- gm/rebaseline_server/imagediffdb.py	(revision 12765)
+++ gm/rebaseline_server/imagediffdb.py	(working copy)
@@ -20,9 +20,9 @@
   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'
 
@@ -34,7 +34,11 @@
 
   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.
 
@@ -54,15 +58,26 @@
       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
@@ -89,10 +104,14 @@
     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(