Issue 551 solidbrush blend performance #552

woutware · 2018-04-29T18:31:05Z

Please check thoroughly, I think I might have overlooked some conditions for skipping the blending in the FillProcessor.

Also I haven't done the FillRegionProcessor yet, I'm gonna need help with that.

The speedup with the image constructor with clear color param compared to the fill is pretty nice! My whole test draw is about 10% faster now overall.

…meter. This is good for performance because this avoids first needing to clear the buffer with zeroes.

tocsoft

just a tweak needed to the logic of activating the fast path

tocsoft · 2018-04-29T18:54:36Z

src/ImageSharp.Drawing/Processing/Drawing/Processors/FillProcessor.cs

+            var solidBrush = this.brush as SolidBrush<TPixel>;

+            // If there's no reason for blending, then avoid it.
+            if (solidBrush != null && this.options.BlendPercentage == 1f && solidBrush.Color.ToVector4().W == 1f)


fast path should be activated when solidBrush != null and any of these things are true:

this.options.BlenderMode == PixelBlenderMode.Normal && this.options.BlendPercentage == 1f && solidBrush.Color.ToVector4().W == 1f
OR

this.options.BlenderMode == PixelBlenderMode.Over && this.options.BlendPercentage == 1f && solidBrush.Color.ToVector4().W == 1f

OR

this.options.BlenderMode == PixelBlenderMode.Src

Changed it, thanks for the help here!

tocsoft · 2018-04-29T18:59:50Z

you will find its nigh on impossible to apply the same sort of fast path logic to the region filling, as we need to take into account anti-aliasing at region edges which in turn requires applying pixel blending.

antonfirsov

Good start, would be even better if we could move the whole logic into SolidBrush<T> but it's not a trivial job, so we can do it later.

I would like to add better assertions to our Fill/Blend tests before we merge this. Gonna do a separate PR for that.

antonfirsov · 2018-04-29T20:27:07Z

src/ImageSharp/Image{TPixel}.cs

+        /// <summary>
+        /// Gets the clear color to initialize the image frame pixels with.
+        /// </summary>
+        internal TPixel? ClearColor => this.clearColor;


Not sure if I like this.

Does it worth to pollute our object model in this manner just to make it easier to add frames of a "default" color when producing gif-s manually? (0.01% of ImageSharp use-cases I guess).
@JimBobSquarePants @tocsoft thoughts?

I'm not a fan of nullable type properties on classes. I'm voting no.

The thought behind it is that if you create an image with a background color, all its frames will have automatically that background color, which could be reasonably expected from a user stand point. Without it you'd have the first frame with the background color, and if the user adds another frame with ImageFrameCollection.CreateFrame(), it wouldn't have that background color.

What's the reason for not having nullable type properties on a class?

My issue with nullable properties is that you cannot now be confident as a consumer of their value. TPixel is a struct anyway so doesn’t need to be nullable. Assign a default(TPixel) initial value.

if we have this atall you would want to distinguish between default and not set because default(TPixel) would be == Transparent and you want to skip the code path if its not set at all.

oh yeah.. your right, ignore that in that case we can just skip the logic on default(TPixel) 👍

Agreed with the default(TPixel) part, so you suggest always initializing the image frame with the clearColor then instead of using memoryManager.AllocateClean2D?

I don't agree with the nullable reasoning, if it's properly documented and default values are not illogical I don't see a reason to refrain from using certain language features, if TPixel were a class it'd have been the same. In this case non-nullable works as well, but in general if nullable is better suitable for a particular situation I wouldn't handicap the design/api out of fear users are not capable enough of using it.

Alright, committed the change to non-nullable clear color.

It wasn't a question of handicapping the API nor one of fear. Nullable properties have their place, just not in any API where you hold all the cards.

Take for example the new ResponseHeaders class from Microsoft.Http.Abstractions. The ContentLength property there is of type long? this makes perfect sense since the HttpResponse might not provide a content-length header.

We have absolute control over that property and assigning it so Nullable would have been the incorrect option design wise.

On that note... I wonder whether this should be a property of the ImageFrame<T> itself? That way we base any non-background color assigned frames on the root frame which would keep the property in line with the Width/Height properties.

I've moved it to ImageFrame, it's indeed more consistent with the Width/Height properties.

antonfirsov · 2018-04-29T20:35:34Z

src/ImageSharp.Drawing/Processing/Drawing/Processors/FillProcessor.cs


+            // If there's no reason for blending, then avoid it.
+            if (solidBrush != null &&
+                (


I refactor this logic into an IsSolidBrushWithoutBlending property or similar for better readability.

…ub.com/woutware/ImageSharp into Issue-551-solidbrush-blend-performance

antonfirsov · 2018-04-29T22:07:45Z

Opened #553 to deal with the coverage issues. Hope it can be merged quickly.

codecov · 2018-04-30T08:54:43Z

Codecov Report

Merging #552 into master will increase coverage by 0.02%.
The diff coverage is 94.61%.

@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
+ Coverage   88.26%   88.28%   +0.02%     
==========================================
  Files         838      838              
  Lines       35096    35186      +90     
  Branches     2534     2538       +4     
==========================================
+ Hits        30976    31064      +88     
  Misses       3377     3377              
- Partials      743      745       +2

Impacted Files	Coverage Δ
...ageSharp.Tests/Image/ImageFramesCollectionTests.cs	`100% <100%> (ø)`	⬆️
tests/ImageSharp.Tests/Image/ImageTests.cs	`100% <100%> (ø)`	⬆️
...geSharp.Tests/TestUtilities/TestImageExtensions.cs	`81.59% <100%> (+0.34%)`	⬆️
src/ImageSharp/ImageFrame{TPixel}.cs	`86.59% <100%> (+3.93%)`	⬆️
...g/Processing/Drawing/Brushes/SolidBrush{TPixel}.cs	`96% <100%> (+0.54%)`	⬆️
src/ImageSharp/Image{TPixel}.cs	`81.63% <80%> (-1.3%)`	⬇️
...ing/Processing/Drawing/Processors/FillProcessor.cs	`92.45% <87.87%> (+3.56%)`	⬆️
src/ImageSharp/ImageFrameCollection.cs	`91.89% <93.75%> (+0.58%)`	⬆️

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 6431723...72cb50c. Read the comment docs.

JimBobSquarePants · 2018-04-30T15:32:19Z

src/ImageSharp/ImageFrame{TPixel}.cs

+        /// <param name="width">The width of the image in pixels.</param>
+        /// <param name="height">The height of the image in pixels.</param>
+        /// <param name="clearColor">The color to clear the image with.</param>
+        internal ImageFrame(Configuration configuration, int width, int height, TPixel clearColor)


Could you please rename this to backgroundColor. Clear would be a verb.

Renamed it. It's probably more user understandable, I liked the clear color as it implies better it's an one time initialization thing rather than a permanent state.

JimBobSquarePants · 2018-04-30T15:32:23Z

src/ImageSharp/Image{TPixel}.cs

+        /// <summary>
+        /// Gets the clear color to initialize the image frame pixels with.
+        /// </summary>
+        internal TPixel? ClearColor => this.clearColor;


It wasn't a question of handicapping the API nor one of fear. Nullable properties have their place, just not in any API where you hold all the cards.

Take for example the new ResponseHeaders class from Microsoft.Http.Abstractions. The ContentLength property there is of type long? this makes perfect sense since the HttpResponse might not provide a content-length header.

We have absolute control over that property and assigning it so Nullable would have been the incorrect option design wise.

On that note... I wonder whether this should be a property of the ImageFrame<T> itself? That way we base any non-background color assigned frames on the root frame which would keep the property in line with the Width/Height properties.

JimBobSquarePants · 2018-04-30T15:33:37Z

src/ImageSharp/ImageFrame{TPixel}.cs

+
+            this.MemoryManager = configuration.MemoryManager;
+            this.PixelBuffer = this.MemoryManager.Allocate2D<TPixel>(width, height, false);
+            this.Clear(configuration.ParallelOptions, clearColor);


this.PixelBuffer.Span.Fill()?

I left in the parallel clearing, on my laptop it's still 10%-20% faster and I can really use all the speed there is as the drawing bit is quite a bit slower at the moment. The user can always set the parallel options to use 1 thread if he insists.

JimBobSquarePants · 2018-04-30T15:34:10Z

src/ImageSharp/ImageFrame{TPixel}.cs

+        /// </summary>
+        /// <param name="parallelOptions">The parallel options.</param>
+        /// <param name="value">The value to initialize the bitmap with.</param>
+        public void Clear(ParallelOptions parallelOptions, TPixel value) {


Clearing is a process and belongs in the processing namespace; we don't want to clutter the ImageFrame class. We also never want to expose the ParallelOptions as a field. They are part of the Configuration Class and are globally set.

Changed method from public to internal. I'd just see this as initialization rather than processing, akin to how Allocate2D allows initialization, which isn't in processing either, but doesn't allow a default value.

JimBobSquarePants · 2018-04-30T15:35:59Z

src/ImageSharp/Image{TPixel}.cs

+        /// <param name="width">The width of the image in pixels.</param>
+        /// <param name="height">The height of the image in pixels.</param>
+        /// <param name="clearColor">The color to initialize the pixels with.</param>
+        public Image(Configuration configuration, int width, int height, TPixel clearColor)


clearColor => backgroundColor

JimBobSquarePants · 2018-04-30T15:39:33Z

src/ImageSharp/Image{TPixel}.cs

+        /// <summary>
+        /// Gets the clear color to initialize the image frame pixels with.
+        /// </summary>
+        internal TPixel ClearColor => this.clearColor;


Move this to the ImageFrame<TPixel> and add to the constructor there. Use RootFrame as a default for subsequent frames

Done, good point there.

JimBobSquarePants · 2018-04-30T15:40:20Z

src/ImageSharp/Image{TPixel}.cs

    {
        private readonly Configuration configuration;
        private readonly ImageFrameCollection<TPixel> frames;
+        private readonly TPixel clearColor;


No need for field. Assign straight to the property. (When you move all this to ImageFrame)

JimBobSquarePants · 2018-04-30T15:42:01Z

src/ImageSharp/Memory/BufferExtensions.cs

+        /// <param name="buffer">The buffer</param>
+        /// <param name="value">The value to fill the buffer with.</param>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static void Fill<T>(this IBuffer<T> buffer, T value)


Does this have a caller? The above code works directly on the Span

Removed again, used this in one of my intermediate experiments I think.

…roundColor.

antonfirsov · 2018-04-30T21:33:45Z

From the Separation of Concerns point of view, I'm still not a big fan of introducing a BackgroundColor property on an imaging API. (It would be fine on a canvas API!)

But if you guys find it powerful and useful & vote to keep it, I would not resists.

One question:

Do we still need BackgroundColorProcessor?
If yes:
- Shouln't we optimize it as well?
- Shouldn't it change the ImageFrame.BackgroundColor for all frames?

woutware · 2018-04-30T22:26:24Z

@antonfirsov I can't comment on the BackgroundColorProcessor, but remember that before this change the pixels were initialized with zeroes by AllocateClean2D. If you're 100% puristic, you'd be making a processor for this as well, which I think at this level is overkill. You can consider the one time initializion of the pixels as integral part of the allocation similarly to how AllocateClean2D does it.

The change is a tremendous speedup: previously you'd always have 1) the initialization with zeroes, and 2) the slow Fill processor. Now you're skipping 2), and made 1) configurable with a default value. So you're basically twice as fast.

Some measured timings:

alloc + fill 00:00:00.0205244, alloc with background color 00:00:00.0093514

When not disposing the bitmap, so with a fresh alloc each time the difference becomes pretty much zero by the way, I'd have expected more difference here:

alloc + fill 00:00:00.0755986, alloc with background color 00:00:00.0741654

antonfirsov · 2018-04-30T23:26:16Z

@woutware the constructor taking backgroundColor + it's auto-filling optimization are cool things, I have no issue with them!

The purist part of me just wants to drop the background color parameter after initialization, and leave the post-constructor job of initializing new frames with the proper color to the user. Only very a small minority of our users do manual multi-frame gif manipulation!

woutware · 2018-04-30T23:30:57Z

I've no preference either way, I'll be using single frame myself only, so I'm fine with whatever fits the current philosophy best. Having said that, if you create an Image with a background color, it would feel more logical to me that new image frames have that same background color automatically. But again it's pretty much equal to me either way.

antonfirsov · 2018-04-30T23:40:39Z

It's @JimBobSquarePants's job to decide it from now. He is the Grand High Eternal Dictator! 😆

(+ my question abut BackgroundColorProcessor still stands, mostly addressed to him.)

JimBobSquarePants · 2018-04-30T23:55:05Z

@antonfirsov BackgroundColor does operate on all frames.

ImageSharp/src/ImageSharp/Processing/Overlays/Processors/BackgroundColorProcessor.cs

Line 43 in 01d0426

    
           protected override void OnFrameApply(ImageFrame<TPixel> source, Rectangle sourceRectangle, Configuration configuration)

We need it because it performs a different action, doesn't require a brush (therefore can live in ImageSharp) and can operate only on the background layer (i.e you cannot overwrite original color data, only blend against it.)

It's super useful for padding out areas of non-transparent image formats following ResizeMode.Pad

JimBobSquarePants · 2018-05-01T00:08:42Z

src/ImageSharp/ImageFrame{TPixel}.cs

+        /// <param name="width">The width of the image in pixels.</param>
+        /// <param name="height">The height of the image in pixels.</param>
+        /// <param name="backgroundColor">The color to clear the image with.</param>
+        internal ImageFrame(Configuration configuration, int width, int height, TPixel backgroundColor)


This is good! I actually wanted to introduce the configuration as a parameter and drop the MemoryManager overload so we're halfway there.

antonfirsov · 2018-05-01T00:10:10Z

@JimBobSquarePants thanks, I understand now!

Shouldn't BackgroundColorProcessor alter ImageFrame.BackgroundColor (if we keep the property)?

JimBobSquarePants · 2018-05-01T04:10:47Z

I think I get @antonfirsov 's issue with the BackgroundColor as a property. As soon as anyone writes to the image then it becomes invalid.

@woutware I think we'll have to drop the convenience of the property and simply rely on setting the background color per frame. (This is seriously fine grained stuff anyway so I doubt it will be an issue.)

JimBobSquarePants · 2018-05-01T04:11:14Z

We're nearly there with this, looking forward to seeing it merged! 👍

woutware · 2018-05-01T08:40:16Z

Alright, removed the ImageFrame.Background property, and just using default if the user creates a new frame after the fact.

@JimBobSquarePants probably add some comments to the BackgroundColorProcessor code that you wrote here explaining the thoughts behind it, that'll be super useful for future users.

JimBobSquarePants · 2018-05-01T10:08:21Z

src/ImageSharp/ImageFrameCollection.cs

        }

        /// <inheritdoc/>
        public ImageFrame<TPixel> CreateFrame()


Let's create an overload for this that accepts a color, then we cover all bases.

I see the method implements a member from interface IImageFrameCollection, wouldn't it be nice to change it to:

CreateFrame(TPixel backgroundColor = default)

I think we don’t actually need the interface. I can’t see why someone would need to implement it themselves.

@JimBobSquarePants I remember having a conversation about optional arguments, with the conclusion that we should prefer overloads instead because of better binary compatibility.

Is this still a thing we should follow?

Yeah, always choose overloads.

…nd added backgroundColor parameter to ImageFrameCollection.CreateFrame() method with default.

# Conflicts: # src/ImageSharp.Drawing/Processing/Drawing/Processors/FillProcessor.cs

…ed by the PR

antonfirsov

Pushed a few changes:

Overload CreateFrame() instead of optional arg
Minor code cleanups
Tests Tests Tests!

I think we are fine to merge this now!

woutware · 2018-05-01T20:57:29Z

Yay! Thanks guys!

JimBobSquarePants · 2018-05-02T04:00:52Z

Thanks @woutware for taking the time to put this together. Much appreciated! 👍

maxvoxel8 · 2018-07-09T21:04:10Z

you will find its nigh on impossible to apply the same sort of fast path logic to the region filling, as we need to take into account anti-aliasing at region edges which in turn requires applying pixel blending.

@tocsoft could the same logic be used if antialiasing is off? Or if a scanline is all ones/zeros?

…lend-performance Issue 551 solidbrush blend performance

woutware added 3 commits April 29, 2018 17:33

Added fast path for SolidBrush in FillProcessor.

5a1569f

Oops, should be W instead of Z ofcourse.

08b033c

Add Image and ImageFrame constructors that take a clear color as para…

58384de

…meter. This is good for performance because this avoids first needing to clear the buffer with zeroes.

tocsoft requested changes Apr 29, 2018

View reviewed changes

woutware added 2 commits April 29, 2018 21:36

Processed Scott's review comment.

8995a73

Merge branch 'master' into Issue-551-solidbrush-blend-performance

a3a1556

antonfirsov reviewed Apr 29, 2018

View reviewed changes

woutware added 2 commits April 29, 2018 23:19

I think I forgot to commit this one.

02fc4ef

Merge branch 'Issue-551-solidbrush-blend-performance' of https://gith…

a38bb5a

…ub.com/woutware/ImageSharp into Issue-551-solidbrush-blend-performance

antonfirsov mentioned this pull request Apr 29, 2018

Better test coverage for SolidBrush + blenders #553

Merged

4 tasks

Processed review comment, made ClearColor not nullable.

2943df1

JimBobSquarePants reviewed Apr 30, 2018

View reviewed changes

JimBobSquarePants requested changes Apr 30, 2018

View reviewed changes

woutware added 2 commits April 30, 2018 18:22

Processed review comments, moved Image.ClearColor to ImageFrame.Backg…

aa4a057

…roundColor.

Merge branch 'master' into Issue-551-solidbrush-blend-performance

7e2fd11

antonfirsov mentioned this pull request Apr 30, 2018

better FillSolidBrushTests + bugfix in FillProcessor #555

Merged

4 tasks

JimBobSquarePants reviewed May 1, 2018

View reviewed changes

Processed review comments, removed ImageFrame.BackgroundColor property.

7c70a5e

JimBobSquarePants reviewed May 1, 2018

View reviewed changes

woutware and others added 4 commits May 1, 2018 15:14

Processed review comments, removed IImageFrameCollection interface, a…

0d81089

…nd added backgroundColor parameter to ImageFrameCollection.CreateFrame() method with default.

Merge branch 'master' into Issue-551-solidbrush-blend-performance

d2bfaf2

# Conflicts: # src/ImageSharp.Drawing/Processing/Drawing/Processors/FillProcessor.cs

minor code cleanup in FillProcessor.cs

24f6ff9

use overloading in ImageFrameCollection + more tests for API-s affect…

a9816fd

…ed by the PR

antonfirsov approved these changes May 1, 2018

View reviewed changes

tocsoft approved these changes May 1, 2018

View reviewed changes

Delete merge conflict backup file.

72cb50c

JimBobSquarePants approved these changes May 2, 2018

View reviewed changes

JimBobSquarePants merged commit 6b4de97 into SixLabors:master May 2, 2018

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#552 from woutware/Issue-551-solidbrush-b…

2ca8950

…lend-performance Issue 551 solidbrush blend performance

Uh oh!

Issue 551 solidbrush blend performance #552

Issue 551 solidbrush blend performance #552

Uh oh!

Conversation

woutware commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tocsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tocsoft commented Apr 29, 2018

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woutware Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Apr 29, 2018

Uh oh!

codecov bot commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woutware commented Apr 29, 2018 •

edited

Loading

woutware Apr 30, 2018 •

edited

Loading

codecov bot commented Apr 30, 2018 •

edited

Loading

JimBobSquarePants Apr 30, 2018 •

edited

Loading

woutware commented Apr 30, 2018 •

edited

Loading

antonfirsov commented Apr 30, 2018 •

edited

Loading

antonfirsov commented Apr 30, 2018 •

edited

Loading