Skip to content
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

Image.WrapMemory<TPixel> APIs wrapping Memory<byte> #1314

Merged
merged 15 commits into from
Sep 1, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
82 changes: 76 additions & 6 deletions src/ImageSharp/Image.WrapMemory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public abstract partial class Image
{
/// <summary>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels,
/// allowing to view/manipulate it as an ImageSharp <see cref="Image{TPixel}"/> instance.
/// allowing to view/manipulate it as an <see cref="Image{TPixel}"/> instance.
/// </summary>
/// <typeparam name="TPixel">The pixel type</typeparam>
/// <param name="configuration">The <see cref="Configuration"/></param>
Expand All @@ -38,14 +38,15 @@ public static Image<TPixel> WrapMemory<TPixel>(
{
Guard.NotNull(configuration, nameof(configuration));
Guard.NotNull(metadata, nameof(metadata));
Guard.IsTrue(pixelMemory.Length == width * height, nameof(pixelMemory), "The length of the input memory doesn't match the specified image size");
Copy link
Member

Choose a reason for hiding this comment

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

Remind me to do a pass one day to remove all these guard allocations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait what allocations? 🤔
Also do you want me to create an issue about this to make sure we don't forget in the future?

Copy link
Member

Choose a reason for hiding this comment

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

Thought that would get you thinking. We always allocate the string whether true or false.

Copy link
Member Author

Choose a reason for hiding this comment

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

What string though? Both of those are constants, so the JIT will just load the address here.
I feel like I might be missing something obvious 😅

Eg. if you have this (full repro here):

public static void Test(Memory<int> memory)
{
    IsTrue(false, nameof(memory), "This is a sample constant string");
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static void IsTrue(bool flag, string a, string b)
{
}

You get this:

C.Test(System.Memory`1<Int32>)
    L0000: mov r8, 0x16e9fc8ab98
    L000a: mov r8, [r8]
    L000d: mov rdx, 0x16e9fc8ab28
    L0017: mov rdx, [rdx]
    L001a: xor ecx, ecx
    L001c: mov rax, C.IsTrue(Boolean, System.String, System.String)
    L0026: jmp rax

So you only pay the two indirect memory accesses to load the strings, but you have no allocations.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, I'm thinking when we interpolate (which I thought we were doing here). It might seem useful but it adds a lot of overhead


var memorySource = MemoryGroup<TPixel>.Wrap(pixelMemory);
return new Image<TPixel>(configuration, memorySource, width, height, metadata);
}

/// <summary>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels,
/// allowing to view/manipulate it as an ImageSharp <see cref="Image{TPixel}"/> instance.
/// allowing to view/manipulate it as an <see cref="Image{TPixel}"/> instance.
/// </summary>
/// <typeparam name="TPixel">The pixel type</typeparam>
/// <param name="configuration">The <see cref="Configuration"/></param>
Expand All @@ -64,7 +65,7 @@ public static Image<TPixel> WrapMemory<TPixel>(

/// <summary>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels,
/// allowing to view/manipulate it as an ImageSharp <see cref="Image{TPixel}"/> instance.
/// allowing to view/manipulate it as an <see cref="Image{TPixel}"/> instance.
/// The memory is being observed, the caller remains responsible for managing it's lifecycle.
/// </summary>
/// <typeparam name="TPixel">The pixel type.</typeparam>
Expand All @@ -81,7 +82,7 @@ public static Image<TPixel> WrapMemory<TPixel>(

/// <summary>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels,
/// allowing to view/manipulate it as an ImageSharp <see cref="Image{TPixel}"/> instance.
/// allowing to view/manipulate it as an <see cref="Image{TPixel}"/> instance.
/// The ownership of the <paramref name="pixelMemoryOwner"/> is being transferred to the new <see cref="Image{TPixel}"/> instance,
/// meaning that the caller is not allowed to dispose <paramref name="pixelMemoryOwner"/>.
/// It will be disposed together with the result image.
Expand All @@ -105,14 +106,15 @@ public static Image<TPixel> WrapMemory<TPixel>(
{
Guard.NotNull(configuration, nameof(configuration));
Guard.NotNull(metadata, nameof(metadata));
Guard.IsTrue(pixelMemoryOwner.Memory.Length == width * height, nameof(pixelMemoryOwner), "The length of the input memory doesn't match the specified image size");

var memorySource = MemoryGroup<TPixel>.Wrap(pixelMemoryOwner);
return new Image<TPixel>(configuration, memorySource, width, height, metadata);
}

/// <summary>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels,
/// allowing to view/manipulate it as an ImageSharp <see cref="Image{TPixel}"/> instance.
/// allowing to view/manipulate it as an <see cref="Image{TPixel}"/> instance.
/// The ownership of the <paramref name="pixelMemoryOwner"/> is being transferred to the new <see cref="Image{TPixel}"/> instance,
/// meaning that the caller is not allowed to dispose <paramref name="pixelMemoryOwner"/>.
/// It will be disposed together with the result image.
Expand All @@ -134,7 +136,7 @@ public static Image<TPixel> WrapMemory<TPixel>(

/// <summary>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels,
/// allowing to view/manipulate it as an ImageSharp <see cref="Image{TPixel}"/> instance.
/// allowing to view/manipulate it as an <see cref="Image{TPixel}"/> instance.
/// The ownership of the <paramref name="pixelMemoryOwner"/> is being transferred to the new <see cref="Image{TPixel}"/> instance,
/// meaning that the caller is not allowed to dispose <paramref name="pixelMemoryOwner"/>.
/// It will be disposed together with the result image.
Expand All @@ -150,5 +152,73 @@ public static Image<TPixel> WrapMemory<TPixel>(
int height)
where TPixel : unmanaged, IPixel<TPixel>
=> WrapMemory(Configuration.Default, pixelMemoryOwner, width, height);

/// <summary>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels,
/// allowing to view/manipulate it as an <see cref="Image{TPixel}"/> instance.
/// </summary>
/// <typeparam name="TPixel">The pixel type</typeparam>
/// <param name="configuration">The <see cref="Configuration"/></param>
/// <param name="byteMemory">The byte memory representing the pixel data.</param>
/// <param name="width">The width of the memory image.</param>
/// <param name="height">The height of the memory image.</param>
/// <param name="metadata">The <see cref="ImageMetadata"/>.</param>
/// <exception cref="ArgumentNullException">The configuration is null.</exception>
/// <exception cref="ArgumentNullException">The metadata is null.</exception>
/// <returns>An <see cref="Image{TPixel}"/> instance</returns>
public static Image<TPixel> WrapMemory<TPixel>(

Choose a reason for hiding this comment

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

https://docs.microsoft.com/en-us/dotnet/standard/memory-and-spans/memory-t-usage-guidelines

Seems beneficial to consider exposing IMemoryOwner<T> overloads of this to allow ownership transfer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't include them here on purpose as they weren't part of the initial API proposal in #1097.
@antonfirsov should this be added here or would that be for another PR?

Copy link
Member

Choose a reason for hiding this comment

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

No preference, other than it's always easier to review multiple smaller (subsequent) PR-s.

Copy link
Member Author

@Sergio0694 Sergio0694 Aug 13, 2020

Choose a reason for hiding this comment

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

Sounds like a preference to me 😄
Will keep this PR restricted to consumed buffers then, and we can create another one after this one is merged to also add support for owned ones through IMemoryOwner<byte>.

Configuration configuration,
Memory<byte> byteMemory,
int width,
int height,
Sergio0694 marked this conversation as resolved.
Show resolved Hide resolved
ImageMetadata metadata)
where TPixel : unmanaged, IPixel<TPixel>
{
Guard.NotNull(configuration, nameof(configuration));
Guard.NotNull(metadata, nameof(metadata));

var memoryManager = new ByteMemoryManager<TPixel>(byteMemory);

Guard.IsTrue(memoryManager.Memory.Length == width * height, nameof(byteMemory), "The length of the input memory doesn't match the specified image size");

var memorySource = MemoryGroup<TPixel>.Wrap(memoryManager.Memory);
return new Image<TPixel>(configuration, memorySource, width, height, metadata);
}

/// <summary>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels,
/// allowing to view/manipulate it as an <see cref="Image{TPixel}"/> instance.
/// </summary>
/// <typeparam name="TPixel">The pixel type</typeparam>
/// <param name="configuration">The <see cref="Configuration"/></param>
/// <param name="byteMemory">The byte memory representing the pixel data.</param>
/// <param name="width">The width of the memory image.</param>
/// <param name="height">The height of the memory image.</param>
/// <exception cref="ArgumentNullException">The configuration is null.</exception>
/// <returns>An <see cref="Image{TPixel}"/> instance.</returns>
public static Image<TPixel> WrapMemory<TPixel>(
Configuration configuration,
Memory<byte> byteMemory,
int width,
int height)
where TPixel : unmanaged, IPixel<TPixel>
=> WrapMemory<TPixel>(configuration, byteMemory, width, height, new ImageMetadata());

/// <summary>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels,
/// allowing to view/manipulate it as an <see cref="Image{TPixel}"/> instance.
/// The memory is being observed, the caller remains responsible for managing it's lifecycle.
/// </summary>
/// <typeparam name="TPixel">The pixel type.</typeparam>
/// <param name="byteMemory">The byte memory representing the pixel data.</param>
/// <param name="width">The width of the memory image.</param>
/// <param name="height">The height of the memory image.</param>
/// <returns>An <see cref="Image{TPixel}"/> instance.</returns>
public static Image<TPixel> WrapMemory<TPixel>(
Memory<byte> byteMemory,
int width,
int height)
where TPixel : unmanaged, IPixel<TPixel>
=> WrapMemory<TPixel>(Configuration.Default, byteMemory, width, height);
}
}
57 changes: 57 additions & 0 deletions src/ImageSharp/Memory/ByteMemoryManager{T}.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Copyright (c) Six Labors.
// Licensed under the Apache License, Version 2.0.
using System;
using System.Buffers;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace SixLabors.ImageSharp.Memory
{
/// <summary>
/// A custom <see cref="MemoryManager{T}"/> that can wrap <see cref="Memory{T}"/> of <see cref="byte"/> instances
/// and cast them to be <see cref="Memory{T}"/> for any arbitrary unmanaged <typeparamref name="T"/> value type.
/// </summary>
/// <typeparam name="T">The value type to use when casting the wrapped <see cref="Memory{T}"/> instance.</typeparam>
internal sealed class ByteMemoryManager<T> : MemoryManager<T>
where T : unmanaged
{
/// <summary>
/// The wrapped <see cref="Memory{T}"/> of <see cref="byte"/> instance.
/// </summary>
private readonly Memory<byte> memory;

/// <summary>
/// Initializes a new instance of the <see cref="ByteMemoryManager{T}"/> class.
/// </summary>
/// <param name="memory">The <see cref="Memory{T}"/> of <see cref="byte"/> instance to wrap.</param>
public ByteMemoryManager(Memory<byte> memory)
{
this.memory = memory;
}

/// <inheritdoc/>
protected override void Dispose(bool disposing)
{
}

/// <inheritdoc/>
public override Span<T> GetSpan()
{
Sergio0694 marked this conversation as resolved.
Show resolved Hide resolved
return MemoryMarshal.Cast<byte, T>(this.memory.Span);
}

/// <inheritdoc/>
public override MemoryHandle Pin(int elementIndex = 0)
{
// We need to adjust the offset into the wrapped byte segment,
// as the input index refers to the target-cast memory of T.
// We just have to shift this index by the byte size of T.
return this.memory.Slice(elementIndex * Unsafe.SizeOf<T>()).Pin();
}

/// <inheritdoc/>
public override void Unpin()
{
}
}
}
30 changes: 26 additions & 4 deletions src/ImageSharp/Memory/MemoryOwnerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,27 @@ namespace SixLabors.ImageSharp.Memory
/// </summary>
internal static class MemoryOwnerExtensions
{
/// <summary>
/// Gets a <see cref="Span{T}"/> from an <see cref="IMemoryOwner{T}"/> instance.
/// </summary>
/// <param name="buffer">The buffer</param>
/// <returns>The <see cref="Span{T}"/></returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Span<T> GetSpan<T>(this IMemoryOwner<T> buffer)
=> buffer.Memory.Span;
{
return buffer.Memory.Span;
}

/// <summary>
/// Gets the length of an <see cref="IMemoryOwner{T}"/> internal buffer.
/// </summary>
/// <param name="buffer">The buffer</param>
/// <returns>The length of the buffer</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int Length<T>(this IMemoryOwner<T> buffer)
=> buffer.GetSpan().Length;
{
return buffer.Memory.Length;
}

/// <summary>
/// Gets a <see cref="Span{T}"/> to an offsetted position inside the buffer.
Expand Down Expand Up @@ -56,8 +70,16 @@ public static void Clear<T>(this IMemoryOwner<T> buffer)
buffer.GetSpan().Clear();
}

/// <summary>
/// Gets a reference to the first item in the internal buffer for an <see cref="IMemoryOwner{T}"/> instance.
/// </summary>
/// <param name="buffer">The buffer</param>
/// <returns>A reference to the first item within the memory wrapped by <paramref name="buffer"/></returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static ref T GetReference<T>(this IMemoryOwner<T> buffer)
where T : struct =>
ref MemoryMarshal.GetReference(buffer.GetSpan());
where T : struct
{
return ref MemoryMarshal.GetReference(buffer.GetSpan());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ public PaletteDitherRowOperation(
this.source = source;
this.bounds = bounds;
this.scale = processor.DitherScale;
this.bitDepth = ImageMaths.GetBitsNeededForColorDepth(processor.Palette.Span.Length);
this.bitDepth = ImageMaths.GetBitsNeededForColorDepth(processor.Palette.Length);
}

[MethodImpl(InliningOptions.ShortMethod)]
Expand Down
Loading