Fix issue when comparing images with different sizes #292
Conversation
| @@ -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): | |||
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
| 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) | ||
| try: | ||
| diff = diff_image.getbbox() | ||
| if diff is None: | ||
| if (im1.size != im2.size) or (im1.getbbox() != im2.getbbox()): |
There was a problem hiding this comment.
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.
| 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))) |
There was a problem hiding this comment.
While debugging tests adding im.show() after these test will quicly open the image with the difference:
| 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)
| 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))) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
You mean checking the corner pixels of the box?
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.
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.
I would suggest to keep these checks to make sure this mistake is covered.
There was a problem hiding this comment.
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.
… check Summary: For some odd reason `ImageChops.difference` does not work at all with RGBA formatted images, so this forces images to be in RGB format prior to diffing (which may also indicate that we're dropping alpha only changes). This also adds a change to check for image dimension differences to partially address the concerns in #292 Differential Revision: D33961675 fbshipit-source-id: 893cb07568893e74a9717b6c90fc9b1ab065feb3
We had bumped into an issue when screenshot changes were not detected when the change was only appending to a screenshot, so the size was different, but the intersection of the screenshots remained the same, more details in the issue desription at #258
This PR atteps to fix this issue