Add arm64 intrinsics for cmyk converter #2400

stefannikolei · 2023-03-13T12:37:03Z

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

Added Simd for CMYK. I added ARM64 because there was no matching method in arm32

Benchmarks:

Method	Mean	Error	StdDev	Ratio	RatioSD
Scalar	288.7 ns	1.75 ns	1.64 ns	1.00	0.00
SimdVector8	125.4 ns	1.74 ns	1.63 ns	0.43	0.01
SimdVectorAvx	NA	NA	NA	?	?
SimdVectorArm64	108.1 ns	0.56 ns	0.43 ns	0.37	0.00

as suggested in SixLabors#2397 (comment)

antonfirsov · 2023-03-14T00:45:40Z

The 1-2% differences reported by the test failures don't seem to be horrible, but @stefannikolei do you have an opportunity to check the output images visually just to be on the safe side?

Also, I would make sure the increased tolerance only applies to ARM, and only for the images where it's necessary. We can define conditional percentage values right in TheoryData definitions:

ImageSharp/tests/ImageSharp.Tests/Formats/Jpg/JpegEncoderTests.cs

Lines 49 to 54 in 5283d77

    
           public static readonly TheoryData<JpegEncodingColor, int, float> CmykEncodingSetups = new() 
        
           { 
        
               { JpegEncodingColor.Cmyk, 100, 0.0159f / 100 }, 
        
               { JpegEncodingColor.Cmyk, 80,  0.3922f / 100 }, 
        
               { JpegEncodingColor.Cmyk, 40,  0.6488f / 100 }, 
        
           };

stefannikolei · 2023-03-14T05:58:51Z

@antonfirsov For me the visual test failed. 😞

br3aker · 2023-03-14T06:56:20Z

Don't have an ARM with me but I'll try to take a look on this today, looks like a clamping issue to me.

stefannikolei · 2023-03-14T13:06:35Z

Don't have an ARM with me but I'll try to take a look on this today, looks like a clamping issue to me.

If you want I can give you temp access to my arm vm.

antonfirsov · 2023-03-14T15:20:58Z

We should also check RGB conversion output from #2397.

antonfirsov · 2023-03-14T15:22:34Z

tests/ImageSharp.Tests/Formats/Jpg/JpegColorConverterTests.cs

@@ -20,7 +20,7 @@ public class JpegColorConverterTests

    private const int TestBufferLength = 40;

-    private const HwIntrinsics IntrinsicsConfig = HwIntrinsics.AllowAll | HwIntrinsics.DisableAVX;
+    private const HwIntrinsics IntrinsicsConfig = HwIntrinsics.AllowAll | HwIntrinsics.DisableHWIntrinsic;


I wonder why don't these tests fail with JpegColorConverterArm64?

Me too, we need to re-review the tests, they should fail.

Ah now I got you... The Encoder tests failed. And this should also fail...

@stefannikolei or @brianpopow since you have the VM - can you try injecting an exception into the CmykArm64 methods and running dotnet test -f net6.0 --filter FullyQualifiedName~JpegColorConverterTests? If it gets hit, there's probably a bug in the tests since it they didn't show the regression, if not, it's probably a bug in FeatureTestRunner.

those failed:
ImageSharp\tests\ImageSharp.Tests\Formats\Jpg\JpegColorConverterTests.cs:line 81

But I also saw all xxxFromVector methods fail because it is not supported on the hardware(vm) --> The DebugGuard throws here because I ran the tests in Debug mode

Child exception: System.InvalidOperationException: YccKVector converter is not supported on current hardware. at SixLabors.DebugGuard.IsTrue(Boolean target, String message) in C:\Users\nikoleis\source\ImageSharp\src\ImageSharp\Common\Helpers\DebugGuard.cs:line 24 at SixLabors.ImageSharp.Formats.Jpeg.Components.JpegColorConverterBase.JpegColorConverterVector.ConvertToRgbInplace(ComponentValues& values) in C:\Users\nikoleis\source\ImageSharp\src\ImageSharp\Formats\Jpeg\Components\ColorConverters\JpegColorConverterVector.cs:line 41 ``

@antonfirsov The FeatureTestRunner seems to work as expected. Some path of the intrinsics methods (ConvertFromRgb) were not covered by tests. That's why the error was not detected. I have added tests for those.

I have decided to revert the IntrinsicsConfig back to HwIntrinsics.AllowAll | HwIntrinsics.DisableAVX2. I think it was a mistake to change it. The IntrinsicsConfig is only used by the Vector color conversion tests. On x64 disabling AVX2 makes sense to test SSE here.

This means on ARM the test will be run twice, once 'HwIntrinsics.AllowAlland onceHwIntrinsics.DisableAVX2'. I dont know howto change it so the second run will be avoided, but I also think its not a big deal. If anyone has a better idea let me know.

Perhaps we can use this lib https://github.com/AArnott/Xunit.SkippableFact and just skip the not supported runs.

So we can define the desired ones (avx/sse/arm) as a theory.

I dont think its really worth the extra effort only to skip an addional test run on ARM. We maybe need to re-evaluate once we add more ARM code / tests.

stefannikolei · 2023-03-14T15:42:04Z

We should also check RGB conversion output from #2397.

The RGB pictures look good to me

Filename: EncodeBaseline_Interleaved_Rgb24_CalliphoraPartial_Rgb-Q100

brianpopow · 2023-03-15T18:27:48Z

src/ImageSharp/Formats/Jpeg/Components/ColorConverters/JpegColorConverter.CmykArm64.cs

+                Vector128<float> ytmp = AdvSimd.Subtract(scale, Unsafe.Add(ref srcB, i));
+                Vector128<float> ktmp = AdvSimd.Min(ctmp, AdvSimd.Min(mtmp, ytmp));
+
+                Vector128<float> kMask = AdvSimd.CompareEqual(ktmp, scale);


The AVX version has Avx.CompareNotEqual here, could that be the problem?

Good catch. I will have a Look. Can not remember changing it while porting 🫣

I tried debugging the code on VS 2022 but it just hangs when I step to the next line :(

This line should not make any difference in my eyes. Because it is ConvertFromRgb. The code which we need to look into is ConvertToRgb? Or am I wrong?

ConvertFromRgb is used during encoding from ImageSharp to actual jpeg image.
ConvertToRgb is used during decding from jpeg to ImageSharp.
Problem can be in any of them.

The tests work now :) Any chance to get the test output from the test run?

I changed it from Negate to Not. That makes sense :)

@stefannikolei great that you found the problem.

Any chance to get the test output from the test run?

Unfortunately I think the artifacts of the test run are only uploaded when it fails. I will try to tests it on ARM tomorrow and see if the results look good.

I ran the tests. The images look good now

I can confirm, the image looks good now.

brianpopow · 2023-03-17T10:47:50Z

@stefannikolei I just tried to build your branch on my ARM device, but I am getting build errors. I pretty sure those build errors are not related to your changes. I am getting compiler error CS8170 in ColorMatrix.impl.cs.

I am using dotnet sdk 6.0.113 on ubuntu. What version are you using? Any idea howto fix this?

stefannikolei · 2023-03-17T10:50:29Z

@stefannikolei I just tried to build your branch on my ARM device, but I am getting build errors. I pretty sure those build errors are not related to your changes. I am getting compiler error CS8170 in ColorMatrix.impl.cs.

I am using dotnet sdk 6.0.113 on ubuntu. What version are you using? Any idea howto fix this?

I am using 7.0.xxx that's why the dotnet6 arm builds/test is disabled. Cause the image only had dotnet6 and in dotnet6 sdk the implementation of colormatrix does not build.

This is discussed in the rgb PR

JimBobSquarePants · 2023-03-17T12:33:33Z

We should update the readme to say we need the NET7 SDK

…rinsics

…leAVX2

JimBobSquarePants · 2023-03-20T08:14:02Z

I'm happy here if everyone else is.

br3aker · 2023-03-20T13:49:40Z

Looks good!

antonfirsov

All good, thanks @brianpopow for cleaning up the tests!

brianpopow

I also think, this good to be merged now.

stefannikolei added 2 commits March 13, 2023 13:36

Add arm64 intrinsics for cmyk converter

b1a98cb

Disable HwIntrinsics for Tests

ebd8114

as suggested in SixLabors#2397 (comment)

brianpopow added arch:arm32 arch:arm64 labels Mar 13, 2023

antonfirsov reviewed Mar 14, 2023

View reviewed changes

brianpopow reviewed Mar 15, 2023

View reviewed changes

stefannikolei added 3 commits March 15, 2023 20:57

Try replicate CompareNotEqual with AdvSimd

5a079c0

Try with not

d7a3e80

remove unused using

595c4bb

stefannikolei and others added 7 commits March 17, 2023 15:58

Update readme Net7 SDK required

7fbe22f

Add ARM tests FromCmykArm and FromRgbArm

e8f68d3

Add comparing intrinsics color conversion to scalar version

1e2607e

Rename ValidateConversion to ValidateConversionToRgb

b7c2e6f

Add ValidateConversionFromRgb for all color conversions which use int…

ec3a08f

…rinsics

Compare with tolerance

8c754c5

Revert IntrinsicsConfig to HwIntrinsics.AllowAll | HwIntrinsics.Disab…

a3f3bfc

…leAVX2

antonfirsov approved these changes Mar 20, 2023

View reviewed changes

brianpopow approved these changes Mar 21, 2023

View reviewed changes

antonfirsov merged commit d7cd46f into SixLabors:main Mar 21, 2023

stefannikolei deleted the stefannikolei/arm/colorConvertercmyk branch March 25, 2023 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Add arm64 intrinsics for cmyk converter #2400

Add arm64 intrinsics for cmyk converter #2400

stefannikolei commented Mar 13, 2023 •

edited

Loading

antonfirsov commented Mar 14, 2023 •

edited

Loading

stefannikolei commented Mar 14, 2023

br3aker commented Mar 14, 2023

stefannikolei commented Mar 14, 2023

antonfirsov commented Mar 14, 2023

antonfirsov Mar 14, 2023

brianpopow Mar 15, 2023

stefannikolei Mar 15, 2023

antonfirsov Mar 16, 2023 •

edited

Loading

stefannikolei Mar 17, 2023 •

edited

Loading

brianpopow Mar 18, 2023

brianpopow Mar 18, 2023

stefannikolei Mar 18, 2023

brianpopow Mar 21, 2023

stefannikolei commented Mar 14, 2023 •

edited

Loading

brianpopow Mar 15, 2023

stefannikolei Mar 15, 2023

stefannikolei Mar 16, 2023

br3aker Mar 16, 2023

stefannikolei Mar 16, 2023

brianpopow Mar 16, 2023

stefannikolei Mar 17, 2023

brianpopow Mar 17, 2023

brianpopow commented Mar 17, 2023

stefannikolei commented Mar 17, 2023

JimBobSquarePants commented Mar 17, 2023

JimBobSquarePants commented Mar 20, 2023

br3aker commented Mar 20, 2023

antonfirsov left a comment

brianpopow left a comment

Add arm64 intrinsics for cmyk converter #2400

Add arm64 intrinsics for cmyk converter #2400

Conversation

stefannikolei commented Mar 13, 2023 • edited Loading

Prerequisites

Description

antonfirsov commented Mar 14, 2023 • edited Loading

stefannikolei commented Mar 14, 2023

br3aker commented Mar 14, 2023

stefannikolei commented Mar 14, 2023

antonfirsov commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

stefannikolei Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefannikolei commented Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianpopow commented Mar 17, 2023

stefannikolei commented Mar 17, 2023

JimBobSquarePants commented Mar 17, 2023

JimBobSquarePants commented Mar 20, 2023

br3aker commented Mar 20, 2023

antonfirsov left a comment

Choose a reason for hiding this comment

brianpopow left a comment

Choose a reason for hiding this comment

stefannikolei commented Mar 13, 2023 •

edited

Loading

antonfirsov commented Mar 14, 2023 •

edited

Loading

antonfirsov Mar 16, 2023 •

edited

Loading

stefannikolei Mar 17, 2023 •

edited

Loading

stefannikolei commented Mar 14, 2023 •

edited

Loading