Skip to content

Commit

Permalink
Rewind ByteBuffers in ImageHeaderParserUtils between each parser.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sjudd authored and glide-copybara-robot committed Mar 31, 2022
1 parent 9840c91 commit 4f29ada
Show file tree
Hide file tree
Showing 2 changed files with 230 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -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);
}
}
});
}
Expand All @@ -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(
Expand All @@ -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();
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
}
}
});
}
Expand All @@ -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 {
Expand All @@ -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(
Expand All @@ -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();
}
Expand All @@ -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;
}
Expand All @@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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<FakeImageHeaderParser> fakeParsers =
Arrays.asList(new FakeImageHeaderParser(), new FakeImageHeaderParser());
private List<ImageHeaderParser> 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<ImageHeaderParser>();
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<Byte> asList(byte[] data) {
List<Byte> 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;
}
}
}

0 comments on commit 4f29ada

Please sign in to comment.