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

@@ -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)).
 

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();
   }

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)

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);
   }
 
   /**