Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,23 @@ internal override void Apply(Span<float> scanline, int x, int y)

MemoryManager memoryManager = this.Target.MemoryManager;

using (IBuffer<float> amountBuffer = memoryManager.Allocate<float>(scanline.Length))
if (this.Options.BlendPercentage == 1f)
{
Span<float> amountSpan = amountBuffer.Span;

for (int i = 0; i < scanline.Length; i++)
this.Blender.Blend(memoryManager, destinationRow, destinationRow, this.Colors.Span, scanline);
}
else
{
using (IBuffer<float> amountBuffer = memoryManager.Allocate<float>(scanline.Length))
{
amountSpan[i] = scanline[i] * this.Options.BlendPercentage;
}
Span<float> amountSpan = amountBuffer.Span;

for (int i = 0; i < scanline.Length; i++)
{
amountSpan[i] = scanline[i] * this.Options.BlendPercentage;
}

this.Blender.Blend(memoryManager, destinationRow, destinationRow, this.Colors.Span, amountSpan);
this.Blender.Blend(memoryManager, destinationRow, destinationRow, this.Colors.Span, amountSpan);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Threading.Tasks;
using SixLabors.ImageSharp.Advanced;
using SixLabors.ImageSharp.Memory;
using SixLabors.ImageSharp.PixelFormats;
using SixLabors.ImageSharp.Processing.Drawing.Brushes;
Expand Down Expand Up @@ -49,38 +50,61 @@ protected override void OnFrameApply(ImageFrame<TPixel> source, Rectangle source
int minY = Math.Max(0, startY);
int maxY = Math.Min(source.Height, endY);

// Reset offset if necessary.
if (minX > 0)
{
startX = 0;
}

if (minY > 0)
{
startY = 0;
}

int width = maxX - minX;

using (IBuffer<float> amount = source.MemoryManager.Allocate<float>(width))
using (BrushApplicator<TPixel> applicator = this.brush.CreateApplicator(
source,
sourceRectangle,
this.options))
{
amount.Span.Fill(this.options.BlendPercentage);
var solidBrush = this.brush as SolidBrush<TPixel>;

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

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 IsSolidBrushWithoutBlending property or similar for better readability.

(this.options.BlenderMode == PixelBlenderMode.Normal && this.options.BlendPercentage == 1f && solidBrush.Color.ToVector4().W == 1f) ||
(this.options.BlenderMode == PixelBlenderMode.Over && this.options.BlendPercentage == 1f && solidBrush.Color.ToVector4().W == 1f) ||
(this.options.BlenderMode == PixelBlenderMode.Src)))
{
Parallel.For(
minY,
maxY,
configuration.ParallelOptions,
y =>
{
int offsetY = y - startY;
int offsetX = minX - startX;
{
int offsetY = y - startY;
int offsetX = minX - startX;
source.GetPixelRowSpan(y).Slice(minX, width).Fill(solidBrush.Color);
});
}
else
{
// Reset offset if necessary.
if (minX > 0)
{
startX = 0;
}

if (minY > 0)
{
startY = 0;
}

using (IBuffer<float> amount = source.MemoryManager.Allocate<float>(width))
using (BrushApplicator<TPixel> applicator = this.brush.CreateApplicator(
source,
sourceRectangle,
this.options))
{
amount.Span.Fill(this.options.BlendPercentage);

Parallel.For(
minY,
maxY,
configuration.ParallelOptions,
y =>
{
int offsetY = y - startY;
int offsetX = minX - startX;

applicator.Apply(amount.Span, offsetX, offsetY);
});
applicator.Apply(amount.Span, offsetX, offsetY);
});
}
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/ImageSharp/ImageFrameCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ internal ImageFrameCollection(Image<TPixel> parent, int width, int height)
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, parent.ClearColor));
}

internal ImageFrameCollection(Image<TPixel> parent, IEnumerable<ImageFrame<TPixel>> frames)
Expand Down Expand Up @@ -143,7 +143,7 @@ public Image<TPixel> CloneFrame(int index)
/// <inheritdoc/>
public ImageFrame<TPixel> CreateFrame()
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CreateFrame(TPixel backgroundColor = default)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@antonfirsov antonfirsov May 1, 2018

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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);
ImageFrame<TPixel> frame = new ImageFrame<TPixel>(this.parent.GetConfiguration(), this.RootFrame.Width, this.RootFrame.Height, this.parent.ClearColor);
this.frames.Add(frame);
return frame;
}
Expand Down
51 changes: 51 additions & 0 deletions src/ImageSharp/ImageFrame{TPixel}.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -52,6 +53,39 @@ 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="clearColor">The color to clear the image with.</param>
internal ImageFrame(Configuration configuration, int width, int height, TPixel clearColor)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

: this(configuration, width, height, clearColor, 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="clearColor">The color to clear the image with.</param>
/// <param name="metaData">The meta data.</param>
internal ImageFrame(Configuration configuration, int width, int height, TPixel clearColor, 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.Clear(configuration.ParallelOptions, clearColor);
Copy link
Member

Choose a reason for hiding this comment

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

this.PixelBuffer.Span.Fill()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

👍

this.MetaData = metaData;
}

/// <summary>
/// Initializes a new instance of the <see cref="ImageFrame{TPixel}" /> class.
/// </summary>
Expand Down Expand Up @@ -267,6 +301,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>
public void Clear(ParallelOptions parallelOptions, TPixel value) {
Copy link
Member

@JimBobSquarePants JimBobSquarePants Apr 30, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Parallel.For(
0,
this.Height,
parallelOptions,
(int y) =>
{
Span<TPixel> targetRow = this.GetPixelRowSpan(y);
targetRow.Fill(value);
});
}

/// <summary>
/// Clones the current instance.
/// </summary>
Expand Down
40 changes: 40 additions & 0 deletions src/ImageSharp/Image{TPixel}.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public sealed class Image<TPixel> : IImage, IConfigurable
{
private readonly Configuration configuration;
private readonly ImageFrameCollection<TPixel> frames;
private readonly TPixel clearColor;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


/// <summary>
/// Initializes a new instance of the <see cref="Image{TPixel}"/> class
Expand All @@ -37,6 +38,21 @@ public Image(Configuration configuration, int width, int height)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="Image{TPixel}"/> class
/// with the height and the width of the image.
/// </summary>
/// <param name="configuration">
/// The configuration providing initialization code which allows extending the library.
/// </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="clearColor">The color to initialize the pixels with.</param>
public Image(Configuration configuration, int width, int height, TPixel clearColor)
Copy link
Member

Choose a reason for hiding this comment

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

clearColor => backgroundColor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

: this(configuration, width, height, clearColor, new ImageMetaData())
{
}

/// <summary>
/// Initializes a new instance of the <see cref="Image{TPixel}"/> class
/// with the height and the width of the image.
Expand Down Expand Up @@ -66,6 +82,25 @@ internal Image(Configuration configuration, int width, int height, ImageMetaData
this.frames = new ImageFrameCollection<TPixel>(this, width, height);
}

/// <summary>
/// Initializes a new instance of the <see cref="Image{TPixel}"/> class
/// with the height and the width of the image.
/// </summary>
/// <param name="configuration">
/// The configuration providing initialization code which allows extending the library.
/// </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="clearColor">The clear color.</param>
/// <param name="metadata">The images metadata.</param>
internal Image(Configuration configuration, int width, int height, TPixel clearColor, ImageMetaData metadata) {
this.configuration = configuration ?? Configuration.Default;
this.PixelType = new PixelTypeInfo(Unsafe.SizeOf<TPixel>() * 8);
this.MetaData = metadata ?? new ImageMetaData();
this.clearColor = clearColor;
this.frames = new ImageFrameCollection<TPixel>(this, width, height);
}

/// <summary>
/// Initializes a new instance of the <see cref="Image{TPixel}" /> class
/// with the height and the width of the image.
Expand Down Expand Up @@ -104,6 +139,11 @@ internal Image(Configuration configuration, ImageMetaData metadata, IEnumerable<
/// </summary>
public IImageFrameCollection<TPixel> Frames => this.frames;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, good point there.


/// <summary>
/// Gets the root frame.
/// </summary>
Expand Down
12 changes: 12 additions & 0 deletions src/ImageSharp/Memory/BufferExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,18 @@ public static void Clear<T>(this IBuffer<T> buffer)
buffer.Span.Clear();
}

/// <summary>
/// Fills the contents of this buffer.
/// </summary>
/// <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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

where T : struct
{
buffer.Span.Fill(value);
}

public static ref T DangerousGetPinnableReference<T>(this IBuffer<T> buffer)
where T : struct =>
ref MemoryMarshal.GetReference(buffer.Span);
Expand Down
24 changes: 12 additions & 12 deletions src/ImageSharp/PixelFormats/PixelBlenderMode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -54,62 +54,62 @@ public enum PixelBlenderMode
HardLight,

/// <summary>
/// returns the source colors
/// returns the source colors.
/// </summary>
Src,

/// <summary>
/// returns the source over the destination
/// returns the source over the destination.
/// </summary>
Atop,

/// <summary>
/// returns the detination over the source
/// returns the destination over the source.
/// </summary>
Over,

/// <summary>
/// the source where the desitnation and source overlap
/// The source where the destination and source overlap.
/// </summary>
In,

/// <summary>
/// the destination where the desitnation and source overlap
/// The destination where the destination and source overlap.
/// </summary>
Out,

/// <summary>
/// the destination where the source does not overlap it
/// The destination where the source does not overlap it.
/// </summary>
Dest,

/// <summary>
/// the source where they dont overlap othersie dest in overlapping parts
/// The source where they don't overlap othersie dest in overlapping parts.
/// </summary>
DestAtop,

/// <summary>
/// the destnation over the source
/// The destination over the source.
/// </summary>
DestOver,

/// <summary>
/// the destination where the desitnation and source overlap
/// The destination where the destination and source overlap.
/// </summary>
DestIn,

/// <summary>
/// the source where the desitnation and source overlap
/// The source where the destination and source overlap.
/// </summary>
DestOut,

/// <summary>
/// the clear.
/// The clear.
/// </summary>
Clear,

/// <summary>
/// clear where they overlap
/// Clear where they overlap.
/// </summary>
Xor
}
Expand Down