Skip to content

Commit 4f29ada

Browse files
sjuddglide-copybara-robot
authored andcommittedMar 31, 2022
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
1 parent 9840c91 commit 4f29ada

File tree

2 files changed

+230
-26
lines changed

2 files changed

+230
-26
lines changed
 

‎library/src/main/java/com/bumptech/glide/load/ImageHeaderParserUtils.java

+33-26
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import com.bumptech.glide.load.data.ParcelFileDescriptorRewinder;
99
import com.bumptech.glide.load.engine.bitmap_recycle.ArrayPool;
1010
import com.bumptech.glide.load.resource.bitmap.RecyclableBufferedInputStream;
11+
import com.bumptech.glide.util.ByteBufferUtil;
1112
import java.io.FileInputStream;
1213
import java.io.IOException;
1314
import java.io.InputStream;
@@ -43,7 +44,7 @@ public static ImageType getType(
4344
parsers,
4445
new TypeReader() {
4546
@Override
46-
public ImageType getType(ImageHeaderParser parser) throws IOException {
47+
public ImageType getTypeAndRewind(ImageHeaderParser parser) throws IOException {
4748
try {
4849
return parser.getType(finalIs);
4950
} finally {
@@ -66,8 +67,12 @@ public static ImageType getType(
6667
parsers,
6768
new TypeReader() {
6869
@Override
69-
public ImageType getType(ImageHeaderParser parser) throws IOException {
70-
return parser.getType(buffer);
70+
public ImageType getTypeAndRewind(ImageHeaderParser parser) throws IOException {
71+
try {
72+
return parser.getType(buffer);
73+
} finally {
74+
ByteBufferUtil.rewind(buffer);
75+
}
7176
}
7277
});
7378
}
@@ -83,10 +88,10 @@ public static ImageType getType(
8388
parsers,
8489
new TypeReader() {
8590
@Override
86-
public ImageType getType(ImageHeaderParser parser) throws IOException {
91+
public ImageType getTypeAndRewind(ImageHeaderParser parser) throws IOException {
8792
// Wrap the FileInputStream into a RecyclableBufferedInputStream to optimize I/O
8893
// performance
89-
InputStream is = null;
94+
RecyclableBufferedInputStream is = null;
9095
try {
9196
is =
9297
new RecyclableBufferedInputStream(
@@ -95,12 +100,11 @@ public ImageType getType(ImageHeaderParser parser) throws IOException {
95100
byteArrayPool);
96101
return parser.getType(is);
97102
} finally {
98-
try {
99-
if (is != null) {
100-
is.close();
101-
}
102-
} catch (IOException e) {
103-
// Ignored.
103+
// If we close the stream, we'll close the file descriptor as well, so we can't do
104+
// that. We do however want to make sure we release any buffers we used back to the
105+
// pool so we call release instead of close.
106+
if (is != null) {
107+
is.release();
104108
}
105109
parcelFileDescriptorRewinder.rewindAndGet();
106110
}
@@ -114,7 +118,7 @@ private static ImageType getTypeInternal(
114118
//noinspection ForLoopReplaceableByForEach to improve perf
115119
for (int i = 0, size = parsers.size(); i < size; i++) {
116120
ImageHeaderParser parser = parsers.get(i);
117-
ImageType type = reader.getType(parser);
121+
ImageType type = reader.getTypeAndRewind(parser);
118122
if (type != ImageType.UNKNOWN) {
119123
return type;
120124
}
@@ -143,8 +147,12 @@ public static int getOrientation(
143147
parsers,
144148
new OrientationReader() {
145149
@Override
146-
public int getOrientation(ImageHeaderParser parser) throws IOException {
147-
return parser.getOrientation(buffer, arrayPool);
150+
public int getOrientationAndRewind(ImageHeaderParser parser) throws IOException {
151+
try {
152+
return parser.getOrientation(buffer, arrayPool);
153+
} finally {
154+
ByteBufferUtil.rewind(buffer);
155+
}
148156
}
149157
});
150158
}
@@ -169,7 +177,7 @@ public static int getOrientation(
169177
parsers,
170178
new OrientationReader() {
171179
@Override
172-
public int getOrientation(ImageHeaderParser parser) throws IOException {
180+
public int getOrientationAndRewind(ImageHeaderParser parser) throws IOException {
173181
try {
174182
return parser.getOrientation(finalIs, byteArrayPool);
175183
} finally {
@@ -189,10 +197,10 @@ public static int getOrientation(
189197
parsers,
190198
new OrientationReader() {
191199
@Override
192-
public int getOrientation(ImageHeaderParser parser) throws IOException {
200+
public int getOrientationAndRewind(ImageHeaderParser parser) throws IOException {
193201
// Wrap the FileInputStream into a RecyclableBufferedInputStream to optimize I/O
194202
// performance
195-
InputStream is = null;
203+
RecyclableBufferedInputStream is = null;
196204
try {
197205
is =
198206
new RecyclableBufferedInputStream(
@@ -201,12 +209,11 @@ public int getOrientation(ImageHeaderParser parser) throws IOException {
201209
byteArrayPool);
202210
return parser.getOrientation(is, byteArrayPool);
203211
} finally {
204-
try {
205-
if (is != null) {
206-
is.close();
207-
}
208-
} catch (IOException e) {
209-
// Ignored.
212+
// If we close the stream, we'll close the file descriptor as well, so we can't do
213+
// that. We do however want to make sure we release any buffers we used back to the
214+
// pool so we call release instead of close.
215+
if (is != null) {
216+
is.release();
210217
}
211218
parcelFileDescriptorRewinder.rewindAndGet();
212219
}
@@ -219,7 +226,7 @@ private static int getOrientationInternal(
219226
//noinspection ForLoopReplaceableByForEach to improve perf
220227
for (int i = 0, size = parsers.size(); i < size; i++) {
221228
ImageHeaderParser parser = parsers.get(i);
222-
int orientation = reader.getOrientation(parser);
229+
int orientation = reader.getOrientationAndRewind(parser);
223230
if (orientation != ImageHeaderParser.UNKNOWN_ORIENTATION) {
224231
return orientation;
225232
}
@@ -229,10 +236,10 @@ private static int getOrientationInternal(
229236
}
230237

231238
private interface TypeReader {
232-
ImageType getType(ImageHeaderParser parser) throws IOException;
239+
ImageType getTypeAndRewind(ImageHeaderParser parser) throws IOException;
233240
}
234241

235242
private interface OrientationReader {
236-
int getOrientation(ImageHeaderParser parser) throws IOException;
243+
int getOrientationAndRewind(ImageHeaderParser parser) throws IOException;
237244
}
238245
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
package com.bumptech.glide.load;
2+
3+
import static com.google.common.truth.Truth.assertThat;
4+
import static org.junit.Assume.assumeTrue;
5+
6+
import android.content.Context;
7+
import android.os.ParcelFileDescriptor;
8+
import androidx.annotation.NonNull;
9+
import androidx.test.core.app.ApplicationProvider;
10+
import androidx.test.ext.junit.runners.AndroidJUnit4;
11+
import com.bumptech.glide.load.data.ParcelFileDescriptorRewinder;
12+
import com.bumptech.glide.load.engine.bitmap_recycle.ArrayPool;
13+
import com.bumptech.glide.load.engine.bitmap_recycle.LruArrayPool;
14+
import com.bumptech.glide.util.ByteBufferUtil;
15+
import java.io.ByteArrayInputStream;
16+
import java.io.File;
17+
import java.io.FileOutputStream;
18+
import java.io.IOException;
19+
import java.io.InputStream;
20+
import java.io.OutputStream;
21+
import java.nio.ByteBuffer;
22+
import java.util.ArrayList;
23+
import java.util.Arrays;
24+
import java.util.List;
25+
import org.junit.Before;
26+
import org.junit.Test;
27+
import org.junit.runner.RunWith;
28+
29+
@RunWith(AndroidJUnit4.class)
30+
public class ImageHeaderParserUtilsTest {
31+
private final List<FakeImageHeaderParser> fakeParsers =
32+
Arrays.asList(new FakeImageHeaderParser(), new FakeImageHeaderParser());
33+
private List<ImageHeaderParser> parsers;
34+
private final Context context = ApplicationProvider.getApplicationContext();
35+
private final byte[] expectedData = new byte[] {0, 1, 2, 3, 4, 5, 6, 7, 8};
36+
private final LruArrayPool lruArrayPool = new LruArrayPool();
37+
38+
@Before
39+
public void setUp() {
40+
parsers = new ArrayList<ImageHeaderParser>();
41+
for (FakeImageHeaderParser parser : fakeParsers) {
42+
parsers.add(parser);
43+
}
44+
}
45+
46+
@Test
47+
public void getType_withTwoParsers_andStream_rewindsBeforeEachParser() throws IOException {
48+
ImageHeaderParserUtils.getType(parsers, new ByteArrayInputStream(expectedData), lruArrayPool);
49+
50+
assertAllParsersReceivedTheSameData();
51+
}
52+
53+
@Test
54+
public void getType_withTwoParsers_andByteBuffer_rewindsBeforeEachParser() throws IOException {
55+
ImageHeaderParserUtils.getType(parsers, ByteBuffer.wrap(expectedData));
56+
57+
assertAllParsersReceivedTheSameData();
58+
}
59+
60+
@Test
61+
public void getType_withTwoParsers_andFileDescriptor_rewindsBeforeEachParser()
62+
throws IOException {
63+
// This test can't work if file descriptor rewinding isn't supported. Sadly that means this
64+
// test doesn't work in Robolectric.
65+
assumeTrue(ParcelFileDescriptorRewinder.isSupported());
66+
67+
ParcelFileDescriptor fileDescriptor = null;
68+
try {
69+
fileDescriptor = asFileDescriptor(expectedData);
70+
ParcelFileDescriptorRewinder rewinder = new ParcelFileDescriptorRewinder(fileDescriptor);
71+
ImageHeaderParserUtils.getType(parsers, rewinder, lruArrayPool);
72+
} finally {
73+
if (fileDescriptor != null) {
74+
fileDescriptor.close();
75+
}
76+
}
77+
78+
assertAllParsersReceivedTheSameData();
79+
}
80+
81+
@Test
82+
public void getOrientation_withTwoParsers_andStream_rewindsBeforeEachParser() throws IOException {
83+
ImageHeaderParserUtils.getOrientation(
84+
parsers, new ByteArrayInputStream(expectedData), lruArrayPool);
85+
86+
assertAllParsersReceivedTheSameData();
87+
}
88+
89+
@Test
90+
public void getOrientation_withTwoParsers_andByteBuffer_rewindsBeforeEachParser()
91+
throws IOException {
92+
ImageHeaderParserUtils.getOrientation(parsers, ByteBuffer.wrap(expectedData), lruArrayPool);
93+
94+
assertAllParsersReceivedTheSameData();
95+
}
96+
97+
@Test
98+
public void getOrientation_withTwoParsers_andFileDescriptor_rewindsBeforeEachParser()
99+
throws IOException {
100+
// This test can't work if file descriptor rewinding isn't supported. Sadly that means this
101+
// test doesn't work in Robolectric.
102+
assumeTrue(ParcelFileDescriptorRewinder.isSupported());
103+
ParcelFileDescriptor fileDescriptor = null;
104+
try {
105+
fileDescriptor = asFileDescriptor(expectedData);
106+
ParcelFileDescriptorRewinder rewinder = new ParcelFileDescriptorRewinder(fileDescriptor);
107+
ImageHeaderParserUtils.getOrientation(parsers, rewinder, lruArrayPool);
108+
} finally {
109+
if (fileDescriptor != null) {
110+
fileDescriptor.close();
111+
}
112+
}
113+
114+
assertAllParsersReceivedTheSameData();
115+
}
116+
117+
private void assertAllParsersReceivedTheSameData() {
118+
for (FakeImageHeaderParser parser : fakeParsers) {
119+
assertThat(parser.data).isNotNull();
120+
assertThat(parser.data).asList().containsExactlyElementsIn(asList(expectedData)).inOrder();
121+
}
122+
}
123+
124+
private static List<Byte> asList(byte[] data) {
125+
List<Byte> result = new ArrayList<>();
126+
for (byte item : data) {
127+
result.add(item);
128+
}
129+
return result;
130+
}
131+
132+
private ParcelFileDescriptor asFileDescriptor(byte[] data) throws IOException {
133+
File file = new File(context.getCacheDir(), "temp");
134+
OutputStream os = null;
135+
try {
136+
os = new FileOutputStream(file);
137+
os.write(data);
138+
os.close();
139+
} finally {
140+
if (os != null) {
141+
os.close();
142+
}
143+
}
144+
return ParcelFileDescriptor.open(file, ParcelFileDescriptor.MODE_READ_ONLY);
145+
}
146+
147+
private static final class FakeImageHeaderParser implements ImageHeaderParser {
148+
149+
private byte[] data;
150+
151+
private void readData(InputStream is) throws IOException {
152+
readData(ByteBufferUtil.fromStream(is));
153+
}
154+
155+
// This is rather roundabout, but it's a simple way of reading the remaining data in the buffer.
156+
private void readData(ByteBuffer byteBuffer) {
157+
158+
byte[] data = new byte[byteBuffer.remaining()];
159+
// A 0 length means we read no data. If we try to pass this to ByteBuffer it will throw. We'd
160+
// rather not get that obscure exception and instead have an assertion above trigger because
161+
// we didn't read enough data. So we work around the exception here if we have no data to
162+
// read.
163+
if (data.length != 0) {
164+
byteBuffer.get(data, byteBuffer.position(), byteBuffer.remaining());
165+
}
166+
this.data = data;
167+
}
168+
169+
@NonNull
170+
@Override
171+
public ImageType getType(@NonNull InputStream is) throws IOException {
172+
readData(is);
173+
return ImageType.UNKNOWN;
174+
}
175+
176+
@NonNull
177+
@Override
178+
public ImageType getType(@NonNull ByteBuffer byteBuffer) throws IOException {
179+
readData(byteBuffer);
180+
return ImageType.UNKNOWN;
181+
}
182+
183+
@Override
184+
public int getOrientation(@NonNull InputStream is, @NonNull ArrayPool byteArrayPool)
185+
throws IOException {
186+
readData(is);
187+
return ImageHeaderParser.UNKNOWN_ORIENTATION;
188+
}
189+
190+
@Override
191+
public int getOrientation(@NonNull ByteBuffer byteBuffer, @NonNull ArrayPool byteArrayPool)
192+
throws IOException {
193+
readData(byteBuffer);
194+
return ImageHeaderParser.UNKNOWN_ORIENTATION;
195+
}
196+
}
197+
}

0 commit comments

Comments
 (0)
Please sign in to comment.