mapbox / pixelmatch

The smallest, simplest and fastest JavaScript pixel-level image comparison library
ISC License
6.1k stars 304 forks source link

Horizontal/Vertical shift threshold #106

Open tybrd916 opened 2 years ago

tybrd916 commented 2 years ago

Proposed addition of two parameters to popular pixelmatch library:

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.

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
spartanatreyu commented 2 years ago

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

tybrd916 commented 2 years ago

@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:

tybrd916 commented 2 years ago

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

ortsevlised commented 2 years ago

This PR would solve many of my issues, totally worth for me πŸ‘

mourner commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

@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 commented 8 months ago

I'd really like to see this merged.