Allow for subpixel deviations + improved unoptimized loop execution in image comparison by pimms · Pull Request #571 · pointfreeco/swift-snapshot-testing

pimms · 2022-02-09T13:26:07Z

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

Go to godbolt.org
Change language to Swift
Copy the following code in the left-hand editor
Wait for the compiler
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 · 2022-02-11T13:54:11Z

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.

finn-no/FinniversKit#1094
finn-no/finnui-ios#66

Deco354 · 2022-03-30T18:46:51Z

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

Deco354 · 2022-03-31T11:57:38Z

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 · 2022-04-03T19:58:10Z

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).

ncipollo · 2022-04-06T23:22:00Z

  public static func image(
    drawHierarchyInKeyWindow: Bool = false,
    precision: Float = 1,
+    subpixelThreshold: UInt8 = 0,


Would it maybe make sense the default the threshold to a value which will avoid the M1 vs Intel issue out of the box (ex - 5)?

I haven't seen any "one" value that would make sense. Sometimes, with corner-rounding for instance, the difference is <= 2, while with images it seems to typically be <= 1. With shadows and rounded corners, it may be as high as <= 5. I think all of these values are anecdotal though, and that the user should have a fully explicit relationship with this value. After all, this is a hacky workaround, and it's probably a good idea to not enable it by default.

YeH, makes sense, I was just hoping there was a magic value haha.

Deco354

@pimms Thanks for sharing this I’m using it now and it works great.

I’ve a few nits that don't really matter but on the off-chance this delays the eventual merge I'm happy to do myself if you're ok with it as they appear to be pretty trivial.

Update the .image entires within Documentation/Available-Snapshot-Strategies.md to include the sub-pixel property.
SceneKit snapshot strategy support
SpriteKit snapshot strategy support

I wasn't able to check that the internal snapshot tests for this project work, there are a fair few failing but I suspect this is due to me not knowing what simulator to use or the tests already failing.

pimms · 2022-05-23T05:15:35Z

@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 · 2022-05-26T17:06:40Z

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 · 2022-06-10T03:23:08Z

+  while offset < pixelCount * 4 {
+    if p1[offset].diff(between: p2[offset]) > subpixelThreshold {
+      differentPixelCount += 1
+      if differentPixelCount > threshold {
+        return false
+      }
    }
-    if Float(differentPixelCount) > threshold { return false }
+    offset += 1


Is there a reason this was changed from a for-loop to a while-loop? My interpretation is that in both implementations the loop should either increment offset or return false.

Using a while loop and manually incrementing the offset makes this less maintainable since it's easy to introduce changes in the loop body that skip incrementing offset and would lead to an infinite loop.

Nevermind, just saw your PR description with the explanation for when compiled without optimization.

katzenbaer · 2022-06-10T03:24:14Z

+  while byte < byteCount {
+    if oldBytes[byte].diff(between: newerBytes[byte]) > subpixelThreshold {
+      differentPixelCount += 1
+      if differentPixelCount >= threshold {
+        return false
+      }
+    }
+    byte += 1


Same comment here about replacing the for loop with a while loop.

katzenbaer · 2022-06-10T03:26:01Z

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.

stephencelis

@pimms Thanks for taking this on! We've been backed up on some of these PRs, but are hoping to merge them slowly.

I think we'll likely merge this as is unless you want to tie up some of the loose ends mentioned above, first. Once we merge we may also make a few small changes on main as far as API naming goes.

Let us know what you think!

pimms · 2022-06-13T04:57:40Z

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 · 2022-06-15T14:19:34Z

@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 · 2022-06-15T16:08:29Z

@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 · 2022-06-16T12:57:38Z

@pimms Thanks a lot!

cameroncooke · 2022-07-13T20:24:18Z

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

CraigSiemens · 2022-07-18T15:24:15Z

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 · 2022-07-21T21:42:18Z

Is there a schedule when this will be merged?

reejosamuel · 2022-08-08T16:59:08Z

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 · 2022-08-10T12:39:29Z

@stephencelis any plans to merge it?

Kaspik · 2022-08-25T22:10:27Z

@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 · 2022-09-21T23:14:37Z

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!

pimms mentioned this pull request Feb 9, 2022

Snapshots on Apple Silicon devices #424

Closed

pimms added 5 commits February 9, 2022 14:44

Added 'pixelDiffThreshold' in image comparison

07cd6eb

Removed debug output

3895720

Improved unoptimized loop execution in image comparison

d626b4d

Only checking if we've exceeded threshold if the current value fails

90db72c

Renamed parameter 'pixelDiffThreshold' to 'subpixelThreshold'

abf8272

pimms force-pushed the optimization branch from 8a0abd4 to abf8272 Compare February 9, 2022 13:49

pimms changed the title ~~Improved unoptimized loop execution in image comparison~~ Allow for subpixel deviations + improved unoptimized loop execution in image comparison Feb 9, 2022

pimms added 3 commits February 9, 2022 15:01

Merge remote-tracking branch 'upstream/main' into optimization

e6a5acd

Updated README

80bfc60

Removed .idea/ directory

67324cb

bstien mentioned this pull request Feb 11, 2022

Fix: Support for snapshot tests on Intel and M1 finn-no/FinniversKit#1094

Merged

dmitri-zganiaiko reviewed Feb 11, 2022

View reviewed changes

Comment thread Sources/SnapshotTesting/Snapshotting/UIImage.swift Outdated

gohanlon mentioned this pull request Feb 11, 2022

Precision checking algorithm is very slow #573

Closed

Fixed bug in threshold calculation

0efeef4

fnazarios approved these changes Feb 14, 2022

View reviewed changes

1ec5 mentioned this pull request Feb 14, 2022

Implement FixLocation.provider field based on CLLocationSourceInformation mapbox/mapbox-navigation-ios#3728

Merged

Jon889 mentioned this pull request Feb 18, 2022

No new releases since Apr 2021 - 1.9.0 #574

Closed

BarredEwe approved these changes Feb 24, 2022

View reviewed changes

anjones-seek mentioned this pull request Mar 30, 2022

m1 fixes seek-oss/swift-snapshot-testing#3

Merged

dmitri-zganiaiko approved these changes Mar 31, 2022

View reviewed changes

ncipollo reviewed Apr 6, 2022

View reviewed changes

timdolenko approved these changes Apr 26, 2022

View reviewed changes

joaofonseca-mindera approved these changes May 5, 2022

View reviewed changes

AngCosmin approved these changes May 12, 2022

View reviewed changes

Deco354 mentioned this pull request May 12, 2022

M1 Snapshot inconsistency fix tumblr/swift-snapshot-testing#1

Merged

Deco354 approved these changes May 20, 2022

View reviewed changes

katzenbaer reviewed Jun 10, 2022

View reviewed changes

stephencelis approved these changes Jun 10, 2022

View reviewed changes

katzenbaer added a commit to katzenbaer/swift-snapshot-testing that referenced this pull request Jun 11, 2022

Cherry-picks pixel compare from upstream PR pointfreeco#571

5eae34e

katzenbaer approved these changes Jun 11, 2022

View reviewed changes

n8chur mentioned this pull request Jun 15, 2022

[Feature] Record mode that only records significant differences #607

Open

ffittschen mentioned this pull request Jul 6, 2022

Allow for subpixel deviations + improved unoptimized loop execution in image comparison ffittschen/swift-snapshot-testing#1

Merged

simondelphia approved these changes Jul 8, 2022

View reviewed changes

gohanlon mentioned this pull request Aug 23, 2022

Test fails if simulator is changed #614

Closed

ejensen mentioned this pull request Sep 12, 2022

Perceptual image precision + 90% speed improvement #628

Merged

stephencelis closed this Sep 21, 2022

msadoon mentioned this pull request Nov 27, 2022

[NTV-648] Fix Screenshot Tests Between Intel Recordings and M1 Verifications kickstarter/ios-oss#1757

Closed

5 tasks

Uh oh!

Conversation

pimms commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Subpixel difference

Optimization

Uh oh!

bstien commented Feb 11, 2022

Uh oh!

Uh oh!

Deco354 commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Deco354 commented Mar 31, 2022

Uh oh!

choulepoka commented Apr 3, 2022

Uh oh!

ncipollo Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

pimms Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

ncipollo Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

Deco354 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pimms commented May 23, 2022

Uh oh!

Deco354 commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katzenbaer Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

katzenbaer Jun 11, 2022

Choose a reason for hiding this comment

Uh oh!

katzenbaer Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

katzenbaer commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephencelis left a comment

Choose a reason for hiding this comment

Uh oh!

pimms commented Jun 13, 2022

Uh oh!

chosa91 commented Jun 15, 2022

Uh oh!

pimms commented Jun 15, 2022

Uh oh!

chosa91 commented Jun 16, 2022

Uh oh!

cameroncooke commented Jul 13, 2022

Uh oh!

CraigSiemens commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afanaian01 commented Jul 21, 2022

Uh oh!

reejosamuel commented Aug 8, 2022

Uh oh!

sschizas commented Aug 10, 2022

Uh oh!

Kaspik commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephencelis commented Sep 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

pimms commented Feb 9, 2022 •

edited

Loading

Deco354 commented Mar 30, 2022 •

edited

Loading

Deco354 left a comment •

edited

Loading

Deco354 commented May 26, 2022 •

edited

Loading

katzenbaer commented Jun 10, 2022 •

edited

Loading

CraigSiemens commented Jul 18, 2022 •

edited

Loading

Kaspik commented Aug 25, 2022 •

edited

Loading