diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/ContentDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/ContentDataSource.java index 75359504bd..0713cfabc7 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/ContentDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/ContentDataSource.java @@ -72,48 +72,61 @@ public final class ContentDataSource extends BaseDataSource { if (assetFileDescriptor == null) { throw new FileNotFoundException("Could not open file descriptor for: " + uri); } + + long assetFileDescriptorLength = assetFileDescriptor.getLength(); FileInputStream inputStream = new FileInputStream(assetFileDescriptor.getFileDescriptor()); this.inputStream = inputStream; - long assetStartOffset = assetFileDescriptor.getStartOffset(); - long skipped = inputStream.skip(assetStartOffset + dataSpec.position) - assetStartOffset; - if (skipped != dataSpec.position) { - // We expect the skip to be satisfied in full. If it isn't then we're probably trying to - // skip beyond the end of the data. + // We can't rely only on the "skipped < dataSpec.position" check below to detect whether the + // position is beyond the end of the asset being read. This is because the file may contain + // multiple assets, and there's nothing to prevent InputStream.skip() from succeeding by + // skipping into the data of the next asset. Hence we also need to check against the asset + // length explicitly, which is guaranteed to be set unless the asset extends to the end of the + // file. + if (assetFileDescriptorLength != AssetFileDescriptor.UNKNOWN_LENGTH + && dataSpec.position > assetFileDescriptorLength) { throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } - if (dataSpec.length != C.LENGTH_UNSET) { - bytesRemaining = dataSpec.length; - } else { - long assetFileDescriptorLength = assetFileDescriptor.getLength(); - if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) { - // The asset must extend to the end of the file. If FileInputStream.getChannel().size() - // returns 0 then the remaining length cannot be determined. - FileChannel channel = inputStream.getChannel(); - long channelSize = channel.size(); - if (channelSize == 0) { - bytesRemaining = C.LENGTH_UNSET; - } else { - bytesRemaining = channelSize - channel.position(); - if (bytesRemaining < 0) { - throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); - } - } + long assetFileDescriptorOffset = assetFileDescriptor.getStartOffset(); + long skipped = + inputStream.skip(assetFileDescriptorOffset + dataSpec.position) + - assetFileDescriptorOffset; + if (skipped != dataSpec.position) { + // We expect the skip to be satisfied in full. If it isn't then we're probably trying to + // read beyond the end of the last resource in the file. + throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); + } + if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) { + // The asset must extend to the end of the file. We can try and resolve the length with + // FileInputStream.getChannel().size(). + FileChannel channel = inputStream.getChannel(); + long channelSize = channel.size(); + if (channelSize == 0) { + bytesRemaining = C.LENGTH_UNSET; } else { - bytesRemaining = assetFileDescriptorLength - skipped; + bytesRemaining = channelSize - channel.position(); if (bytesRemaining < 0) { + // The skip above was satisfied in full, but skipped beyond the end of the file. throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } } + } else { + bytesRemaining = assetFileDescriptorLength - skipped; + if (bytesRemaining < 0) { + throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); + } } } catch (IOException e) { throw new ContentDataSourceException(e); } + if (dataSpec.length != C.LENGTH_UNSET) { + bytesRemaining = + bytesRemaining == C.LENGTH_UNSET ? dataSpec.length : min(bytesRemaining, dataSpec.length); + } opened = true; transferStarted(dataSpec); - - return bytesRemaining; + return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : bytesRemaining; } @Override diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/RawResourceDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/RawResourceDataSource.java index 2568d49c3a..ccdf7975a6 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/RawResourceDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/RawResourceDataSource.java @@ -31,6 +31,7 @@ import java.io.EOFException; import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; +import java.nio.channels.FileChannel; /** * A {@link DataSource} for reading a raw resource inside the APK. @@ -149,6 +150,7 @@ public final class RawResourceDataSource extends BaseDataSource { long assetFileDescriptorLength = assetFileDescriptor.getLength(); FileInputStream inputStream = new FileInputStream(assetFileDescriptor.getFileDescriptor()); this.inputStream = inputStream; + try { // We can't rely only on the "skipped < dataSpec.position" check below to detect whether the // position is beyond the end of the resource being read. This is because the file will @@ -160,31 +162,45 @@ public final class RawResourceDataSource extends BaseDataSource { && dataSpec.position > assetFileDescriptorLength) { throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } - inputStream.skip(assetFileDescriptor.getStartOffset()); - long skipped = inputStream.skip(dataSpec.position); - if (skipped < dataSpec.position) { + long assetFileDescriptorOffset = assetFileDescriptor.getStartOffset(); + long skipped = + inputStream.skip(assetFileDescriptorOffset + dataSpec.position) + - assetFileDescriptorOffset; + if (skipped != dataSpec.position) { // We expect the skip to be satisfied in full. If it isn't then we're probably trying to // read beyond the end of the last resource in the file. throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } + if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) { + // The asset must extend to the end of the file. We can try and resolve the length with + // FileInputStream.getChannel().size(). + FileChannel channel = inputStream.getChannel(); + if (channel.size() == 0) { + bytesRemaining = C.LENGTH_UNSET; + } else { + bytesRemaining = channel.size() - channel.position(); + if (bytesRemaining < 0) { + // The skip above was satisfied in full, but skipped beyond the end of the file. + throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); + } + } + } else { + bytesRemaining = assetFileDescriptorLength - skipped; + if (bytesRemaining < 0) { + throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); + } + } } catch (IOException e) { throw new RawResourceDataSourceException(e); } if (dataSpec.length != C.LENGTH_UNSET) { - bytesRemaining = dataSpec.length; - } else { - // If the length is UNKNOWN_LENGTH then the asset extends to the end of the file. bytesRemaining = - assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH - ? C.LENGTH_UNSET - : (assetFileDescriptorLength - dataSpec.position); + bytesRemaining == C.LENGTH_UNSET ? dataSpec.length : min(bytesRemaining, dataSpec.length); } - opened = true; transferStarted(dataSpec); - - return bytesRemaining; + return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : bytesRemaining; } @Override diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java index c2bbdbb893..2c1b52ad3b 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java @@ -388,8 +388,9 @@ public final class CacheDataSource implements DataSource { @Nullable private Uri actualUri; @Nullable private DataSpec requestDataSpec; + @Nullable private DataSpec currentDataSpec; @Nullable private DataSource currentDataSource; - private boolean currentDataSpecLengthUnset; + private long currentDataSourceBytesRead; private long readPosition; private long bytesRemaining; @Nullable private CacheSpan currentHoleSpan; @@ -565,19 +566,27 @@ public final class CacheDataSource implements DataSource { notifyCacheIgnored(reason); } - if (dataSpec.length != C.LENGTH_UNSET || currentRequestIgnoresCache) { - bytesRemaining = dataSpec.length; + if (currentRequestIgnoresCache) { + bytesRemaining = C.LENGTH_UNSET; } else { bytesRemaining = ContentMetadata.getContentLength(cache.getContentMetadata(key)); if (bytesRemaining != C.LENGTH_UNSET) { bytesRemaining -= dataSpec.position; - if (bytesRemaining <= 0) { + if (bytesRemaining < 0) { throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } } } - openNextSource(requestDataSpec, false); - return bytesRemaining; + if (dataSpec.length != C.LENGTH_UNSET) { + bytesRemaining = + bytesRemaining == C.LENGTH_UNSET + ? dataSpec.length + : min(bytesRemaining, dataSpec.length); + } + if (bytesRemaining > 0 || bytesRemaining == C.LENGTH_UNSET) { + openNextSource(requestDataSpec, false); + } + return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : bytesRemaining; } catch (Throwable e) { handleBeforeThrow(e); throw e; @@ -587,6 +596,7 @@ public final class CacheDataSource implements DataSource { @Override public int read(byte[] buffer, int offset, int readLength) throws IOException { DataSpec requestDataSpec = checkNotNull(this.requestDataSpec); + DataSpec currentDataSpec = checkNotNull(this.currentDataSpec); if (readLength == 0) { return 0; } @@ -603,10 +613,16 @@ public final class CacheDataSource implements DataSource { totalCachedBytesRead += bytesRead; } readPosition += bytesRead; + currentDataSourceBytesRead += bytesRead; if (bytesRemaining != C.LENGTH_UNSET) { bytesRemaining -= bytesRead; } - } else if (currentDataSpecLengthUnset) { + } else if (isReadingFromUpstream() + && (currentDataSpec.length == C.LENGTH_UNSET + || currentDataSourceBytesRead < currentDataSpec.length)) { + // We've encountered RESULT_END_OF_INPUT from the upstream DataSource at a position not + // imposed by the current DataSpec. This must mean that we've reached the end of the + // resource. setNoBytesRemainingAndMaybeStoreLength(castNonNull(requestDataSpec.key)); } else if (bytesRemaining > 0 || bytesRemaining == C.LENGTH_UNSET) { closeCurrentSource(); @@ -615,7 +631,16 @@ public final class CacheDataSource implements DataSource { } return bytesRead; } catch (IOException e) { - if (currentDataSpecLengthUnset && DataSourceException.isCausedByPositionOutOfRange(e)) { + // TODO: This is not correct, because position-out-of-range exceptions should only be thrown + // if the requested position is more than one byte beyond the end of the resource. Conversely, + // this code is assuming that a position-out-of-range exception indicates the requested + // position is exactly one byte beyond the end of the resource, which is not a case for which + // this type of exception should be thrown. This exception handling may be required for + // interop with current HttpDataSource implementations that do (incorrectly) throw a + // position-out-of-range exception at this position. It should be removed when the + // HttpDataSource implementations are fixed. + if (currentDataSpec.length == C.LENGTH_UNSET + && DataSourceException.isCausedByPositionOutOfRange(e)) { setNoBytesRemainingAndMaybeStoreLength(castNonNull(requestDataSpec.key)); return C.RESULT_END_OF_INPUT; } @@ -760,12 +785,13 @@ public final class CacheDataSource implements DataSource { currentHoleSpan = nextSpan; } currentDataSource = nextDataSource; - currentDataSpecLengthUnset = nextDataSpec.length == C.LENGTH_UNSET; + currentDataSpec = nextDataSpec; + currentDataSourceBytesRead = 0; long resolvedLength = nextDataSource.open(nextDataSpec); // Update bytesRemaining, actualUri and (if writing to cache) the cache metadata. ContentMetadataMutations mutations = new ContentMetadataMutations(); - if (currentDataSpecLengthUnset && resolvedLength != C.LENGTH_UNSET) { + if (nextDataSpec.length == C.LENGTH_UNSET && resolvedLength != C.LENGTH_UNSET) { bytesRemaining = resolvedLength; ContentMetadataMutations.setContentLength(mutations, readPosition + bytesRemaining); } @@ -816,8 +842,8 @@ public final class CacheDataSource implements DataSource { try { currentDataSource.close(); } finally { + currentDataSpec = null; currentDataSource = null; - currentDataSpecLengthUnset = false; if (currentHoleSpan != null) { cache.releaseHoleSpan(currentHoleSpan); currentHoleSpan = null; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheWriter.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheWriter.java index 0d2266b7db..62a017df8e 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheWriter.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheWriter.java @@ -23,6 +23,7 @@ import com.google.android.exoplayer2.upstream.DataSpec; import com.google.android.exoplayer2.util.PriorityTaskManager; import com.google.android.exoplayer2.util.PriorityTaskManager.PriorityTooLowException; import com.google.android.exoplayer2.util.Util; +import java.io.EOFException; import java.io.IOException; import java.io.InterruptedIOException; @@ -142,6 +143,14 @@ public final class CacheWriter { nextPosition += readBlockToCache(nextPosition, nextRequestLength); } } + + // TODO: Remove allowShortContent parameter, this code block, and return the number of bytes + // cached. The caller can then check whether fewer bytes were cached than were requested. + if (!allowShortContent + && dataSpec.length != C.LENGTH_UNSET + && nextPosition != dataSpec.position + dataSpec.length) { + throw new EOFException(); + } } /** diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java index dd72f2cf30..8acc33e640 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java @@ -154,7 +154,7 @@ public final class CacheDataSourceTest { try { cacheDataSource = createCacheDataSource(/* setReadException= */ false, /* unknownLength= */ false); - cacheDataSource.open(buildDataSpec(TEST_DATA.length, /* length= */ 1, defaultCacheKey)); + cacheDataSource.open(buildDataSpec(TEST_DATA.length + 1, /* length= */ 1, defaultCacheKey)); fail(); } catch (IOException e) { // Expected. @@ -204,7 +204,7 @@ public final class CacheDataSourceTest { cacheDataSource = createCacheDataSource( /* setReadException= */ false, /* unknownLength= */ false, cacheKeyFactory); - cacheDataSource.open(buildDataSpec(TEST_DATA.length, /* length= */ 1, customCacheKey)); + cacheDataSource.open(buildDataSpec(TEST_DATA.length + 1, /* length= */ 1, customCacheKey)); fail(); } catch (IOException e) { // Expected. @@ -257,7 +257,7 @@ public final class CacheDataSourceTest { cacheDataSource = createCacheDataSource( /* setReadException= */ false, /* unknownLength= */ false, cacheKeyFactory); - cacheDataSource.open(buildDataSpec(TEST_DATA.length, /* length= */ 1, customCacheKey)); + cacheDataSource.open(buildDataSpec(TEST_DATA.length + 1, /* length= */ 1, customCacheKey)); fail(); } catch (IOException e) { // Expected. diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheWriterTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheWriterTest.java index 265ac7c28f..26c856b1a0 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheWriterTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheWriterTest.java @@ -27,16 +27,15 @@ import com.google.android.exoplayer2.testutil.FailOnCloseDataSink; import com.google.android.exoplayer2.testutil.FakeDataSet; import com.google.android.exoplayer2.testutil.FakeDataSource; import com.google.android.exoplayer2.testutil.TestUtil; -import com.google.android.exoplayer2.upstream.DataSourceException; import com.google.android.exoplayer2.upstream.DataSpec; import com.google.android.exoplayer2.upstream.FileDataSource; import com.google.android.exoplayer2.util.Util; +import java.io.EOFException; import java.io.File; import java.io.IOException; import java.util.concurrent.atomic.AtomicBoolean; import org.junit.After; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.MockitoAnnotations; @@ -175,7 +174,6 @@ public final class CacheWriterTest { assertCachedData(cache, fakeDataSet); } - @Ignore("Currently broken. See https://github.com/google/ExoPlayer/issues/7326.") @Test public void cacheLengthExceedsActualDataLength() throws Exception { FakeDataSet fakeDataSet = new FakeDataSet().setRandomData("test_data", 100); @@ -206,18 +204,14 @@ public final class CacheWriterTest { Uri testUri = Uri.parse("test_data"); DataSpec dataSpec = new DataSpec(testUri, /* position= */ 0, /* length= */ 1000); - IOException exception = - assertThrows( - IOException.class, - () -> - new CacheWriter( - new CacheDataSource(cache, dataSource), - dataSpec, - /* allowShortContent= */ false, - /* temporaryBuffer= */ null, - /* progressListener= */ null) - .cache()); - assertThat(DataSourceException.isCausedByPositionOutOfRange(exception)).isTrue(); + CacheWriter cacheWriter = + new CacheWriter( + new CacheDataSource(cache, dataSource), + dataSpec, + /* allowShortContent= */ false, + /* temporaryBuffer= */ null, + /* progressListener= */ null); + assertThrows(EOFException.class, cacheWriter::cache); } @Test diff --git a/testutils/src/main/java/com/google/android/exoplayer2/testutil/DataSourceContractTest.java b/testutils/src/main/java/com/google/android/exoplayer2/testutil/DataSourceContractTest.java index 8edd7c2058..5a15f46919 100644 --- a/testutils/src/main/java/com/google/android/exoplayer2/testutil/DataSourceContractTest.java +++ b/testutils/src/main/java/com/google/android/exoplayer2/testutil/DataSourceContractTest.java @@ -233,16 +233,22 @@ public abstract class DataSourceContractTest { new DataSpec.Builder().setUri(resource.getUri()).setPosition(resourceLength).build(); try { long length = dataSource.open(dataSpec); - // TODO: For any cases excluded from the requirement that a position-out-of-range exception - // is thrown, decide what the allowed behavior should be for the first read, and assert it. - + // The DataSource.open() contract requires the returned length to equal the length in the + // DataSpec if set. This is true even though the DataSource implementation may know that + // fewer bytes will be read in this case. if (length != C.LENGTH_UNSET) { assertThat(length).isEqualTo(0); } + + try { + byte[] data = + unboundedReadsAreIndefinite() ? Util.EMPTY_BYTE_ARRAY : Util.readToEnd(dataSource); + assertThat(data).isEmpty(); + } catch (IOException e) { + // TODO: Remove this catch and require that implementations do not throw. + } } catch (IOException e) { - // TODO: Decide whether to assert that a position-out-of-range exception must or must not be - // thrown (with exclusions if necessary), rather than just asserting it must be a - // position-out-of-range exception *if* one is thrown at all. + // TODO: Remove this catch and require that implementations do not throw. assertThat(DataSourceException.isCausedByPositionOutOfRange(e)).isTrue(); } finally { dataSource.close(); @@ -270,17 +276,20 @@ public abstract class DataSourceContractTest { .build(); try { long length = dataSource.open(dataSpec); - // TODO: For any cases excluded from the requirement that a position-out-of-range exception - // is thrown, decide what the allowed behavior should be for the first read, and assert it. - // The DataSource.open() contract requires the returned length to equal the length in the // DataSpec if set. This is true even though the DataSource implementation may know that // fewer bytes will be read in this case. assertThat(length).isEqualTo(1); + + try { + byte[] data = + unboundedReadsAreIndefinite() ? Util.EMPTY_BYTE_ARRAY : Util.readToEnd(dataSource); + assertThat(data).isEmpty(); + } catch (IOException e) { + // TODO: Remove this catch and require that implementations do not throw. + } } catch (IOException e) { - // TODO: Decide whether to assert that a position-out-of-range exception must or must not be - // thrown (with exclusions if necessary), rather than just asserting it must be a - // position-out-of-range exception *if* one is thrown at all. + // TODO: Remove this catch and require that implementations do not throw. assertThat(DataSourceException.isCausedByPositionOutOfRange(e)).isTrue(); } finally { dataSource.close(); diff --git a/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeDataSource.java b/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeDataSource.java index ee141547dd..ec03f0443c 100644 --- a/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeDataSource.java +++ b/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeDataSource.java @@ -117,7 +117,7 @@ public class FakeDataSource extends BaseDataSource { throw new IOException("Data is empty: " + dataSpec.uri); } - if (dataSpec.position >= totalLength) { + if (dataSpec.position > totalLength) { throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE); } diff --git a/testutils/src/test/java/com/google/android/exoplayer2/testutil/FakeDataSourceTest.java b/testutils/src/test/java/com/google/android/exoplayer2/testutil/FakeDataSourceTest.java index ed4266b4e2..1a03134d98 100644 --- a/testutils/src/test/java/com/google/android/exoplayer2/testutil/FakeDataSourceTest.java +++ b/testutils/src/test/java/com/google/android/exoplayer2/testutil/FakeDataSourceTest.java @@ -211,22 +211,6 @@ public final class FakeDataSourceTest { } finally { dataSource.close(); } - - // DataSpec out of bounds. - dataSource = - new FakeDataSource( - new FakeDataSet() - .newDefaultData() - .appendReadData(TestUtil.buildTestData(/* length= */ 10)) - .endData()); - try { - dataSource.open(new DataSpec(uri, /* position= */ 10, C.LENGTH_UNSET)); - fail("IOException expected."); - } catch (IOException e) { - // Expected. - } finally { - dataSource.close(); - } } private static void assertBuffer(byte[] expected) {