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

Horizontal/Vertical shift threshold #106

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tybrd916
Copy link

@tybrd916 tybrd916 commented Mar 1, 2022

Proposed addition of two parameters to popular pixelmatch library:

  • horizontalShiftPixels
  • verticalShiftPixels

Setting these parameters > 0 makes pixelmatch do additional checking for neighboring pixels within plus-or-minus horizontal/vertical "shift pixels" to avoid false-positives when the browser misaligns text by a few pixes.

  • Only execute this more expensive operation if the original pixel position mis-matches

Useful for jest-puppeteer testing screenshot matching where Chrome/Firefox text rendering is slightly shifted even on the same machine like this difference with original pixelmatch:
image

@tybrd916 tybrd916 changed the title Horizontal shift threshold Horizontal/Vertical shift threshold Mar 2, 2022
@spartanatreyu
Copy link

Looks like the CI only failed due to some basic linting mistakes.

@tybrd916
Copy link
Author

tybrd916 commented Aug 4, 2022

@spartanatreyu , Thanks for pointing out my neglecting the CI tests. I fixed the linting issues, and then added a test case showing how the pixel shifting due to Chrome font rendering can be mitigated with my two new parameters:

  • horizontalShiftPixels
  • verticalShiftPixels

@tybrd916
Copy link
Author

@mourner do you think this PR is worth merging, now that it has tests included?

@ortsevlised
Copy link

This PR would solve many of my issues, totally worth for me 👍

@mourner
Copy link
Member

mourner commented Aug 15, 2022

@tybrd916 this looks like a useful addition, I'm sorry that I haven't had the time to review closely yet. On a cursory glance, I'm only worried about possible performance issues (the check seems very expensive, even though if it's disabled by default), and also potential conflicts with another incoming PRs like #113. I'll take a closer look when I have a chance.

@tybrd916
Copy link
Author

Agreed the horizontal/vertical shift pixel checking does increase computational effort. One optimization I did was to only do the extra effort of looking at neighboring/shifted pixels when the original pixel fails to match given the other the threshold and anti aliasing parameters.

So for regression testing, almost identical images do not have significantly higher computation effort. But comparing too very different images is significantly more computationally expensive

@d-ivashchuk
Copy link

Hey fellow Ukrainian @mourner 👋 🇺🇦 We are using pixelmatch in Lost Pixel - it's an open source vis reg tool that we built with a friend and the only thing which drives me crazy is the font flakiness even when ran inside docker - I'd love this PR to land into pixelmatch so we can try and reduce that flakiness.

@tybrd916 if you'd need any help with this - ping me, I am excited to see how the thresholds you introduce could mitigate anti-aliasing problems in the machine browsers! thanks for your work!

@tybrd916
Copy link
Author

tybrd916 commented Oct 8, 2022

@mourner , is there anything else I can do to help prove out this PR? It's amazing how much adoption your pixelmatch solution has, millions of downloads per week!

@Dvoreth
Copy link

Dvoreth commented Dec 14, 2023

I'd really like to see this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants