Introduce basic Memory<T> API-s #607

antonfirsov · 2018-06-10T01:24:07Z

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)

Description

This PR introduces some memory API-s specified in #565. I think I managed to understand and follow the ownership management rules of Memory<T>. beta5 was originally intended to be a hotfix release, but I think it's worth to add these API-s.

Features

AdvancedImageExtensions:
- GetPixelMemory() / GetPixelSpan() for both Image<T> and ImageFrame<T>
- GetPixelRowMemory() / GetPixelRowSpan() for both Image<T> and ImageFrame<T>
Image.WrapMemory(Memory<T>) for wrapping an existing external memory area (eg. contents of a System.Drawing.BitmapData)
- This is kept internal for now. We need to validate it by testing it against an actual interop scenario (eg. System.Drawing)

Internal Refactors

Adapting System.Buffers.MemoryManager<T> (formerly known as OwnedMemory<T>) by IBuffer<T> implementations
Refactor ShapeRegion to allow dropping AllocateFake

Breaking Changes

Renamed MemoryManager to MemoryAllocator to avoid name conflicts with System.Buffers.MemoryManager<T>
Memory management primitives should be moved to SixLabors.Core, which means that they should be defined under the SixLabors.Memory namespace instead of SixLabors.ImageSharp.Memory. In order to concentrate the breaking changes into a single release, I decided to do the namespace move in this PR.
DangerousGetPinnableReferenceToPixelBuffer() is marked obsolete. MemoryMarshal.GetReference(source.GetPixelSpan()) should be used instead

…s.MemoryManager<T> API

JimBobSquarePants

Nice work! I had a good read of the linked rules and I cannot see anything that violates those rules.

Naming is good and I'm glad to see GetSpan(), I much prefer a definitive method of a property, less sleight-of-hand.

JimBobSquarePants · 2018-06-10T02:56:11Z

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

-            DebugGuard.MustBeGreaterThanOrEqualTo(rectangle.Y, 0, nameof(rectangle));
-            DebugGuard.MustBeLessThanOrEqualTo(rectangle.Width, destinationBuffer.Width, nameof(rectangle));
-            DebugGuard.MustBeLessThanOrEqualTo(rectangle.Height, destinationBuffer.Height, nameof(rectangle));
+            ImageSharp.DebugGuard.MustBeGreaterThanOrEqualTo(rectangle.X, 0, nameof(rectangle));


We'll need to clean up and consolidate these Guard classes sometime. I don't think there's anything in the ImageSharp ones that couldn't be moved to Core. I'm actually tempted to make them public.

JimBobSquarePants · 2018-06-10T03:00:49Z

src/ImageSharp/PixelAccessor{TPixel}.cs

@@ -5,8 +5,8 @@
 using System.Diagnostics;


When do we get to ditch this class? I don't think it's used much now.

tocsoft

Image.SwapPixelsBuffers() and ImageFrame.SwapPixelsBuffers() won't work when the image is backed by an externally owned buffer thus those methods/processors that relay on that api need to know they won't work and throw a not supported exception. (this will effect all apis that resize the images backing buffer, resize, crop etc) this is why my original proposal for IBuffer has the method bool SwitchBuffer(IBuffer<T> buffer); so that it would handle that aspect internally an allow this sort of API.

antonfirsov · 2018-06-10T13:24:31Z

@tocsoft good finding thanks!
I think I can find a way to manage this without touching the IBuffer<T> API. Now I think we should make that interface as narrow as possible, because it might make sense to replace it with IMemoryOwner<T>.

codecov · 2018-06-10T15:00:05Z

Codecov Report

Merging #607 into master will increase coverage by 0.09%.
The diff coverage is 95.54%.

@@            Coverage Diff             @@
##           master     #607      +/-   ##
==========================================
+ Coverage   88.46%   88.56%   +0.09%     
==========================================
  Files         872      879       +7     
  Lines       36822    37039     +217     
  Branches     2620     2631      +11     
==========================================
+ Hits        32574    32802     +228     
- Misses       3450     3455       +5     
+ Partials      798      782      -16

Impacted Files	Coverage Δ
src/ImageSharp/Formats/Png/PngChunk.cs	`100% <ø> (ø)`	⬆️
.../Jpeg/Components/Decoder/JpegBlockPostProcessor.cs	`100% <ø> (ø)`	⬆️
...rocessing/Convolution/Processors/SobelProcessor.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp/Memory/SimpleGcMemoryAllocator.cs	`100% <ø> (ø)`
src/ImageSharp/Memory/BasicByteBuffer.cs	`100% <ø> (ø)`	⬆️
...ts/PixelBlenders/DefaultPixelBlenders.Generated.cs	`96.42% <ø> (ø)`	⬆️
...eSharp/Formats/Jpeg/Components/Block8x8F.CopyTo.cs	`90% <ø> (+1.11%)`	⬆️
tests/ImageSharp.Tests/Image/ImageTests.cs	`100% <ø> (ø)`	⬆️
.../ImageSharp.Tests/TestDataIcc/IccTestDataMatrix.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp/Memory/MemoryAllocator.cs	`0% <ø> (ø)`
... and 138 more

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 cdc63ad...4ae524d. Read the comment docs.

antonfirsov · 2018-06-10T23:45:25Z

@tocsoft after thinking a lot, it seems still most reasonable to manage the ownership move/swap logic in Buffer2D<TPixel>. This way all IBuffer<T>-s are returned to their own allocators/pools on .Dispose().

Image<T> and ImageFrame<T> swapping logic is now delegated to the implementation in Buffer2D<TPixel>.SwapOrCopyContent(). It has the following rules:

We can always swap the contents of two Buffer2D<T> instances if their inner buffers both own their memory (regardless of their original MemoryAllocator-s).
If one of the inner buffers is only consumed, and the sizes are same, we can copy the contents to the destination buffer
If one of the inner buffers is only consumed, and the sizes are different, the operation is invalid, and an exception is thrown.

For me it seems we are good with these rules, added a plenty of tests to ensure they work.

tocsoft · 2018-06-11T19:13:56Z

src/ImageSharp/Advanced/AdvancedImageExtensions.cs

            where TPixel : struct, IPixel<TPixel>
-         => ref DangerousGetPinnableReferenceToPixelBuffer((IPixelSource<TPixel>)source);
+        {
+            return source.PixelBuffer.Buffer.Memory;


we shouldn't expose the buffer as a Memory as we then can't swap the buffer and return the memory to the Array pool other wise someone could have a reference to a piece of memory that has been returned to someone else or was owned by an external process and it could leak dangerous things

The image itself should be treated as Memory of sorts but can never share the hand over directly indefinitely.

@tocsoft the same could be true for Span<T>:

Span<int> testSpan = default; using (IBuffer<T> buffer = allocator.Allocate<int>(10)) { // span is assigned, we are on the same method stack in the using block: testSpan = buffer.GetSpan(); } using (IBuffer<T> somethingElse= allocator.Allocate<int>(10)) { somethingElse.GetSpan()[5] = 666; } // we are out of the intended scope here: int naiveUsersValue = testSpan[5];

Despite the dangers I think both image.GetPixelMemory() and image.GetPixelRowMemory(y) are useful methods for advanced users, because they allow heaping pixel rows, so users can use them in async API-s etc.

What we can do is:

Document these pitfals

Introduce a new level of indirection: Decouple IBuffer<T> from MemoryManager<T>, so we can transparently swap the buffer behind the Memory<T> instance.

Implementing point 2. would take many hours, delaying the open-up of memory API-s further. We can actually do it later, as an additional increment.

We have no one actively after Memory<T> when Span<T> as having a reference to the image is right next to it.

I vote we remove it from the API until we can expose it safely or at least its requested. Until then its an API we have to support which very easily (unlike span which is much harder and can only leak within a single stack) can leak the contents of memory at any random point in the future of the whole application.

I think its too much risk with near to zero benefits that can be handled by keeping a reference to the Image.

@tocsoft you can't do the following with span:

async Task DoOperationOnPixelsAsync(Span<Rgba32> pixels) { await .....; await ........; }

One of our users (on gitter) walked into this pitfall resulting in a runtime failure, and he blamed ImageSharp for it. (This is fine, we can't expect everyone to understand stack-only types.) We decided to hide the Span<T> API-s because of this story. There was a quite valid reasoning, that we should expose only the Memory<T> variant.

I say we should expose either both the Span and the Memory API-s or neither of them. If we can't agree, I say let's internalize everything for now, turning this PR into a refactor one.

@JimBobSquarePants your thoughts?

Actually ... you don't even need to have a goal of implementing something in a separate async operation for let's say perf. If you are implementing an ASP.NET controller, being in an async context most of the time, it's very easy to walk into a pitfall of this kind.

Span<T> is now hidden away in the advanced namespace so we are much less likely to get people confused about using Span with async etc.

you won't be doing pixel level operations over spans in a controller in MVC.. they will be advised to create a Processor and do there stuff there (MVC sorted)

if you changes Span<T> in DoOperationOnPixelsAsync with Image<T> you can now work async and only have a manor (aggressively in lines called over to get the Span<T> as required)

If you are using the API's that expose Span<T> we are deeming the user knowledgeable enough about the framework to handle/understand how async and Spans don't mix well.

If we don't expose Span<T> then users can't write new IImageProcessors they can do that without Memory<T> as that's exactly what we are doing internally.

@mellinoe sorry for pulling you in, but I think your opinion might be very useful here!
@iamcarbon - same for you :)

TLDR:

Exposing only Span<TPixel> API-s is dangerous, because it may lead to heap captures for users who don't follow (or don't know about) coding patterns we suggest.

Exposing Memory<TPixel> API-s is dangerous, because the memory may point to an "outdated" location after certain processors being executed. (Unless we spend many extra hours of coding.)

@tocsoft if no one disagrees, I'm being tempted to accept your reasoning.

Exposing Memory<T> could be an incremental addition later.

antonfirsov · 2018-06-16T16:59:30Z

@tocsoft made GetPixelMemory() and GetPixelRowMemory(y) internal for now. I plan to do the IBuffer<T> ↔️ MemoryManager<T> separation in a separate PR (very soon).

Gonna merge this, if you are fine with the current status.

tocsoft · 2018-06-16T19:34:51Z

I can't see anything I'm not happy with now. 👍

Introduce basic Memory<T> API-s

antonfirsov added 12 commits June 8, 2018 00:43

Change IBuffer<T>.Span to IBuffer<T>.GetSpan() to match System.Buffer…

aaba1a1

…s.MemoryManager<T> API

System.Buffers.MemoryManager<T> is adapted

8c45946

introduce .GetPixelMemory(), get rid of BufferExtensions.GetMemory()

8457372

normalize IBuffer2D<T> API

de717cf

GetPixelMemory() tests

141d39c

GetPixelRowMemory(), GetPixelRowSpan()

e33a167

refactor SixLabors.ImageSharp.Primitives.Region

d71ba3d

fix ShapeRegionTests

cac9d10

refactor FillRegionProcessor, drop MemoryManager.AllocateFake

a8c9893

Image.WrapMemory works

441f036

RENAME: MemoryManager -> MemoryAllocator

86e6f86

rename namespace: SixLabors.ImageSharp.Memory -> SixLabors.Memory

e417ea2

antonfirsov requested review from JimBobSquarePants, tocsoft and dlemstra June 10, 2018 01:24

antonfirsov added 3 commits June 10, 2018 03:25

Merge remote-tracking branch 'origin/master' into af/memory-bridge

8c8fd32

mark DangerousGetPinnableReferenceToPixelBuffer() obsolete

b6d86d8

fix Benchmarks project

8af0df9

JimBobSquarePants approved these changes Jun 10, 2018

View reviewed changes

tocsoft requested changes Jun 10, 2018

View reviewed changes

antonfirsov added 2 commits June 11, 2018 01:09

IBuffer<T>.IsMemoryOwner, Buffer2D<T>.SwapOrCopyContent()

9ab51ff

CloningImageProcessor uses SwapOrCopyPixelsBuffersFrom()

c87726e

tocsoft requested changes Jun 11, 2018

View reviewed changes

JimBobSquarePants added the area:metadata label Jun 12, 2018

hide GetPixelMemory() and GetPixelRowMemory() for now

4ae524d

tocsoft approved these changes Jun 16, 2018

View reviewed changes

antonfirsov merged commit 7005488 into master Jun 16, 2018

antonfirsov mentioned this pull request Jun 16, 2018

System.Memory API integration #565

Closed

7 tasks

antonfirsov added a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#607 from SixLabors/af/memory-bridge

cb76f44

Introduce basic Memory<T> API-s

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce basic Memory<T> API-s #607

Introduce basic Memory<T> API-s #607

antonfirsov commented Jun 10, 2018 •

edited

Loading

JimBobSquarePants left a comment

JimBobSquarePants Jun 10, 2018

JimBobSquarePants Jun 10, 2018

tocsoft left a comment •

edited

Loading

antonfirsov commented Jun 10, 2018

codecov bot commented Jun 10, 2018 •

edited

Loading

antonfirsov commented Jun 10, 2018 •

edited

Loading

tocsoft Jun 11, 2018

tocsoft Jun 11, 2018

antonfirsov Jun 11, 2018 •

edited

Loading

tocsoft Jun 11, 2018 •

edited

Loading

antonfirsov Jun 11, 2018 •

edited

Loading

antonfirsov Jun 11, 2018

tocsoft Jun 11, 2018

antonfirsov Jun 11, 2018 •

edited

Loading

antonfirsov Jun 11, 2018

antonfirsov commented Jun 16, 2018 •

edited

Loading

tocsoft commented Jun 16, 2018

Introduce basic Memory<T> API-s #607

Introduce basic Memory<T> API-s #607

Conversation

antonfirsov commented Jun 10, 2018 • edited Loading

Prerequisites

Description

Features

Internal Refactors

Breaking Changes

JimBobSquarePants left a comment

Choose a reason for hiding this comment

JimBobSquarePants Jun 10, 2018

Choose a reason for hiding this comment

JimBobSquarePants Jun 10, 2018

Choose a reason for hiding this comment

tocsoft left a comment • edited Loading

Choose a reason for hiding this comment

antonfirsov commented Jun 10, 2018

codecov bot commented Jun 10, 2018 • edited Loading

Codecov Report

antonfirsov commented Jun 10, 2018 • edited Loading

tocsoft Jun 11, 2018

Choose a reason for hiding this comment

tocsoft Jun 11, 2018

Choose a reason for hiding this comment

antonfirsov Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

tocsoft Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

antonfirsov Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

antonfirsov Jun 11, 2018

Choose a reason for hiding this comment

tocsoft Jun 11, 2018

Choose a reason for hiding this comment

antonfirsov Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

antonfirsov Jun 11, 2018

Choose a reason for hiding this comment

antonfirsov commented Jun 16, 2018 • edited Loading

tocsoft commented Jun 16, 2018

antonfirsov commented Jun 10, 2018 •

edited

Loading

tocsoft left a comment •

edited

Loading

codecov bot commented Jun 10, 2018 •

edited

Loading

antonfirsov commented Jun 10, 2018 •

edited

Loading

antonfirsov Jun 11, 2018 •

edited

Loading

tocsoft Jun 11, 2018 •

edited

Loading

antonfirsov Jun 11, 2018 •

edited

Loading

antonfirsov Jun 11, 2018 •

edited

Loading

antonfirsov commented Jun 16, 2018 •

edited

Loading