rebaseline_server: add ImagePair class, a step towards new intermediate JSON schema

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 3a2ce63b954f6244454e76ddf085e4fca66ef643..936301e1cdef393e736ad852283768482037a63c 100644
--- a/gm/rebaseline_server/imagediffdb.py
+++ b/gm/rebaseline_server/imagediffdb.py
@@ -12,6 +12,7 @@ Calulate differences between image pairs, and store them in a database.
 import contextlib
 import logging
 import os
+import re
 import shutil
 import urllib
 try:
@@ -23,6 +24,8 @@ except ImportError:
 DEFAULT_IMAGE_SUFFIX = '.png'
 DEFAULT_IMAGES_SUBDIR = 'images'
 
+DISALLOWED_FILEPATH_CHAR_REGEX = re.compile('[^\w\-]')
+
 DIFFS_SUBDIR = 'diffs'
 WHITEDIFFS_SUBDIR = 'whitediffs'
 
@@ -61,6 +64,9 @@ class DiffRecord(object):
       actual_images_subdir: the subdirectory actual images are stored in.
       image_suffix: the suffix of images.
     """
+    expected_image_locator = _sanitize_locator(expected_image_locator)
+    actual_image_locator = _sanitize_locator(actual_image_locator)
+
     # Download the expected/actual images, if we don't have them already.
     # TODO(rmistry): Add a parameter that makes _download_and_open_image raise
     # an exception if images are not found locally (instead of trying to
@@ -132,6 +138,16 @@ class DiffRecord(object):
     for each R/G/B channel, as a list."""
     return self._max_diff_per_channel
 
+  def as_dict(self):
+    """Returns a dictionary representation of this DiffRecord, as needed when
+    constructing the JSON representation."""
+    return {
+        'numDifferingPixels': self._num_pixels_differing,
+        'percentDifferingPixels': self.get_percent_pixels_differing(),
+        'weightedDiffMeasure': self.get_weighted_diff_measure(),
+        'maxDiffPerChannel': self._max_diff_per_channel,
+    }
+
 
 class ImageDiffDB(object):
   """ Calculates differences between image pairs, maintaining a database of
@@ -174,6 +190,8 @@ class ImageDiffDB(object):
           actual image within storage_root (probably including a checksum to
           guarantee uniqueness)
     """
+    expected_image_locator = _sanitize_locator(expected_image_locator)

[epoger, 2014/02/07 21:36:17]

I needed to start removing slashes from the image locators, because in the new
approach we will use the partial URLs as image locators...

"imageBUrl”: “gradients_local_perspective/14463581150258917673.png”,

Now, difference images will be stored with names like this:
gradients_local_perspective_111111.png-vs-gradients_local_perspective_22222.png.png

The extra ".png"s are kind of silly, but they won't hurt anyone.

[rmistry, 2014/02/10 17:34:54]

Yes it will not hurt, but out of curiosity what would it take to remove the
extra '.png's?
Is the 'image_suffix=DEFAULT_IMAGE_SUFFIX' causing complications with this?
without it we should be able to strip out the IMAGE_SUFFIX?

[epoger, 2014/02/10 18:01:16]

Here's my thinking; maybe you can help me "thread the needle" on this one.

As noted in the design doc ( https://goto.google.com/ChangingRbsJson ), I want
the intermediate JSON to look like this:

{
  “ImageSets”: [
    {
      “description”: “expected image”,
      “imageUrlBase”:
“http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5”
    },
  ...
  ],
  ...
  "ImagePairs": [
    {
      "imageAUrl”: “gradients_local_perspective/7894968237305173737.png”,
      "imageBUrl”: “gradients_local_perspective/14463581150258917673.png”,
      ...
    },

The idea here is that the URL paths make no assumptions--just assemble
imageUrlBase and imageAUrl, and the client has a link to the image in question.

I could imagine situations when we want those URLs to point at something other
than PNGs.

A couple of thoughts that come to mind:

1. Add "fileSuffix" to ImageSet description ("to generate the image URL, combine
imageUrlBase + imageAUrl + fileSuffix").  This would require that all images in
a given ImageSet have the same suffix.

2. Leave the intermediate JSON as it is, but strip out known file extensions in
_sanitize_locator().  ("If the locator ends in '.png', we know we can strip that
out and the locators will still be unique.")  If we do that, though, when the
client comes back and asks for a diff image, it will ask for
"gradients_local_perspective/7894968237305173737.png-vs-gradients_local_perspective/14463581150258917673.png",
and we'll have to know to convert that to
"gradients_local_perspective/7894968237305173737-vs-gradients_local_perspective/14463581150258917673"


Maybe I should leave it as I have it right now, put in a TODO noting the
discussion, and we can discuss it more live tomorrow?

[rmistry, 2014/02/10 18:09:29]

SGTM. Lets discuss live tomorrow (if weather permits).

[epoger, 2014/02/10 18:18:40]

"If weather permits", indeed.  I'll make a note to discuss it with you SOMETIME
soon.

+    actual_image_locator = _sanitize_locator(actual_image_locator)
     key = (expected_image_locator, actual_image_locator)
     if not key in self._diff_dict:
       try:
@@ -193,7 +211,8 @@ class ImageDiffDB(object):
 
     Raises a KeyError if we don't have a DiffRecord for this image pair.
     """
-    key = (expected_image_locator, actual_image_locator)
+    key = (_sanitize_locator(expected_image_locator),
+           _sanitize_locator(actual_image_locator))
     return self._diff_dict[key]
 
 
@@ -322,6 +341,15 @@ def _mkdir_unless_exists(path):
   if not os.path.isdir(path):
     os.makedirs(path)
 
+def _sanitize_locator(locator):
+  """Returns a sanitized version of a locator (one in which we know none of the
+  characters will have special meaning in filenames).
+
+  Args:
+    locator: string, or something that can be represented as a string
+  """
+  return DISALLOWED_FILEPATH_CHAR_REGEX.sub('_', str(locator))

[rmistry, 2014/02/10 17:34:54]

Isn't the locator always a string? Did it complain about unicode?

[epoger, 2014/02/10 18:01:16]

It will be a string most of the time in production after this change, but in the
past (see unittest data) we used numbers.  That caused failures until I added
the str().

[rmistry, 2014/02/10 18:09:29]

Got it. I was just curious.

[epoger, 2014/02/10 18:18:40]

No problem, thanks for checking into it.

+
 def _get_difference_locator(expected_image_locator, actual_image_locator):
   """Returns the locator string used to look up the diffs between expected_image
   and actual_image.
@@ -330,7 +358,8 @@ def _get_difference_locator(expected_image_locator, actual_image_locator):
     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
+  Returns: already-sanitized locator where the diffs between expected and
+      actual images can be found
   """
-  return "%s-vs-%s" % (expected_image_locator, actual_image_locator)
+  return "%s-vs-%s" % (_sanitize_locator(expected_image_locator),
+                       _sanitize_locator(actual_image_locator))