Investigate skipped tests in PackedPixelTests

[antonfirsov]

Prerequisites

• I have written a descriptive issue title
• I have verified that I am running the latest version of ImageSharp
• I have verified if the problem exist in both DEBUG and RELEASE mode
• I have searched open and closed issues to ensure it has not already been reported

Description

Issue opened for a reason similar to #576:
Had to skip some (less valuable) tests on travis/linux to finish the work on #585: NormalizedShort4, Short4, NormalizedByte4 all in PackedPixelTests. Previously had to skip PackedPixelTests.Rgba64 for a similar reason.

I have a feeling that this time it's not the CLR, these tests seem to depend on implementation specific details for me. They are also poorly isolated (not AAA), that makes them harder to troubleshoot.

Steps to Reproduce

Unskip tests & run travis build. (It might be enough to do a local execution on Linux).

See: failing build log

[JimBobSquarePants]

Yeah those tests need rewriting. The initial tests were copied from monogame. I keep planning on cleaning them up but it’s a time consuming job.

[brianpopow]

Hi Anton and James,

i have tried to figure out this issue and it seems similar to #576, because it fails also only in Release build not on debug.

One difference between Debug and Release is AggressiveInlining, so i thought i might give it a try and disable it and indeed disabling it in NormalizedShort4, Short4, NormalizedByte4 on the method ToByteScaledVector4 seems to fix this issue.

I can not completely explain yet why this AggressiveInlining at that specific method is causing it. I found some similar issue: dotnet/roslyn#21323
but this is windows and not linux (on windows this does not happen again)

I could provide an PR, if you think disabling AggressiveInlining on this specific method is a viable workaround for this.

[brianpopow]

i have started working on refactoring the PackedPixelTests. Hopefully this will help to find the real cause of this strange issue.

[brianpopow]

i have good news and bad news. The good news is, i think i am done with the refactoring: #603

the bad news: Splitting up the tests in smaller chunks, makes the issue go away.
Even changing the original Unit Test just a little bit makes the issue either go away or another Assert to fail. My attempt to create a smaller Reproduction of this failed, because of this.

I could not find the real issue of this and i am of out of ideas here. I hope the refactoring is still helpful

[antonfirsov]

@brianpopow good news, thanks, the code looks much better now!

This issue is totally crazy.

[JimBobSquarePants]

CLosing these as I don't think we have any more skipped tests and all the pixel formats have been rewritten anyway.

[antonfirsov]

We still have the old test code.

I wonder what portion of the original test code is covered by @brianpopow 's new tests in #603.
I've read the original PR comment:

but by splitting the tests up in smaller pieces, the issue no longer occurs.

We probably need to check what happens on travis by re-enabling them. Or maybe we should just delete them.

[brianpopow]

I will test this in docker and see what happens, maybe we can re-enable them.

[brianpopow]

I have tested Issue594 with the following docker image mcr.microsoft.com/dotnet/core/sdk:2.1.603 (Debian GNU/Linux 9), which uses the same dotnet sdk as travis right now.

In the NormalizedByte4 test, there is an assert which fails both on windows and linux (and in Debug and Release mode):

var n = default(NormalizedByte4);
var rgba = new Rgba32(141, 90, 192, 39);
n.FromRgba32(new Rgba32(141, 90, 192, 39));
Assert.Equal(0xA740DA0D, n.PackedValue);

Im not 100% sure if the assert assumptions are correct or if this is some kind of floating point inacurracy.

All other parts wich are not uncommented work with windows and linux.

If the above mentioned assert would not fail, i would advise to delete the Issue594.cs,
because those tests are splitted up now in the different PixelTypeTests classes (e.g. NormalizedByte4Tests, NormalizedShort4Tests, Short4Tests).

[JimBobSquarePants]

It's been nearly 4 years.... But.

I believe the initial problem was caused by a combination of early compiler bug, our aggressive inlining, and us passing Vector4 by ref in our pixel formats.

var n = default(NormalizedByte4);
var rgba = new Rgba32(141, 90, 192, 39);
n.FromRgba32(new Rgba32(141, 90, 192, 39));
Assert.Equal(0xA740DA0D, n.PackedValue);

This test is actually junk. The expected RGBA values are incorrect. That would have been due to misunderstanding on my part when implementing the formats.

I'm working on a complete rewrite of our pixel formats to use static interface methods and can confirm that the new implementation builds and passes all tests on all of our CI platforms.

#2645