make compare_rendered_pictures process render_pictures's new JSON output format

gm/rebaseline_server/compare_rendered_pictures.py

Index: gm/rebaseline_server/compare_rendered_pictures.py
diff --git a/gm/rebaseline_server/compare_rendered_pictures.py b/gm/rebaseline_server/compare_rendered_pictures.py
index 80a42e51f9b1e83c1481c93a7e268d4c7668a0bb..67903ead8e48a9db3c6019a3bd04df11026b535e 100755
--- a/gm/rebaseline_server/compare_rendered_pictures.py
+++ b/gm/rebaseline_server/compare_rendered_pictures.py
@@ -39,9 +39,6 @@ import imagepair
 import imagepairset
 import results
 
-# Characters we don't want popping up just anywhere within filenames.
-DISALLOWED_FILEPATH_CHAR_REGEX = re.compile('[^\w\-]')
-
 # URL under which all render_pictures images can be found in Google Storage.
 # TODO(epoger): Move this default value into
 # https://skia.googlesource.com/buildbot/+/master/site_config/global_variables.json
@@ -97,9 +94,9 @@ class RenderedPicturesComparisons(results.BaseComparisons):
         'Reading actual-results JSON files from %s subdirs within %s...' % (
             subdirs, actuals_root))
     subdirA, subdirB = subdirs
-    subdirA_builder_dicts = self._read_dicts_from_root(
+    subdirA_dicts = self._read_dicts_from_root(
         os.path.join(actuals_root, subdirA))
-    subdirB_builder_dicts = self._read_dicts_from_root(
+    subdirB_dicts = self._read_dicts_from_root(
         os.path.join(actuals_root, subdirB))
     logging.info('Comparing subdirs %s and %s...' % (subdirA, subdirB))
 
@@ -122,87 +119,143 @@ class RenderedPicturesComparisons(results.BaseComparisons):
             results.KEY__RESULT_TYPE__NOCOMPARISON,
         ])
 
-    builders = sorted(set(subdirA_builder_dicts.keys() +
-                          subdirB_builder_dicts.keys()))
-    num_builders = len(builders)
-    builder_num = 0
-    for builder in builders:
-      builder_num += 1
-      logging.info('Generating pixel diffs for builder #%d of %d, "%s"...' %
-                   (builder_num, num_builders, builder))
-      # TODO(epoger): This will fail if we have results for this builder in
-      # subdirA but not subdirB (or vice versa).
-      subdirA_results = results.BaseComparisons.combine_subdicts(
-          subdirA_builder_dicts[builder][gm_json.JSONKEY_ACTUALRESULTS])
-      subdirB_results = results.BaseComparisons.combine_subdicts(
-          subdirB_builder_dicts[builder][gm_json.JSONKEY_ACTUALRESULTS])
-      image_names = sorted(set(subdirA_results.keys() +
-                               subdirB_results.keys()))
-      for image_name in image_names:
-        # The image name may contain funny characters or be ridiculously long
-        # (see https://code.google.com/p/skia/issues/detail?id=2344#c10 ),
-        # so make sure we sanitize it before using it in a URL path.
-        #
-        # TODO(epoger): Rather than sanitizing/truncating the image name here,
-        # do it in render_pictures instead.
-        # Reason: we will need to be consistent in applying this rule, so that
-        # the process which uploads the files to GS using these paths will
-        # match the paths created by downstream processes.
-        # So, we should make render_pictures write out images to paths that are
-        # "ready to upload" to Google Storage, like gm does.
-        sanitized_test_name = DISALLOWED_FILEPATH_CHAR_REGEX.sub(
-            '_', image_name)[:30]
-
-        subdirA_image_relative_url = (
-            results.BaseComparisons._create_relative_url(
-                hashtype_and_digest=subdirA_results.get(image_name),
-                test_name=sanitized_test_name))
-        subdirB_image_relative_url = (
-            results.BaseComparisons._create_relative_url(
-                hashtype_and_digest=subdirB_results.get(image_name),
-                test_name=sanitized_test_name))
-
-        # If we have images for at least one of these two subdirs,
-        # add them to our list.
-        if subdirA_image_relative_url or subdirB_image_relative_url:
-          if subdirA_image_relative_url == subdirB_image_relative_url:
-            result_type = results.KEY__RESULT_TYPE__SUCCEEDED
-          elif not subdirA_image_relative_url:
-            result_type = results.KEY__RESULT_TYPE__NOCOMPARISON
-          elif not subdirB_image_relative_url:
-            result_type = results.KEY__RESULT_TYPE__NOCOMPARISON
-          else:
-            result_type = results.KEY__RESULT_TYPE__FAILED
-
-        extra_columns_dict = {
-            results.KEY__EXTRACOLUMN__RESULT_TYPE: result_type,
-            results.KEY__EXTRACOLUMN__BUILDER: builder,
-            results.KEY__EXTRACOLUMN__TEST: image_name,
-            # TODO(epoger): Right now, the client UI crashes if it receives
-            # results that do not include a 'config' column.
-            # Until we fix that, keep the client happy.
-            results.KEY__EXTRACOLUMN__CONFIG: 'TODO',
-        }
-
-        try:
-          image_pair = imagepair.ImagePair(
-              image_diff_db=self._image_diff_db,
-              base_url=self._image_base_url,
-              imageA_relative_url=subdirA_image_relative_url,
-              imageB_relative_url=subdirB_image_relative_url,
-              extra_columns=extra_columns_dict)
-          all_image_pairs.add_image_pair(image_pair)
-          if result_type != results.KEY__RESULT_TYPE__SUCCEEDED:
-            failing_image_pairs.add_image_pair(image_pair)
-        except (KeyError, TypeError):
-          logging.exception(
-              'got exception while creating ImagePair for image_name '
-              '"%s", builder "%s"' % (image_name, builder))
+    common_dict_paths = sorted(set(subdirA_dicts.keys() + subdirB_dicts.keys()))
+    num_common_dict_paths = len(common_dict_paths)
+    dict_num = 0
+    for dict_path in common_dict_paths:
+      dict_num += 1
+      logging.info('Generating pixel diffs for dict #%d of %d, "%s"...' %
+                   (dict_num, num_common_dict_paths, dict_path))
+      dictA = subdirA_dicts[dict_path]
+      dictB = subdirB_dicts[dict_path]
+      self._validate_dict_version(dictA)
+      self._validate_dict_version(dictB)
+      dictA_results = dictA[gm_json.JSONKEY_ACTUALRESULTS]
+      dictB_results = dictB[gm_json.JSONKEY_ACTUALRESULTS]
+      skp_names = sorted(set(dictA_results.keys() + dictB_results.keys()))
+      for skp_name in skp_names:
+        imagepairs_for_this_skp = []
+
+        whole_image_A = dictA_results[skp_name].get(
+            gm_json.JSONKEY_SOURCE_WHOLEIMAGE, None)

[borenet, 2014/05/06 14:35:48]

Is it possible that dictA contains SKPs not in dictB? If so, this will cause an
error.

[epoger, 2014/05/06 15:13:53]

Good catch, thank you!

I added this case to the unittest data, and fixed the code to handle it.

+        whole_image_B = dictB_results[skp_name].get(
+            gm_json.JSONKEY_SOURCE_WHOLEIMAGE, None)
+        imagepairs_for_this_skp.append(self._create_image_pair(
+            test=skp_name, config=gm_json.JSONKEY_SOURCE_WHOLEIMAGE,
+            image_dict_A=whole_image_A, image_dict_B=whole_image_B))
+
+        tiled_images_A = dictA_results[skp_name].get(
+            gm_json.JSONKEY_SOURCE_TILEDIMAGES, None)
+        tiled_images_B = dictB_results[skp_name].get(
+            gm_json.JSONKEY_SOURCE_TILEDIMAGES, None)
+        # TODO(epoger): Report an error if we find tiles for A but not B?
+        if tiled_images_A and tiled_images_B:
+          # TODO(epoger): Report an error if we find a different number of tiles
+          # for A and B?
+          num_tiles = len(tiled_images_A)
+          for tile_num in range(num_tiles):
+            imagepairs_for_this_skp.append(self._create_image_pair(
+                test=skp_name,
+                config='%s-%d' % (gm_json.JSONKEY_SOURCE_TILEDIMAGES, tile_num),
+                image_dict_A=tiled_images_A[tile_num],
+                image_dict_B=tiled_images_B[tile_num]))
+
+        for imagepair in imagepairs_for_this_skp:
+          if imagepair:
+            all_image_pairs.add_image_pair(imagepair)
+            result_type = imagepair.extra_columns_dict\
+                [results.KEY__EXTRACOLUMN__RESULT_TYPE]
+            if result_type != results.KEY__RESULT_TYPE__SUCCEEDED:
+              failing_image_pairs.add_image_pair(imagepair)
 
     self._results = {
       results.KEY__HEADER__RESULTS_ALL: all_image_pairs.as_dict(),
       results.KEY__HEADER__RESULTS_FAILURES: failing_image_pairs.as_dict(),
     }
 
+  def _validate_dict_version(self, result_dict):
+    """Raises Exception if the dict is not the type/version we know how to read.
+
+    Args:
+      result_dict: dictionary holding output of render_pictures
+    """
+    expected_header_type = 'ChecksummedImages'
+    expected_header_revision = 1
+
+    header = result_dict[gm_json.JSONKEY_HEADER]
+    header_type = header[gm_json.JSONKEY_HEADER_TYPE]
+    if header_type != expected_header_type:
+      raise Exception('expected header_type "%s", but got "%s"' % (
+          expected_header_type, header_type))
+    header_revision = header[gm_json.JSONKEY_HEADER_REVISION]
+    if header_revision != expected_header_revision:
+      raise Exception('expected header_revision %d, but got %d' % (
+          expected_header_revision, header_revision))
+
+  def _create_image_pair(self, test, config, image_dict_A, image_dict_B):
+    """Creates an ImagePair object for this pair of images.
+
+    Args:
+      test: string; name of the test
+      config: string; name of the config
+      image_dict_A: dict with JSONKEY_IMAGE_* keys, or None if no image
+      image_dict_B: dict with JSONKEY_IMAGE_* keys, or None if no image
+
+    Returns:
+      An ImagePair object, or None if both image_dict_A and image_dict_B are
+      None.
+    """
+    if (not image_dict_A) and (not image_dict_B):
+      return None
+
+    if image_dict_A:
+      imageA_relative_url = image_dict_A[gm_json.JSONKEY_IMAGE_FILEPATH]
+      imageA_checksum = (image_dict_A[gm_json.JSONKEY_IMAGE_CHECKSUMALGORITHM],
+                         image_dict_A[gm_json.JSONKEY_IMAGE_CHECKSUMVALUE])
+    else:
+      imageA_relative_url = None
+      imageA_checksum = None
+
+    if image_dict_B:
+      imageB_relative_url = image_dict_B[gm_json.JSONKEY_IMAGE_FILEPATH]
+      imageB_checksum = (image_dict_B[gm_json.JSONKEY_IMAGE_CHECKSUMALGORITHM],
+                         image_dict_B[gm_json.JSONKEY_IMAGE_CHECKSUMVALUE])
+    else:
+      imageB_relative_url = None
+      imageB_checksum = None

[borenet, 2014/05/06 14:35:48]

An inline function might be nice to remove this duplication:

def _checksum_and_relative_url(d):
  if d:
    return (d[gm_json.JSONKEY_IMAGE_FILEPATH],
            (d[gm_json.JSONKEY_IMAGE_CHECKSUMALGORITHM],
             d[gm_json.JSONKEY_IMAGE_CHECKSUMVALUE]))
  else:
    return None, None

imageA_relative_url, imageA_checksum = _checksum_and_relative_url(image_dict_A)
imageB_relative_url, imageB_checksum = _checksum_and_relative_url(image_dict_B)

[epoger, 2014/05/06 15:13:53]

Good idea, done!

+
+    if not imageA_checksum:
+      result_type = results.KEY__RESULT_TYPE__NOCOMPARISON
+    elif not imageB_checksum:
+      result_type = results.KEY__RESULT_TYPE__NOCOMPARISON
+    elif imageA_checksum == imageB_checksum:
+      result_type = results.KEY__RESULT_TYPE__SUCCEEDED
+    else:
+      result_type = results.KEY__RESULT_TYPE__FAILED
+
+    extra_columns_dict = {
+        results.KEY__EXTRACOLUMN__CONFIG: config,
+        results.KEY__EXTRACOLUMN__RESULT_TYPE: result_type,
+        results.KEY__EXTRACOLUMN__TEST: test,
+        # TODO(epoger): Right now, the client UI crashes if it receives
+        # results that do not include this column.
+        # Until we fix that, keep the client happy.
+        results.KEY__EXTRACOLUMN__BUILDER: 'TODO',
+    }
+
+    try:
+      return imagepair.ImagePair(
+          image_diff_db=self._image_diff_db,
+          base_url=self._image_base_url,
+          imageA_relative_url=imageA_relative_url,
+          imageB_relative_url=imageB_relative_url,
+          extra_columns=extra_columns_dict)
+    except (KeyError, TypeError):
+      logging.exception(
+          'got exception while creating ImagePair for'
+          ' test="%s", config="%s", urlPair=("%s","%s")' % (
+              test, config, imageA_relative_url, imageB_relative_url))
+      return None
+
 
 # TODO(epoger): Add main() so this can be called by vm_run_skia_try.sh