Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
20 changes: 18 additions & 2 deletions src/ImageSharp/ImageFrameCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,14 @@ 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));
if (parent.ClearColor.HasValue)
{
this.frames.Add(new ImageFrame<TPixel>(parent.GetConfiguration(), width, height, parent.ClearColor.Value));
}
else
{
this.frames.Add(new ImageFrame<TPixel>(parent.GetConfiguration().MemoryManager, width, height));
}
}

internal ImageFrameCollection(Image<TPixel> parent, IEnumerable<ImageFrame<TPixel>> frames)
Expand Down Expand Up @@ -143,7 +150,16 @@ 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;
if (this.parent.ClearColor.HasValue)
{
frame = new ImageFrame<TPixel>(this.parent.GetConfiguration(), this.RootFrame.Width, this.RootFrame.Height, this.parent.ClearColor.Value);
}
else
{
frame = new ImageFrame<TPixel>(this.parent.GetConfiguration().MemoryManager, this.RootFrame.Width, this.RootFrame.Height);
}

this.frames.Add(frame);
return frame;
}
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;

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@woutware woutware Apr 30, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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've moved it to ImageFrame, it's indeed more consistent with the Width/Height properties.


/// <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