Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ImageCloneTests.CloneAs_ToBgr24() failing sporadically #576

Closed
4 tasks done
antonfirsov opened this issue May 15, 2018 · 8 comments
Closed
4 tasks done

ImageCloneTests.CloneAs_ToBgr24() failing sporadically #576

antonfirsov opened this issue May 15, 2018 · 8 comments

Comments

@antonfirsov
Copy link
Member

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

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
Copy link
Collaborator

brianpopow commented May 19, 2018

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
Copy link
Collaborator

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 added a commit to brianpopow/ImageSharp that referenced this issue May 23, 2018
@brianpopow
Copy link
Collaborator

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
Copy link
Member Author

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

@JimBobSquarePants
Copy link
Member

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

brianpopow added a commit to brianpopow/ImageSharp that referenced this issue May 23, 2018
@brianpopow
Copy link
Collaborator

i have opened a PR for this: #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
Copy link
Member

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.

JimBobSquarePants added a commit that referenced this issue May 24, 2018
Fix for #576, using explicit struct layout for bgr24 and rgb24
@antonfirsov
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants