Fix Vp8Residual costs calculation

[brianpopow]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
• I have provided test coverage for my change (where applicable)

Description

This PR fixes an issue with the AVX2 version of the residual costs calculation. In some rare cases it does not match the scalar version. I have switch back to the SSE2 variant, which matches the original libwebp implementation.

AVX version was

Vector256<short> c0 = Unsafe.As<short, Vector256<byte>>(ref outputRef).AsInt16();
Vector256<short> d0 = Avx2.Subtract(Vector256<short>.Zero, c0);
Vector256<short> e0 = Avx2.Max(c0, d0); // abs(v), 16b
Vector256<sbyte> f = Avx2.PackSignedSaturate(e0, e0);
Vector256<byte> g = Avx2.Min(f.AsByte(), Vector256.Create((byte)2));
Vector256<byte> h = Avx2.Min(f.AsByte(), Vector256.Create((byte)67)); // clampLevel in [0..67]

SSE version:

Vector128<short> c0 = Unsafe.As<short, Vector128<byte>>(ref outputRef).AsInt16();
Vector128<short> c1 = Unsafe.As<short, Vector128<byte>>(ref Unsafe.Add(ref outputRef, 8)).AsInt16();
Vector128<short> d0 = Sse2.Subtract(Vector128<short>.Zero, c0);
Vector128<short> d1 = Sse2.Subtract(Vector128<short>.Zero, c1);
Vector128<short> e0 = Sse2.Max(c0, d0); // abs(v), 16b
Vector128<short> e1 = Sse2.Max(c1, d1);
Vector128<sbyte> f = Sse2.PackSignedSaturate(e0, e1);
Vector128<byte> g = Sse2.Min(f.AsByte(), Vector128.Create((byte)2)); // context = 0, 1, 2
Vector128<byte> h = Sse2.Min(f.AsByte(), Vector128.Create((byte)67)); // clampLevel in [0..67]

The problem was that Avx2.PackSignedSaturate(e0, e0); is not the same as Sse2.PackSignedSaturate(e0, e1);

Related to #2414

[JimBobSquarePants]

We should probably use a custom JSON converter in the tests rather than adding attributes to the source.

[JimBobSquarePants]

Sorry @brianpopow I've given you the runaround here. I didn't realize you'd introduced the constructors.

Let's go with your original plan but wrap the namespace imports and constructor code in the following conditional.

#if SIXLABORS_TESTING || SIXLABORS_TESTING_PREVIEW

[brianpopow]

Let's go with your original plan but wrap the namespace imports and constructor code in the following conditional.
#if SIXLABORS_TESTING || SIXLABORS_TESTING_PREVIEW

I dont think that's a good idea. If someone does a fresh clone of the repo and tries to run the tests, those webp tests will fail without any obvious reason. I dont think test should behave different when a environment is set. Unfortunately I dont have a better idea so far.

[JimBobSquarePants]

Can we avoid the json and populate the test instance with equivalent data generated in a class using string builder or similar?

[brianpopow]

Can we avoid the json and populate the test instance with equivalent data generated in a class using string builder or similar?

Usually I would create a test instance in the unittest and populate it with data, but for Vp8Residual that does not seem feasible. There are so many nested arrays, that I dont know how this can be populated manually. Thats why I have chosen to go for json serialization.

Also I dont understand your suggestion with the string builder.

I now tried to use the binary serialization and this was looking promising. We would not need the additional constructors.
But now I noticed its marked as obsolete 😢

See: https://learn.microsoft.com/de-de/dotnet/standard/serialization/binaryformatter-security-guide

[brianpopow]

Changed serialization now to use BinaryWriter/BinaryReader, according to https://learn.microsoft.com/de-de/dotnet/standard/serialization/binaryformatter-security-guide this should be fine.

No additional constructors required now.

[JimBobSquarePants]

Ah that’s fantastic! We’ll done finding a solution!