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

Sergio0694 · 2020-08-13T14:17:58Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Closes #1097

Description

This PR adds the following APIs to the Image class:

namespace SixLabors.ImageSharp
{
    public abstract partial class Image
    {
        public static Image<TPixel> WrapMemory<TPixel>(
            Configuration configuration,
            Memory<byte> byteMemory,
            int width,
            int height,
            ImageMetadata metadata)
            where TPixel : unmanaged, IPixel<TPixel>;

        public static Image<TPixel> WrapMemory<TPixel>(
            Configuration configuration,
            Memory<byte> byteMemory,
            int width,
            int height)
            where TPixel : unmanaged, IPixel<TPixel>;

        public static Image<TPixel> WrapMemory<TPixel>(
            Memory<byte> byteMemory,
            int width,
            int height)
            where TPixel : unmanaged, IPixel<TPixel>;
    }
}

The internal implementation includes a new ByteMemoryManager<T> type that acts as a bridge to wrap Memory<byte> instances and use that data to produce a Memory<TPixel> instance that can be used to load the image as requested.

src/ImageSharp/Memory/ByteMemoryManager{T}.cs

john-h-k · 2020-08-13T14:37:26Z

src/ImageSharp/Image.WrapMemory.cs

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


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?

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?

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

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

src/ImageSharp/Memory/ByteMemoryManager{T}.cs

tests/ImageSharp.Tests/Image/ImageTests.WrapMemory.cs

Sergio0694 · 2020-08-13T14:48:53Z

@JimBobSquarePants my build is failing locally due to a bunch of errors about a missing assembly key in AssemblyInfo.cs:

Is there some new additional configuration step I'm missing?
My local clone should be up to date and I'm not seeing anything in the readme 🤔

Thanks!

JimBobSquarePants · 2020-08-13T15:07:02Z

@Sergio0694 Likely ~~your submodules are not up to date and~~ your solution requires cleaning.

Sergio0694 · 2020-08-13T15:10:43Z

@JimBobSquarePants I had already tried to clean the solution and to run both git clean -fdx and git submodule update --init --recursive, that didn't work. I guess I can just try nuking the entire clone and reload it again, for good measure 😄

Sergio0694 · 2020-08-13T15:35:00Z

@JimBobSquarePants Well surprise, I disabled the .NET 5 Preview SDK and I can build just fine now 😅

JimBobSquarePants · 2020-08-13T15:45:34Z

Well surprise, I disabled the .NET 5 Preview SDK and I can build just fine now

How do you disable a preview? I've been terrified to install any of them.

Sergio0694 · 2020-08-13T15:48:34Z

@JimBobSquarePants It's pretty easy actually, you need to go to Tools > Options > Environment > Preview Features and then untick the option regarding preview versions of the .NET Core SDK, and restart VS:

I could finally debug the tests locally with that out of the picture, fixed crossed for the CI now! 🚀

codecov · 2020-08-13T16:00:55Z

Codecov Report

Merging #1314 into master will decrease coverage by 0.00%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #1314      +/-   ##
==========================================
- Coverage   82.77%   82.77%   -0.01%     
==========================================
  Files         689      690       +1     
  Lines       30956    30973      +17     
  Branches     3511     3511              
==========================================
+ Hits        25625    25639      +14     
- Misses       4610     4613       +3     
  Partials      721      721

Flag	Coverage Δ
#unittests	`82.77% <85.71%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/ImageSharp/Memory/ByteMemoryManager{T}.cs	`57.14% <57.14%> (ø)`
src/ImageSharp/Image.WrapMemory.cs	`100.00% <100.00%> (ø)`
src/ImageSharp/Memory/MemoryOwnerExtensions.cs	`85.71% <100.00%> (ø)`
...p/Processing/Processors/Dithering/OrderedDither.cs	`98.76% <100.00%> (ø)`

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 c4dabd9...26b948d. Read the comment docs.

Sergio0694 · 2020-08-13T16:03:02Z

CI passed! 🚀

The PR should be ready for review with the original API surface mentioned in the linked issue.
Added James and Anton as reviewers 👍

antonfirsov

Just a few quick remarks, will do deeper review later.

tests/ImageSharp.Tests/Image/ImageTests.WrapMemory.cs

antonfirsov · 2020-08-13T17:09:21Z

src/ImageSharp/Image.WrapMemory.cs

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


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

src/ImageSharp/Image.WrapMemory.cs

JimBobSquarePants · 2020-08-19T16:44:27Z

@Sergio0694 I will get to this. Just fixing a critical issue #1316 first

Sergio0694 · 2020-08-19T18:11:01Z

@JimBobSquarePants Don't worry, no rush! 😄
Just got very excited about that sponsorship from the MS OSS program and wanted to get back and contribute to something 🚀

Sergio0694 · 2020-08-25T20:19:53Z

@JimBobSquarePants Just noticed this bit while adding the unit tests:

ImageSharp/src/ImageSharp/Memory/MemoryOwnerExtensions.cs

Lines 16 to 22 in 801961d

    
           [MethodImpl(MethodImplOptions.AggressiveInlining)] 
        
           public static Span<T> GetSpan<T>(this IMemoryOwner<T> buffer) 
        
               => buffer.Memory.Span; 
        
           [MethodImpl(MethodImplOptions.AggressiveInlining)] 
        
           public static int Length<T>(this IMemoryOwner<T> buffer) 
        
               => buffer.GetSpan().Length;

Wouldn't it be more efficient to just have GetLength return the length from the Memory<T>?
Accessing the Span property of a Memory<T> instance is relatively expensive.
Unless there's a specific reason this was done? Not seeing a comment here explaining that though 🤔

JimBobSquarePants · 2020-08-25T20:29:01Z

@Sergio0694 Wow! Never actually looked at the code for Memory<T> before. Yeah, can't think of any reason why we would favor getting the length of the span. If you see any other instances where we do that please update them also.

JimBobSquarePants

Not sure we need the new code analysis attributes (we don't use them elsewhere) but otherwise looks great! 👍

JimBobSquarePants · 2020-08-25T21:56:01Z

src/ImageSharp/Image.WrapMemory.cs

@@ -38,6 +38,7 @@ public abstract partial class Image
        {
            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");


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

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

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

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.

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

src/ImageSharp/Image.WrapMemory.cs

src/ImageSharp/Memory/MemoryOwnerExtensions.cs

## PR Type What kind of change does this PR introduce?  - Feature ## What is the current behavior?  Right now there is no (easy) way to cast a `Memory<TFrom>` instance to a `Memory<TTo>` instance. There are APIs to to do that for `Span<T>` instances, but not for `Memory<T>`. The reason for that is that with a `Span<T>` it's just a matter of retrieving the wrapped reference, reinterpreting it and then adjusting the size, then creating a new `Span<T>` instance. But a `Memory<T>` instance is completely different: it wraps an object which could be either a `T[]` array, a `MemoryManager<T>` instance, etc. The result is that currently there are no APIs in the BCL nor in the toolkit to just "cast" a `Memory<T>`. This feature has been requested by a number of developers, including in a well known library such as `ImageSharp`: > Yes, that's exactly what I would need. But I'm wondering how would you implement it. > It's certainly non trivial to cast a `Memory<byte>` to a `Memory<TPixel>` and if there's an API for that I would gladly want to know... > So I pressume `ImageSharp` would need to do some work under the hood. (_`ImageSharp` issue, [here](SixLabors/ImageSharp#1097 (comment)) To solve that, I created a very simplified version of the code included in this PR, into a PR [here](SixLabors/ImageSharp#1314). Having this available right out of the box in the `HighPerformance` package would be helpful in a number of similar situations, especially with `Memory<T>` APIs becoming more and more common across libraries now (as they've been out for a while). ## What is the new behavior?  This PR includes 4 new extensions for the `Memory<T>` and `ReadOnlyMemory<T>` types that enable the following: ```csharp // Cast between two Memory<T> instances... Memory<byte> memoryOfBytes = new byte[128].AsMemory(); Memory<float> memoryOfFloats = memoryOfBytes.Cast<byte, float>(); // ...any number of times is needed Memory<int> memoryOfInts = memoryOfFloats.Cast<float, int>(); Memory<byte> backToBytesMemory = memoryOfInts.Cast<int, byte>(); // Or just convert into bytes directly Memory<int> sourceAsInts = new int[128].AsMemory(); Memory<byte> sourceAsBytes = sourceAsInts.AsBytes(); // Want to get a stream from a string? Why not! 😄 using (Stream stream = "Hello world".AsMemory().AsBytes().AsStream()) { // Use the stream here, which reads *directly* from the string data! } ``` Here is the full list of the new APIs introduced in this PR: ```csharp namespace Microsoft.Toolkit.HighPerformance.Extensions { public static class MemoryExtensions { public static Memory<byte> AsBytes<T>(this Memory<T> memory) where T : unmanaged; public static Memory<TTo> Cast<TFrom, TTo>(this Memory<TFrom> memory) where TFrom : unmanaged where TTo : unmanaged; } public static class ReadOnlyMemoryExtensions { public static ReadOnlyMemory<byte> AsBytes<T>(this ReadOnlyMemory<T> memory) where T : unmanaged; public static ReadOnlyMemory<TTo> Cast<TFrom, TTo>(this ReadOnlyMemory<TFrom> memory) where TFrom : unmanaged where TTo : unmanaged; } } ``` ## Notes Marking as draft as this is still being worked on, but feedbacks and reviews are welcome! 😄 ## PR Checklist Please check if your PR fulfills the following requirements: - [X] Tested code with current [supported SDKs](../readme.md#supported) - [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: ~~ - [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~ - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~ - [X] Tests for the changes have been added (for bug fixes / features) (if applicable) - [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*) - [X] Contains **NO** breaking changes

Sergio0694 added 3 commits August 13, 2020 15:49

Add ByteMemoryManager<T> type

3984e80

Add Image.WrapMemory<TPixel> from Memory<byte>

b3f8572

Add tests for new Image.WrapMemory APIs

e26e57f

Sergio0694 added enhancement API labels Aug 13, 2020

john-h-k reviewed Aug 13, 2020

View reviewed changes

src/ImageSharp/Memory/ByteMemoryManager{T}.cs Show resolved Hide resolved

Remove unnecessary indirection in ByteMemoryManager<T>

32380d2

john-h-k suggested changes Aug 13, 2020

View reviewed changes

Fix bug in Pin() method, minor code tweaks

eb22996

Fix relative offsetting in Pin() methods

2383245

Fix comparison in wrapping bytes test

85f91f6

Sergio0694 requested review from JimBobSquarePants and antonfirsov August 13, 2020 16:02

antonfirsov reviewed Aug 13, 2020

View reviewed changes

Replace Default.Clone with CreateDefaultConfiguration

ff5e301

Sergio0694 added 3 commits August 25, 2020 22:35

Add input size validation in Image.WrapMemory

fa59a73

Minor optimizations, improve XML docs and annotations

1de45b7

Remove unnecessary Memory<T>.Span access

567f3a6

Merge branch 'master' into sp/byte-to-tpixel-wrapping

9295e93

JimBobSquarePants approved these changes Aug 25, 2020

View reviewed changes

Sergio0694 and others added 3 commits August 26, 2020 01:25

Simplify XML docs for WrapMemory APIs

d7380f0

Remove [Pure] attributes

d861805

Merge branch 'master' into sp/byte-to-tpixel-wrapping

26b948d

JimBobSquarePants merged commit ade5ba5 into master Sep 1, 2020

JimBobSquarePants deleted the sp/byte-to-tpixel-wrapping branch September 1, 2020 13:00

JimBobSquarePants mentioned this pull request Sep 1, 2020

Fix compatibility with NET 5 SDK InternalsVisibleTo #1337

Merged

4 tasks

Sergio0694 mentioned this pull request Sep 14, 2020

Improve Code Coverage/Add to CI CommunityToolkit/WindowsCommunityToolkit#3475

Open

Sergio0694 mentioned this pull request Oct 4, 2020

New [ReadOnly]Memory<T>.Cast<TFrom, TTo> extensions CommunityToolkit/WindowsCommunityToolkit#3520

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Sergio0694 commented Aug 13, 2020

john-h-k Aug 13, 2020

Sergio0694 Aug 13, 2020

antonfirsov Aug 13, 2020

Sergio0694 Aug 13, 2020 •

edited

Loading

Sergio0694 commented Aug 13, 2020

JimBobSquarePants commented Aug 13, 2020 •

edited

Loading

Sergio0694 commented Aug 13, 2020

Sergio0694 commented Aug 13, 2020

JimBobSquarePants commented Aug 13, 2020

Sergio0694 commented Aug 13, 2020

codecov bot commented Aug 13, 2020 •

edited

Loading

Sergio0694 commented Aug 13, 2020

antonfirsov left a comment

antonfirsov Aug 13, 2020

JimBobSquarePants commented Aug 19, 2020 •

edited

Loading

Sergio0694 commented Aug 19, 2020

Sergio0694 commented Aug 25, 2020

JimBobSquarePants commented Aug 25, 2020

JimBobSquarePants left a comment

JimBobSquarePants Aug 25, 2020

Sergio0694 Aug 26, 2020

JimBobSquarePants Aug 26, 2020

Sergio0694 Aug 26, 2020

JimBobSquarePants Aug 26, 2020

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

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

Conversation

Sergio0694 commented Aug 13, 2020

Prerequisites

Closes #1097

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Sergio0694 commented Aug 13, 2020

JimBobSquarePants commented Aug 13, 2020 • edited Loading

Sergio0694 commented Aug 13, 2020

Sergio0694 commented Aug 13, 2020

JimBobSquarePants commented Aug 13, 2020

Sergio0694 commented Aug 13, 2020

codecov bot commented Aug 13, 2020 • edited Loading

Codecov Report

Sergio0694 commented Aug 13, 2020

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants commented Aug 19, 2020 • edited Loading

Sergio0694 commented Aug 19, 2020

Sergio0694 commented Aug 25, 2020

JimBobSquarePants commented Aug 25, 2020

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Aug 13, 2020 •

edited

Loading

JimBobSquarePants commented Aug 13, 2020 •

edited

Loading

codecov bot commented Aug 13, 2020 •

edited

Loading

JimBobSquarePants commented Aug 19, 2020 •

edited

Loading