Non-Metal based perceptual image comparison

[ejensen]

Overview

Remove Metal usage since some virtualized environments don’t support it and silently fail (only logging something in the console). Replaces the CoreImage operations that require Metal with CPU-based calculation.

Context

Metal is supported by all iOS/tvOS devices (2013 models or later) and Macs (2012 models or later). Older devices that don't support Metal also do not support iOS/tvOS 13 and macOS 10.15 which are the minimum versions of swift-snapshot-testing. However, some virtualized Mac environments do not have a GPU and therefore do not support Metal. In this case, macOS falls back to a CPU-based OpenGL ES renderer that silently fails when Metal commands are issued. The output of the silent failure is a solid black image. And since the threshold filter returns black pixels when it passes, the solid black image from the silent failure causes false positives.

Related PRs

• Utilizes the byte buffer iteration speedup from Improve byte buffer iteration speed by 90% #664

[christianflanders]

Thank you! I spent a bunch of time recently trying to debug why we were having false positive on our CI/CD pipeline, I was able to narrow it down to trying to use perceptual precision on a virtualized MacOS and ended up putting some other bandaids in place to fix it temporarily.

[IlyaPuchkaTW]

I just recently switched to the revision with this change and what I see in the CI logs is that every failing test is now taking 12 seconds and more where without using precision (or setting it to 1.0) it takes ~2s

[IlyaPuchkaTW]

couldn't this check (or rather the opposite) be inside the for loop for a quicker exist? we know the size of the deltaOutputImage before the loop, so we can calculate the number of pixels that may fail (rather than computing precision every iteration after incrementing failed pixel count) and then the loop can be aborted as soon as this number is exceeded. Otherwise we are comparing all the pixels even when we know that snapshot is not precise enough.
I guess we don't have this problem when snapshots match since they would be compared for exact match before this.

[IlyaPuchkaTW]

I've tried that in my fork but it didn't give me any significant improvements

[jlcvp]

Yeah, as the complexity would not change from O(n) (n = number of pixels) and unless you're comparing a very large images with huge custom frame sizes, the tests times will not improve significantly

[IlyaPuchkaTW]

If there is a way to know if Metal is supported could we keep the previous implementation in case it is more performant than the alternative?

[IlyaPuchkaTW]

Also if only metal backport of the threshold filter is the issue why to remove the use of all core image filters rather than using CIColorThreshold for iOS 14+ and fallback to CPU otherwise?

[KSmith100]

You can check metal support like so:

MTLCreateSystemDefaultDevice() == nil

[jlcvp]

Having the same issue here using Xcode Cloud.

It's outputting No Metal renderer available in the logs but all the tests passes.
Even when we break the UI completely, trying to force it to fail, it still passes, so I believe it's generating all-black images there as well.

[IlyaPuchkaTW]

I don't think any more that my issue is related to Metal not being available. In that case I would expect 100% failures, yet some tests are still failing on CI as expected but some are not.

[jlcvp]

I don't think any more that my issue is related to Metal not being available. In that case I would expect 100% failures, yet some tests are still failing on CI as expected but some are not.

With my investigations here, when this "No Metal renderer available" runs, it renders a completely blank image for any snapshot, either the newly taken or the reference one. So, the images will always match.

It'll only fail when having different dimensions/frames. Maybe it's your case

[IlyaPuchkaTW]

It'll only fail when having different dimensions/frames

Yes, you are most likely right about that!

[IlyaPuchkaTW]

@ejensen is there any changes you could get back to this? Since it's either false positives or degraded performance will have to revert to using intel machines for recording snapshots but don't want to do that if you have an idea how to solve this.

[ejensen]

ejensen is there any changes you could get back to this? Since it's either false positives or degraded performance will have to revert to using intel machines for recording snapshots but don't want to do that if you have an idea how to solve this.

Commit 8d402bf adds the Metal-based method back. There is not a straightforward way to determine if the (virtualized) machine supports Metal, so there is a tricky bit of conditional logic that would benefit from testing on more machine configurations.

[IlyaPuchkaTW]

@ejensen thanks for the update, although I don't think it will help with performance when Metal is not supported. Is there a way to use a similar filter from CoreImage?

[barrault01]

Hello, I am having the issue described here: #710, and this PR really fixed the problem. Great works! Is it possible to merge it as soon as possible?

[michaelmcguire]

@ejensen In your original PR you had used the CIColorThreshold filter for comparison and then switched to using Metal so you could target older versions of macOS. What do you think about going back to that? We're having this same problem in our CI environment, but it would be nice to retain the speed benefits since we use macOS 12.0 on CI.

[Alex-Ozun]

@ejensen In your #628 you had used the CIColorThreshold filter for comparison and then switched to using Metal so you could target older versions of macOS. What do you think about going back to that?

Sounds like we should be able to cover all bases if we add one more availability check:

1. If MPS is available, use that.
2. Otherwise, If macOS 11.0+ is available, use CIColorThreshold.
3. Otherwise, fall back to vImage buffer iteration.

[stephencelis]

Thanks for this! And sorry for the delay. Strategy-specific PRs tend to take on a lot of complexity due to all the branching and platforms involved. We finally took some time to look things over and take the branch for a spin and it looks like a definite improvement over the buggy behavior we previously had.