Skip to content

Conversation

@stefannikolei
Copy link
Contributor

Port Grayscaleconverter to ARM

Benchmarks:

|        Method |      Mean |    Error |   StdDev | Ratio | RatioSD |
|-------------- |----------:|---------:|---------:|------:|--------:|
|        Scalar | 118.76 ns | 0.499 ns | 0.443 ns |  1.00 |    0.00 |
| SimdVectorAvx |        NA |       NA |       NA |     ? |       ? |
| SimdVectorArm |  53.57 ns | 0.410 ns | 0.364 ns |  0.45 |    0.00 |

@stefannikolei stefannikolei force-pushed the stefannikolei/arm/colorconvertergrayscale branch from a4556fd to a4fe64a Compare March 22, 2023 19:11
@JimBobSquarePants JimBobSquarePants added this to the v3.1.0 milestone Mar 22, 2023
Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great speed up! All these additions are going to be a gamechanger for ARM

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Mar 24, 2023

Looks like something in #2401 has changed the result of some of the WebP output on ARM.

@brianpopow
Copy link
Collaborator

Looks like something in #2401 has changed the result of some of the WebP output on ARM.

Just stating the obvious: It seems impossible to me, that the changes from this PR could be responsible for WebP test failures.
@stefannikolei If you run the webp tests on you machine, do you see any test failing?
You can run only Webp tests with: dotnet test -c Release -f net6.0 --filter "Format=Webp"

@stefannikolei
Copy link
Contributor Author

   WebpDecoderTests
    WebpDecoder_CanDecode_Lossless_WithThreeTransforms
     WebpDecoder_CanDecode_Lossless_WithThreeTransforms<Rgba32>(provider: Webp/bike_lossless.webp[Rgba32])
     WebpDecoder_CanDecode_Lossless_WithThreeTransforms<Rgba32>(provider: Webp/color_cache_bits_11.webp[Rgba32])
    WebpDecoder_Decode_Resize
     WebpDecoder_Decode_Resize<Rgba32>(provider: Webp/bike_lossless.webp[Rgba32])

Yeah this tests fail

@JimBobSquarePants
Copy link
Member

Looks like something in #2401 has changed the result of some of the WebP output on ARM.

Just stating the obvious: It seems impossible to me, that the changes from this PR could be responsible for WebP test failures. @stefannikolei If you run the webp tests on you machine, do you see any test failing? You can run only Webp tests with: dotnet test -c Release -f net6.0 --filter "Format=Webp"

@brianpopow #2401 is the low hanging fruit PR. WebP is touched there. I'm doing another code review as we didn't label the PR to catch ARM issues.

WebpDecoder_CanDecode_Lossless_WithThreeTransforms_Rgba32_color_cache_bits_11

@gfoidl
Copy link
Contributor

gfoidl commented Mar 24, 2023

I also have a look at this right now.

@gfoidl
Copy link
Contributor

gfoidl commented Mar 24, 2023

Just a FYI for easier location of error on x86-dev-machine: run the tests with DOTNET_EnableSSE=0 dotnet test -c Release -f net6.0 --filter "Format=Webp"

-- or --

Update the .runsettings to

<?xml version="1.0" encoding="utf-8" ?>
<RunSettings>
    <RunConfiguration>
        <!--Used in conjunction with ActiveIssueAttribute to skip tests with known issues-->
        <TestCaseFilter>category!=failing</TestCaseFilter>

        <EnvironmentVariables>
            <DOTNET_EnableSSE>0</DOTNET_EnableSSE>
        </EnvironmentVariables>
    </RunConfiguration>
</RunSettings>

@gfoidl
Copy link
Contributor

gfoidl commented Mar 24, 2023

Hm, so far I can't spot the bug.

State so far:

  • AVX --> ✔️
  • ARM --> 🐛
  • SSE disabled --> scalar --> 🐛

So I assume the bug isn't specific to ARM, rather it's anywhere in the scalar-path (that is hit by ARM too, but not with SSE/AVX).
Will search and debug further to smash the bug down....

@gfoidl
Copy link
Contributor

gfoidl commented Mar 24, 2023

I found something, will send a PR in a moment (just checking if a similar bug doesn't exist elsewhere).

Edit: PR --> #2413

@gfoidl gfoidl mentioned this pull request Mar 24, 2023
4 tasks
@JimBobSquarePants
Copy link
Member

OK. Let's get this merged as the changes are covered. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants