From b0ba58c07fc8092eeb25e555163d95d5d4ac12e2 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sat, 1 Oct 2022 01:15:50 +1000 Subject: [PATCH] Fix position handling --- src/ImageSharp/IO/BufferedReadStream.cs | 8 +- .../IO/BufferedReadStreamTests.cs | 423 +++++++++--------- 2 files changed, 208 insertions(+), 223 deletions(-) diff --git a/src/ImageSharp/IO/BufferedReadStream.cs b/src/ImageSharp/IO/BufferedReadStream.cs index 7e233c965..1afff2192 100644 --- a/src/ImageSharp/IO/BufferedReadStream.cs +++ b/src/ImageSharp/IO/BufferedReadStream.cs @@ -49,7 +49,7 @@ internal sealed class BufferedReadStream : Stream this.BaseStream = stream; this.Length = stream.Length; - this.Position = (int)stream.Position; + this.readerPosition = stream.Position; this.BufferSize = configuration.StreamProcessingBufferSize; this.maxBufferIndex = this.BufferSize - 1; this.readBuffer = ArrayPool.Shared.Rent(this.BufferSize); @@ -96,9 +96,8 @@ internal sealed class BufferedReadStream : Stream else { // Base stream seek will throw for us if invalid. - this.BaseStream.Seek(value, SeekOrigin.Begin); this.readerPosition = value; - this.readBufferIndex = this.BufferSize; + this.FillReadBuffer(); } } } @@ -147,6 +146,7 @@ internal sealed class BufferedReadStream : Stream } this.readerPosition++; + unsafe { return this.pinnedReadBuffer[this.readBufferIndex++]; @@ -202,7 +202,7 @@ internal sealed class BufferedReadStream : Stream if (this.readerPosition != baseStream.Position) { baseStream.Seek(this.readerPosition, SeekOrigin.Begin); - this.readerPosition = (int)baseStream.Position; + this.readerPosition = baseStream.Position; } // Reset to trigger full read on next attempt. diff --git a/tests/ImageSharp.Tests/IO/BufferedReadStreamTests.cs b/tests/ImageSharp.Tests/IO/BufferedReadStreamTests.cs index 6a9c34486..dab5db1c5 100644 --- a/tests/ImageSharp.Tests/IO/BufferedReadStreamTests.cs +++ b/tests/ImageSharp.Tests/IO/BufferedReadStreamTests.cs @@ -10,12 +10,10 @@ public class BufferedReadStreamTests private readonly Configuration configuration; public BufferedReadStreamTests() - { - this.configuration = Configuration.CreateDefaultInstance(); - } + => this.configuration = Configuration.CreateDefaultInstance(); public static readonly TheoryData BufferSizes = - new TheoryData() + new() { 1, 2, 4, 8, 16, 97, 503, @@ -28,21 +26,19 @@ public class BufferedReadStreamTests public void BufferedStreamCanReadSingleByteFromOrigin(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize * 3)) + using MemoryStream stream = CreateTestStream(bufferSize * 3); + byte[] expected = stream.ToArray(); + using (BufferedReadStream reader = new(this.configuration, stream)) { - byte[] expected = stream.ToArray(); - using (var reader = new BufferedReadStream(this.configuration, stream)) - { - Assert.Equal(expected[0], reader.ReadByte()); - - // We've read a whole chunk but increment by 1 in our reader. - Assert.True(stream.Position >= bufferSize); - Assert.Equal(1, reader.Position); - } + Assert.Equal(expected[0], reader.ReadByte()); - // Position of the stream should be reset on disposal. - Assert.Equal(1, stream.Position); + // We've read a whole chunk but increment by 1 in our reader. + Assert.True(stream.Position >= bufferSize); + Assert.Equal(1, reader.Position); } + + // Position of the stream should be reset on disposal. + Assert.Equal(1, stream.Position); } [Theory] @@ -50,23 +46,21 @@ public class BufferedReadStreamTests public void BufferedStreamCanReadSingleByteFromOffset(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize * 3)) + using MemoryStream stream = CreateTestStream(bufferSize * 3); + byte[] expected = stream.ToArray(); + int offset = expected.Length / 2; + using (BufferedReadStream reader = new(this.configuration, stream)) { - byte[] expected = stream.ToArray(); - int offset = expected.Length / 2; - using (var reader = new BufferedReadStream(this.configuration, stream)) - { - reader.Position = offset; + reader.Position = offset; - Assert.Equal(expected[offset], reader.ReadByte()); - - // We've read a whole chunk but increment by 1 in our reader. - Assert.Equal(bufferSize + offset, stream.Position); - Assert.Equal(offset + 1, reader.Position); - } + Assert.Equal(expected[offset], reader.ReadByte()); - Assert.Equal(offset + 1, stream.Position); + // We've read a whole chunk but increment by 1 in our reader. + Assert.Equal(bufferSize + offset, stream.Position); + Assert.Equal(offset + 1, reader.Position); } + + Assert.Equal(offset + 1, stream.Position); } [Theory] @@ -74,36 +68,34 @@ public class BufferedReadStreamTests public void BufferedStreamCanReadSubsequentSingleByteCorrectly(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize * 3)) + using MemoryStream stream = CreateTestStream(bufferSize * 3); + byte[] expected = stream.ToArray(); + int i; + using (BufferedReadStream reader = new(this.configuration, stream)) { - byte[] expected = stream.ToArray(); - int i; - using (var reader = new BufferedReadStream(this.configuration, stream)) + for (i = 0; i < expected.Length; i++) { - for (i = 0; i < expected.Length; i++) + Assert.Equal(expected[i], reader.ReadByte()); + Assert.Equal(i + 1, reader.Position); + + if (i < bufferSize) { - Assert.Equal(expected[i], reader.ReadByte()); - Assert.Equal(i + 1, reader.Position); - - if (i < bufferSize) - { - Assert.Equal(stream.Position, bufferSize); - } - else if (i >= bufferSize && i < bufferSize * 2) - { - // We should have advanced to the second chunk now. - Assert.Equal(stream.Position, bufferSize * 2); - } - else - { - // We should have advanced to the third chunk now. - Assert.Equal(stream.Position, bufferSize * 3); - } + Assert.Equal(stream.Position, bufferSize); + } + else if (i >= bufferSize && i < bufferSize * 2) + { + // We should have advanced to the second chunk now. + Assert.Equal(stream.Position, bufferSize * 2); + } + else + { + // We should have advanced to the third chunk now. + Assert.Equal(stream.Position, bufferSize * 3); } } - - Assert.Equal(i, stream.Position); } + + Assert.Equal(i, stream.Position); } [Theory] @@ -111,21 +103,17 @@ public class BufferedReadStreamTests public void BufferedStreamCanReadMultipleBytesFromOrigin(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize * 3)) - { - var buffer = new byte[2]; - byte[] expected = stream.ToArray(); - using (var reader = new BufferedReadStream(this.configuration, stream)) - { - Assert.Equal(2, reader.Read(buffer, 0, 2)); - Assert.Equal(expected[0], buffer[0]); - Assert.Equal(expected[1], buffer[1]); - - // We've read a whole chunk but increment by the buffer length in our reader. - Assert.True(stream.Position >= bufferSize); - Assert.Equal(buffer.Length, reader.Position); - } - } + using MemoryStream stream = CreateTestStream(bufferSize * 3); + byte[] buffer = new byte[2]; + byte[] expected = stream.ToArray(); + using BufferedReadStream reader = new(this.configuration, stream); + Assert.Equal(2, reader.Read(buffer, 0, 2)); + Assert.Equal(expected[0], buffer[0]); + Assert.Equal(expected[1], buffer[1]); + + // We've read a whole chunk but increment by the buffer length in our reader. + Assert.True(stream.Position >= bufferSize); + Assert.Equal(buffer.Length, reader.Position); } [Theory] @@ -133,49 +121,45 @@ public class BufferedReadStreamTests public void BufferedStreamCanReadSubsequentMultipleByteCorrectly(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize * 3)) + using MemoryStream stream = CreateTestStream(bufferSize * 3); + const int increment = 2; + byte[] buffer = new byte[2]; + byte[] expected = stream.ToArray(); + using BufferedReadStream reader = new(this.configuration, stream); + for (int i = 0, o = 0; i < expected.Length / increment; i++, o += increment) { - const int increment = 2; - var buffer = new byte[2]; - byte[] expected = stream.ToArray(); - using (var reader = new BufferedReadStream(this.configuration, stream)) + // Check values are correct. + Assert.Equal(increment, reader.Read(buffer, 0, increment)); + Assert.Equal(expected[o], buffer[0]); + Assert.Equal(expected[o + 1], buffer[1]); + Assert.Equal(o + increment, reader.Position); + + // These tests ensure that we are correctly reading + // our buffer in chunks of the given size. + int offset = i * increment; + + // First chunk. + if (offset < bufferSize) { - for (int i = 0, o = 0; i < expected.Length / increment; i++, o += increment) - { - // Check values are correct. - Assert.Equal(increment, reader.Read(buffer, 0, increment)); - Assert.Equal(expected[o], buffer[0]); - Assert.Equal(expected[o + 1], buffer[1]); - Assert.Equal(o + increment, reader.Position); - - // These tests ensure that we are correctly reading - // our buffer in chunks of the given size. - int offset = i * increment; - - // First chunk. - if (offset < bufferSize) - { - // We've read an entire chunk once and are - // now reading from that chunk. - Assert.True(stream.Position >= bufferSize); - continue; - } - - // Second chunk - if (offset < bufferSize * 2) - { - Assert.True(stream.Position > bufferSize); - - // Odd buffer size with even increments can - // jump to the third chunk on final read. - Assert.True(stream.Position <= bufferSize * 3); - continue; - } - - // Third chunk - Assert.True(stream.Position > bufferSize * 2); - } + // We've read an entire chunk once and are + // now reading from that chunk. + Assert.True(stream.Position >= bufferSize); + continue; } + + // Second chunk + if (offset < bufferSize * 2) + { + Assert.True(stream.Position > bufferSize); + + // Odd buffer size with even increments can + // jump to the third chunk on final read. + Assert.True(stream.Position <= bufferSize * 3); + continue; + } + + // Third chunk + Assert.True(stream.Position > bufferSize * 2); } } @@ -184,49 +168,45 @@ public class BufferedReadStreamTests public void BufferedStreamCanReadSubsequentMultipleByteSpanCorrectly(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize * 3)) + using MemoryStream stream = CreateTestStream(bufferSize * 3); + const int increment = 2; + Span buffer = new byte[2]; + byte[] expected = stream.ToArray(); + using BufferedReadStream reader = new(this.configuration, stream); + for (int i = 0, o = 0; i < expected.Length / increment; i++, o += increment) { - const int increment = 2; - Span buffer = new byte[2]; - byte[] expected = stream.ToArray(); - using (var reader = new BufferedReadStream(this.configuration, stream)) + // Check values are correct. + Assert.Equal(increment, reader.Read(buffer, 0, increment)); + Assert.Equal(expected[o], buffer[0]); + Assert.Equal(expected[o + 1], buffer[1]); + Assert.Equal(o + increment, reader.Position); + + // These tests ensure that we are correctly reading + // our buffer in chunks of the given size. + int offset = i * increment; + + // First chunk. + if (offset < bufferSize) { - for (int i = 0, o = 0; i < expected.Length / increment; i++, o += increment) - { - // Check values are correct. - Assert.Equal(increment, reader.Read(buffer, 0, increment)); - Assert.Equal(expected[o], buffer[0]); - Assert.Equal(expected[o + 1], buffer[1]); - Assert.Equal(o + increment, reader.Position); - - // These tests ensure that we are correctly reading - // our buffer in chunks of the given size. - int offset = i * increment; - - // First chunk. - if (offset < bufferSize) - { - // We've read an entire chunk once and are - // now reading from that chunk. - Assert.True(stream.Position >= bufferSize); - continue; - } - - // Second chunk - if (offset < bufferSize * 2) - { - Assert.True(stream.Position > bufferSize); - - // Odd buffer size with even increments can - // jump to the third chunk on final read. - Assert.True(stream.Position <= bufferSize * 3); - continue; - } - - // Third chunk - Assert.True(stream.Position > bufferSize * 2); - } + // We've read an entire chunk once and are + // now reading from that chunk. + Assert.True(stream.Position >= bufferSize); + continue; + } + + // Second chunk + if (offset < bufferSize * 2) + { + Assert.True(stream.Position > bufferSize); + + // Odd buffer size with even increments can + // jump to the third chunk on final read. + Assert.True(stream.Position <= bufferSize * 3); + continue; } + + // Third chunk + Assert.True(stream.Position > bufferSize * 2); } } @@ -235,34 +215,28 @@ public class BufferedReadStreamTests public void BufferedStreamCanSkip(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize * 4)) - { - byte[] expected = stream.ToArray(); - using (var reader = new BufferedReadStream(this.configuration, stream)) - { - int skip = 1; - int plusOne = 1; - int skip2 = bufferSize; + using MemoryStream stream = CreateTestStream(bufferSize * 4); + byte[] expected = stream.ToArray(); + using BufferedReadStream reader = new(this.configuration, stream); + const int skip = 1; + const int plusOne = 1; + int skip2 = bufferSize; - // Skip - reader.Skip(skip); - Assert.Equal(skip, reader.Position); - Assert.Equal(stream.Position, reader.Position); + // Skip + reader.Skip(skip); + Assert.Equal(skip, reader.Position); - // Read - Assert.Equal(expected[skip], reader.ReadByte()); + // Read + Assert.Equal(expected[skip], reader.ReadByte()); - // Skip Again - reader.Skip(skip2); + // Skip Again + reader.Skip(skip2); - // First Skip + First Read + Second Skip - int position = skip + plusOne + skip2; + // First Skip + First Read + Second Skip + int position = skip + plusOne + skip2; - Assert.Equal(position, reader.Position); - Assert.Equal(stream.Position, reader.Position); - Assert.Equal(expected[position], reader.ReadByte()); - } - } + Assert.Equal(position, reader.Position); + Assert.Equal(expected[position], reader.ReadByte()); } [Theory] @@ -272,23 +246,21 @@ public class BufferedReadStreamTests this.configuration.StreamProcessingBufferSize = bufferSize; // Create a stream smaller than the default buffer length - using (MemoryStream stream = this.CreateTestStream(Math.Max(1, bufferSize / 4))) + using MemoryStream stream = CreateTestStream(Math.Max(1, bufferSize / 4)); + byte[] expected = stream.ToArray(); + int offset = expected.Length / 2; + using (BufferedReadStream reader = new(this.configuration, stream)) { - byte[] expected = stream.ToArray(); - int offset = expected.Length / 2; - using (var reader = new BufferedReadStream(this.configuration, stream)) - { - reader.Position = offset; + reader.Position = offset; - Assert.Equal(expected[offset], reader.ReadByte()); + Assert.Equal(expected[offset], reader.ReadByte()); - // We've read a whole length of the stream but increment by 1 in our reader. - Assert.Equal(Math.Max(1, bufferSize / 4), stream.Position); - Assert.Equal(offset + 1, reader.Position); - } - - Assert.Equal(offset + 1, stream.Position); + // We've read a whole length of the stream but increment by 1 in our reader. + Assert.Equal(Math.Max(1, bufferSize / 4), stream.Position); + Assert.Equal(offset + 1, reader.Position); } + + Assert.Equal(offset + 1, stream.Position); } [Theory] @@ -296,16 +268,12 @@ public class BufferedReadStreamTests public void BufferedStreamReadsCanReadAllAsSingleByteFromOrigin(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize * 3)) + using MemoryStream stream = CreateTestStream(bufferSize * 3); + byte[] expected = stream.ToArray(); + using BufferedReadStream reader = new(this.configuration, stream); + for (int i = 0; i < expected.Length; i++) { - byte[] expected = stream.ToArray(); - using (var reader = new BufferedReadStream(this.configuration, stream)) - { - for (int i = 0; i < expected.Length; i++) - { - Assert.Equal(expected[i], reader.ReadByte()); - } - } + Assert.Equal(expected[i], reader.ReadByte()); } } @@ -314,13 +282,9 @@ public class BufferedReadStreamTests public void BufferedStreamThrowsOnNegativePosition(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize)) - { - using (var reader = new BufferedReadStream(this.configuration, stream)) - { - Assert.Throws(() => reader.Position = -stream.Length); - } - } + using MemoryStream stream = CreateTestStream(bufferSize); + using BufferedReadStream reader = new(this.configuration, stream); + Assert.Throws(() => reader.Position = -stream.Length); } [Theory] @@ -328,13 +292,9 @@ public class BufferedReadStreamTests public void BufferedStreamCanSetPositionToEnd(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize * 2)) - { - using (var reader = new BufferedReadStream(this.configuration, stream)) - { - reader.Position = reader.Length; - } - } + using MemoryStream stream = CreateTestStream(bufferSize * 2); + using BufferedReadStream reader = new(this.configuration, stream); + reader.Position = reader.Length; } [Theory] @@ -342,20 +302,47 @@ public class BufferedReadStreamTests public void BufferedStreamCanSetPositionPastTheEnd(int bufferSize) { this.configuration.StreamProcessingBufferSize = bufferSize; - using (MemoryStream stream = this.CreateTestStream(bufferSize * 2)) + using MemoryStream stream = CreateTestStream(bufferSize * 2); + using BufferedReadStream reader = new(this.configuration, stream); + reader.Position = reader.Length + 1; + Assert.Equal(stream.Length + 1, stream.Position); + } + + [Fact] + public void BufferedStreamCanSetPositionMultipleTimes() + { + Configuration configuration = new() { - using (var reader = new BufferedReadStream(this.configuration, stream)) - { - reader.Position = reader.Length + 1; - Assert.Equal(stream.Length + 1, stream.Position); - } + StreamProcessingBufferSize = 16 + }; + + byte[] buffer = new byte[255]; + for (int i = 0; i < buffer.Length; i++) + { + buffer[i] = (byte)i; + } + + BufferedReadStream bufferedStream = new(configuration, new MemoryStream(buffer)); + + // Read more then fits into the buffer. + for (int i = 0; i < 20; i++) + { + bufferedStream.ReadByte(); } + + // Set the Position twice. + bufferedStream.Position = 10; + bufferedStream.Position = 3; + + // readValue is 25, but should be 3 + int readValue = bufferedStream.ReadByte(); + Assert.Equal(3, readValue); } - private MemoryStream CreateTestStream(int length) + private static MemoryStream CreateTestStream(int length) { - var buffer = new byte[length]; - var random = new Random(); + byte[] buffer = new byte[length]; + Random random = new(); random.NextBytes(buffer); return new EvilStream(buffer); @@ -371,8 +358,6 @@ public class BufferedReadStreamTests } public override int Read(byte[] buffer, int offset, int count) - { - return base.Read(buffer, offset, 1); - } + => base.Read(buffer, offset, 1); } }