-
-
Notifications
You must be signed in to change notification settings - Fork 855
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
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
3984e80
Add ByteMemoryManager<T> type
Sergio0694 b3f8572
Add Image.WrapMemory<TPixel> from Memory<byte>
Sergio0694 e26e57f
Add tests for new Image.WrapMemory APIs
Sergio0694 32380d2
Remove unnecessary indirection in ByteMemoryManager<T>
Sergio0694 eb22996
Fix bug in Pin() method, minor code tweaks
Sergio0694 2383245
Fix relative offsetting in Pin() methods
Sergio0694 85f91f6
Fix comparison in wrapping bytes test
Sergio0694 ff5e301
Replace Default.Clone with CreateDefaultConfiguration
Sergio0694 fa59a73
Add input size validation in Image.WrapMemory
Sergio0694 1de45b7
Minor optimizations, improve XML docs and annotations
Sergio0694 567f3a6
Remove unnecessary Memory<T>.Span access
Sergio0694 9295e93
Merge branch 'master' into sp/byte-to-tpixel-wrapping
Sergio0694 d7380f0
Simplify XML docs for WrapMemory APIs
Sergio0694 d861805
Remove [Pure] attributes
Sergio0694 26b948d
Merge branch 'master' into sp/byte-to-tpixel-wrapping
JimBobSquarePants File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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() | ||
{ | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
.