#414: Fix for BufferedImageInputStream alignment/partial read issues.

Bonus clean-up of tests.

imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/stream/BufferedImageInputStream.java

@@ -2,14 +2,16 @@ package com.twelvemonkeys.imageio.stream;
 
 import javax.imageio.stream.ImageInputStream;
 import javax.imageio.stream.ImageInputStreamImpl;
+import java.io.EOFException;
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
 
 import static com.twelvemonkeys.lang.Validate.notNull;
 
 /**
  * A buffered {@code ImageInputStream}.
- * Experimental - seems to be effective for {@link javax.imageio.stream.FileImageInputStream} 
+ * Experimental - seems to be effective for {@link javax.imageio.stream.FileImageInputStream}
  * and {@link javax.imageio.stream.FileCacheImageInputStream} when doing a lot of single-byte reads
  * (or short byte-array reads) on OS X at least.
  * Code that uses the {@code readFully} methods are not affected by the issue.
@@ -21,11 +23,12 @@ import static com.twelvemonkeys.lang.Validate.notNull;
 // TODO: Create a provider for this (wrapping the FileIIS and FileCacheIIS classes), and disable the Sun built-in spis?
 // TODO: Test on other platforms, might be just an OS X issue
 public final class BufferedImageInputStream extends ImageInputStreamImpl implements ImageInputStream {
-   static final int DEFAULT_BUFFER_SIZE = 8192;
+    static final int DEFAULT_BUFFER_SIZE = 8192;
 
     private ImageInputStream stream;
 
     private ByteBuffer buffer;
+    private ByteBuffer integralCache = ByteBuffer.allocate(8);
 
     public BufferedImageInputStream(final ImageInputStream pStream) throws IOException {
         this(pStream, DEFAULT_BUFFER_SIZE);
@@ -44,13 +47,7 @@ public final class BufferedImageInputStream extends ImageInputStreamImpl impleme
         int length = stream.read(buffer.array(), 0, buffer.capacity());
 
         if (length >= 0) {
-            try {
+            buffer.position(length);
-                buffer.position(length);
-            }
-            catch (IllegalArgumentException e) {
-                System.err.println("length: " + length);
-                throw e;
-            }
             buffer.flip();
         }
         else {
@@ -58,9 +55,16 @@ public final class BufferedImageInputStream extends ImageInputStreamImpl impleme
         }
     }
 
+    @Override
+    public void setByteOrder(ByteOrder byteOrder) {
+        super.setByteOrder(byteOrder);
+        integralCache.order(byteOrder);
+    }
 
     @Override
     public int read() throws IOException {
+        checkClosed();
+
         if (!buffer.hasRemaining()) {
             fillBuffer();
         }
@@ -77,10 +81,11 @@ public final class BufferedImageInputStream extends ImageInputStreamImpl impleme
 
     @Override
     public int read(final byte[] pBuffer, final int pOffset, final int pLength) throws IOException {
+        checkClosed();
         bitOffset = 0;
 
         if (!buffer.hasRemaining()) {
-            // Bypass cache if cache is empty for reads longer than buffer
+            // Bypass buffer if buffer is empty for reads longer than buffer
             if (pLength >= buffer.capacity()) {
                 return readDirect(pBuffer, pOffset, pLength);
             }
@@ -93,8 +98,9 @@ public final class BufferedImageInputStream extends ImageInputStreamImpl impleme
     }
 
     private int readDirect(final byte[] pBuffer, final int pOffset, final int pLength) throws IOException {
-        // TODO: Figure out why reading more than the buffer length causes alignment issues...
+        // Invalidate the buffer, as its contents is no longer in sync with the stream's position.
-        int read = stream.read(pBuffer, pOffset, Math.min(buffer.capacity(), pLength));
+        buffer.limit(0);
+        int read = stream.read(pBuffer, pOffset, pLength);
 
         if (read > 0) {
             streamPos += read;
@@ -103,7 +109,6 @@ public final class BufferedImageInputStream extends ImageInputStreamImpl impleme
         return read;
     }
 
-
     private int readBuffered(final byte[] pBuffer, final int pOffset, final int pLength) {
         if (!buffer.hasRemaining()) {
             return -1;
@@ -116,7 +121,6 @@ public final class BufferedImageInputStream extends ImageInputStreamImpl impleme
             int position = buffer.position();
             System.arraycopy(buffer.array(), position, pBuffer, pOffset, length);
             buffer.position(position + length);
-
         }
 
         streamPos += length;
@@ -124,16 +128,152 @@ public final class BufferedImageInputStream extends ImageInputStreamImpl impleme
         return length;
     }
 
+    // Need to override the readShort(), readInt() and readLong() methods,
+    // because the implementations in ImageInputStreamImpl expects the
+    // read(byte[], int, int) to always read the expected number of bytes,
+    // causing uninitialized values, alignment issues and EOFExceptions at
+    // random places...
+    // Notes:
+    // * readUnsignedXx() is covered by their signed counterparts
+    // * readChar() is covered by readShort()
+    // * readFloat() and readDouble() is covered by readInt() and readLong()
+    //   respectively.
+    // * readLong() may be covered by two readInt()s, we'll override to be safe
+
+    @Override
+    public short readShort() throws IOException {
+        readFully(integralCache.array(), 0, 2);
+
+        return integralCache.getShort(0);
+    }
+
+    @Override
+    public int readInt() throws IOException {
+        readFully(integralCache.array(), 0, 4);
+
+        return integralCache.getInt(0);
+    }
+
+    @Override
+    public long readLong() throws IOException {
+        readFully(integralCache.array(), 0, 8);
+
+        return integralCache.getLong(0);
+    }
+
+    @Override
+    public int readBit() throws IOException {
+        checkClosed();
+
+        if (!buffer.hasRemaining()) {
+            fillBuffer();
+
+            if (!buffer.hasRemaining()) {
+                throw new EOFException();
+            }
+        }
+
+        // Compute final bit offset before we call read() and seek()
+        int newBitOffset = (this.bitOffset + 1) & 0x7;
+
+        int val = buffer.get() & 0xff;
+
+        if (newBitOffset != 0) {
+            // Move byte position back if in the middle of a byte
+            buffer.position(buffer.position() - 1);
+
+            // Shift the bit to be read to the rightmost position
+            val >>= 8 - newBitOffset;
+        }
+        else {
+            streamPos++;
+        }
+
+        this.bitOffset = newBitOffset;
+
+        return val & 0x1;
+    }
+
+    @Override
+    public long readBits(int numBits) throws IOException {
+        checkClosed();
+
+        if (numBits < 0 || numBits > 64) {
+            throw new IllegalArgumentException();
+        }
+        if (numBits == 0) {
+            return 0L;
+        }
+
+        // Have to read additional bits on the left equal to the bit offset
+        int bitsToRead = numBits + bitOffset;
+
+        // Compute final bit offset before we call read() and seek()
+        int newBitOffset = (this.bitOffset + numBits) & 0x7;
+
+        // Read a byte at a time, accumulate
+        long accum = 0L;
+        while (bitsToRead > 0) {
+            if (!buffer.hasRemaining()) {
+                fillBuffer();
+
+                if (!buffer.hasRemaining()) {
+                    throw new EOFException();
+                }
+            }
+
+            int val = buffer.get() & 0xff;
+
+            accum <<= 8;
+            accum |= val;
+            bitsToRead -= 8;
+        }
+
+        // Move byte position back if in the middle of a byte
+        if (newBitOffset != 0) {
+            buffer.position(buffer.position() - 1);
+        }
+        else {
+            streamPos++;
+        }
+
+        this.bitOffset = newBitOffset;
+
+        // Shift away unwanted bits on the right.
+        accum >>>= (-bitsToRead); // Negative of bitsToRead == extra bits read
+
+        // Mask out unwanted bits on the left
+        accum &= (-1L >>> (64 - numBits));
+
+        return accum;
+    }
+
     @Override
     public void seek(long pPosition) throws IOException {
-        // TODO: Could probably be optimized to not invalidate buffer if new position is within current buffer
+        checkClosed();
-        stream.seek(pPosition);
+        bitOffset = 0;
-        buffer.limit(0); // Will invalidate buffer
+
-        streamPos = stream.getStreamPosition();
+        if (streamPos == pPosition) {
+            return;
+        }
+
+        // Optimized to not invalidate buffer if new position is within current buffer
+        long newBufferPos = buffer.position() + pPosition - streamPos;
+        if (newBufferPos >= 0 && newBufferPos <= buffer.limit()) {
+            buffer.position((int) newBufferPos);
+        }
+        else {
+            // Will invalidate buffer
+            buffer.limit(0);
+            stream.seek(pPosition);
+        }
+
+        streamPos = pPosition;
     }
 
     @Override
     public void flushBefore(long pos) throws IOException {
+        checkClosed();
         stream.flushBefore(pos);
     }
 

imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/BufferedImageInputStreamTest.java

@@ -2,7 +2,7 @@ package com.twelvemonkeys.imageio.stream;
 
 import com.twelvemonkeys.io.ole2.CompoundDocument;
 import com.twelvemonkeys.io.ole2.Entry;
-import junit.framework.TestCase;
+import org.junit.Test;
 
 import javax.imageio.stream.ImageInputStream;
 import javax.imageio.stream.MemoryCacheImageInputStream;
@@ -10,20 +10,25 @@ import java.io.IOException;
 import java.nio.ByteOrder;
 import java.util.Random;
 
+import static java.util.Arrays.fill;
+import static org.junit.Assert.*;
+
 /**
- * BufferedImageInputStreamTestCase
+ * BufferedImageInputStreamTest
  *
  * @author <a href="mailto:harald.kuhr@gmail.com">Harald Kuhr</a>
  * @author last modified by $Author: haraldk$
- * @version $Id: BufferedImageInputStreamTestCase.java,v 1.0 Jun 30, 2008 3:07:42 PM haraldk Exp$
+ * @version $Id: BufferedImageInputStreamTest.java,v 1.0 Jun 30, 2008 3:07:42 PM haraldk Exp$
  */
-public class BufferedImageInputStreamTestCase extends TestCase {
+public class BufferedImageInputStreamTest {
-    protected final Random random = new Random();
+    private final Random random = new Random(3450972865211L);
 
+    @Test
     public void testCreate() throws IOException {
         new BufferedImageInputStream(new ByteArrayImageInputStream(new byte[0]));
     }
 
+    @Test
     public void testCreateNull() throws IOException {
         try {
             new BufferedImageInputStream(null);
@@ -41,20 +46,23 @@ public class BufferedImageInputStreamTestCase extends TestCase {
 
     // TODO: Create test that exposes read += -1 (eof) bug
 
+    @Test
     public void testArrayIndexOutOfBoundsBufferedReadBug() throws IOException {
         // TODO: Create a more straight forward way to prove correctness, for now this is good enough to avoid regression
         ImageInputStream input = new BufferedImageInputStream(new MemoryCacheImageInputStream(getClass().getResourceAsStream("/Thumbs-camera.db")));
         input.setByteOrder(ByteOrder.LITTLE_ENDIAN);
         Entry root = new CompoundDocument(input).getRootEntry();
 
-        Entry child = root.getChildEntry("Catalog");
+        Entry catalog = root.getChildEntry("Catalog");
 
-        assertNotNull("Input stream can never be null", child.getInputStream());
+        assertNotNull("Catalog should not be null", catalog);
+        assertNotNull("Input stream can never be null", catalog.getInputStream());
     }
 
+    @Test
     public void testReadResetReadDirectBufferBug() throws IOException {
         // Make sure we use the exact size of the buffer
-        final int size = BufferedImageInputStream.DEFAULT_BUFFER_SIZE;
+        int size = BufferedImageInputStream.DEFAULT_BUFFER_SIZE;
 
         // Fill bytes
         byte[] bytes = new byte[size * 2];
@@ -76,6 +84,7 @@ public class BufferedImageInputStreamTestCase extends TestCase {
         assertTrue(rangeEquals(bytes, size, result, 0, size));
     }
 
+    @Test
     public void testBufferPositionCorrect() throws IOException {
         // Fill bytes
         byte[] bytes = new byte[1024];
@@ -105,6 +114,25 @@ public class BufferedImageInputStreamTestCase extends TestCase {
         assertEquals(1020, stream.getStreamPosition());
     }
 
+    @Test
+    public void testReadIntegralOnBufferBoundary() throws IOException {
+        // Make sure we use the exact size of the buffer
+        int size = BufferedImageInputStream.DEFAULT_BUFFER_SIZE;
+
+        // Fill bytes
+        byte[] bytes = new byte[size * 2];
+        fill(bytes, size - 4, size + 4, (byte) 0xff);
+
+        // Create wrapper stream
+        BufferedImageInputStream stream = new BufferedImageInputStream(new ByteArrayImageInputStream(bytes));
+
+        // Read to fill the buffer, then seek to almost end of buffer
+        assertEquals(0, stream.readInt());
+        stream.seek(size - 3);
+        assertEquals(0xffffffff, stream.readInt());
+        assertEquals(size + 1, stream.getStreamPosition());
+    }
+
     /**
      * Test two arrays for range equality. That is, they contain the same elements for some specified range.
      *

imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/ByteArrayImageInputStreamTest.java

@@ -1,11 +1,12 @@
 package com.twelvemonkeys.imageio.stream;
 
-import junit.framework.TestCase;
+import org.junit.Test;
 
 import java.io.IOException;
 import java.util.Random;
 
-import static com.twelvemonkeys.imageio.stream.BufferedImageInputStreamTestCase.rangeEquals;
+import static com.twelvemonkeys.imageio.stream.BufferedImageInputStreamTest.rangeEquals;
+import static org.junit.Assert.*;
 
 /**
  * ByteArrayImageInputStreamTestCase
@@ -14,14 +15,16 @@ import static com.twelvemonkeys.imageio.stream.BufferedImageInputStreamTestCase.
  * @author last modified by $Author: haraldk$
  * @version $Id: ByteArrayImageInputStreamTestCase.java,v 1.0 Apr 21, 2009 10:58:48 AM haraldk Exp$
  */
-public class ByteArrayImageInputStreamTestCase extends TestCase {
+public class ByteArrayImageInputStreamTest {
-    protected final Random random = new Random();
+    private final Random random = new Random(1709843507234566L);
 
+    @Test
     public void testCreate() {
         ByteArrayImageInputStream stream = new ByteArrayImageInputStream(new byte[0]);
         assertEquals("Data length should be same as stream length", 0, stream.length());
     }
 
+    @Test
     public void testCreateNull() {
         try {
             new ByteArrayImageInputStream(null);
@@ -35,6 +38,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testCreateNullOffLen() {
         try {
             new ByteArrayImageInputStream(null, 0, -1);
@@ -48,6 +52,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testCreateNegativeOff() {
         try {
             new ByteArrayImageInputStream(new byte[0], -1, 1);
@@ -61,6 +66,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testCreateBadOff() {
         try {
             new ByteArrayImageInputStream(new byte[1], 2, 0);
@@ -74,6 +80,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testCreateNegativeLen() {
         try {
             new ByteArrayImageInputStream(new byte[0], 0, -1);
@@ -87,6 +94,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testCreateBadLen() {
         try {
             new ByteArrayImageInputStream(new byte[1], 0, 2);
@@ -100,6 +108,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testRead() throws IOException {
         byte[] data = new byte[1024 * 1024];
         random.nextBytes(data);
@@ -113,6 +122,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testReadOffsetLen() throws IOException {
         byte[] data = new byte[1024 * 1024];
         random.nextBytes(data);
@@ -128,6 +138,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testReadArray() throws IOException {
         byte[] data = new byte[1024 * 1024];
         random.nextBytes(data);
@@ -144,6 +155,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testReadArrayOffLen() throws IOException {
         byte[] data = new byte[1024 * 1024];
         random.nextBytes(data);
@@ -162,6 +174,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testReadSkip() throws IOException {
         byte[] data = new byte[1024 * 14];
         random.nextBytes(data);
@@ -179,6 +192,7 @@ public class ByteArrayImageInputStreamTestCase extends TestCase {
         }
     }
 
+    @Test
     public void testReadSeek() throws IOException {
         byte[] data = new byte[1024 * 18];
         random.nextBytes(data);

imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/SubImageInputStreamTest.java

@@ -1,6 +1,6 @@
 package com.twelvemonkeys.imageio.stream;
 
-import junit.framework.TestCase;
+import org.junit.Test;
 
 import javax.imageio.stream.ImageInputStream;
 import javax.imageio.stream.ImageInputStreamImpl;
@@ -10,6 +10,8 @@ import java.io.IOException;
 import java.util.Arrays;
 import java.util.Random;
 
+import static org.junit.Assert.*;
+
 /**
  * SubImageInputStreamTestCase
  *
@@ -17,9 +19,9 @@ import java.util.Random;
  * @author last modified by $Author: haraldk$
  * @version $Id: SubImageInputStreamTestCase.java,v 1.0 Nov 8, 2009 3:03:32 PM haraldk Exp$
  */
-public class SubImageInputStreamTestCase extends TestCase {
+public class SubImageInputStreamTest {
     // TODO: Extract super test case for all stream tests
-    private final Random random = new Random(837468l);
+    private final Random random = new Random(837468L);
 
     private ImageInputStream createStream(final int pSize) {
         byte[] bytes = new byte[pSize];
@@ -34,24 +36,19 @@ public class SubImageInputStreamTestCase extends TestCase {
         };
     }
 
+    @Test(expected = IllegalArgumentException.class)
     public void testCreateNullStream() throws IOException {
-        try {
+        new SubImageInputStream(null, 1);
-            new SubImageInputStream(null, 1);
+        fail("Expected IllegalArgumentException with null stream");
-            fail("Expected IllegalArgumentException with null stream");
-        }
-        catch (IllegalArgumentException e) {
-        }
     }
 
+    @Test(expected = IllegalArgumentException.class)
     public void testCreateNegativeLength() throws IOException {
-        try {
+        new SubImageInputStream(createStream(0), -1);
-            new SubImageInputStream(createStream(0), -1);
+        fail("Expected IllegalArgumentException with negative length");
-            fail("Expected IllegalArgumentException with negative length");
-        }
-        catch (IllegalArgumentException e) {
-        }
     }
 
+    @Test
     public void testCreate() throws IOException {
         ImageInputStream stream = new SubImageInputStream(createStream(11), 7);
 
@@ -59,11 +56,13 @@ public class SubImageInputStreamTestCase extends TestCase {
         assertEquals(7, stream.length());
     }
 
+    @Test
     public void testWraphBeyondWrappedLength() throws IOException {
         SubImageInputStream stream = new SubImageInputStream(createStream(5), 6);
         assertEquals(5, stream.length());
     }
 
+    @Test
     public void testWrapUnknownLength() throws IOException {
         SubImageInputStream stream = new SubImageInputStream(new ImageInputStreamImpl() {
             @Override
@@ -85,6 +84,7 @@ public class SubImageInputStreamTestCase extends TestCase {
         assertEquals(-1, stream.length());
     }
 
+    @Test
     public void testRead() throws IOException {
         ImageInputStream wrapped = createStream(42);
 
@@ -106,6 +106,7 @@ public class SubImageInputStreamTestCase extends TestCase {
         assertEquals(25, wrapped.getStreamPosition());
     }
 
+    @Test
     public void testReadResetRead() throws IOException {
         ImageInputStream stream = new SubImageInputStream(createStream(32), 16);
         stream.mark();
@@ -121,6 +122,7 @@ public class SubImageInputStreamTestCase extends TestCase {
         assertTrue(Arrays.equals(first, second));
     }
 
+    @Test
     public void testSeekNegative() throws IOException {
         ImageInputStream stream = new SubImageInputStream(createStream(7), 5);
         try {
@@ -128,11 +130,13 @@ public class SubImageInputStreamTestCase extends TestCase {
             fail("Expected IndexOutOfBoundsException");
         }
         catch (IndexOutOfBoundsException expected) {
+            assertTrue(expected.getMessage().contains("pos"));
         }
 
         assertEquals(0, stream.getStreamPosition());
     }
 
+    @Test
     public void testSeekBeforeFlushedPos() throws IOException {
         ImageInputStream stream = new SubImageInputStream(createStream(7), 5);
         stream.seek(3);
@@ -145,11 +149,13 @@ public class SubImageInputStreamTestCase extends TestCase {
             fail("Expected IndexOutOfBoundsException");
         }
         catch (IndexOutOfBoundsException expected) {
+            assertTrue(expected.getMessage().contains("pos"));
         }
 
         assertEquals(3, stream.getStreamPosition());
     }
 
+    @Test
     public void testSeekAfterEOF() throws IOException {
         ImageInputStream stream = new SubImageInputStream(createStream(7), 5);
         stream.seek(6);
@@ -157,6 +163,7 @@ public class SubImageInputStreamTestCase extends TestCase {
         assertEquals(-1, stream.read());
     }
 
+    @Test
     public void testSeek() throws IOException {
         ImageInputStream stream = new SubImageInputStream(createStream(7), 5);
         stream.seek(5);