From 42ba6abf5a548c05120c586edf2af411f579ca14 Mon Sep 17 00:00:00 2001 From: eguven Date: Wed, 29 May 2019 20:42:25 +0100 Subject: [PATCH] Fix CacheUtil.cache() use too much data cache() opens all connections with unset length to avoid position errors. This makes more data then needed to be downloading by the underlying network stack. This fix makes makes it open connections for only required length. Issue:#5927 PiperOrigin-RevId: 250546175 --- RELEASENOTES.md | 11 ++-- .../upstream/cache/CacheDataSource.java | 18 +----- .../exoplayer2/upstream/cache/CacheUtil.java | 63 +++++++++++++------ .../exoplayer2/testutil/CacheAsserts.java | 16 +++-- 4 files changed, 62 insertions(+), 46 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 3b1ccc3d432..b73d95ba034 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -26,16 +26,19 @@ ([#5784](https://github.com/google/ExoPlayer/issues/5784)). * Add a workaround for broken raw audio decoding on Oppo R9 ([#5782](https://github.com/google/ExoPlayer/issues/5782)). -* Offline: Add Scheduler implementation which uses WorkManager. +* Offline: + * Add Scheduler implementation which uses WorkManager. + * Prevent unexpected `DownloadHelper.Callback.onPrepared` callbacks after the + preparation of the `DownloadHelper` failed + ([#5915](https://github.com/google/ExoPlayer/issues/5915)). + * Fix CacheUtil.cache() use too much data + ([#5927](https://github.com/google/ExoPlayer/issues/5927)). * Add a playWhenReady flag to MediaSessionConnector.PlaybackPreparer methods to indicate whether a controller sent a play or only a prepare command. This allows to take advantage of decoder reuse with the MediaSessionConnector ([#5891](https://github.com/google/ExoPlayer/issues/5891)). * Add ProgressUpdateListener to PlayerControlView ([#5834](https://github.com/google/ExoPlayer/issues/5834)). -* Prevent unexpected `DownloadHelper.Callback.onPrepared` callbacks after the - preparation of the `DownloadHelper` failed - ([#5915](https://github.com/google/ExoPlayer/issues/5915)). * Allow enabling decoder fallback with `DefaultRenderersFactory` ([#5942](https://github.com/google/ExoPlayer/issues/5942)). 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 058489f8f01..69bb99451ed 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 @@ -136,7 +136,7 @@ public interface EventListener { private boolean currentDataSpecLengthUnset; @Nullable private Uri uri; @Nullable private Uri actualUri; - private @HttpMethod int httpMethod; + @HttpMethod private int httpMethod; private int flags; @Nullable private String key; private long readPosition; @@ -319,7 +319,7 @@ public int read(byte[] buffer, int offset, int readLength) throws IOException { } return bytesRead; } catch (IOException e) { - if (currentDataSpecLengthUnset && isCausedByPositionOutOfRange(e)) { + if (currentDataSpecLengthUnset && CacheUtil.isCausedByPositionOutOfRange(e)) { setNoBytesRemainingAndMaybeStoreLength(); return C.RESULT_END_OF_INPUT; } @@ -485,20 +485,6 @@ private static Uri getRedirectedUriOrDefault(Cache cache, String key, Uri defaul return redirectedUri != null ? redirectedUri : defaultUri; } - private static boolean isCausedByPositionOutOfRange(IOException e) { - Throwable cause = e; - while (cause != null) { - if (cause instanceof DataSourceException) { - int reason = ((DataSourceException) cause).reason; - if (reason == DataSourceException.POSITION_OUT_OF_RANGE) { - return true; - } - } - cause = cause.getCause(); - } - return false; - } - private boolean isReadingFromUpstream() { return !isReadingFromCache(); } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheUtil.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheUtil.java index 219d736835e..9c80becdebc 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheUtil.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheUtil.java @@ -20,6 +20,7 @@ import android.util.Pair; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.upstream.DataSource; +import com.google.android.exoplayer2.upstream.DataSourceException; import com.google.android.exoplayer2.upstream.DataSpec; import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.PriorityTaskManager; @@ -195,37 +196,42 @@ public static void cache( long contentLength = ContentMetadata.getContentLength(cache.getContentMetadata(key)); bytesLeft = contentLength == C.LENGTH_UNSET ? C.LENGTH_UNSET : contentLength - position; } + boolean lengthUnset = bytesLeft == C.LENGTH_UNSET; while (bytesLeft != 0) { throwExceptionIfInterruptedOrCancelled(isCanceled); long blockLength = - cache.getCachedLength( - key, position, bytesLeft != C.LENGTH_UNSET ? bytesLeft : Long.MAX_VALUE); + cache.getCachedLength(key, position, lengthUnset ? Long.MAX_VALUE : bytesLeft); if (blockLength > 0) { // Skip already cached data. } else { // There is a hole in the cache which is at least "-blockLength" long. blockLength = -blockLength; + long length = blockLength == Long.MAX_VALUE ? C.LENGTH_UNSET : blockLength; + boolean isLastBlock = length == bytesLeft; long read = readAndDiscard( dataSpec, position, - blockLength, + length, dataSource, buffer, priorityTaskManager, priority, progressNotifier, + isLastBlock, isCanceled); if (read < blockLength) { // Reached to the end of the data. - if (enableEOFException && bytesLeft != C.LENGTH_UNSET) { + if (enableEOFException && !lengthUnset) { throw new EOFException(); } break; } } position += blockLength; - bytesLeft -= bytesLeft == C.LENGTH_UNSET ? 0 : blockLength; + if (!lengthUnset) { + bytesLeft -= blockLength; + } } } @@ -242,6 +248,7 @@ public static void cache( * caching. * @param priority The priority of this task. * @param progressNotifier A notifier through which to report progress updates, or {@code null}. + * @param isLastBlock Whether this read block is the last block of the content. * @param isCanceled An optional flag that will interrupt caching if set to true. * @return Number of read bytes, or 0 if no data is available because the end of the opened range * has been reached. @@ -255,6 +262,7 @@ private static long readAndDiscard( PriorityTaskManager priorityTaskManager, int priority, @Nullable ProgressNotifier progressNotifier, + boolean isLastBlock, AtomicBoolean isCanceled) throws IOException, InterruptedException { long positionOffset = absoluteStreamPosition - dataSpec.absoluteStreamPosition; @@ -263,22 +271,23 @@ private static long readAndDiscard( // Wait for any other thread with higher priority to finish its job. priorityTaskManager.proceed(priority); } + throwExceptionIfInterruptedOrCancelled(isCanceled); try { - throwExceptionIfInterruptedOrCancelled(isCanceled); - // Create a new dataSpec setting length to C.LENGTH_UNSET to prevent getting an error in - // case the given length exceeds the end of input. - dataSpec = - new DataSpec( - dataSpec.uri, - dataSpec.httpMethod, - dataSpec.httpBody, - absoluteStreamPosition, - /* position= */ dataSpec.position + positionOffset, - C.LENGTH_UNSET, - dataSpec.key, - dataSpec.flags); - long resolvedLength = dataSource.open(dataSpec); - if (progressNotifier != null && resolvedLength != C.LENGTH_UNSET) { + long resolvedLength; + try { + resolvedLength = dataSource.open(dataSpec.subrange(positionOffset, length)); + } catch (IOException exception) { + if (length == C.LENGTH_UNSET + || !isLastBlock + || !isCausedByPositionOutOfRange(exception)) { + throw exception; + } + Util.closeQuietly(dataSource); + // Retry to open the data source again, setting length to C.LENGTH_UNSET to prevent + // getting an error in case the given length exceeds the end of input. + resolvedLength = dataSource.open(dataSpec.subrange(positionOffset, C.LENGTH_UNSET)); + } + if (isLastBlock && progressNotifier != null && resolvedLength != C.LENGTH_UNSET) { progressNotifier.onRequestLengthResolved(positionOffset + resolvedLength); } long totalBytesRead = 0; @@ -340,6 +349,20 @@ public static void remove(Cache cache, String key) { } } + /*package*/ static boolean isCausedByPositionOutOfRange(IOException e) { + Throwable cause = e; + while (cause != null) { + if (cause instanceof DataSourceException) { + int reason = ((DataSourceException) cause).reason; + if (reason == DataSourceException.POSITION_OUT_OF_RANGE) { + return true; + } + } + cause = cause.getCause(); + } + return false; + } + private static String buildCacheKey( DataSpec dataSpec, @Nullable CacheKeyFactory cacheKeyFactory) { return (cacheKeyFactory != null ? cacheKeyFactory : DEFAULT_CACHE_KEY_FACTORY) diff --git a/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/CacheAsserts.java b/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/CacheAsserts.java index 9a179043793..a48f88b5c08 100644 --- a/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/CacheAsserts.java +++ b/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/CacheAsserts.java @@ -83,7 +83,8 @@ public static void assertCachedData(Cache cache, FakeDataSet fakeDataSet, Uri... * @throws IOException If an error occurred reading from the Cache. */ public static void assertDataCached(Cache cache, Uri uri, byte[] expected) throws IOException { - DataSpec dataSpec = new DataSpec(uri); + // TODO Make tests specify if the content length is stored in cache metadata. + DataSpec dataSpec = new DataSpec(uri, 0, expected.length, null, 0); assertDataCached(cache, dataSpec, expected); } @@ -95,15 +96,18 @@ public static void assertDataCached(Cache cache, Uri uri, byte[] expected) throw public static void assertDataCached(Cache cache, DataSpec dataSpec, byte[] expected) throws IOException { DataSource dataSource = new CacheDataSource(cache, DummyDataSource.INSTANCE, 0); - dataSource.open(dataSpec); + byte[] bytes; try { - byte[] bytes = TestUtil.readToEnd(dataSource); - assertWithMessage("Cached data doesn't match expected for '" + dataSpec.uri + "',") - .that(bytes) - .isEqualTo(expected); + dataSource.open(dataSpec); + bytes = TestUtil.readToEnd(dataSource); + } catch (IOException e) { + throw new IOException("Opening/reading cache failed: " + dataSpec, e); } finally { dataSource.close(); } + assertWithMessage("Cached data doesn't match expected for '" + dataSpec.uri + "',") + .that(bytes) + .isEqualTo(expected); } /**