@tocsoft yes indeed that would be better. I will change that.

Codecov Report

Merging #603 into master will decrease coverage by 0.25%.
The diff coverage is 89.04%.

@@            Coverage Diff             @@
##           master     #603      +/-   ##
==========================================
- Coverage   88.79%   88.53%   -0.26%     
==========================================
  Files         854      872      +18     
  Lines       36207    36822     +615     
  Branches     2624     2620       -4     
==========================================
+ Hits        32149    32602     +453     
- Misses       3278     3444     +166     
+ Partials      780      776       -4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a8202c...03faf82. Read the comment docs.

@tocsoft travis seems to fail with:

The command "sudo apt-get install -qq dotnet-sdk-2.1.300-rc1-008673" failed and exited with 100 during .

I guess this will be fixed if i change the travis file from dotnet: 2.1.300-rc1-008673 to dotnet: 2.1.300.
2.1.300 was released 3 days ago.

I am not sure if that would fix it, i am not familiar with travis.

Is it ok, if i give it a try?

@brianpopow yeah feel free!

We need to update our CI, so I'm curious what will be the outcome of this.

seems to have worked :-)

Different tests failing now Rgba64 to Bgr24 and Rgba32 weird!

@JimBobSquarePants @brianpopow we need to run those tests locally in 32 bit + older runtime, and analyse whether they need skipping, deleting, or a fix/workaround.

Unfortunately I'm out of "ImageSharp office" at least for a week :(

i thought i may have made a mistake during refactoring and double checked the original and the refactored version. As far as i can tell, the tests before and after refactoring should be the same.

I did not notice this, because i always tested with netcoreapp2.0.

This failing tests are not new. I checked the current master branch. The tests in Rgba64 is skipped in the PackedPixelTests, thats why it was not noticed.

This time this issue seems to be at least reproducable in Debug and Release mode.

@antonfirsov, @JimBobSquarePants: i think i have found the reason for the rgba64 tests failing.

It happens when build with x86 and with net462/net471.

In the Constructor of Rgba64 rounding is applied after multiplying the value with 65535. The test values for the B-Channel which was failing was 0.30f. Multiplying by 65535 leads to 19660,5. Rounding this should be 19660, but for net462/net471 this gives 19661.

here is a simple reproduction, which mimics what happens in the RGBA Constructor:

[Fact]
public void Rgba64_Rounding_Test()
{
    var b = 0.30f;

    ulong actual = (ulong)System.Math.Round(b * 65535F);

    Assert.Equal(19660UL, actual);
}

i think this is maybe an edge case for float not being able to represent decimal values accurately enough.

We probably shouldn't be using Math.Round() here without specifying a MidpointRounding setting. We should probably be using MidpointRounding.AwayFromZero (not the .net default) as that is way more intuitive to most users and less likely to cause issues in the future.

@tocsoft my suggestion would be: changing the rgba64 constructor to use double instead of float. This would fix this issue. That does not mean there are no such edge cases with double too, but its at least much more unlikely.

yes I think your right, that does sound like its probably the better fix.

@brianpopow Nice detective work! 🥇

As long as we can interop with MonoGame's Rgba64 I'm happy with using double.

However, I would suggest changing the Pack method and keeping the current constructor intact.

We should probably raise an issue with MonoGame also.

I think the difference in the result is negligable, our test assertion shouldn't be that strict when dealing with floats. We need R, G, B, A properties on Rgba64 however to implement tolerant assertions.

i must admit, i have rushed too fast to a conclusion yesterday. It is not actually a float issue.
The error comes from converting to double actually (which is implicit done in Round).

Here is a simple example to illustrate this

var b = 0.30f;
var foo = b * 65535F;
var bar = (double)(b * 65535F);

This results in x86 to

foo = 19660,5
bar = 19660,5007812381

Which explains why Round gives 19661 for that.

There is a simple fix for that. Im just using MathF.Round from Sixlabors.Core.
This will cast the expression z.Clamp(0, 1) * 65535F to float before rounding.

There is still one test failing Rgba32_ToRgb24. This is some special kind of snowflake.

If you look at the assertion:

Expected: (0,0,128)
Actual:   (26,0,128)

But if you look into the testcase, the expected value is: var expected = new Rgb24(0x1a, 0, 0x80);, which is exactly the actual value. No clue yet why this assert is failing...

This issue with Rgba32_ToRgb24 seems to happen only in Release Mode again (just for the record). I could not find the real cause of it.

The best thing i have got is a workaround, which changes the assertion slightly: assert workaround

This workaround seems to indicate that the Equals in Rgb24 is maybe the issue, but im very sure there is no mistake in it. My best guess is, that this is some IL code generation issue, but i have no experience in debugging those kind of issues.

@brianpopow with which framework did it fail? The older ones only?

@antonfirsov net462/net471 at least. I suspect this issue only happens on x86, but that's just a guess. I could not test it on netcore2.0 x86 yet.

@brianpopow definitely an untraceable JIT bug that got lost in the fog fixed meanwhile.

Your output suggests that the bug is probably in the code generated for the aggressively inlined Rgb24(byte r, byte g, byte b) constructor.

Your test change is fine, TBH I do not want to spend more efforts on this kind of stuff - unless a user finds an actual bug related to this issue.

Agreed. I think we should be safe to merge this and move on.

@brianpopow Thanks for making such an awesome effort to refactor these tests and investigate issues. It must have been a chore of a job!