Skip to content

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

This PR add AVX2 versions of TransformColor and TransformColorInverse which are used during webp encoding and decoding.
Relates to #1786

Profiling results:

master:

TransformColor

TransformColorInverse

PR:

TransformColorAvx

TransformColorInverseAvx

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #1830 (306d127) into master (2f048ca) will decrease coverage by 0%.
The diff coverage is 99%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1830   +/-   ##
======================================
- Coverage      87%     87%   -1%     
======================================
  Files         937     937           
  Lines       48385   48420   +35     
  Branches     6048    6054    +6     
======================================
- Hits        42285   42217   -68     
- Misses       5097    5196   +99     
- Partials     1003    1007    +4     
Flag Coverage Δ
unittests 87% <99%> (-1%) ⬇️

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

Impacted Files Coverage Δ
.../ImageSharp/Formats/Webp/Lossless/LosslessUtils.cs 89% <99%> (-9%) ⬇️
...ageSharp/Formats/Webp/Lossless/PredictorEncoder.cs 87% <0%> (-11%) ⬇️
...mageSharp/Formats/Webp/Lossless/NearLosslessEnc.cs 88% <0%> (-8%) ⬇️
...ImageSharp/Formats/Webp/Lossless/Vp8LBitEntropy.cs 100% <0%> (+1%) ⬆️

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 2f048ca...306d127. Read the comment docs.

{
int numPixels = pixelData.Length;
fixed (uint* p = pixelData)
int i;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int i;
nint i;

? As well on the other loop-variables to avoid the movsxd machine-instruction.

Copy link
Collaborator Author

@brianpopow brianpopow Nov 15, 2021

Choose a reason for hiding this comment

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

The slice down below requires an int. Is it still worth using nint here and cast with the slice or should i keep it rather int then? The Slice is outside of the loop and only for the leftover.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd cast to int in the slice. It's actually a nop in machine code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok will do that then, thx!

Copy link
Member

Choose a reason for hiding this comment

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

@brianpopow I went ahead and did the nint change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, i was unsure in some places, if I should go for it or not.

@JimBobSquarePants JimBobSquarePants merged commit 29ea6af into master Nov 16, 2021
@JimBobSquarePants JimBobSquarePants deleted the bp/transformcoloravx branch November 16, 2021 11:30
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