From 5ce40fa04b69c2bdd56f8cf8c15954315297710a Mon Sep 17 00:00:00 2001 From: eguven Date: Tue, 23 Jan 2018 15:06:13 -0800 Subject: [PATCH] Fix CacheDataSource trying to read more after EOS setBytesRemaining() doesn't work when called after closeCurrentSource() ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=182999254 --- .../upstream/cache/CacheDataSource.java | 12 ++-- .../upstream/cache/CacheDataSourceTest.java | 58 +++++++++++++------ 2 files changed, 45 insertions(+), 25 deletions(-) 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 210a0d1195..30b926348d 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 @@ -240,14 +240,12 @@ public final class CacheDataSource implements DataSource { if (bytesRemaining != C.LENGTH_UNSET) { bytesRemaining -= bytesRead; } - } else { + } else if (currentDataSpecLengthUnset) { + setBytesRemaining(0); + } else if (bytesRemaining > 0 || bytesRemaining == C.LENGTH_UNSET) { closeCurrentSource(); - if (currentDataSpecLengthUnset) { - setBytesRemaining(0); - } else if (bytesRemaining > 0 || bytesRemaining == C.LENGTH_UNSET) { - openNextSource(false); - return read(buffer, offset, readLength); - } + openNextSource(false); + return read(buffer, offset, readLength); } return bytesRead; } catch (IOException e) { 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 3e9e26372e..bd8752355c 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 @@ -15,10 +15,8 @@ */ package com.google.android.exoplayer2.upstream.cache; -import static com.google.android.exoplayer2.C.LENGTH_UNSET; import static com.google.android.exoplayer2.upstream.cache.CacheAsserts.assertCacheEmpty; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.fail; import android.net.Uri; @@ -35,7 +33,6 @@ import java.util.Arrays; import java.util.NavigableSet; import org.junit.After; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.robolectric.RobolectricTestRunner; @@ -97,10 +94,8 @@ public final class CacheDataSourceTest { assertCacheAndRead(false, true); } - // Disabled test as we don't support caching of definitely unknown length content - @Ignore @Test - public void disabledTestCacheAndReadUnboundedRequestUnknownLength() throws Exception { + public void testCacheAndReadUnboundedRequestUnknownLength() throws Exception { assertCacheAndRead(true, true); } @@ -130,7 +125,7 @@ public final class CacheDataSourceTest { // Read partial at EOS but don't cross it so length is unknown CacheDataSource cacheDataSource = createCacheDataSource(false, true); assertReadData(cacheDataSource, true, TEST_DATA.length - 2, 2); - assertThat(cache.getContentLength(testDataKey)).isEqualTo(LENGTH_UNSET); + assertThat(cache.getContentLength(testDataKey)).isEqualTo(C.LENGTH_UNSET); // Now do an unbounded request for whole data. This will cause a bounded request from upstream. // End of data from upstream shouldn't be mixed up with EOS and cause length set wrong. @@ -140,21 +135,48 @@ public final class CacheDataSourceTest { // Now the length set correctly do an unbounded request with offset assertThat( cacheDataSource.open( - new DataSpec(testDataUri, TEST_DATA.length - 2, LENGTH_UNSET, testDataKey))) + new DataSpec(testDataUri, TEST_DATA.length - 2, C.LENGTH_UNSET, testDataKey))) .isEqualTo(2); // An unbounded request with offset for not cached content assertThat( cacheDataSource.open( - new DataSpec(Uri.parse("notCachedUri"), TEST_DATA.length - 2, LENGTH_UNSET, null))) - .isEqualTo(LENGTH_UNSET); + new DataSpec( + Uri.parse("notCachedUri"), TEST_DATA.length - 2, C.LENGTH_UNSET, null))) + .isEqualTo(C.LENGTH_UNSET); + } + + @Test + public void testCantResolvedLengthContentReadInOneConnectionAndLengthIsResolved() + throws Exception { + FakeDataSource upstream = new FakeDataSource(); + upstream + .getDataSet() + .newData(testDataUri) + .appendReadData(TEST_DATA) + .setSimulateUnknownLength(true); + CacheDataSource cacheDataSource = new CacheDataSource(cache, upstream, 0); + + cacheDataSource.open(new DataSpec(testDataUri, 0, C.LENGTH_UNSET, testDataKey)); + TestUtil.readToEnd(cacheDataSource); + cacheDataSource.close(); + + assertThat(upstream.getAndClearOpenedDataSpecs()).hasLength(1); + assertThat(cache.getContentLength(testDataKey)).isEqualTo(TEST_DATA.length); } @Test public void testIgnoreCacheForUnsetLengthRequests() throws Exception { - CacheDataSource cacheDataSource = createCacheDataSource(false, true, - CacheDataSource.FLAG_IGNORE_CACHE_FOR_UNSET_LENGTH_REQUESTS); - assertReadData(cacheDataSource, true, 0, C.LENGTH_UNSET); + FakeDataSource upstream = new FakeDataSource(); + upstream.getDataSet().setData(testDataUri, TEST_DATA); + CacheDataSource cacheDataSource = + new CacheDataSource( + cache, upstream, CacheDataSource.FLAG_IGNORE_CACHE_FOR_UNSET_LENGTH_REQUESTS); + + cacheDataSource.open(new DataSpec(testDataUri, 0, C.LENGTH_UNSET, testDataKey)); + TestUtil.readToEnd(cacheDataSource); + cacheDataSource.close(); + assertThat(cache.getKeys()).isEmpty(); } @@ -347,10 +369,8 @@ public final class CacheDataSourceTest { boolean unboundedRequest, boolean unknownLength) throws IOException { int length = unboundedRequest ? C.LENGTH_UNSET : TEST_DATA.length; assertReadData(cacheDataSource, unknownLength, 0, length); - assertWithMessage( - "When the range specified, CacheDataSource doesn't reach EOS so shouldn't " - + "cache content length") - .that(cache.getContentLength(testDataKey)) + // If !unboundedRequest, CacheDataSource doesn't reach EOS so shouldn't cache content length + assertThat(cache.getContentLength(testDataKey)) .isEqualTo(!unboundedRequest ? C.LENGTH_UNSET : TEST_DATA.length); } @@ -360,7 +380,9 @@ public final class CacheDataSourceTest { if (length != C.LENGTH_UNSET) { testDataLength = Math.min(testDataLength, length); } - DataSpec dataSpec = new DataSpec(testDataUri, position, length, testDataKey); + DataSpec dataSpec = + new DataSpec( + testDataUri, position, length, testDataKey, DataSpec.FLAG_ALLOW_CACHING_UNKNOWN_LENGTH); assertThat(cacheDataSource.open(dataSpec)).isEqualTo(unknownLength ? length : testDataLength); cacheDataSource.close();