Python Tools for Pixel-by-Pixel Image Comparison

A collection of tools for pixel by pixel image comparison to be used on the I Spy project.

BUG=249404
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207930

[cgrimm, 2013-06-13 17:46:49]

Requesting review for image_tools and test

[craigdh, 2013-06-13 19:28:31]

Looks pretty good.

Add a more descriptive subject (so general chromium sheriff would have some idea
what this patch is) and a better description.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:5: """A Module containing useful
image tools for use by I Spy."""
"""Utilities for performing pixel-by-pixel image comparisons."""

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:7: __author__ = 'cgrimm@google.com
(Chris Grimm)'
We don't do this in chromium.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:9: from itertools import izip
Prefer importing the module and then calling module.funct()

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:10: from PIL import Image
ditto

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:13: class Error(Exception):
This adds little additional meaning. Just use Exception directly (or a more
descriptive built-in exception like ValueError). If you want specific errors so
they can be caught, create more specific errors with clear names.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:17: def AreTheSameSize(*images):
Just pass the list by value. The contents of the list will still be references
to the original images.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:30: if i1.size == i2.size:
No need for the pass, just do:

if i1.size != i2.size:
  return false

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:34: return True
Actually, you can probably do this whole function using any() instead of a loop.
I think this one-liner should work:

return not any(images[0].size != img.size for img in images[1:])

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:39: def GetColorDist(px1, px2):
Functions local to the module should be prefixed with an _

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:54: dist += (px1[i]-px2[i])**2
Use sum() for efficiency as this is in the critical inner loop.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:55: return dist/(3*255**2)
Does python calculate this constant every time? I'm not sure. If so, maybe put
it in a constant at the top of the file.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:76: """gets the brightness of a
pixel.
g -> G

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:78: returns the sum of a given
pixel's color channels.
r -> R

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:85: return px[0]+px[1]+px[2]
Just sum(px)

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:100: average_px1 = Brightness(px1)
no need for the intermediate variables here.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:120: average_px1 = Brightness(px1)
ditto

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:128: def CreateMask(is_hard, cutoff,
*images):
No need for the *, as mentioned earlier.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:141: is_hard: boolean, whether or not
to threshold the mask
is_hard -> threshold

Also, I would pass images first and give cutoff and threshold default values to
mask ANY difference between images.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:149: if AreTheSameSize(*images):
Instead of indenting the whole function, just do this at the top:

if not AreTheSameSize(images):
  raise Exception('All images must be the same size')

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:151: mask_data = list(mask.getdata())
The data structure returned is already iterable, it would be much more efficient
to not copy it into a list.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:152: data_set = [list(x.getdata())
for x in images]
data_set is unnecessary. Just pass images[0].getdata() on line 155 instead of
i1.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:153: i1 = data_set[0]
more descriptive variable names than i1, i2

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:162: ]
Instead of constructing a list, just create an iterator like so:

mask.putdata((255, 255, 255) if Brightness(x) > cutoff else (0, 0, 0) for x in
mask_data)

And x -> px

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:188: return [
Should be able to create an iterator instead of a list.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:191: ]
Also, this indentation doesn't look like chromium style.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:238: between 0 and 1, where 1 is the
same image, and 0 is a
We're doing pixel-by-pixel comparisons, so this would probably be better
reported as the number of non-identical pixels.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:277: parameter: a number between 0
and 1 that represents the
As above, take a difference in pixels for the cutoff.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:292: if similar >= certainty:
just:

return similar >= certainty

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
File chrome/test/functional/ispy/image_tools_test.py (right):

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools_test.py:5: from google3.pyglib import
flags
Chrome is open source. You can't use google3. Instead use the standard unittest
module.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools_test.py:194: def _GenImage(self, size,
color):
This can be a module level function.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools_test.py:197: def _AllPixelsOfColor(self,
image, color):
ditto

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools_test.py:199: for i in data:
Just use any()

[cgrimm, 2013-06-14 18:43:58]

Finished making changes.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:5: """A Module containing useful
image tools for use by I Spy."""
On 2013/06/13 19:28:31, craigdh wrote:
> """Utilities for performing pixel-by-pixel image comparisons."""

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:7: __author__ = 'cgrimm@google.com
(Chris Grimm)'
On 2013/06/13 19:28:31, craigdh wrote:
> We don't do this in chromium.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:9: from itertools import izip
On 2013/06/13 19:28:31, craigdh wrote:
> Prefer importing the module and then calling module.funct()

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:10: from PIL import Image
On 2013/06/13 19:28:31, craigdh wrote:
> ditto

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:13: class Error(Exception):
On 2013/06/13 19:28:31, craigdh wrote:
> This adds little additional meaning. Just use Exception directly (or a more
> descriptive built-in exception like ValueError). If you want specific errors
so
> they can be caught, create more specific errors with clear names.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:17: def AreTheSameSize(*images):
On 2013/06/13 19:28:31, craigdh wrote:
> Just pass the list by value. The contents of the list will still be references
> to the original images.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:30: if i1.size == i2.size:
On 2013/06/13 19:28:31, craigdh wrote:
> No need for the pass, just do:
> 
> if i1.size != i2.size:
>   return false

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:34: return True
On 2013/06/13 19:28:31, craigdh wrote:
> Actually, you can probably do this whole function using any() instead of a
loop.
> I think this one-liner should work:
> 
> return not any(images[0].size != img.size for img in images[1:])

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:39: def GetColorDist(px1, px2):
On 2013/06/13 19:28:31, craigdh wrote:
> Functions local to the module should be prefixed with an _

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:54: dist += (px1[i]-px2[i])**2
On 2013/06/13 19:28:31, craigdh wrote:
> Use sum() for efficiency as this is in the critical inner loop.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:55: return dist/(3*255**2)
On 2013/06/13 19:28:31, craigdh wrote:
> Does python calculate this constant every time? I'm not sure. If so, maybe put
> it in a constant at the top of the file.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:76: """gets the brightness of a
pixel.
On 2013/06/13 19:28:31, craigdh wrote:
> g -> G

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:78: returns the sum of a given
pixel's color channels.
On 2013/06/13 19:28:31, craigdh wrote:
> r -> R

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:85: return px[0]+px[1]+px[2]
On 2013/06/13 19:28:31, craigdh wrote:
> Just sum(px)

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:100: average_px1 = Brightness(px1)
On 2013/06/13 19:28:31, craigdh wrote:
> no need for the intermediate variables here.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:120: average_px1 = Brightness(px1)
On 2013/06/13 19:28:31, craigdh wrote:
> ditto

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:128: def CreateMask(is_hard, cutoff,
*images):
On 2013/06/13 19:28:31, craigdh wrote:
> No need for the *, as mentioned earlier.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:141: is_hard: boolean, whether or not
to threshold the mask
On 2013/06/13 19:28:31, craigdh wrote:
> is_hard -> threshold
> 
> Also, I would pass images first and give cutoff and threshold default values
to
> mask ANY difference between images.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:149: if AreTheSameSize(*images):
On 2013/06/13 19:28:31, craigdh wrote:
> Instead of indenting the whole function, just do this at the top:
> 
> if not AreTheSameSize(images):
>   raise Exception('All images must be the same size')

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:151: mask_data = list(mask.getdata())
On 2013/06/13 19:28:31, craigdh wrote:
> The data structure returned is already iterable, it would be much more
efficient
> to not copy it into a list.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:152: data_set = [list(x.getdata())
for x in images]
On 2013/06/13 19:28:31, craigdh wrote:
> data_set is unnecessary. Just pass images[0].getdata() on line 155 instead of
> i1.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:153: i1 = data_set[0]
On 2013/06/13 19:28:31, craigdh wrote:
> more descriptive variable names than i1, i2

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:162: ]
On 2013/06/13 19:28:31, craigdh wrote:
> Instead of constructing a list, just create an iterator like so:
> 
> mask.putdata((255, 255, 255) if Brightness(x) > cutoff else (0, 0, 0) for x in
> mask_data)
> 
> And x -> px

putdata requires a sequence not an iterator.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:188: return [
On 2013/06/13 19:28:31, craigdh wrote:
> Should be able to create an iterator instead of a list.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:191: ]
On 2013/06/13 19:28:31, craigdh wrote:
> Also, this indentation doesn't look like chromium style.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:238: between 0 and 1, where 1 is the
same image, and 0 is a
On 2013/06/13 19:28:31, craigdh wrote:
> We're doing pixel-by-pixel comparisons, so this would probably be better
> reported as the number of non-identical pixels.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:277: parameter: a number between 0
and 1 that represents the
On 2013/06/13 19:28:31, craigdh wrote:
> As above, take a difference in pixels for the cutoff.

Done.

https://codereview.chromium.org/16855010/diff/1/chrome/test/functional/ispy/i...
chrome/test/functional/ispy/image_tools.py:292: if similar >= certainty:
On 2013/06/13 19:28:31, craigdh wrote:
> just:
> 
> return similar >= certainty

Done.

[craigdh, 2013-06-14 19:08:40]

Looking good, just some nits. Let's wait for frank's review (he's OOO today).

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:14: """Returns if a set of images are
the same size.
It returns either way :)

Returns *whether* a set...

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:17: images: a list of images to
compare
Put blank line between Args/Returns/Raises.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:77: return sum(px[0:3])
Skip the [0:3], makes an unnecessary copy of the list and this is called many
times.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:175: return iter(
You shouldn't need the iter

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:193: either 1. or 0. depending on the
distance of px1 and px2.
why not integers?

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:236:
_MaskToValue(m)*_ThresholdColorDiff(px1, px2, 0)
space before and after operators

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:257: """Returns a boolean
representing if the images are the Same.
if -> whether
Same -> same

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:259: Returns a boolean relating to
whether or not two images
relating to -> indicating

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools_test.py (right):

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:6: from PIL import Image
just import PIL and use PIL.Image

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:15: data = image.getdata()
no need for the intermediate variable

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:226: for i in data:
just return not any(color != px for px in image.getdata())

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:232: 
two blank lines here

[cgrimm, 2013-06-17 16:24:57]

updated code

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:14: """Returns if a set of images are
the same size.
On 2013/06/14 19:08:40, craigdh wrote:
> It returns either way :)
> 
> Returns *whether* a set...

Done.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:17: images: a list of images to
compare
On 2013/06/14 19:08:40, craigdh wrote:
> Put blank line between Args/Returns/Raises.

Done.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:77: return sum(px[0:3])
On 2013/06/14 19:08:40, craigdh wrote:
> Skip the [0:3], makes an unnecessary copy of the list and this is called many
> times.

Done.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:175: return iter(
On 2013/06/14 19:08:40, craigdh wrote:
> You shouldn't need the iter

Done.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:193: either 1. or 0. depending on the
distance of px1 and px2.
On 2013/06/14 19:08:40, craigdh wrote:
> why not integers?

Done.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:236:
_MaskToValue(m)*_ThresholdColorDiff(px1, px2, 0)
On 2013/06/14 19:08:40, craigdh wrote:
> space before and after operators

Done.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:257: """Returns a boolean
representing if the images are the Same.
On 2013/06/14 19:08:40, craigdh wrote:
> if -> whether
> Same -> same

Done.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:259: Returns a boolean relating to
whether or not two images
On 2013/06/14 19:08:40, craigdh wrote:
> relating to -> indicating

Done.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools_test.py (right):

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:6: from PIL import Image
On 2013/06/14 19:08:40, craigdh wrote:
> just import PIL and use PIL.Image

The __init__.py file in PIL isn't configured to import anything. Calling
PIL.Image results in "AttributeError: 'module' object has no attribute 'Image'"

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:15: data = image.getdata()
On 2013/06/14 19:08:40, craigdh wrote:
> no need for the intermediate variable

Done.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:226: for i in data:
On 2013/06/14 19:08:40, craigdh wrote:
> just return not any(color != px for px in image.getdata())

Done.

https://codereview.chromium.org/16855010/diff/22001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:232: 
On 2013/06/14 19:08:40, craigdh wrote:
> two blank lines here

Done.

[frankf, 2013-06-17 21:48:31]

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:277: cutoff for Sameness.
I don't thinkg Sameness has defined meaning. "Similarity" is a well-defined term
though.

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:280: image1: an Image to compare
Document how this is encoded

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:282: max_different_pixels: a number
that is the cutoff for image sameness
Once you add this threshold, then this brute force algorithm might not be
sufficient. There are much more sophisticated algorithms for image similarity
that deal with "fuzziness" much better:
http://stackoverflow.com/questions/843972/image-comparison-fast-algorithm

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:287: a boolean representing if the
images are the same.
"True if the images are similar, false otherwise"

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:290: Error: if the images (and mask)
are different sizes.
Is Error a type of exception?

[frankf, 2013-06-17 21:49:35]

Also what's up with the CL description?

[cgrimm, 2013-06-17 23:41:18]

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:277: cutoff for Sameness.
On 2013/06/17 21:48:31, frankf wrote:
> I don't thinkg Sameness has defined meaning. "Similarity" is a well-defined
term
> though.

Done.

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:280: image1: an Image to compare
On 2013/06/17 21:48:31, frankf wrote:
> Document how this is encoded

Done.

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:282: max_different_pixels: a number
that is the cutoff for image sameness
On 2013/06/17 21:48:31, frankf wrote:
> Once you add this threshold, then this brute force algorithm might not be
> sufficient. There are much more sophisticated algorithms for image similarity
> that deal with "fuzziness" much better:
> http://stackoverflow.com/questions/843972/image-comparison-fast-algorithm

resolved

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:287: a boolean representing if the
images are the same.
On 2013/06/17 21:48:31, frankf wrote:
> "True if the images are similar, false otherwise"

Done.

https://codereview.chromium.org/16855010/diff/26001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:290: Error: if the images (and mask)
are different sizes.
On 2013/06/17 21:48:31, frankf wrote:
> Is Error a type of exception?

This is a remnant from when I had a subclass of Exception called Error. I
noticed it shortly after I submitted the CL, it's fixed now.

[craigdh, 2013-06-17 23:53:06]

lgtm with nit.

Please wait for frank's lgtm before clicking the submit checkbox.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:280: image1: an RGB PIL.Image to
compare
end all sentences with a period

[cgrimm, 2013-06-18 00:01:08]

will do


On Mon, Jun 17, 2013 at 4:53 PM, <craigdh@chromium.org> wrote:

> lgtm with nit.
>
> Please wait for frank's lgtm before clicking the submit checkbox.
>
>
> https://codereview.chromium.**org/16855010/diff/31001/**
>
chrome/test/functional/ispy/**image_tools.py<https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/ispy/image_tools.py>
> File chrome/test/functional/ispy/**image_tools.py (right):
>
> https://codereview.chromium.**org/16855010/diff/31001/**
>
chrome/test/functional/ispy/**image_tools.py#newcode280<https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/ispy/image_tools.py#newcode280>
> chrome/test/functional/ispy/**image_tools.py:280: image1: an RGB PIL.Image
> to compare
> end all sentences with a period
>
>
https://codereview.chromium.**org/16855010/<https://codereview.chromium.org/1...
>

[frankf, 2013-06-18 01:52:17]

I'm half-way though. These are my comments so far.

Look at more CLs to get an idea for a good description.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:26: return not any(images[0].size !=
img.size for img in images[1:])
not any -> all

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:28: raise Exception('No images passed
in')
Please choose the most specific exception, in this case ValueError seems
reasonable.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:46: (ch1 - ch2) ** 2
Why squared? If you just want absolute value, use abs. Otherwise, document why
you do this?

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:47: for ch1, ch2 in
itertools.izip(px1[0:3], px2[0:3])
Why do you need [0:3]?

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:51: def _GetColorDistAsColor(px1,
px2):
Dist is ambiguous (Distribution, Dist)

-> GetColorDistanceAsColor

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:52: """Computes Color Distance as a
greyscale pixel.
Why is there random capitalization?

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:85: def _MinPixel(px1, px2):
It might be better to define a Pixel class and override __cmp__

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:129: such that (255,255,255)
represents great difference,
largest

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:151: for other_image in images[1:]:
I think it's clear to use max(map...

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:169: A helper function used to
generate masks. The mask, as
Aren't all these "helper functions"?

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools_test.py (right):

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:7: import image_tools
Separate sytem libraries from user-defined libraries by a blank line

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:56: px2 = (255, 255, 255)
If you reusing these, put up the fixtures in a SetUp()

[craigdh, 2013-06-18 16:25:01]

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:46: (ch1 - ch2) ** 2
On 2013/06/18 01:52:18, frankf wrote:
> Why squared? If you just want absolute value, use abs. Otherwise, document why
> you do this?

Agreed, I don't think you need to do these distance calculations, considering at
the end you're just reporting whether the pixels were the same or not. Same with
_GetColorDistAsColor(), we don't need grayscale in the mask.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:85: def _MinPixel(px1, px2):
On 2013/06/18 01:52:18, frankf wrote:
> It might be better to define a Pixel class and override __cmp__

I think in this case it's alright to use a 3-tuple for performance reasons. It's
not too complex and this is in a hot path where all the additional python class
member accesses are likely have a noticeable performance hit. Additionally it's
how the PIL library represents pixels.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:225: if px[0:3] == (0, 0, 0):
No need for [0:3]. If it's it's not already a 3-tuple, how could it be equal to
one?

[cgrimm, 2013-06-18 18:53:01]

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:26: return not any(images[0].size !=
img.size for img in images[1:])
On 2013/06/18 01:52:18, frankf wrote:
> not any -> all

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:28: raise Exception('No images passed
in')
On 2013/06/18 01:52:18, frankf wrote:
> Please choose the most specific exception, in this case ValueError seems
> reasonable.

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:46: (ch1 - ch2) ** 2
On 2013/06/18 01:52:18, frankf wrote:
> Why squared? If you just want absolute value, use abs. Otherwise, document why
> you do this?

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:46: (ch1 - ch2) ** 2
On 2013/06/18 16:25:01, craigdh wrote:
> On 2013/06/18 01:52:18, frankf wrote:
> > Why squared? If you just want absolute value, use abs. Otherwise, document
why
> > you do this?
> 
> Agreed, I don't think you need to do these distance calculations, considering
at
> the end you're just reporting whether the pixels were the same or not. Same
with
> _GetColorDistAsColor(), we don't need grayscale in the mask.

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:47: for ch1, ch2 in
itertools.izip(px1[0:3], px2[0:3])
On 2013/06/18 01:52:18, frankf wrote:
> Why do you need [0:3]?

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:51: def _GetColorDistAsColor(px1,
px2):
On 2013/06/18 01:52:18, frankf wrote:
> Dist is ambiguous (Distribution, Dist)
> 
> -> GetColorDistanceAsColor

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:51: def _GetColorDistAsColor(px1,
px2):
On 2013/06/18 01:52:18, frankf wrote:
> Dist is ambiguous (Distribution, Dist)
> 
> -> GetColorDistanceAsColor

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:52: """Computes Color Distance as a
greyscale pixel.
On 2013/06/18 01:52:18, frankf wrote:
> Why is there random capitalization?

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:85: def _MinPixel(px1, px2):
On 2013/06/18 01:52:18, frankf wrote:
> It might be better to define a Pixel class and override __cmp__

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:129: such that (255,255,255)
represents great difference,
On 2013/06/18 01:52:18, frankf wrote:
> largest

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:151: for other_image in images[1:]:
On 2013/06/18 01:52:18, frankf wrote:
> I think it's clear to use max(map...

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:169: A helper function used to
generate masks. The mask, as
On 2013/06/18 01:52:18, frankf wrote:
> Aren't all these "helper functions"?

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:225: if px[0:3] == (0, 0, 0):
On 2013/06/18 16:25:01, craigdh wrote:
> No need for [0:3]. If it's it's not already a 3-tuple, how could it be equal
to
> one?

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:280: image1: an RGB PIL.Image to
compare
On 2013/06/17 23:53:06, craigdh wrote:
> end all sentences with a period

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools_test.py (right):

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:7: import image_tools
On 2013/06/18 01:52:18, frankf wrote:
> Separate sytem libraries from user-defined libraries by a blank line

Done.

https://codereview.chromium.org/16855010/diff/31001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools_test.py:56: px2 = (255, 255, 255)
On 2013/06/18 01:52:18, frankf wrote:
> If you reusing these, put up the fixtures in a SetUp()

Done.

[craigdh, 2013-06-18 19:16:11]

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:60: data = list(
use [] instead of list(). That will result in this being a list comprehension
instead of a list that gets constructed from a generator expression.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:74: data = list(
ditto

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:88: passed in. The mask is generated
by continually
Just say the mask will have a white pixel anywhere the input images differ and
black everywhere else. Don't need to go through how it's implemented.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:147: mask: an optional mask to
occlude parts of the images from calculation.
Mention the format of the mask. Is it an image or a list of booleans? If it is
an image is black masked out or white?

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:155: if mask:
Entirely different codepaths are being followed depending whether there is a
mask. Just have two separate functions, one that takes a mask and one that
doesn't. SameImage() can have the logic to decide which to call.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:157: return sum([
remove the []. No need to create a list in memory, just have it as a generator
expression.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:171: return sum([
ditto

[cgrimm, 2013-06-18 20:21:01]

- fixed split code paths that craig brought up. 

- edited comments to be more clear about what a mask is.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:60: data = list(
On 2013/06/18 19:16:11, craigdh wrote:
> use [] instead of list(). That will result in this being a list comprehension
> instead of a list that gets constructed from a generator expression.

Done.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:74: data = list(
On 2013/06/18 19:16:11, craigdh wrote:
> ditto

Done.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:88: passed in. The mask is generated
by continually
On 2013/06/18 19:16:11, craigdh wrote:
> Just say the mask will have a white pixel anywhere the input images differ and
> black everywhere else. Don't need to go through how it's implemented.

Done.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:147: mask: an optional mask to
occlude parts of the images from calculation.
On 2013/06/18 19:16:11, craigdh wrote:
> Mention the format of the mask. Is it an image or a list of booleans? If it is
> an image is black masked out or white?

Done.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:155: if mask:
On 2013/06/18 19:16:11, craigdh wrote:
> Entirely different codepaths are being followed depending whether there is a
> mask. Just have two separate functions, one that takes a mask and one that
> doesn't. SameImage() can have the logic to decide which to call.

Similar problem in _GetDifferenceWithMask, fixed both.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:157: return sum([
On 2013/06/18 19:16:11, craigdh wrote:
> remove the []. No need to create a list in memory, just have it as a generator
> expression.

Done.

https://codereview.chromium.org/16855010/diff/42001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:171: return sum([
On 2013/06/18 19:16:11, craigdh wrote:
> ditto

Done.

[craigdh, 2013-06-18 23:34:27]

looks good to me.

@frankf: ptal

[frankf, 2013-06-19 03:53:26]

https://codereview.chromium.org/16855010/diff/46001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/46001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:153: 0 if m == (255, 255, 255)
This is unreadable

https://codereview.chromium.org/16855010/diff/46001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:177: max_different_pixels: a number
that is the cutoff for whether or not
I thought we decided to remove this.

[frankf, 2013-06-19 03:53:26]

https://codereview.chromium.org/16855010/diff/46001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/46001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:153: 0 if m == (255, 255, 255)
This is unreadable

https://codereview.chromium.org/16855010/diff/46001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:177: max_different_pixels: a number
that is the cutoff for whether or not
I thought we decided to remove this.

[cgrimm, 2013-06-19 16:17:26]

- changed unreadable list comprehensions to for-loops. 
- removed max_different_pixels parameter from SameImage function
- modified tests to reflect changes in structure of SameImage function.

https://codereview.chromium.org/16855010/diff/46001/chrome/test/functional/is...
File chrome/test/functional/ispy/image_tools.py (right):

https://codereview.chromium.org/16855010/diff/46001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:153: 0 if m == (255, 255, 255)
On 2013/06/19 03:53:26, frankf wrote:
> This is unreadable

changed this and another instance of the same pattern into for-loops (line 64).
There was a slow-down, but it doesn't seem to dramatic. 
(.52s -> .57s for a mask generated from 3 images)

https://codereview.chromium.org/16855010/diff/46001/chrome/test/functional/is...
chrome/test/functional/ispy/image_tools.py:177: max_different_pixels: a number
that is the cutoff for whether or not
On 2013/06/19 03:53:26, frankf wrote:
> I thought we decided to remove this.

Done.

[frankf, 2013-06-19 18:41:03]

lgtm

[commit-bot: I haz the power, 2013-06-20 19:08:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cgrimm@google.com/16855010/53001

[commit-bot: I haz the power, 2013-06-20 19:22:27]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-21 17:00:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cgrimm@google.com/16855010/62001

[cgrimm, 2013-06-21 17:02:20]

- fixed typo in copyright in image_tools
- added in trivial serialize and deserialize functions.

[commit-bot: I haz the power, 2013-06-21 22:08:05]

Change committed as 207930

[cgrimm, 2013-06-24 18:11:09]

- added in file/package structure

- added in bucket_manager 

- added in tests for bucket_manager.