From 630166211c7c3d1b6b56f250bf198cc967d3e42b Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 4 Nov 2024 10:12:11 +1000 Subject: [PATCH] Feedback updates and massively expand write tests --- src/ImageSharp/IO/ChunkedMemoryStream.cs | 148 ++++++------------ .../IO/ChunkedMemoryStreamTests.cs | 88 ++++++++--- 2 files changed, 119 insertions(+), 117 deletions(-) diff --git a/src/ImageSharp/IO/ChunkedMemoryStream.cs b/src/ImageSharp/IO/ChunkedMemoryStream.cs index 59c42ec387..53de2c3cb5 100644 --- a/src/ImageSharp/IO/ChunkedMemoryStream.cs +++ b/src/ImageSharp/IO/ChunkedMemoryStream.cs @@ -2,7 +2,6 @@ // Licensed under the Six Labors Split License. using System.Buffers; -using System.Collections; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using SixLabors.ImageSharp.Memory; @@ -14,15 +13,13 @@ namespace SixLabors.ImageSharp.IO; /// Chunks are allocated by the assigned via the constructor /// and is designed to take advantage of buffer pooling when available. /// -public class ChunkedMemoryStream : Stream +internal sealed class ChunkedMemoryStream : Stream { private readonly MemoryChunkBuffer memoryChunkBuffer; - private readonly byte[] singleByteBuffer = new byte[1]; - private long length; private long position; - private int currentChunk; - private int currentChunkIndex; + private int bufferIndex; + private int chunkIndex; private bool isDisposed; /// @@ -95,21 +92,13 @@ public override void SetLength(long value) /// public override int ReadByte() { - this.EnsureNotDisposed(); - if (this.position >= this.length) - { - return -1; - } - - _ = this.Read(this.singleByteBuffer, 0, 1); - return MemoryMarshal.GetReference(this.singleByteBuffer); + Unsafe.SkipInit(out byte b); + return this.Read(MemoryMarshal.CreateSpan(ref b, 1)) == 1 ? b : -1; } /// public override int Read(byte[] buffer, int offset, int count) { - this.EnsureNotDisposed(); - Guard.NotNull(buffer, nameof(buffer)); Guard.MustBeGreaterThanOrEqualTo(offset, 0, nameof(offset)); Guard.MustBeGreaterThanOrEqualTo(count, 0, nameof(count)); @@ -135,19 +124,14 @@ public override int Read(Span buffer) return 0; } - if (remaining > count) - { - remaining = count; - } - - int bytesToRead = (int)remaining; + int bytesToRead = count; int bytesRead = 0; - while (bytesToRead != 0 && this.currentChunk != this.memoryChunkBuffer.Length) + while (bytesToRead > 0 && this.bufferIndex != this.memoryChunkBuffer.Length) { bool moveToNextChunk = false; - MemoryChunk chunk = this.memoryChunkBuffer[this.currentChunk]; + MemoryChunk chunk = this.memoryChunkBuffer[this.bufferIndex]; int n = bytesToRead; - int remainingBytesInCurrentChunk = chunk.Length - this.currentChunkIndex; + int remainingBytesInCurrentChunk = chunk.Length - this.chunkIndex; if (n >= remainingBytesInCurrentChunk) { n = remainingBytesInCurrentChunk; @@ -155,19 +139,19 @@ public override int Read(Span buffer) } // Read n bytes from the current chunk - chunk.Buffer.Memory.Span.Slice(this.currentChunkIndex, n).CopyTo(buffer.Slice(offset, n)); + chunk.Buffer.Memory.Span.Slice(this.chunkIndex, n).CopyTo(buffer.Slice(offset, n)); bytesToRead -= n; offset += n; bytesRead += n; if (moveToNextChunk) { - this.currentChunkIndex = 0; - this.currentChunk++; + this.chunkIndex = 0; + this.bufferIndex++; } else { - this.currentChunkIndex += n; + this.chunkIndex += n; } } @@ -177,11 +161,7 @@ public override int Read(Span buffer) /// public override void WriteByte(byte value) - { - this.EnsureNotDisposed(); - MemoryMarshal.Write(this.singleByteBuffer, ref value); - this.Write(this.singleByteBuffer, 0, 1); - } + => this.Write(MemoryMarshal.CreateSpan(ref value, 1)); /// public override void Write(byte[] buffer, int offset, int count) @@ -213,19 +193,14 @@ public override void Write(ReadOnlySpan buffer) remaining = this.memoryChunkBuffer.Length - this.position; } - if (remaining > count) - { - remaining = count; - } - - int bytesToWrite = (int)remaining; + int bytesToWrite = count; int bytesWritten = 0; - while (bytesToWrite != 0 && this.currentChunk != this.memoryChunkBuffer.Length) + while (bytesToWrite > 0 && this.bufferIndex != this.memoryChunkBuffer.Length) { bool moveToNextChunk = false; - MemoryChunk chunk = this.memoryChunkBuffer[this.currentChunk]; + MemoryChunk chunk = this.memoryChunkBuffer[this.bufferIndex]; int n = bytesToWrite; - int remainingBytesInCurrentChunk = chunk.Length - this.currentChunkIndex; + int remainingBytesInCurrentChunk = chunk.Length - this.chunkIndex; if (n >= remainingBytesInCurrentChunk) { n = remainingBytesInCurrentChunk; @@ -233,19 +208,19 @@ public override void Write(ReadOnlySpan buffer) } // Write n bytes to the current chunk - buffer.Slice(offset, n).CopyTo(chunk.Buffer.Slice(this.currentChunkIndex, n)); + buffer.Slice(offset, n).CopyTo(chunk.Buffer.Slice(this.chunkIndex, n)); bytesToWrite -= n; offset += n; bytesWritten += n; if (moveToNextChunk) { - this.currentChunkIndex = 0; - this.currentChunk++; + this.chunkIndex = 0; + this.bufferIndex++; } else { - this.currentChunkIndex += n; + this.chunkIndex += n; } } @@ -275,12 +250,12 @@ public void WriteTo(Stream stream) int bytesToRead = (int)remaining; int bytesRead = 0; - while (bytesToRead != 0 && this.currentChunk != this.memoryChunkBuffer.Length) + while (bytesToRead > 0 && this.bufferIndex != this.memoryChunkBuffer.Length) { bool moveToNextChunk = false; - MemoryChunk chunk = this.memoryChunkBuffer[this.currentChunk]; + MemoryChunk chunk = this.memoryChunkBuffer[this.bufferIndex]; int n = bytesToRead; - int remainingBytesInCurrentChunk = chunk.Length - this.currentChunkIndex; + int remainingBytesInCurrentChunk = chunk.Length - this.chunkIndex; if (n >= remainingBytesInCurrentChunk) { n = remainingBytesInCurrentChunk; @@ -288,18 +263,18 @@ public void WriteTo(Stream stream) } // Read n bytes from the current chunk - stream.Write(chunk.Buffer.Memory.Span.Slice(this.currentChunkIndex, n)); + stream.Write(chunk.Buffer.Memory.Span.Slice(this.chunkIndex, n)); bytesToRead -= n; bytesRead += n; if (moveToNextChunk) { - this.currentChunkIndex = 0; - this.currentChunk++; + this.chunkIndex = 0; + this.bufferIndex++; } else { - this.currentChunkIndex += n; + this.chunkIndex += n; } } @@ -338,8 +313,8 @@ protected override void Dispose(bool disposing) this.memoryChunkBuffer.Dispose(); } - this.currentChunk = 0; - this.currentChunkIndex = 0; + this.bufferIndex = 0; + this.chunkIndex = 0; this.position = 0; this.length = 0; } @@ -366,8 +341,8 @@ private void SetPosition(long value) // If the new position is greater than the length of the stream, set the position to the end of the stream if (offset > 0 && offset >= this.memoryChunkBuffer.Length) { - this.currentChunk = this.memoryChunkBuffer.ChunkCount - 1; - this.currentChunkIndex = this.memoryChunkBuffer[this.currentChunk].Length - 1; + this.bufferIndex = this.memoryChunkBuffer.ChunkCount - 1; + this.chunkIndex = this.memoryChunkBuffer[this.bufferIndex].Length - 1; return; } @@ -386,10 +361,10 @@ private void SetPosition(long value) currentChunkIndex++; } - this.currentChunk = currentChunkIndex; + this.bufferIndex = currentChunkIndex; // Safe to cast here as we know the offset is less than the chunk length. - this.currentChunkIndex = (int)offset; + this.chunkIndex = (int)offset; } [MethodImpl(MethodImplOptions.AggressiveInlining)] @@ -404,7 +379,7 @@ private void EnsureNotDisposed() [MethodImpl(MethodImplOptions.NoInlining)] private static void ThrowDisposed() => throw new ObjectDisposedException(nameof(ChunkedMemoryStream), "The stream is closed."); - private sealed class MemoryChunkBuffer : IEnumerable, IDisposable + private sealed class MemoryChunkBuffer : IDisposable { private readonly List memoryChunks = new(); private readonly MemoryAllocator allocator; @@ -439,15 +414,19 @@ public void Expand() public void Dispose() { - this.Dispose(true); - GC.SuppressFinalize(this); - } + if (!this.isDisposed) + { + foreach (MemoryChunk chunk in this.memoryChunks) + { + chunk.Dispose(); + } - public IEnumerator GetEnumerator() - => ((IEnumerable)this.memoryChunks).GetEnumerator(); + this.memoryChunks.Clear(); - IEnumerator IEnumerable.GetEnumerator() - => ((IEnumerable)this.memoryChunks).GetEnumerator(); + this.Length = 0; + this.isDisposed = true; + } + } [MethodImpl(MethodImplOptions.AggressiveInlining)] private static int GetChunkSize(int i) @@ -459,25 +438,6 @@ private static int GetChunkSize(int i) const int b4M = 1 << 22; return i < 16 ? b128K * (1 << (int)((uint)i / 4)) : b4M; } - - private void Dispose(bool disposing) - { - if (!this.isDisposed) - { - if (disposing) - { - foreach (MemoryChunk chunk in this.memoryChunks) - { - chunk.Dispose(); - } - - this.memoryChunks.Clear(); - } - - this.Length = 0; - this.isDisposed = true; - } - } } private sealed class MemoryChunk : IDisposable @@ -490,23 +450,13 @@ private sealed class MemoryChunk : IDisposable public int Length { get; init; } - private void Dispose(bool disposing) + public void Dispose() { if (!this.isDisposed) { - if (disposing) - { - this.Buffer.Dispose(); - } - + this.Buffer.Dispose(); this.isDisposed = true; } } - - public void Dispose() - { - this.Dispose(disposing: true); - GC.SuppressFinalize(this); - } } } diff --git a/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs b/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs index 8d7ea9a33e..b1bb7a9f5a 100644 --- a/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs +++ b/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs @@ -13,6 +13,8 @@ namespace SixLabors.ImageSharp.Tests.IO; /// public class ChunkedMemoryStreamTests { + private readonly Random bufferFiller = new(); + /// /// The default length in bytes of each buffer chunk when allocating large buffers. /// @@ -63,7 +65,7 @@ public void MemoryStream_ReadTest_Negative() [InlineData(DefaultSmallChunkSize * 16)] public void MemoryStream_ReadByteTest(int length) { - using MemoryStream ms = CreateTestStream(length); + using MemoryStream ms = this.CreateTestStream(length); using ChunkedMemoryStream cms = new(this.allocator); ms.CopyTo(cms); @@ -84,7 +86,7 @@ public void MemoryStream_ReadByteTest(int length) [InlineData(DefaultSmallChunkSize * 16)] public void MemoryStream_ReadByteBufferTest(int length) { - using MemoryStream ms = CreateTestStream(length); + using MemoryStream ms = this.CreateTestStream(length); using ChunkedMemoryStream cms = new(this.allocator); ms.CopyTo(cms); @@ -105,9 +107,10 @@ public void MemoryStream_ReadByteBufferTest(int length) [InlineData(DefaultSmallChunkSize * 4)] [InlineData((int)(DefaultSmallChunkSize * 5.5))] [InlineData(DefaultSmallChunkSize * 16)] + [InlineData(DefaultSmallChunkSize * 32)] public void MemoryStream_ReadByteBufferSpanTest(int length) { - using MemoryStream ms = CreateTestStream(length); + using MemoryStream ms = this.CreateTestStream(length); using ChunkedMemoryStream cms = new(this.allocator); ms.CopyTo(cms); @@ -122,13 +125,19 @@ public void MemoryStream_ReadByteBufferSpanTest(int length) } } - [Fact] - public void MemoryStream_WriteToTests() + [Theory] + [InlineData(DefaultSmallChunkSize)] + [InlineData((int)(DefaultSmallChunkSize * 1.5))] + [InlineData(DefaultSmallChunkSize * 4)] + [InlineData((int)(DefaultSmallChunkSize * 5.5))] + [InlineData(DefaultSmallChunkSize * 16)] + [InlineData(DefaultSmallChunkSize * 32)] + public void MemoryStream_WriteToTests(int length) { using (ChunkedMemoryStream ms2 = new(this.allocator)) { byte[] bytArrRet; - byte[] bytArr = new byte[] { byte.MinValue, byte.MaxValue, 1, 2, 3, 4, 5, 6, 128, 250 }; + byte[] bytArr = this.CreateTestBuffer(length); // [] Write to memoryStream, check the memoryStream ms2.Write(bytArr, 0, bytArr.Length); @@ -150,7 +159,7 @@ public void MemoryStream_WriteToTests() using (ChunkedMemoryStream ms3 = new(this.allocator)) { byte[] bytArrRet; - byte[] bytArr = new byte[] { byte.MinValue, byte.MaxValue, 1, 2, 3, 4, 5, 6, 128, 250 }; + byte[] bytArr = this.CreateTestBuffer(length); ms2.Write(bytArr, 0, bytArr.Length); ms2.WriteTo(ms3); @@ -164,13 +173,19 @@ public void MemoryStream_WriteToTests() } } - [Fact] - public void MemoryStream_WriteToSpanTests() + [Theory] + [InlineData(DefaultSmallChunkSize)] + [InlineData((int)(DefaultSmallChunkSize * 1.5))] + [InlineData(DefaultSmallChunkSize * 4)] + [InlineData((int)(DefaultSmallChunkSize * 5.5))] + [InlineData(DefaultSmallChunkSize * 16)] + [InlineData(DefaultSmallChunkSize * 32)] + public void MemoryStream_WriteToSpanTests(int length) { using (ChunkedMemoryStream ms2 = new(this.allocator)) { Span bytArrRet; - Span bytArr = new byte[] { byte.MinValue, byte.MaxValue, 1, 2, 3, 4, 5, 6, 128, 250 }; + Span bytArr = this.CreateTestBuffer(length); // [] Write to memoryStream, check the memoryStream ms2.Write(bytArr, 0, bytArr.Length); @@ -194,7 +209,7 @@ public void MemoryStream_WriteToSpanTests() using (ChunkedMemoryStream ms3 = new(this.allocator)) { Span bytArrRet; - Span bytArr = new byte[] { byte.MinValue, byte.MaxValue, 1, 2, 3, 4, 5, 6, 128, 250 }; + Span bytArr = this.CreateTestBuffer(length); ms2.Write(bytArr, 0, bytArr.Length); @@ -307,7 +322,7 @@ public static IEnumerable GetAllTestImages() return result; } - public static IEnumerable AllTestImages = GetAllTestImages(); + public static IEnumerable AllTestImages { get; } = GetAllTestImages(); [Theory] [WithFileCollection(nameof(AllTestImages), PixelTypes.Rgba32)] @@ -337,9 +352,45 @@ public void DecoderIntegrationTest(TestImageProvider provider) using FileStream fs = File.OpenRead(fullPath); using NonSeekableStream nonSeekableStream = new(fs); - Image actual = Image.Load(nonSeekableStream); + using Image actual = Image.Load(nonSeekableStream); + + ImageComparer.Exact.VerifySimilarity(expected, actual); + expected.Dispose(); + } + + [Theory] + [WithFileCollection(nameof(AllTestImages), PixelTypes.Rgba32)] + public void EncoderIntegrationTest(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + if (!TestEnvironment.Is64BitProcess) + { + return; + } + + Image expected; + try + { + expected = provider.GetImage(); + } + catch + { + // The image is invalid + return; + } + + string fullPath = Path.Combine( + TestEnvironment.InputImagesDirectoryFullPath, + ((TestImageProvider.FileProvider)provider).FilePath); + + using MemoryStream ms = new(); + using NonSeekableStream nonSeekableStream = new(ms); + expected.SaveAsWebp(nonSeekableStream); + + using Image actual = Image.Load(nonSeekableStream); ImageComparer.Exact.VerifySimilarity(expected, actual); + expected.Dispose(); } public static IEnumerable CopyToData() @@ -363,12 +414,13 @@ public static IEnumerable CopyToData() yield return new object[] { stream3, Array.Empty() }; } - private static MemoryStream CreateTestStream(int length) + private byte[] CreateTestBuffer(int length) { byte[] buffer = new byte[length]; - Random random = new(); - random.NextBytes(buffer); - - return new MemoryStream(buffer); + this.bufferFiller.NextBytes(buffer); + return buffer; } + + private MemoryStream CreateTestStream(int length) + => new(this.CreateTestBuffer(length)); }