Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue when comparing images with different sizes #292

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix issue when comparing images with different sizes #292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions plugin/src/py/android_screenshot_tests/recorder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,39 @@ def _clean(self):
shutil.rmtree(self._output)
os.makedirs(self._output)

def _draw_diff_outline(self, failure_file, diff, image_to_draw_on):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Utility to highlight changed area on the larger screenshot

draw = ImageDraw.Draw(image_to_draw_on)
draw.rectangle(list(diff), outline=(255, 0, 0))
image_to_draw_on.save(failure_file)

def _is_image_same(self, file1, file2, failure_file):
with Image.open(file1) as im1, Image.open(file2) as im2:
diff_image = ImageChops.difference(im1, im2)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it turned out that ImageChops.difference does not consider different image sizes as different images, but if only compares the intersections https://github.com/python-pillow/Pillow/blob/ee079ae67e7e24ec789d3cc7d180820a70d32fe6/src/libImaging/Chops.c#L74

This is definately not a bug in facebook/screenshot-tests-for-android but IMHO PIL can also claim that this is the intended behaviour. My suggestion is to mitigate this issue here, and check size differences after running ImageChops.difference

try:
diff = diff_image.getbbox()
if diff is None:
if (im1.size != im2.size) or (im1.getbbox() != im2.getbbox()):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in case there is no difference reported by ImageChops.difference it can still happen that their size differ. If their size does not differ, and there is no difference reporter, we can be sure screenshots are the same.

If their size differ, but otherwise ImageChops.difference reported no difference, we can be sure they only differ by the additional pixels that the larger image has, so it makes sense to highlight those as differences.

If one image is not a direct subset of the other ImageChops.difference would report changes, so there is no need to handle those cases.

# First image is longer than the second
if im1.getbbox()[3] > im2.getbbox()[3]:
diff = [(0, im2.getbbox()[3]), (im1.getbbox()[2]-1, im1.getbbox()[3] - 1)]
self._draw_diff_outline(failure_file, diff, im1)

# First image is shorter than the first
if im1.getbbox()[3] < im2.getbbox()[3]:
diff = [(0, im1.getbbox()[3]), (im2.getbbox()[2]-1, im2.getbbox()[3] - 1)]
self._draw_diff_outline(failure_file, diff, im2)

# First image is wider than the second
if im1.getbbox()[2] > im2.getbbox()[2]:
diff = [(im2.getbbox()[2], 0), (im1.getbbox()[2]-1, im1.getbbox()[3] - 1)]
self._draw_diff_outline(failure_file, diff, im1)

# First image is narrower than the first
if im1.getbbox()[2] < im2.getbbox()[2]:
diff = [(im1.getbbox()[2], 0), (im2.getbbox()[2]-1, im2.getbbox()[3] - 1)]
self._draw_diff_outline(failure_file, diff, im2)

return False
return True
else:
if failure_file:
Expand Down
215 changes: 215 additions & 0 deletions plugin/src/py/android_screenshot_tests/test_recorder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,221 @@ def test_verify_failure(self):
self.assertEqual((0, 128, 0, 255), im.getpixel((1, 1)))
self.assertEqual((0, 128, 0, 255), im.getpixel((9, 1)))

def test_verify_different_sizes_longer(self):
self.create_temp_image("foobar.png", (10, 10), "blue")
self.make_metadata(
# language=json
"""
[
{
"name": "foobar",
"tileWidth": 1,
"tileHeight": 1
}
]"""
)

self.recorder.record()
os.unlink(join(self.inputdir, "foobar.png"))
self.create_temp_image("foobar.png", (10, 20), "blue")

try:
self.recorder.verify()
self.fail("expected exception")
except VerifyError:
pass # expected

self.assertTrue(os.path.exists(join(self.failureDir, "foobar_actual.png")))
self.assertTrue(os.path.exists(join(self.failureDir, "foobar_expected.png")))
self.assertTrue(os.path.exists(join(self.failureDir, "foobar_diff.png")))

# check colored diff
with Image.open(join(self.failureDir, "foobar_diff.png")) as im:
(w, h) = im.size
self.assertEqual(10, w)
self.assertEqual(20, h)

self.assertEqual((0, 0, 255, 255), im.getpixel((0, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((9, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((0, 5)))

self.assertEqual((0, 0, 255, 255), im.getpixel((0, 9)))
self.assertEqual((0, 0, 255, 255), im.getpixel((9, 9)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 9)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 5)))

self.assertEqual((255, 0, 0, 255), im.getpixel((0, 10)))
self.assertEqual((255, 0, 0, 255), im.getpixel((9, 10)))
self.assertEqual((255, 0, 0, 255), im.getpixel((5, 10)))
self.assertEqual((255, 0, 0, 255), im.getpixel((0, 15)))

self.assertEqual((255, 0, 0, 255), im.getpixel((0, 19)))
self.assertEqual((255, 0, 0, 255), im.getpixel((9, 19)))
self.assertEqual((255, 0, 0, 255), im.getpixel((5, 19)))
self.assertEqual((255, 0, 0, 255), im.getpixel((9, 15)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While debugging tests adding im.show() after these test will quicly open the image with the difference:

Suggested change
self.assertEqual((255, 0, 0, 255), im.getpixel((9, 15)))
self.assertEqual((255, 0, 0, 255), im.getpixel((9, 15)))
im.show()

It requires imagemagick installed, but make verification much quicker. (Also you can right click > View > Double size on the image as these are quite small)

Screenshot from 2021-09-02 10-46-34

Comment on lines +280 to +298
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking all pixels for the border I think we can get away with just checking the four corners (here and in the other tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean checking the corner pixels of the box?

4_corners

I recall that initially I had started with this approach, just checking the four corner pixels, but then during the implementation when I had made a mistake, and the rectange got wider, and lthe right side of the box where rendered ourside of the image, the test still had passed, as the top and bottom lines where still covering all four corner pixels.

4_corners_no_right_line

This is why I had decided to check the pixels in the middle of the lines as well, as this approach would catch that mistake.

8_corners_and_middle

8_corners_and_middleno_right_line

I would suggest to keep these checks to make sure this mistake is covered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, makes sense. We just published a new version that partially addresses the issues this PR fixes, do you mind rebasing onto that? It seems like ImageChops.difference in general is not very suitable for ARGB PNGs, unfortunately, but I'm not sure off the top of my head on what would be an easy / better alternative within Pillow.


def test_verify_different_sizes_shorter(self):
self.create_temp_image("foobar.png", (10, 20), "blue")
self.make_metadata(
# language=json
"""
[
{
"name": "foobar",
"tileWidth": 1,
"tileHeight": 1
}
]"""
)

self.recorder.record()
os.unlink(join(self.inputdir, "foobar.png"))
self.create_temp_image("foobar.png", (10, 10), "blue")

try:
self.recorder.verify()
self.fail("expected exception")
except VerifyError:
pass # expected

self.assertTrue(os.path.exists(join(self.failureDir, "foobar_actual.png")))
self.assertTrue(os.path.exists(join(self.failureDir, "foobar_expected.png")))
self.assertTrue(os.path.exists(join(self.failureDir, "foobar_diff.png")))

# check colored diff
with Image.open(join(self.failureDir, "foobar_diff.png")) as im:
(w, h) = im.size
self.assertEqual(10, w)
self.assertEqual(20, h)

self.assertEqual((0, 0, 255, 255), im.getpixel((0, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((9, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((0, 5)))

self.assertEqual((0, 0, 255, 255), im.getpixel((0, 9)))
self.assertEqual((0, 0, 255, 255), im.getpixel((9, 9)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 9)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 5)))

self.assertEqual((255, 0, 0, 255), im.getpixel((0, 10)))
self.assertEqual((255, 0, 0, 255), im.getpixel((9, 10)))
self.assertEqual((255, 0, 0, 255), im.getpixel((5, 10)))
self.assertEqual((255, 0, 0, 255), im.getpixel((0, 15)))

self.assertEqual((255, 0, 0, 255), im.getpixel((0, 19)))
self.assertEqual((255, 0, 0, 255), im.getpixel((9, 19)))
self.assertEqual((255, 0, 0, 255), im.getpixel((5, 19)))
self.assertEqual((255, 0, 0, 255), im.getpixel((9, 15)))

def test_verify_different_sizes_wider(self):
self.create_temp_image("foobar.png", (10, 10), "blue")
self.make_metadata(
# language=json
"""
[
{
"name": "foobar",
"tileWidth": 1,
"tileHeight": 1
}
]"""
)

self.recorder.record()
os.unlink(join(self.inputdir, "foobar.png"))
self.create_temp_image("foobar.png", (20, 10), "blue")

try:
self.recorder.verify()
self.fail("expected exception")
except VerifyError:
pass # expected

self.assertTrue(os.path.exists(join(self.failureDir, "foobar_actual.png")))
self.assertTrue(os.path.exists(join(self.failureDir, "foobar_expected.png")))
self.assertTrue(os.path.exists(join(self.failureDir, "foobar_diff.png")))

# check colored diff
with Image.open(join(self.failureDir, "foobar_diff.png")) as im:
(w, h) = im.size
self.assertEqual(20, w)
self.assertEqual(10, h)

self.assertEqual((0, 0, 255, 255), im.getpixel((0, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((9, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((0, 5)))

self.assertEqual((0, 0, 255, 255), im.getpixel((0, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((9, 9)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 9)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 5)))

self.assertEqual((255, 0, 0, 255), im.getpixel((10, 0)))
self.assertEqual((255, 0, 0, 255), im.getpixel((10, 9)))
self.assertEqual((255, 0, 0, 255), im.getpixel((10, 5)))
self.assertEqual((255, 0, 0, 255), im.getpixel((15, 0)))

self.assertEqual((255, 0, 0, 255), im.getpixel((19, 0)))
self.assertEqual((255, 0, 0, 255), im.getpixel((19, 9)))
self.assertEqual((255, 0, 0, 255), im.getpixel((19, 5)))
self.assertEqual((255, 0, 0, 255), im.getpixel((15, 9)))

def test_verify_different_sizes_narrower(self):
self.create_temp_image("foobar.png", (20, 10), "blue")
self.make_metadata(
# language=json
"""
[
{
"name": "foobar",
"tileWidth": 1,
"tileHeight": 1
}
]"""
)

self.recorder.record()
os.unlink(join(self.inputdir, "foobar.png"))
self.create_temp_image("foobar.png", (10, 10), "blue")

try:
self.recorder.verify()
self.fail("expected exception")
except VerifyError:
pass # expected

self.assertTrue(os.path.exists(join(self.failureDir, "foobar_actual.png")))
self.assertTrue(os.path.exists(join(self.failureDir, "foobar_expected.png")))
self.assertTrue(os.path.exists(join(self.failureDir, "foobar_diff.png")))

# check colored diff
with Image.open(join(self.failureDir, "foobar_diff.png")) as im:
(w, h) = im.size
self.assertEqual(20, w)
self.assertEqual(10, h)

self.assertEqual((0, 0, 255, 255), im.getpixel((0, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((9, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((0, 5)))

self.assertEqual((0, 0, 255, 255), im.getpixel((0, 0)))
self.assertEqual((0, 0, 255, 255), im.getpixel((9, 9)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 9)))
self.assertEqual((0, 0, 255, 255), im.getpixel((5, 5)))

self.assertEqual((255, 0, 0, 255), im.getpixel((10, 0)))
self.assertEqual((255, 0, 0, 255), im.getpixel((10, 9)))
self.assertEqual((255, 0, 0, 255), im.getpixel((10, 5)))
self.assertEqual((255, 0, 0, 255), im.getpixel((15, 0)))

self.assertEqual((255, 0, 0, 255), im.getpixel((19, 0)))
self.assertEqual((255, 0, 0, 255), im.getpixel((19, 9)))
self.assertEqual((255, 0, 0, 255), im.getpixel((19, 5)))
self.assertEqual((255, 0, 0, 255), im.getpixel((15, 9)))

if __name__ == "__main__":
unittest.main()