diff --git a/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/stream/BufferedImageInputStream.java b/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/stream/BufferedImageInputStream.java index 3bc4be58..7e9a8678 100644 --- a/imageio/imageio-core/src/main/java/com/twelvemonkeys/imageio/stream/BufferedImageInputStream.java +++ b/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); - } - catch (IllegalArgumentException e) { - System.err.println("length: " + length); - throw e; - } + buffer.position(length); 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... - int read = stream.read(pBuffer, pOffset, Math.min(buffer.capacity(), pLength)); + // Invalidate the buffer, as its contents is no longer in sync with the stream's position. + 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 - stream.seek(pPosition); - buffer.limit(0); // Will invalidate buffer - streamPos = stream.getStreamPosition(); + checkClosed(); + bitOffset = 0; + + 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); } diff --git a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/BufferedImageInputStreamTestCase.java b/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/BufferedImageInputStreamTest.java similarity index 78% rename from imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/BufferedImageInputStreamTestCase.java rename to imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/BufferedImageInputStreamTest.java index 90e4b12b..8bbf9b36 100644 --- a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/BufferedImageInputStreamTestCase.java +++ b/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 Harald Kuhr * @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 { - protected final Random random = new Random(); +public class BufferedImageInputStreamTest { + 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. * diff --git a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/ByteArrayImageInputStreamTestCase.java b/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/ByteArrayImageInputStreamTest.java similarity index 96% rename from imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/ByteArrayImageInputStreamTestCase.java rename to imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/ByteArrayImageInputStreamTest.java index 868b082c..ff4e2f03 100755 --- a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/ByteArrayImageInputStreamTestCase.java +++ b/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 { - protected final Random random = new Random(); +public class ByteArrayImageInputStreamTest { + 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); diff --git a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/SubImageInputStreamTestCase.java b/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/SubImageInputStreamTest.java similarity index 86% rename from imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/SubImageInputStreamTestCase.java rename to imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/SubImageInputStreamTest.java index 674afa18..fc7ab1ec 100644 --- a/imageio/imageio-core/src/test/java/com/twelvemonkeys/imageio/stream/SubImageInputStreamTestCase.java +++ b/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); - fail("Expected IllegalArgumentException with null stream"); - } - catch (IllegalArgumentException e) { - } + new SubImageInputStream(null, 1); + fail("Expected IllegalArgumentException with null stream"); } + @Test(expected = IllegalArgumentException.class) public void testCreateNegativeLength() throws IOException { - try { - new SubImageInputStream(createStream(0), -1); - fail("Expected IllegalArgumentException with negative length"); - } - catch (IllegalArgumentException e) { - } + new SubImageInputStream(createStream(0), -1); + fail("Expected IllegalArgumentException with negative length"); } + @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);