diff --git a/library/src/main/java/com/bumptech/glide/load/ImageHeaderParserUtils.java b/library/src/main/java/com/bumptech/glide/load/ImageHeaderParserUtils.java index 96c71cb765..58b52a667c 100644 --- a/library/src/main/java/com/bumptech/glide/load/ImageHeaderParserUtils.java +++ b/library/src/main/java/com/bumptech/glide/load/ImageHeaderParserUtils.java @@ -8,6 +8,7 @@ import com.bumptech.glide.load.data.ParcelFileDescriptorRewinder; import com.bumptech.glide.load.engine.bitmap_recycle.ArrayPool; import com.bumptech.glide.load.resource.bitmap.RecyclableBufferedInputStream; +import com.bumptech.glide.util.ByteBufferUtil; import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; @@ -43,7 +44,7 @@ public static ImageType getType( parsers, new TypeReader() { @Override - public ImageType getType(ImageHeaderParser parser) throws IOException { + public ImageType getTypeAndRewind(ImageHeaderParser parser) throws IOException { try { return parser.getType(finalIs); } finally { @@ -66,8 +67,12 @@ public static ImageType getType( parsers, new TypeReader() { @Override - public ImageType getType(ImageHeaderParser parser) throws IOException { - return parser.getType(buffer); + public ImageType getTypeAndRewind(ImageHeaderParser parser) throws IOException { + try { + return parser.getType(buffer); + } finally { + ByteBufferUtil.rewind(buffer); + } } }); } @@ -83,10 +88,10 @@ public static ImageType getType( parsers, new TypeReader() { @Override - public ImageType getType(ImageHeaderParser parser) throws IOException { + public ImageType getTypeAndRewind(ImageHeaderParser parser) throws IOException { // Wrap the FileInputStream into a RecyclableBufferedInputStream to optimize I/O // performance - InputStream is = null; + RecyclableBufferedInputStream is = null; try { is = new RecyclableBufferedInputStream( @@ -95,12 +100,11 @@ public ImageType getType(ImageHeaderParser parser) throws IOException { byteArrayPool); return parser.getType(is); } finally { - try { - if (is != null) { - is.close(); - } - } catch (IOException e) { - // Ignored. + // If we close the stream, we'll close the file descriptor as well, so we can't do + // that. We do however want to make sure we release any buffers we used back to the + // pool so we call release instead of close. + if (is != null) { + is.release(); } parcelFileDescriptorRewinder.rewindAndGet(); } @@ -114,7 +118,7 @@ private static ImageType getTypeInternal( //noinspection ForLoopReplaceableByForEach to improve perf for (int i = 0, size = parsers.size(); i < size; i++) { ImageHeaderParser parser = parsers.get(i); - ImageType type = reader.getType(parser); + ImageType type = reader.getTypeAndRewind(parser); if (type != ImageType.UNKNOWN) { return type; } @@ -143,8 +147,12 @@ public static int getOrientation( parsers, new OrientationReader() { @Override - public int getOrientation(ImageHeaderParser parser) throws IOException { - return parser.getOrientation(buffer, arrayPool); + public int getOrientationAndRewind(ImageHeaderParser parser) throws IOException { + try { + return parser.getOrientation(buffer, arrayPool); + } finally { + ByteBufferUtil.rewind(buffer); + } } }); } @@ -169,7 +177,7 @@ public static int getOrientation( parsers, new OrientationReader() { @Override - public int getOrientation(ImageHeaderParser parser) throws IOException { + public int getOrientationAndRewind(ImageHeaderParser parser) throws IOException { try { return parser.getOrientation(finalIs, byteArrayPool); } finally { @@ -189,10 +197,10 @@ public static int getOrientation( parsers, new OrientationReader() { @Override - public int getOrientation(ImageHeaderParser parser) throws IOException { + public int getOrientationAndRewind(ImageHeaderParser parser) throws IOException { // Wrap the FileInputStream into a RecyclableBufferedInputStream to optimize I/O // performance - InputStream is = null; + RecyclableBufferedInputStream is = null; try { is = new RecyclableBufferedInputStream( @@ -201,12 +209,11 @@ public int getOrientation(ImageHeaderParser parser) throws IOException { byteArrayPool); return parser.getOrientation(is, byteArrayPool); } finally { - try { - if (is != null) { - is.close(); - } - } catch (IOException e) { - // Ignored. + // If we close the stream, we'll close the file descriptor as well, so we can't do + // that. We do however want to make sure we release any buffers we used back to the + // pool so we call release instead of close. + if (is != null) { + is.release(); } parcelFileDescriptorRewinder.rewindAndGet(); } @@ -219,7 +226,7 @@ private static int getOrientationInternal( //noinspection ForLoopReplaceableByForEach to improve perf for (int i = 0, size = parsers.size(); i < size; i++) { ImageHeaderParser parser = parsers.get(i); - int orientation = reader.getOrientation(parser); + int orientation = reader.getOrientationAndRewind(parser); if (orientation != ImageHeaderParser.UNKNOWN_ORIENTATION) { return orientation; } @@ -229,10 +236,10 @@ private static int getOrientationInternal( } private interface TypeReader { - ImageType getType(ImageHeaderParser parser) throws IOException; + ImageType getTypeAndRewind(ImageHeaderParser parser) throws IOException; } private interface OrientationReader { - int getOrientation(ImageHeaderParser parser) throws IOException; + int getOrientationAndRewind(ImageHeaderParser parser) throws IOException; } } diff --git a/library/test/src/test/java/com/bumptech/glide/load/ImageHeaderParserUtilsTest.java b/library/test/src/test/java/com/bumptech/glide/load/ImageHeaderParserUtilsTest.java new file mode 100644 index 0000000000..7134055151 --- /dev/null +++ b/library/test/src/test/java/com/bumptech/glide/load/ImageHeaderParserUtilsTest.java @@ -0,0 +1,197 @@ +package com.bumptech.glide.load; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assume.assumeTrue; + +import android.content.Context; +import android.os.ParcelFileDescriptor; +import androidx.annotation.NonNull; +import androidx.test.core.app.ApplicationProvider; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.bumptech.glide.load.data.ParcelFileDescriptorRewinder; +import com.bumptech.glide.load.engine.bitmap_recycle.ArrayPool; +import com.bumptech.glide.load.engine.bitmap_recycle.LruArrayPool; +import com.bumptech.glide.util.ByteBufferUtil; +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +@RunWith(AndroidJUnit4.class) +public class ImageHeaderParserUtilsTest { + private final List fakeParsers = + Arrays.asList(new FakeImageHeaderParser(), new FakeImageHeaderParser()); + private List parsers; + private final Context context = ApplicationProvider.getApplicationContext(); + private final byte[] expectedData = new byte[] {0, 1, 2, 3, 4, 5, 6, 7, 8}; + private final LruArrayPool lruArrayPool = new LruArrayPool(); + + @Before + public void setUp() { + parsers = new ArrayList(); + for (FakeImageHeaderParser parser : fakeParsers) { + parsers.add(parser); + } + } + + @Test + public void getType_withTwoParsers_andStream_rewindsBeforeEachParser() throws IOException { + ImageHeaderParserUtils.getType(parsers, new ByteArrayInputStream(expectedData), lruArrayPool); + + assertAllParsersReceivedTheSameData(); + } + + @Test + public void getType_withTwoParsers_andByteBuffer_rewindsBeforeEachParser() throws IOException { + ImageHeaderParserUtils.getType(parsers, ByteBuffer.wrap(expectedData)); + + assertAllParsersReceivedTheSameData(); + } + + @Test + public void getType_withTwoParsers_andFileDescriptor_rewindsBeforeEachParser() + throws IOException { + // This test can't work if file descriptor rewinding isn't supported. Sadly that means this + // test doesn't work in Robolectric. + assumeTrue(ParcelFileDescriptorRewinder.isSupported()); + + ParcelFileDescriptor fileDescriptor = null; + try { + fileDescriptor = asFileDescriptor(expectedData); + ParcelFileDescriptorRewinder rewinder = new ParcelFileDescriptorRewinder(fileDescriptor); + ImageHeaderParserUtils.getType(parsers, rewinder, lruArrayPool); + } finally { + if (fileDescriptor != null) { + fileDescriptor.close(); + } + } + + assertAllParsersReceivedTheSameData(); + } + + @Test + public void getOrientation_withTwoParsers_andStream_rewindsBeforeEachParser() throws IOException { + ImageHeaderParserUtils.getOrientation( + parsers, new ByteArrayInputStream(expectedData), lruArrayPool); + + assertAllParsersReceivedTheSameData(); + } + + @Test + public void getOrientation_withTwoParsers_andByteBuffer_rewindsBeforeEachParser() + throws IOException { + ImageHeaderParserUtils.getOrientation(parsers, ByteBuffer.wrap(expectedData), lruArrayPool); + + assertAllParsersReceivedTheSameData(); + } + + @Test + public void getOrientation_withTwoParsers_andFileDescriptor_rewindsBeforeEachParser() + throws IOException { + // This test can't work if file descriptor rewinding isn't supported. Sadly that means this + // test doesn't work in Robolectric. + assumeTrue(ParcelFileDescriptorRewinder.isSupported()); + ParcelFileDescriptor fileDescriptor = null; + try { + fileDescriptor = asFileDescriptor(expectedData); + ParcelFileDescriptorRewinder rewinder = new ParcelFileDescriptorRewinder(fileDescriptor); + ImageHeaderParserUtils.getOrientation(parsers, rewinder, lruArrayPool); + } finally { + if (fileDescriptor != null) { + fileDescriptor.close(); + } + } + + assertAllParsersReceivedTheSameData(); + } + + private void assertAllParsersReceivedTheSameData() { + for (FakeImageHeaderParser parser : fakeParsers) { + assertThat(parser.data).isNotNull(); + assertThat(parser.data).asList().containsExactlyElementsIn(asList(expectedData)).inOrder(); + } + } + + private static List asList(byte[] data) { + List result = new ArrayList<>(); + for (byte item : data) { + result.add(item); + } + return result; + } + + private ParcelFileDescriptor asFileDescriptor(byte[] data) throws IOException { + File file = new File(context.getCacheDir(), "temp"); + OutputStream os = null; + try { + os = new FileOutputStream(file); + os.write(data); + os.close(); + } finally { + if (os != null) { + os.close(); + } + } + return ParcelFileDescriptor.open(file, ParcelFileDescriptor.MODE_READ_ONLY); + } + + private static final class FakeImageHeaderParser implements ImageHeaderParser { + + private byte[] data; + + private void readData(InputStream is) throws IOException { + readData(ByteBufferUtil.fromStream(is)); + } + + // This is rather roundabout, but it's a simple way of reading the remaining data in the buffer. + private void readData(ByteBuffer byteBuffer) { + + byte[] data = new byte[byteBuffer.remaining()]; + // A 0 length means we read no data. If we try to pass this to ByteBuffer it will throw. We'd + // rather not get that obscure exception and instead have an assertion above trigger because + // we didn't read enough data. So we work around the exception here if we have no data to + // read. + if (data.length != 0) { + byteBuffer.get(data, byteBuffer.position(), byteBuffer.remaining()); + } + this.data = data; + } + + @NonNull + @Override + public ImageType getType(@NonNull InputStream is) throws IOException { + readData(is); + return ImageType.UNKNOWN; + } + + @NonNull + @Override + public ImageType getType(@NonNull ByteBuffer byteBuffer) throws IOException { + readData(byteBuffer); + return ImageType.UNKNOWN; + } + + @Override + public int getOrientation(@NonNull InputStream is, @NonNull ArrayPool byteArrayPool) + throws IOException { + readData(is); + return ImageHeaderParser.UNKNOWN_ORIENTATION; + } + + @Override + public int getOrientation(@NonNull ByteBuffer byteBuffer, @NonNull ArrayPool byteArrayPool) + throws IOException { + readData(byteBuffer); + return ImageHeaderParser.UNKNOWN_ORIENTATION; + } + } +}