path_geometry::tests::find_cubic_max_curvature_1 fails on i686 (no SSE)

[jamessan]

thread 'path_geometry::tests::find_cubic_max_curvature_1' panicked at 'assertion failed: `(left == right)`
  left: `[NormalizedF32(FiniteF32(0.0)), NormalizedF32(FiniteF32(0.50000006)), NormalizedF32(FiniteF32(1.0))]`,
 right: `[NormalizedF32(FiniteF32(0.0)), NormalizedF32(FiniteF32(0.5)), NormalizedF32(FiniteF32(1.0))]`', path/src/path_geometry.rs:892:9

Encountered in Debian's build of 0.8.2. Also reproduces in 4983d28.

[CryZe]

i386 (to be clear that's not i686) afaik is not compliant with IEEE 754 which kinda means it's not "a valid Rust target" to some degree.

[RazrFalcon]

Well... as long as Skia produces the same output then this is not a bug.
Looking at code it's just a basic math. So I would say that @CryZe is right and this is not a bug.

All we can do is to disable this test on i386 targets. But resvg tests would fail anyway. So it doesn't really matter.

PS: by the way, do this target supports SIMD to begin with? If not, then it's out of scope. While tiny-skia supports scalar fallback, using it without AVX (aka Haswell) is mostly pointless.

[CryZe]

Oh wait, I believe they are actually talking about i686 though, which does support IEEE-754, so I feel like that should work.

[RazrFalcon]

@CryZe This is not the first time such an error was reported: #43

[RazrFalcon]

Ideally we should have a compile time check for proper IEEE-754. While it's not easy, I'm striving to have reproducible output int tiny-skia.

[RazrFalcon]

@jamessan Is there even a way I can reproduce this target locally? I can bootup live cd with Linux if needed.

[CryZe]

It should just be the i686-unknown-linux-gnu target it seems. And I've checked and it seems like SSE and SSE2 are active there by default just like with x86-64 (a little surprising tbh). So I'm very much not seeing a reason any test should fail anymore.

Rust defines i686 as a minimum of "pentium4": Here
And LLVM defines the following feature set for "pentium4": Here

[jamessan]

@CryZe This is not the first time such an error was reported: #43

That issue mentions only arm64 and amd64 being supported. This doesn't seem to be declared anywhere, so that people wanting to use tiny-skia know where it should be used. What would they use instead on other architectures?

I was going to open a separate issue about tests failing on mipsel/mips64el, but it sounds like that might be considered out of scope too.

[RazrFalcon]

@CryZe While find_cubic_max_curvature is a pretty heavy function, it's just float math + some libm stuff (sqrt, acos, powf). So it's either a Rust bug or idk. I need a way to reproduce it locally first.

All I know is that it works just fine on a modern hardware. At least amd64 and aarch64 are just fine.

[CryZe]

Alright, so I got it to reproduce by specifying -C target-feature=-sse,-sse2 which I guess Debian does as well somewhere at which point you lose the XMM registers and instead the x86 FPU / x87 seems to be used, which produces heavily different results throughout the board. Apparently since x87 uses 80-bit instead of proper 32-bit / 64-bit floating point numbers, the results are not the same:

Source

[RazrFalcon]

@jamessan tiny-skia works on the same hardware where the original Skia works. That's all I can say.

1. I have tested it/care about only amd64/aarch64/wasm.
2. Using tiny-skia without at least SSE2 makes no sense. It would be slow as hell.

[RazrFalcon]

@CryZe last time I've tried -sse2 rustc was not able to compile the code to begin with. I guess it was fixed since?

Is there a way to check it during compilation? Or we can have a dedicated test that will always fail on such architectures.

[CryZe]

I'd honestly just live with the fact that it's less precise there (maybe compile out the problematic tests). In fact I use tiny-skia in my i586 tests as well and I simply allow for some more precision errors there: My Tests

It's not wildly different, so it still produces reasonably close images.

[RazrFalcon]

Then checking for sse on x86 should do the trick. There is no way to support such old targets. tiny-skia must produce the same output on all supported targets. I have no time supporting those edge-cases.

[RazrFalcon]

Basically I propose to add:

#[cfg(all(target_arch = "x86", not(target_feature = "sse2")))]
compile_error!("Your hardware is too old.");

[CryZe]

Why must it produce the same output on all targets? Different browsers don't render the exact same as the others either and it's "good enough" for the web. And as a user of tiny-skia I'm totally fine with that too, so if the user of the crate is fine with such slight differences, they should be able to make that decision imo. I'd honestly just update the README to strongly clarify which platforms are guaranteed to produce the correct results and which aren't.

Also @jamessan I'm not sure what Debian is doing here is even correct. Removing features from a target is usually the "wrong thing to do" as the standard library is precompiled and uses those features. So while you may compile tiny-skia without SSE here, the standard library will still use it, meaning the executable won't work on a machine without SSE anyway. This is all under the assumption that Debian doesn't compile std itself the same way of course.

[jamessan]

Looks like Debian changes the i686 target to pentiumpro.

As far as which architectures are supported, it doesn't particularly matter to me. :) I'm certainly not trying to impose work on anyone. If the builds fail, then that will naturally define which architectures in Debian will have projects that use tiny-skia.

If that support broadens in the future, then it will automatically be handled as the builds stop failing. The only issue is when previously building architectures start failing.

[jamessan]

Also, going back to my comment about mips(64)el eariler, this is the test that fails, in case it's of interest.

---- scalar::tests::bound stdout ----
thread 'scalar::tests::bound' panicked at 'assertion failed: `(left == right)`
  left: `0.0`,
 right: `1.0`', src/scalar.rs:160:9

[RazrFalcon]

@jamessan as for the mips bug, min/max if failing. I'm not sure if this is a Rust bug or what. According to Platform Support, mips(64)el is Tier 2, so this one is definitely out of scope. We support only Tier 1 targets. I will update the readme just in case.

[RazrFalcon]

@CryZe tiny-skia must produce the same output on all platforms because it was written for resvg. And testing resvg is a nightmare. The less variation I have - the better.

Not to mention that there is still a chance to get a vastly different output, it just not happened yet.

Honestly, I barely have time to work on my projects to begin with. The smaller the scope of each crate - the better for me.