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

Ensure Span length of source and destination are equal during pixel conversion #1443

Merged
merged 6 commits into from
Nov 29, 2020

Conversation

brianpopow
Copy link
Collaborator

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

As reported in #1442, there are some failing tests in the master branch. The reason for this is a Debug.Guard which ensures the span length are equal during pixel conversion. Sometimes the source span was smaller the the destination. This got unnoticed during Release CI runs.

@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #1443 (7c77e7e) into master (976bdd5) will increase coverage by 0.00%.
The diff coverage is 93.61%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1443   +/-   ##
=======================================
  Coverage   83.68%   83.69%           
=======================================
  Files         734      734           
  Lines       31990    31995    +5     
  Branches     3605     3605           
=======================================
+ Hits        26772    26777    +5     
  Misses       4505     4505           
  Partials      713      713           
Flag Coverage Δ
unittests 83.69% <93.61%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ns/Generated/Bgra5551.PixelOperations.Generated.cs 98.16% <66.66%> (ø)
...ations/Generated/La16.PixelOperations.Generated.cs 98.16% <66.66%> (ø)
...ations/Generated/La32.PixelOperations.Generated.cs 98.16% <66.66%> (ø)
src/ImageSharp/Formats/Bmp/BmpEncoderCore.cs 96.62% <100.00%> (+0.11%) ⬆️
...ions/Generated/Argb32.PixelOperations.Generated.cs 100.00% <100.00%> (ø)
...tions/Generated/Bgr24.PixelOperations.Generated.cs 100.00% <100.00%> (ø)
...ions/Generated/Bgra32.PixelOperations.Generated.cs 100.00% <100.00%> (ø)
...rations/Generated/L16.PixelOperations.Generated.cs 100.00% <100.00%> (ø)
...erations/Generated/L8.PixelOperations.Generated.cs 100.00% <100.00%> (ø)
...tions/Generated/Rgb24.PixelOperations.Generated.cs 100.00% <100.00%> (ø)
... and 3 more

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 976bdd5...7c77e7e. Read the comment docs.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@antonfirsov
Copy link
Member

The reason for this is a Debug.Guard which ensures the span length are equal during pixel conversion.

This shouldn't be the case, the PixelOperations contract allows destination to be longer. @JimBobSquarePants looks like SimdUtils.Shuffle* does a strict verification, which is fine (modeled after the Vector4 conversion stuff I guess), but we should slice down destination in PixelOperations then.

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Nov 28, 2020

Aha! I actually had to do a hack to allow for this in a debug check.

1ad9fcd#diff-ba27a760948c3e593e3ac6a18ad7b8ea04bc0255376575088b7fad70814040e3R227-R231

Yep. Slicing is probably wise.

@brianpopow
Copy link
Collaborator Author

i will leave this as it is now. Maybe someone should give a quick review of 7c77e7e again

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@brianpopow brianpopow merged commit 4aed9f4 into master Nov 29, 2020
@brianpopow brianpopow deleted the bp/bmpEnsureSpanLengthIsEqual branch November 29, 2020 16:39
JimBobSquarePants pushed a commit that referenced this pull request Mar 13, 2021
Ensure Span length of source and destination are equal during pixel conversion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants