Imagesharp: ImageCloneTests.CloneAs_ToBgr24() failing sporadically

[antonfirsov]

Prerequisites

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

Description

Decided to open an issue before skipping ImageCloneTests.CloneAs_ToBgr24() in #571, because it might indicate a bug.

Steps to Reproduce

Unskip & run CloneAs_ToBgr24() on Travis. It will fail in 30%-50% of the executions. Sometimes it also fails locally for me.

[brianpopow]

i have noticed, that this seems to happen only on linux (at least i have not seen this on windows yet). Also only in Release mode. Never seen this in Debug mode so far.

Weird thing is really that it only happens occasionally, not always. So i was expecting some kind of race condition, because the tests are run in parallel, but changing the xunit config to:

"maxParallelThreads": 1,           
"parallelizeAssembly": false,      
"parallelizeTestCollections": false

and its still happens some times

[brianpopow]

Even running only the test CloneAs_ToBgr24 produces the error. I still suspect some race condition, because of parallel execution, but its not because other tests are running at the same time.

I have noticed, that when it fails, its every time the value in the B channel which is wrong.

I suspect the ParallelFor in CloneAs is causing this, but i could not pinpoint exactly where the error is. Setting the ParallelOptions.MaxDegreeOfParallelism to 1 reliable makes the test run successfully.

What i do not get is why this is not happening to CloneAs_ToRgb24. It uses the same CloneAs method.

[brianpopow]

i do not think anymore there is an issue in ParallelFor. I think it has to do with memory alignment of the bgr24 struct. If i change it to explicit and field offsets 0, 1, 2 accordingly to b, g , r, this issue does not happen anymore.

I am not sure why LayoutKind.Sequential produces a different result than this explicit one.

[antonfirsov]

@brianpopow should be a CLR code generation bug again. Thanks for figuring this out!

[JimBobSquarePants]

@brianpopow Good work! We should double check our other blittable types. Rgba32 Rgb24, Bgr24, and Argb32.

[brianpopow]

i have opened a PR for this: https://github.com/SixLabors/ImageSharp/pull/591

I have changed rgb24 also, because it may also be affected by this.

Im not sure if Rgba32 and Argb32 can be affected by this.

[JimBobSquarePants]

Ok, let's leave it at that, no point changing working code.

I've just updated your PR from master, will merge once it's built.

[antonfirsov]

It pretty much looks like this is fixed with #591.