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.

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

Ssim integration #220

Merged
merged 4 commits into from
Jul 23, 2020
Merged

Conversation

omnisip
Copy link
Contributor

@omnisip omnisip commented Jun 17, 2020

Fixes #201

This implementation adds SSIM support. Compared to pixel by pixel analysis, it works by looking for structural similarity in an image thus being far less susceptible to color changes or minor rendering nuances in the comparison process.

The implementation I've built here supports both inband operation and out of band by virtue of a new parameter 'comparisonMethod'. Options can be passed in like they were before, except they're passed to ssimMatch instead of pixelmatch.

ssimMatch is implemented in such a way that it mirrors the pixelmatch default behavior, as such, it's supported via drop-in replacement. Several integration tests are added to demonstrate the behavior of SSIM.

3 Important Implementation Notes:

  1. A new diff generation method was added that creates a transparent picture with a red overlay for areas where structural changes are expected. This removes any requirement to use pixelmatch for diff generation.

  2. The current implementation of the SSIM algorithm converts to grayscale for comparison underneath the hood. For most formatting, size, image, and even color issues, this behavior proves to be perfectly acceptable even in the fastest mode {ssim: 'bezkrovny'}. See tests/stubs/LargeTestImage-LargeTestImageFailure-ssim-diff.png. However, there may be cases where we want to do color comparison SSIM. In which case, we can consider adding an SSIM implementation that handles colors too.

  3. To get the proper test coverage and to evaluate the integration tests, I needed to run at least one SSIM test inband which makes the test suite run 30 seconds longer.

Please review and let me know what you think. With this functionality, you can reasonably set a failure threshold percent of 1% (0.01) and not get false positives.

@omnisip omnisip requested a review from a team as a code owner June 17, 2020 20:58
@CLAassistant
Copy link

CLAassistant commented Jun 17, 2020

CLA assistant check
All committers have signed the CLA.

@omnisip
Copy link
Contributor Author

omnisip commented Jun 18, 2020

I've asked the upstream author of the SSIM guys for help on the image diff and have proposed the implementation I'm working from there.

obartra/ssim#219

@omnisip
Copy link
Contributor Author

omnisip commented Jun 18, 2020

Got the issue fixed upstream. obartra/ssim#220

With the changes there, I should be able to finish this without pixelmatch today! :-)

@omnisip omnisip force-pushed the ssim-integration branch from e8eef23 to 713be37 Compare June 19, 2020 17:52
@omnisip
Copy link
Contributor Author

omnisip commented Jun 19, 2020

I tried cleaning up the whitespace changes, but it seems your git commit reformatted it again anyway. :-(

threshold: 0.01,
};

const defaultSSIMDiffConfig = { ssim: 'bezkrovny' };
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment above this code about what this means and where we can find out more about it? For subsequent contributors to the code to know!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done along with updates to the README providing options, best practices, and links to other sources.

@anescobar1991
Copy link
Member

I tried cleaning up the whitespace changes, but it seems your git commit reformatted it again anyway. :-(

what whitespace issues?

Also can you update the readme? I think this is great as an experimental feature! Maybe we can even make it the default in 4.0.0!

@omnisip
Copy link
Contributor Author

omnisip commented Jul 3, 2020

I tried cleaning up the whitespace changes, but it seems your git commit reformatted it again anyway. :-(

what whitespace issues?

The default git commit code formatter updated the integration test in areas I didn't. I tried fixing it, but it overwrote the changes I made. :-(

@anescobar1991
Copy link
Member

That’s weird about the white space formatting. We don’t have anything that does https://github.com/americanexpress/jest-image-snapshot/blob/master/package.json#L77

@@ -156,6 +160,42 @@ expect.extend({ toMatchImageSnapshot });
### jest.retryTimes()
Jest supports [automatic retries on test failures](https://jestjs.io/docs/en/jest-object#jestretrytimes). This can be useful for browser screenshot tests which tend to have more frequent false positives. Note that when using jest.retryTimes you'll have to use a unique customSnapshotIdentifier as that's the only way to reliably identify snapshots.

### Recommendations when using SSIM comparison
Since SSIM calculates differences in structural similarity by building a moving 'window' over an images pixels, it does not particularly benefit from pixel count comparisons, especially when you factor in that it has a lot of floating point arithmetic in javascript. However, SSIM gains two key benefits over pixel by pixel comparison:
- Reduced false positives (failing tests when the images look the same)
Copy link
Member

Choose a reason for hiding this comment

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

Can you link to some sources on this method of image comparison and it’s benefits you just described?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Can I cite the wikipedia page? https://en.wikipedia.org/wiki/Structural_similarity

It's tough to beat stuff like "SSIM subsequently found strong adoption in the image processing community. The 2004 SSIM paper has been cited over 20,000 times according to Google Scholar,[2] making it one of the highest cited papers in the image processing and video engineering fields. It was accorded the IEEE Signal Processing Society Best Paper Award for 2009.[3] It also received the IEEE Signal Processing Society Sustained Impact Award for 2016, indicative of a paper having an unusually high impact for at least 10 years following its publication. The inventors of SSIM were each accorded an individual Primetime Engineering Emmy Award by the Television Academy in 2015. "

or...

"Due to its popularity, SSIM is often compared to other metrics, including more simple metrics such as MSE and PSNR, and other perceptual image and video quality metrics. SSIM has been repeatedly shown to significantly outperform MSE and its derivates in accuracy, including research by its own authors and others.[7][15][16][17][18][19]"

Side note: Pixel by Pixel comparison is almost identical PSNR (peak signal to noise ratio). The difference is an aggregation function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is resolved in my latest commit.

@omnisip
Copy link
Contributor Author

omnisip commented Jul 5, 2020

That’s weird about the white space formatting. We don’t have anything that does https://github.com/americanexpress/jest-image-snapshot/blob/master/package.json#L77

I've checked through the settings and the underlying presets and am equally perplexed. I'm willing to go through it and fix it one more time. I will try squashing and force pushing later.

@omnisip
Copy link
Contributor Author

omnisip commented Jul 5, 2020

That’s weird about the white space formatting. We don’t have anything that does https://github.com/americanexpress/jest-image-snapshot/blob/master/package.json#L77

I've checked through the settings and the underlying presets and am equally perplexed. I'm willing to go through it and fix it one more time. I will try squashing and force pushing later.

It looks like I succeeded in fixing this the first time. I didn't see any that I missed.

@omnisip
Copy link
Contributor Author

omnisip commented Jul 5, 2020

That’s weird about the white space formatting. We don’t have anything that does https://github.com/americanexpress/jest-image-snapshot/blob/master/package.json#L77

I've checked through the settings and the underlying presets and am equally perplexed. I'm willing to go through it and fix it one more time. I will try squashing and force pushing later.

It looks like I succeeded in fixing this the first time. I didn't see any that I missed.

That’s weird about the white space formatting. We don’t have anything that does https://github.com/americanexpress/jest-image-snapshot/blob/master/package.json#L77

I've checked through the settings and the underlying presets and am equally perplexed. I'm willing to go through it and fix it one more time. I will try squashing and force pushing later.

It looks like I succeeded in fixing this the first time. I didn't see any that I missed.

It merely looks like whitespace changes now because the diff algorithm on github is highlighting sections that would overlap between the new tests I added and the old tests that existed before.

@anescobar1991 anescobar1991 requested a review from a team July 18, 2020 18:30
@anescobar1991
Copy link
Member

Approved and then we can play with it more as we go. As this is behind an experimental flag at the moment we can definitely improve on it and possibly make it the default behavior next breaking version.

@omnisip
Copy link
Contributor Author

omnisip commented Jul 18, 2020 via email

@jhildenbiddle
Copy link

@anescobar1991 @JAdshead --

Anxiously waiting on a chance to try SSIM instead of twiddling pixelmatch knobs to try and avoid false positives. Any idea when SSIM will make its way to a public release?

@anescobar1991 anescobar1991 merged commit e2b304a into americanexpress:master Jul 23, 2020
oneamexbot added a commit that referenced this pull request Jul 23, 2020
# [4.1.0](v4.0.2...v4.1.0) (2020-07-23)

### Features

* **ssim:** add integration ([#220](#220)) ([e2b304a](e2b304a))
@anescobar1991
Copy link
Member

@jhildenbiddle just released! It’s in 4.1.0!

Side note @JamesSingleton @nellyk the release not didn’t comment on this PR to let it know it was included in the release, any idea why?

@nellyk
Copy link
Contributor

nellyk commented Jul 23, 2020

I'm not sure exactly what happened, i can see the commit was included on the release but during the process of adding the release comment it was missed.

@jhildenbiddle
Copy link

Fantastic!

Thanks @anescobar1991, and a huge thanks for @omnisip for all of the hard work. Very much appreciated!

goverdhan07 pushed a commit to goverdhan07/jest-image-snapshot that referenced this pull request Jul 23, 2023
goverdhan07 pushed a commit to goverdhan07/jest-image-snapshot that referenced this pull request Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SSIM Comparison
6 participants