-
-
Notifications
You must be signed in to change notification settings - Fork 888
Issue 551 solidbrush blend performance #552
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
Changes from 10 commits
5a1569f
08b033c
58384de
8995a73
a3a1556
02fc4ef
a38bb5a
2943df1
aa4a057
7e2fd11
7c70a5e
0d81089
d2bfaf2
24f6ff9
a9816fd
72cb50c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,14 +16,14 @@ internal sealed class ImageFrameCollection<TPixel> : IImageFrameCollection<TPixe | |
| private readonly IList<ImageFrame<TPixel>> frames = new List<ImageFrame<TPixel>>(); | ||
| private readonly Image<TPixel> parent; | ||
|
|
||
| internal ImageFrameCollection(Image<TPixel> parent, int width, int height) | ||
| internal ImageFrameCollection(Image<TPixel> parent, int width, int height, TPixel backgroundColor) | ||
| { | ||
| Guard.NotNull(parent, nameof(parent)); | ||
|
|
||
| this.parent = parent; | ||
|
|
||
| // Frames are already cloned within the caller | ||
| this.frames.Add(new ImageFrame<TPixel>(parent.GetConfiguration().MemoryManager, width, height)); | ||
| this.frames.Add(new ImageFrame<TPixel>(parent.GetConfiguration(), width, height, backgroundColor)); | ||
| } | ||
|
|
||
| internal ImageFrameCollection(Image<TPixel> parent, IEnumerable<ImageFrame<TPixel>> frames) | ||
|
|
@@ -143,7 +143,7 @@ public Image<TPixel> CloneFrame(int index) | |
| /// <inheritdoc/> | ||
| public ImageFrame<TPixel> CreateFrame() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's create an overload for this that accepts a color, then we cover all bases.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the method implements a member from interface IImageFrameCollection, wouldn't it be nice to change it to:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we don’t actually need the interface. I can’t see why someone would need to implement it themselves.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, always choose overloads. |
||
| { | ||
| var frame = new ImageFrame<TPixel>(this.parent.GetConfiguration().MemoryManager, this.RootFrame.Width, this.RootFrame.Height); | ||
| var frame = new ImageFrame<TPixel>(this.parent.GetConfiguration(), this.RootFrame.Width, this.RootFrame.Height, this.RootFrame.BackgroundColor); | ||
| this.frames.Add(frame); | ||
| return frame; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| using System; | ||
| using System.Numerics; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Runtime.InteropServices; | ||
| using System.Threading.Tasks; | ||
| using SixLabors.ImageSharp.Advanced; | ||
| using SixLabors.ImageSharp.Memory; | ||
|
|
@@ -18,8 +19,7 @@ namespace SixLabors.ImageSharp | |
| /// </summary> | ||
| /// <typeparam name="TPixel">The pixel format.</typeparam> | ||
| public sealed class ImageFrame<TPixel> : IPixelSource<TPixel>, IDisposable | ||
| where TPixel : struct, IPixel<TPixel> | ||
| { | ||
| where TPixel : struct, IPixel<TPixel> { | ||
| private bool isDisposed; | ||
|
|
||
| /// <summary> | ||
|
|
@@ -29,8 +29,7 @@ public sealed class ImageFrame<TPixel> : IPixelSource<TPixel>, IDisposable | |
| /// <param name="width">The width of the image in pixels.</param> | ||
| /// <param name="height">The height of the image in pixels.</param> | ||
| internal ImageFrame(MemoryManager memoryManager, int width, int height) | ||
| : this(memoryManager, width, height, new ImageFrameMetaData()) | ||
| { | ||
| : this(memoryManager, width, height, new ImageFrameMetaData()) { | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -52,15 +51,47 @@ internal ImageFrame(MemoryManager memoryManager, int width, int height, ImageFra | |
| this.MetaData = metaData; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="ImageFrame{TPixel}" /> class. | ||
| /// </summary> | ||
| /// <param name="configuration">The <see cref="Configuration"/> to use for buffer allocation and parallel options to clear the buffer with.</param> | ||
| /// <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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is good! I actually wanted to introduce the configuration as a parameter and drop the |
||
| : this(configuration, width, height, backgroundColor, new ImageFrameMetaData()) { | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="ImageFrame{TPixel}" /> class. | ||
| /// </summary> | ||
| /// <param name="configuration">The <see cref="Configuration"/> to use for buffer allocation and parallel options to clear the buffer with.</param> | ||
| /// <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> | ||
| /// <param name="metaData">The meta data.</param> | ||
| internal ImageFrame(Configuration configuration, int width, int height, TPixel backgroundColor, ImageFrameMetaData metaData) | ||
| { | ||
| Guard.NotNull(configuration, nameof(configuration)); | ||
| Guard.MustBeGreaterThan(width, 0, nameof(width)); | ||
| Guard.MustBeGreaterThan(height, 0, nameof(height)); | ||
| Guard.NotNull(metaData, nameof(metaData)); | ||
|
|
||
| this.MemoryManager = configuration.MemoryManager; | ||
| this.PixelBuffer = this.MemoryManager.Allocate2D<TPixel>(width, height, false); | ||
| this.BackgroundColor = backgroundColor; | ||
| this.Clear(configuration.ParallelOptions, backgroundColor); | ||
| this.MetaData = metaData; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="ImageFrame{TPixel}" /> class. | ||
| /// </summary> | ||
| /// <param name="memoryManager">The <see cref="MemoryManager"/> to use for buffer allocations.</param> | ||
| /// <param name="size">The <see cref="Size"/> of the frame.</param> | ||
| /// <param name="metaData">The meta data.</param> | ||
| internal ImageFrame(MemoryManager memoryManager, Size size, ImageFrameMetaData metaData) | ||
| : this(memoryManager, size.Width, size.Height, metaData) | ||
| { | ||
| : this(memoryManager, size.Width, size.Height, metaData) { | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -99,6 +130,11 @@ internal ImageFrame(MemoryManager memoryManager, ImageFrame<TPixel> source) | |
| /// </summary> | ||
| public int Height => this.PixelBuffer.Height; | ||
|
|
||
| /// <summary> | ||
| /// Gets the background color. | ||
| /// </summary> | ||
| public TPixel BackgroundColor { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the meta data of the frame. | ||
| /// </summary> | ||
|
|
@@ -267,6 +303,23 @@ internal ImageFrame<TPixel2> CloneAs<TPixel2>() | |
| return target; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Clears the bitmap. | ||
| /// </summary> | ||
| /// <param name="parallelOptions">The parallel options.</param> | ||
| /// <param name="value">The value to initialize the bitmap with.</param> | ||
| internal void Clear(ParallelOptions parallelOptions, TPixel value) { | ||
| Parallel.For( | ||
| 0, | ||
| this.Height, | ||
| parallelOptions, | ||
| (int y) => | ||
| { | ||
| Span<TPixel> targetRow = this.GetPixelRowSpan(y); | ||
| targetRow.Fill(value); | ||
| }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Clones the current instance. | ||
| /// </summary> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactor this logic into an
IsSolidBrushWithoutBlendingproperty or similar for better readability.