Merge pull request #2249 from SixLabors/js/buffered-read-fix

Fix position handling in BufferedReadStream

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<byte>.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.

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<int> BufferSizes =
-        new TheoryData<int>()
+        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<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;
-            Span<byte> 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<ArgumentOutOfRangeException>(() => reader.Position = -stream.Length);
-            }
-        }
+        using MemoryStream stream = CreateTestStream(bufferSize);
+        using BufferedReadStream reader = new(this.configuration, stream);
+        Assert.Throws<ArgumentOutOfRangeException>(() => 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,46 @@ 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;
+
+        int actual = bufferedStream.ReadByte();
+        Assert.Equal(3, actual);
     }
 
-    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 +357,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);
     }
 }