From 4f29adab64f6b43a10652f4d26ba3ecdaf0b5e47 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Thu, 31 Mar 2022 12:39:49 -0700 Subject: [PATCH] Rewind ByteBuffers in ImageHeaderParserUtils between each parser. Previously we were rewinding InputStreams reliably, but not rewinding ByteBuffers. This could mean that only the first parser had the chance to read the data correctly, even if multiple were specified. ParcelFileDescriptor is pretty confusing. Prior to this change we were closing the InputStream. In a Robolectric environment, closing the InputStream seems to close the file descriptor, causing subsequent parsers to throw IOExceptions when they tried to read data. However on an emulator, closing the stream doesn't seem to close the file descriptor and things seem to work. I have made a change to explicitly avoid closing the stream when we create one from a ParcelFileDescriptor because that seems to align with the expected behavior in the method. The caller should be responsible for creating and closing the resource. All that said though, I'm not sure this makes a difference outside of Robolectric. I believe this fixes #4778 PiperOrigin-RevId: 438635301 --- .../glide/load/ImageHeaderParserUtils.java | 59 +++--- .../load/ImageHeaderParserUtilsTest.java | 197 ++++++++++++++++++ 2 files changed, 230 insertions(+), 26 deletions(-) create mode 100644 library/test/src/test/java/com/bumptech/glide/load/ImageHeaderParserUtilsTest.java 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; + } + } +}