From 334f16bf7ca2c3d856ca7763fe7b25fe882ebf3e Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Sat, 8 Feb 2020 03:25:28 +0100 Subject: [PATCH] fix bug found by DetectEdges_WorksOnWrappedMemoryImage --- .../ArrayPoolMemoryAllocator.Buffer{T}.cs | 10 ++++++- src/ImageSharp/Memory/Buffer2D{T}.cs | 15 ++++++---- .../DiscontiguousBuffers/MemoryGroup{T}.cs | 4 ++- .../ImageSharp.Tests/Memory/Buffer2DTests.cs | 29 ++++++++++++++++++- .../MemoryGroupTests.SwapOrCopyContent.cs | 6 ++-- 5 files changed, 53 insertions(+), 11 deletions(-) diff --git a/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.Buffer{T}.cs b/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.Buffer{T}.cs index 0d7e0b784..7a8b4f8bd 100644 --- a/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.Buffer{T}.cs +++ b/src/ImageSharp/Memory/Allocators/ArrayPoolMemoryAllocator.Buffer{T}.cs @@ -46,7 +46,15 @@ namespace SixLabors.ImageSharp.Memory protected byte[] Data { get; private set; } /// - public override Span GetSpan() => MemoryMarshal.Cast(this.Data.AsSpan()).Slice(0, this.length); + public override Span GetSpan() + { + if (this.Data == null) + { + throw new ObjectDisposedException("ArrayPoolMemoryAllocator.Buffer"); + } + + return MemoryMarshal.Cast(this.Data.AsSpan()).Slice(0, this.length); + } /// protected override void Dispose(bool disposing) diff --git a/src/ImageSharp/Memory/Buffer2D{T}.cs b/src/ImageSharp/Memory/Buffer2D{T}.cs index 29e22767f..ef9898ad3 100644 --- a/src/ImageSharp/Memory/Buffer2D{T}.cs +++ b/src/ImageSharp/Memory/Buffer2D{T}.cs @@ -149,14 +149,14 @@ namespace SixLabors.ImageSharp.Memory /// internal static void SwapOrCopyContent(Buffer2D destination, Buffer2D source) { - MemoryGroup.SwapOrCopyContent(destination.MemoryGroup, source.MemoryGroup); - SwapOwnData(destination, source); + bool swap = MemoryGroup.SwapOrCopyContent(destination.MemoryGroup, source.MemoryGroup); + SwapOwnData(destination, source, swap); } [MethodImpl(InliningOptions.ColdPath)] private Memory GetRowMemorySlow(int y) => this.MemoryGroup.GetBoundedSlice(y * this.Width, this.Width); - private static void SwapOwnData(Buffer2D a, Buffer2D b) + private static void SwapOwnData(Buffer2D a, Buffer2D b, bool swapCachedMemory) { Size aSize = a.Size(); Size bSize = b.Size(); @@ -167,9 +167,12 @@ namespace SixLabors.ImageSharp.Memory a.Width = bSize.Width; a.Height = bSize.Height; - Memory aCached = a.cachedMemory; - a.cachedMemory = b.cachedMemory; - b.cachedMemory = aCached; + if (swapCachedMemory) + { + Memory aCached = a.cachedMemory; + a.cachedMemory = b.cachedMemory; + b.cachedMemory = aCached; + } } } } diff --git a/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.cs b/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.cs index 00bd27b34..497f0a354 100644 --- a/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.cs +++ b/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.cs @@ -166,12 +166,13 @@ namespace SixLabors.ImageSharp.Memory /// copies the contents of 'source' to 'target' otherwise (2). /// Groups should be of same TotalLength in case 2. /// - public static void SwapOrCopyContent(MemoryGroup target, MemoryGroup source) + public static bool SwapOrCopyContent(MemoryGroup target, MemoryGroup source) { if (source is Owned ownedSrc && ownedSrc.Swappable && target is Owned ownedTarget && ownedTarget.Swappable) { Owned.SwapContents(ownedTarget, ownedSrc); + return true; } else { @@ -182,6 +183,7 @@ namespace SixLabors.ImageSharp.Memory } source.CopyTo(target); + return false; } } } diff --git a/tests/ImageSharp.Tests/Memory/Buffer2DTests.cs b/tests/ImageSharp.Tests/Memory/Buffer2DTests.cs index b44e4c903..36adf9856 100644 --- a/tests/ImageSharp.Tests/Memory/Buffer2DTests.cs +++ b/tests/ImageSharp.Tests/Memory/Buffer2DTests.cs @@ -130,7 +130,7 @@ namespace SixLabors.ImageSharp.Tests.Memory } [Fact] - public void SwapOrCopyContent() + public void SwapOrCopyContent_WhenBothAllocated() { using (Buffer2D a = this.MemoryAllocator.Allocate2D(10, 5, AllocationOptions.Clean)) using (Buffer2D b = this.MemoryAllocator.Allocate2D(3, 7, AllocationOptions.Clean)) @@ -154,6 +154,33 @@ namespace SixLabors.ImageSharp.Tests.Memory } } + [Fact] + public void SwapOrCopyContent_WhenDestinationIsOwned_ShouldNotSwapInDisposedSourceBuffer() + { + using var destData = MemoryGroup.Wrap(new int[100]); + using var dest = new Buffer2D(destData, 10, 10); + + using (Buffer2D source = this.MemoryAllocator.Allocate2D(10, 10, AllocationOptions.Clean)) + { + source[0, 0] = 1; + dest[0, 0] = 2; + + Buffer2D.SwapOrCopyContent(dest, source); + } + + int actual1 = dest.GetRowSpanUnchecked(0)[0]; + int actual2 = dest.GetRowSpan(0)[0]; + int actual3 = dest.GetRowMemorySafe(0).Span[0]; + int actual4 = dest.GetRowMemoryFast(0).Span[0]; + int actual5 = dest[0, 0]; + + Assert.Equal(1, actual1); + Assert.Equal(1, actual2); + Assert.Equal(1, actual3); + Assert.Equal(1, actual4); + Assert.Equal(1, actual5); + } + [Theory] [InlineData(100, 20, 0, 90, 10)] [InlineData(100, 3, 0, 50, 50)] diff --git a/tests/ImageSharp.Tests/Memory/DiscontiguousBuffers/MemoryGroupTests.SwapOrCopyContent.cs b/tests/ImageSharp.Tests/Memory/DiscontiguousBuffers/MemoryGroupTests.SwapOrCopyContent.cs index b3a522cc6..c10fdc15d 100644 --- a/tests/ImageSharp.Tests/Memory/DiscontiguousBuffers/MemoryGroupTests.SwapOrCopyContent.cs +++ b/tests/ImageSharp.Tests/Memory/DiscontiguousBuffers/MemoryGroupTests.SwapOrCopyContent.cs @@ -24,8 +24,9 @@ namespace SixLabors.ImageSharp.Tests.Memory.DiscontiguousBuffers Memory b0 = b[0]; Memory b1 = b[1]; - MemoryGroup.SwapOrCopyContent(a, b); + bool swap = MemoryGroup.SwapOrCopyContent(a, b); + Assert.True(swap); Assert.Equal(b0, a[0]); Assert.Equal(b1, a[1]); Assert.Equal(a0, b[0]); @@ -72,9 +73,10 @@ namespace SixLabors.ImageSharp.Tests.Memory.DiscontiguousBuffers source[0].Span[10] = color; // Act: - MemoryGroup.SwapOrCopyContent(dest, source); + bool swap = MemoryGroup.SwapOrCopyContent(dest, source); // Assert: + Assert.False(swap); Assert.Equal(color, dest[0].Span[10]); Assert.NotEqual(source[0], dest[0]); }