- Notifications
You must be signed in to change notification settings - Fork 1.1k
perf: use blazediff instead of pixelmatch #7690
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
perf: use blazediff instead of pixelmatch #7690
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## main #7690 +/- ## ========================================== - Coverage 99.38% 99.38% -0.01% ========================================== Files 1089 1089 Lines 97550 97550 ========================================== - Hits 96949 96948 -1 - Misses 601 602 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@seunomonije since you've been working on cirq-web lately, would you be able to weigh in on this PR? |
It seems to me that the proposed benefits of blazediff versus pixelmatch for cirq-web are:
These benchmarks are webpages, landscape images, and images from the pixelmatch repo. It seems as if the biggest speed increases are dealing with bigger image sizes (>9MB), which we are unlikely to hit in cirq-web, even if we were to add new widgets that handle much larger, fault tolerant circuits. Pixelmatch is lightweight, robust, still being maintained, and has a large community using it. It is also focused solely on image diffs. Blazediff seems to be building an entire ecosystem around diff checking, and is maintained by just the author. @teimurjan can you comment on any other benefits of blazediff that I may have missed? Your work seems great, however, the benefits to cirq-web aren't obvious, and moving away from a mature package might be a rough point when trying to integrate into bigger projects. Especially for a dev dependency. As an aside, why not try and integrate the identical speedup algo into pixelmatch itself? |
928747c
to 340b0ef
Compare Hey @seunomonije! You're absolutely right about the trade-offs, but I’d like to add a bit more context:
As for why I didn’t upstream these improvements to pixelmatch, I’m intentionally taking a different direction:
|
In our case the image comparison needs are tiny as cirq-web has only 3 images to compare. pixelmatch is only used for testing, we do not deploy it with the cirq-web Python package so the typescript package size is not of concern. Also supply-chain attacks would be more likely noticed for pixelmatch with its larger community of contributors. Thank you @teimurjan for contributing this, but at this time we see no clear benefit to make the change. |
Description
This PR replaces pixelmatch with blazediff in the E2E testing infrastructure to improve image comparison performance.
BlazeDiff is ~1.5x times faster than pixelmatch, producing the same results in type-safe code that is available in ESM and CJS.
BlazeDiff is already integrated in: