Skip to content

Fix macOS Catalina issue#288

Closed
maxx-cyber wants to merge 3 commits into
pointfreeco:masterfrom
maxx-cyber:fix_macos_catalina_issue
Closed

Fix macOS Catalina issue#288
maxx-cyber wants to merge 3 commits into
pointfreeco:masterfrom
maxx-cyber:fix_macos_catalina_issue

Conversation

@maxx-cyber

Copy link
Copy Markdown

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
Copy Markdown
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

maxx-cyber commented Dec 21, 2019

Copy link
Copy Markdown
Author

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-

mRs- commented Feb 24, 2020

Copy link
Copy Markdown

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

brentleyjones commented Apr 1, 2020

Copy link
Copy Markdown

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

gscalzo commented May 4, 2020

Copy link
Copy Markdown

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-

mRs- commented May 5, 2020

Copy link
Copy Markdown

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
Copy Markdown
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-

mRs- commented May 11, 2020

Copy link
Copy Markdown

@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

gobetti commented May 16, 2020

Copy link
Copy Markdown

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

stephencelis commented May 16, 2020

Copy link
Copy Markdown
Member

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

gobetti commented May 16, 2020

Copy link
Copy Markdown

that idea sounds great to me 👏

@maxx-cyber

Copy link
Copy Markdown
Author

Hi guys, do you have any updates on this?

@bretldan

Copy link
Copy Markdown

Any updates on this?

@IlyaPuchkaTW

Copy link
Copy Markdown

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
Copy Markdown
Author

Any updates?

@gzvsky

gzvsky commented Apr 13, 2021

Copy link
Copy Markdown

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
Copy Markdown
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.

9 participants