Added FillRectangleExtensions overloads which use the FillProcessor for perf improvements #838
Conversation
… and color or brush to quickly fill a rectangle via the FillProcessor instead of the FillRegionProcessor.
|
Thanks for this. Got four failing tests now that will need fixing as their expecting the old processor. https://ci.appveyor.com/project/six-labors/imagesharp/builds/22506131/job/t3ow5thwuq4awq8q/tests |
|
This means you actually have unit-tests which can check, in the forest of tests my time-boxing prevented me to find them. I'll check this 👍 |
… the tests possible I had to modify the FillProcessor to have public properties, for this I used FillRegionProcessor as example.
|
I made the needed changes to the test cases, but I'm not 100% pleased as it felt like I was removing test-code which might be sensible for different cases. I'm not sure if RectangleF is forgotten, let me check that before you merge things (if AppVeyor finally finishes) Maybe I'll get the old code back, but replace the Rectangle with RectangleF and have the new code for Rectangle parallel... this might make more sense. The question remains if RectangleF really needs the FillRegionProcessor or also should use the FillProcessor with degrading the RectangleF (float) to Rectangle (int), or keep the FillRegionProcessor? Let me know what you think! P.S. |
Codecov Report
@@ Coverage Diff @@
## master #838 +/- ##
=======================================
Coverage 88.87% 88.87%
=======================================
Files 1015 1015
Lines 44240 44222 -18
Branches 3202 3202
=======================================
- Hits 39319 39303 -16
Misses 4200 4200
+ Partials 721 719 -2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #838 +/- ##
=======================================
Coverage 88.89% 88.89%
=======================================
Files 1014 1014
Lines 44295 44277 -18
Branches 3209 3209
=======================================
- Hits 39376 39360 -16
Misses 4198 4198
+ Partials 721 719 -2
Continue to review full report at Codecov.
|
|
We have will end up needing to have both options for |
|
@tocsoft Is there more this PR requires or are we good to merge? |
|
I need to check if there is still something open, was distracted a bit due to being a candidate for the dotnet foundation election... |
|
@Lakritzator Get well soon! |
There was a problem hiding this comment.
@Lakritzator do you have some time to finish this in the next few days? Otherwise it's just simpler for me to to close this PR and re-apply the optimization after we merged #910.
| /// <returns>The <see cref="Image{TPixel}"/>.</returns> | ||
| public static IImageProcessingContext<TPixel> Fill<TPixel>(this IImageProcessingContext<TPixel> source, GraphicsOptions options, IBrush<TPixel> brush, Rectangle rectangle) | ||
| where TPixel : struct, IPixel<TPixel> | ||
| => source.ApplyProcessor(new FillProcessor<TPixel>(brush, options), rectangle); |
There was a problem hiding this comment.
If I understand @tocsoft 's comment well, it would be the best to select between FillRegionProcessor and FillProcessor at this point, depending on the value of options.Antialias. All overloads should delegate into this method, including the ones which haven't been touched.
There was a problem hiding this comment.
Been thinking about this, and now I'm unsure about the expected default behavior for these use cases.
- If a rectangle is being filled with a solid color, is it expected to antialias corners by default?
- What about non-solid brushes?
There was a problem hiding this comment.
I thought the internal processor code checked to see if the bush was a solid color?
I though I responded some time ago, so sorry for this... probably an issue while I was underway Closing this, and will try again when I find some time. |
4 participants
Prerequisites
Description
While doing some performance testing with ImageSharp, I noticed that a simple Fill with a solid brush and a rectangle was using a lot of memory and taking a long time. My analysis, and a little discussion on gitter, showed that although the functionality was fine, the steps taking to do the filling were far from optimal. Everything was already there, but the FillRegionProcessor instead of the FillProcessor was used.
This PR will add the missing overloads to the FillRectangleExtensions, thus causing a performance boost and memory reduction when drawing a simple filled rectangle.
Notes:
I'm not 100% sure if I missed some overloads, or if more with different signatures are needed. I noticed that a lot of drawing methods only specify RectangleF. The used overload before my PR would convert a Rectangle implicitly into a RectangleF and the overload would convert this into a RectangularPolygon and call into the FillPathExtensions. But the path of modifying those overloads doesn't seem feasible as ApplyProcessor only takes a Rectangle and I wouldn't like "downgrading".
I also have a hard time coming up with a test-case to prove that in all cases the correct overload is taken, maybe one has an idea about this.