-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Increase tolerance for alternate architectures #17800
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
Conversation
I hope that most of the differences are in small anti-aliasing artifacts so I would expect the svg tests (which do the rasterization locally) should be a bit better than the png tests? |
For my understanding:
|
When we have dug into these issues in detail in the past they have either been true broken (because we were doing nasty things an the c-level that assumed byte order and I think we have fixed all of those) or issues with very small variations in float values. In the past the differences have been single bit flips in a single channel in ~dozen pixels in the image. We are essentially binning full float precision down to 256 (or less) bins as the final step of the rendering (because we save 32bit RGBA images). In most cases changes at the limits of float precision don't really matter (because it all goes down to the same bin), but if we get unlucky a tiny change can flip which side of a boundary the value is on and increase / decrease the value in the in the final output by 1 (which then massively magnifies the change). |
I was essentially wondering if it makes sense to pull out the architectures and not define the same machine lookup at every place individually: i.e. instead of
use
or
with
For this to make sense, there should be some comment why the extra tolerance is added and I don't know how to describe it. Maybe it's just "machines that empirically need larger tolerances". |
These are values based on actual results, though I did not check all the results directly as I have no access to all those systems. I did set the values to match aarch64, but only in the cases where the RMS difference was already the same. Note, there's at least one case where aarch64 has a special tolerance that the other ones here don't. Looking a little closer, I think it's actually almost all != x86_64, except that the 32-bit systems are already covered by the additional 0.06, so we could flip the check the other way (0 if x86_64, something else otherwise). |
I'm a not clear if @timhoffm is advocating for changing the default threshold on all tests on non-x86_64 or just inverting the logic on the handlful that need it. I may have over-learned worrying about non-0 tolerances (we used to have a blanket tolerance and then had text tests rendering the wrong glyphs but still come in under the threshold), but I think there is value in only tweaking the tests that need it (presumably because we for what ever reason we get unlucky on them) rather than on all tests. |
That's not exactly my point. I was just seeing a lot
where the same val (per case) was always applied to the same 3 platforms. This seems to be a common pattern that could be factored out. The functionality can stay exactly as it is now, the only question is if we want to factor it out. |
Sorry, I mis-understood you @timhoffm . |
Around 900 tests fail on 32-bit systems, but with rather small RMS.
These are mostly the same as the aarch64 tolerances, and also fail the same way on 32-bit systems (though this is hidden by the previous commit), so just increase tolerance everywhere but x86_64.
I flipped these all to |
Let's wait and give @tacaswell a final say. |
…800-on-v3.3.x Backport PR #17800 on branch v3.3.x (Increase tolerance for alternate architectures)
PR Summary
Most of the differences are just adding the other architectures alongside
aarch64
.The other change is a blanket increase of tolerance on 32-bit systems. There are too many failures, and the RMS is so small, that I don't think there's any need to investigate much further.
This should pass tests on all Fedora-supported architectures (x86_64, i686, armv7hl, aarch64, ppc64le, and s390x), though we do not currently test SVG output.
PR Checklist