Allow for subpixel deviations + improved unoptimized loop execution in image comparison

[pimms]

Subpixel difference

Workaround for the issue described in #313 and #424.

The readme clearly states that the reference-device should be identical to the recorder-device, but as M1 Macs are becoming popular with developers at a different rate to CI & cloud providers, that is becoming increasingly difficult.

Allow each subpixel (R, G, B) to deviate by up to a certain degree. Currently, a precision: 0.9 means that "90% of the pixels must match 100%". The issues described in particularly #313, where some experience that the pixels deviate by only 1, can be resolved by comparing with subpixelThreshold: 1. Comparing with precision: 0.9, subpixelThreshold: 1 then means "90% of the subpixels' value must be ±1 of the reference".

Optimization

When an image comparison memcmp fails and we need to compare each byte of the failing image, the current loop iteration is slow when compiled without optimization on Intel machines.

My team defaulted to adding custom compiler flags to SnapshotTesting, but the problem can also be diminished significantly by using a more basic loop implementation.

The difference between looping over a range (current implementation) and looping with a condition can be seen on Godbolt.

Example in Godbolt

1. Go to godbolt.org
2. Change language to Swift
3. Copy the following code in the left-hand editor
4. Wait for the compiler
5. Compare sections LBB1_4-LBB1_10 with LBB3_1

func byteDiff1(reference: [UInt8], comparison: [UInt8]) -> Bool {
    let maxDiffs = 100
    var diffs = 0
    for byte in 0..<reference.count {
        if reference[byte] != comparison[byte] {
            diffs += 1
            if diffs >= maxDiffs {
                return false
            }
        }
    }
    return true
}

func byteDiff2(reference: [UInt8], comparison: [UInt8]) -> Bool {
    let maxDiffs = 100
    var diffs = 0
    var byte = 0
    while byte < reference.count {
        if reference[byte] != comparison[byte] {
            diffs += 1
            if diffs >= maxDiffs {
                return false
            }
        }
        byte += 1
    }
    return true
}

[bstien]

This is an great fix for a weird problem, and I just want to voice my support in the hope that this PR gets merged.

We have the same issue with differences in produced colors on M1 and Intel and this will solve our issue until the whole team (and possibly CircleCI) is on M1. Lowering the precision threshold is not an option for us, since we primarily use this library to test placement and size of elements. Minor differences in colors, however, is something we can excuse since we now need to have a hybrid M1/Intel solution in our team.

I've opened PRs in our component libraries that will use @pimms fork until this (hopefully) gets merged.

https://github.com/finn-no/FinniversKit/pull/1094 https://github.com/finn-no/finnui-ios/pull/66

[Deco354]

@dmitri-zganiaiko Nice find on the threshhold calculation I think @pimms has addressed it and this is ready for review again.

[Deco354]

This is still blocked I think we need an approval from someone with write access. @stephencelis @mbrandonw are you able to help with this?

[choulepoka]

Just to emphasize the important of this PR, we have a team that begins to use M1 as the main development platform, but our CI will be on Intel for the foreseeable future (At least until Github provides M1-Based Github Runners).

[pimms]

@Deco354 Thanks, good points. I believe that a handful of tests are failing on main, but I may still have broken some more.

I don't think I'll have time to look at this this week at least, so if you or someone else wants to have a look at it it'd be great — otherwise I'll get to it at some point.

[Deco354]

One more thing, what exactly does 1 byte of the pixelDensityThreshold equate to? I'm not very familiar with the ins and outs of Image diffing. i.e. What value would this need to be set to for all possible pixel colors to be deemed equal?

[katzenbaer]

One more thing, what exactly does 1 byte of the pixelDensityThreshold equate to? I'm not very familiar with the ins and outs of Image diffing. i.e. What value would this need to be set to for all possible pixel colors to be deemed equal?

I believe it's using the UInt8 value representation of RGB. So a threshold value of 255 would mean that pixels would always match regardless of their color.

[pimms]

Great news, @stephencelis! 🎉 I've not had as much free time to look into the follow-ups as I thought I'd have, and I know I don't have much time coming up. If you're fine with merging this as-is, that sounds great 💯

[chosa91]

@pimms Can you tell us, what flags are used?

My team defaulted to adding custom compiler flags to SnapshotTesting, but the problem can also be diminished significantly by using a more basic loop implementation.

[pimms]

@chosa91 We were using CocoaPods at the time, and added the following at the end of our Podfile. I can't fully recall what Xcode-values this would translate to.

post_install do |installer|
    installer.pods_project.targets.each do |target|
        if target.name == "SnapshotTesting"
            target.build_configurations.each do |config|
                xcconfig_path = config.base_configuration_reference.real_path
                puts "Setting optimization flags on #{xcconfig_path}..."
                File.open(xcconfig_path, "a") {|file|
                  file.puts 'SWIFT_OPTIMIZATION_LEVEL = -O'
                  file.puts 'GCC_OPTIMIZATION_LEVEL = 3'
                }
            end
        end
    end
end

[chosa91]

@pimms Thanks a lot!

• SWIFT_OPTIMIZATION_LEVEL values
• GCC_OPTIMIZATION_LEVEL values

[cameroncooke]

Will this be merged soon, would be good to be able to start testing this?

[CraigSiemens]

We started using this fork in our project and it's been working great so far. Setting subpixelThreshold: 1 was enough to handle the differences in shadow rendering across different architectures for most of our failing tests.

[afanaian01]

Is there a schedule when this will be merged?

[reejosamuel]

Great news, @stephencelis! 🎉 I've not had as much free time to look into the follow-ups as I thought I'd have, and I know I don't have much time coming up. If you're fine with merging this as-is, that sounds great 💯

@stephencelis any thoughts on merging this?

[sschizas]

@stephencelis any plans to merge it?

[Kaspik]

@mbrandonw Trying to ping someone else from this org to take a look and merge it 🙏🏼

Edit: Seems like perceptualPrecision is probably better solution?

[stephencelis]

Thanks everyone in the community for their contributions towards landing this, and their patience! We've gone ahead and merged #628, which supersedes this one, so I'm going to close this out.

Thanks again @pimms for the PR, and if you believe #628 is missing anything, please let us know!