Fixed wrong division hack

[gfoidl]

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

In #2401 I introduced a bug (sorry for that) -- this PR fixes that.
The bit-hack for divison by powers of two is only valid for positive numerators, and in the failed testcase (cf. #2409 (comment)) it was negative. As showcase see this sharplab.

Note to maintainers: please run CI on ARM too (as I understand that's not on by default, so that bug even came into main-branch)

[gfoidl]

While hunting this bug I tried several things (although in hindsight it's quite obvious that the wrong use of the bit-hack is the bug), amongst others disabling SSE with this

ImageSharp/.runsettings

Line 2 in 0a1f05b

<RunSettings>

<?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>

and more tests started to fail -- even when going back before #2401 (e.g. to 5283d77).
To reproduce together with that runsettings one can try this playlist.zip (for filtered tests).

To me it looks like there are latent bugs in the scalar code-pathes. Can anyone confirm? Should I open a separate issue to handle these (potential) failures?

[brianpopow]

In #2401 I introduced a bug (sorry for that) -- this PR fixes that.

No need to be sorry here, it happens, no problem.

Note to maintainers: please run CI on ARM too (as I understand that's not on by default, so that bug even came into main-branch)

ARM CI runner only runs when the PR is tagged with ARM. That's a compromise we need to make, because it costs us money to run those ARM CI runner. I know that's not ideal, but usually changes to ARM code was very rare in the past.

and more tests started to fail -- even when going back before #2401 (e.g. to 5283d77).
To reproduce together with that runsettings one can try this playlist.zip (for filtered tests).
To me it looks like there are latent bugs in the scalar code-pathes. Can anyone confirm? Should I open a separate issue to handle these (potential) failures?

Yes please open a issue for that, we should investigate that. We have a code coverage report, I will look into the report tomorrow and see if we would have known that its not covered.

One thing I would like to have is a simple unit test for AddSubtractComponentHalf to cover this issue. Just pick some random input values for a and b and make sure the right result is produced.

[gfoidl]

simple unit test for AddSubtractComponentHalf to cover this issue

It's a private method

ImageSharp/src/ImageSharp/Formats/Webp/Lossless/LosslessUtils.cs

Line 1459 in ca21b6d

private static int AddSubtractComponentHalf(int a, int b) => (int)Clip255((uint)(a + ((a - b) / 2)));

So for my PoV

• it's not worth it to expose it public / internal just for that test
• as we've seen it should be covered by tests -- but I'll open an issue (Some unit tests fail with SSE disabled (i.e. scalar path only) #2414) for the other point (see above) and test-coverage will be handled in that issue for this piece of code too

_{No need to be sorry here, it happens, no problem.}

_{I know, but my own quality standard shouldn't allow such a mistake 😁}

[brianpopow]

simple unit test for AddSubtractComponentHalf to cover this issue

It's a private method

Ah, yes your right, I overlooked that it's private.

[brianpopow]

@gfoidl thanks for looking into this issue and providing the fix.