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

Optimize bokeh blur convolution #1475

Merged
merged 22 commits into from
Dec 15, 2020
Merged

Conversation

Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Dec 12, 2020

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

Optimized the bokeh blur effect, with a more specialized variation of the changes introduced in #1465.

Benchmark

image

EDIT: updated benchmark after 2e53a44.

image

EDIT: updated benchmark after e4ba017.

image

EDIT: updated benchmark after 442e467 and c38fc81.

image

@Sergio0694 Sergio0694 added this to the 1.1.0 milestone Dec 12, 2020
@Sergio0694
Copy link
Member Author

Sergio0694 commented Dec 12, 2020

Investigating the test failures with the effect on smaller image regions, and also noticed a previously existing bug that was likely causing some out of bounds writes in the past, very weird we hadn't spotted that one before. Glad it popped up now! 😄

EDIT: fixed in 7db1225 🎉

@Sergio0694 Sergio0694 force-pushed the sp/bokeh-blur-speedup branch from 2913201 to 3180ec4 Compare December 12, 2020 23:16
@Sergio0694
Copy link
Member Author

Removed the WorksWithDiscoBuffers test for the bokeh blur processor, as it was just always throwing due to insufficient memory. @JimBobSquarePants I guess you could reintroduce it later on if you wanted to, but for now I wanted to have the CI run to verify all the other tests and the changes in this PR. Also, noticed that eg. the gaussian blur doesn't have this test either, so... 🤷‍♂️

[Theory]
[WithTestPatternImages(100, 300, PixelTypes.Bgr24)]
public void WorksWithDiscoBuffers<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
provider.RunBufferCapacityLimitProcessorTest(41, c => c.BokehBlur());
}

@JimBobSquarePants
Copy link
Member

Removed the WorksWithDiscoBuffers test for the bokeh blur processor, as it was just always throwing due to insufficient memory.

This is concerning. We weren't getting any throws for this in previous runs.

@codecov
Copy link

codecov bot commented Dec 13, 2020

Codecov Report

Merging #1475 (a8cae3f) into master (ff94d20) will increase coverage by 0.06%.
The diff coverage is 97.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1475      +/-   ##
==========================================
+ Coverage   83.49%   83.55%   +0.06%     
==========================================
  Files         742      741       -1     
  Lines       32347    32462     +115     
  Branches     3639     3648       +9     
==========================================
+ Hits        27009    27125     +116     
  Misses       4625     4625              
+ Partials      713      712       -1     
Flag Coverage Δ
unittests 83.55% <97.10%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
...s/Convolution/Convolution2PassProcessor{TPixel}.cs 100.00% <ø> (ø)
...essing/Processors/Convolution/KernelSamplingMap.cs 94.11% <33.33%> (-5.89%) ⬇️
src/ImageSharp/Common/Helpers/Numerics.cs 97.65% <95.31%> (-1.01%) ⬇️
...ssing/Processors/Convolution/BokehBlurProcessor.cs 100.00% <100.00%> (ø)
...ocessors/Convolution/BokehBlurProcessor{TPixel}.cs 99.34% <100.00%> (+0.49%) ⬆️
...mageSharp/Common/Helpers/SimdUtils.HwIntrinsics.cs 99.56% <0.00%> (+1.30%) ⬆️

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 ff94d20...a8cae3f. Read the comment docs.

@Sergio0694 Sergio0694 marked this pull request as draft December 13, 2020 14:27
@Sergio0694
Copy link
Member Author

Reverted to draft as I just had some new ideas for some other possibly juicy optimizations to add 😋

@Sergio0694 Sergio0694 marked this pull request as ready for review December 14, 2020 03:12
@Sergio0694
Copy link
Member Author

Alright, happy with the progress so far, I'd say this PR is good for review, and we can always revisit later on 🎉

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. Very nice improvement! 🚀

@JimBobSquarePants JimBobSquarePants merged commit f1a0fb6 into master Dec 15, 2020
@JimBobSquarePants JimBobSquarePants deleted the sp/bokeh-blur-speedup branch December 15, 2020 00:32
JimBobSquarePants added a commit that referenced this pull request Mar 13, 2021
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.

2 participants