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

Fix macOS Catalina issue #288

Closed

Conversation

maxx-cyber
Copy link

The problem is that rendering system on different hardwares with the same macOS Catalina version may produce not matched image snapshots. There may be several bytes that have difference just of 1, e.g. in one bitmap some byte has value 212 and in other bitmap the same byte has value 213. Such difference isn't visible by eye even in generated difference_<uuid>.png image. So snapshots generated on developer computer always fail on CI computer.
The solution is to ignore the difference of 1 and fail only in case if difference greater than 1.

@stephencelis
Copy link
Member

👋 Hi there!

These changes appear to be for iOS simulator screen shots, not macOS screen shots. Can you clarify as to the issue? Is this for the same iOS simulator version across Catalina and Mojave? Or are you trying to solve for other differences?

In general we think that CI and developer machines should be running similar specs, and if developers want to introduce precision differences, they should do so with the precision parameter on the strategy. We could see this being annoying in an entire suite, though, so maybe we should introduce a global override. Thoughts, @mbrandonw?

…etween color component values to assume them as match
@maxx-cyber
Copy link
Author

maxx-cyber commented Dec 21, 2019

Yes, it is iOS simulator screen shots.

It is known issue that screenshots created on Mojave doesn’t match screenshots created on Catalina and vice versa because font antialiasing is differ on Catalina, so some pixels around font glyphs are a little bit different. That is why we’ve synced environment on all machines - it is macOS Catalina 10.15.2, Xcode 11.3 and iOS Simulator 13.3 on all machines. And we were surprised, screenshots generated on MacBook Pro with i7 and this environment don’t match screenshots generated on iMac with i5 and same environment, graphics cards are different as well, and we don’t see any difference in generated difference image, even after changing brightness and contrast on it. After debugging I found out that on a particular screenshot there are only 46 different bytes out of 3 882 000 bytes and difference is exactly 1. On other screenshots which have more text there are more such bytes and again all different bytes have difference exactly 1.

I think it is happens because floating point accuracy is differ on different hardware, e.g. floating point value in 0.0…1.0 during conversion to integer in 0…255 can produce difference value on different hardware and difference is exactly 1. Or something like this.

What about precision parameter, it is actually not the same thing, precision parameter means how many pixels should be equal to assume images as equal, so it allows percentage of completely different pixels. On other side I introduced other value that means how difference between color components can be to assume color components as equal. I’ve pushed one more commit where I make it as additional parameter with default value 0 to not affect all current users.

Only precision parameter doesn’t fit our needs, because each screenshot can have different number of such mentioned differences and it requires to pick up different precision value for each screenshot. It is not an option in case of thousands screenshots. And it doesn’t guarantee that if some pixels will be really different it pass threshold to be failed, but we need it to be failed in this case.

@mRs-
Copy link

mRs- commented Feb 24, 2020

I have some kind of similiar Problems. When generating screenshots on my hardware there are different from the screenshot on my CI hardware (macOS Application without Simulator). Is there a known workaround for this?

@brentleyjones
Copy link

brentleyjones commented Apr 1, 2020

We also need this. The original iOSSnapshotTestCase added this exact functionality for the same reasons that @maxx-cyber laid out above: https://github.com/uber/ios-snapshot-test-case/blob/0bbf957c9556e143d94d2e209bcea35a63efad3f/FBSnapshotTestCase/Categories/UIImage%2BCompare.m#L117

On Catalina, with the simulator generating everything with Metal, even on the same machine you will get rounding issues. We need per-pixel checking but want the existing precision to stay at 0.

@gscalzo
Copy link

gscalzo commented May 4, 2020

We also need this one for the same reason @brentleyjones highlighted.
We forked and updated the fork to the last version; however, an official solution would be better.
An opt-in global override as @stephencelis suggested would be the perfect API.
Any thoughts @stephencelis, @mbrandonw ?

@mRs-
Copy link

mRs- commented May 5, 2020

The other problem I see is still that you can't really decide if you want a "retina"-Screenshot or a "non-retina" one. Currently we need to create the Screenshots on the CI, download it and check it in to get the right results.

@stephencelis
Copy link
Member

I wonder if we could overload the behavior of precision to include this. Precision based on color difference seems more useful than total number of pixels that differ. Brandon and I will discuss tomorrow and hopefully get this behavior closer to merged in some way.

@mRs-
Copy link

mRs- commented May 11, 2020

@stephencelis did you have any news to share what you discussed with Brandon? Would be awesome not to write an CI Jobs for recording the Screenshots 😄

@gobetti
Copy link

gobetti commented May 16, 2020

hey guys, first of all, thank you for this.

This might help some cases where time isn't crucial, but like I had mentioned when considering this possible solution earlier, the performance is too bad. Having to iterate over many pixels is super slow and therefore I wouldn't even recommend using a precision lower than 1, unless your CI setup is either very fast somehow or the time it takes isn't relevant to you. For me each failing snapshot test used to take about 15 seconds when using a precision above 0.99 - that means less than 1% of the pixels have been swept and still it took that long. Imagine what happens if you have to sweep every single pixel, which is what this solution ultimately does given the compared images won't pass the first memcmp test 😢

@stephencelis
Copy link
Member

stephencelis commented May 16, 2020

Having to iterate over many pixels is super slow and therefore I wouldn't even recommend using a precision lower than 1, unless your CI setup is either very fast somehow or the time it takes isn't relevant to you.

It's probably out of scope for something I'd have time to try or the ability to do well, but I wonder if it's possible to use memcmp in a binary search-like fashion (like git bisect).

Such that, if the first call to memcmp fails: try again on the first half of the memory block. If that passes, split the second half and try memcmp on its first half. If that fails, split that first half into two, etc.

@gobetti
Copy link

gobetti commented May 16, 2020

that idea sounds great to me 👏

@maxx-cyber
Copy link
Author

Hi guys, do you have any updates on this?

@bretldan
Copy link

Any updates on this?

@IlyaPuchkaTW
Copy link

As a matter of fact we had exactly the same issue (might be a different reason, but it's unknown) on some local machines when using FB snapshot library and it disappeared completely when we switched to this one. I couldn't pin point it to any particular reason, but it was not happening on all the local machines, and CI was not flaky (we use bitrise). In the past I used to work on the project that undergo the same migration but there were no issues like that with FB library as well. So it feels like it's a local issue.

@maxx-cyber
Copy link
Author

Any updates?

jmbene pushed a commit to uberforcede/swift-snapshot-testing that referenced this pull request Apr 12, 2021
@gzvsky
Copy link

gzvsky commented Apr 13, 2021

This worked for our team.
Copy your current Mac ColorSync profile ( ~ Library/ColorSync/Profiles/… .icc file) and set this profile as default on CI agent using ColorSync Utility.

@stephencelis
Copy link
Member

Sorry for the long delay on this, and thanks for the original contribution! We believe this can be closed out with the merging of #628, which adds a perceptualPrecision option, and also comes with some performance optimizations. If this other PR does not address everything, please let us know!

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

9 participants