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 final class CacheDataSource implements DataSource {
   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 final class CacheDataSource implements DataSource {
       }
       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 @@ public final class CacheDataSource implements DataSource {
     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 androidx.annotation.Nullable;
 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 final class CacheUtil {
       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 final class CacheUtil {
    *     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 @@ public final class CacheUtil {
       PriorityTaskManager priorityTaskManager,
       int priority,
       @Nullable ProgressNotifier progressNotifier,
+      boolean isLastBlock,
       AtomicBoolean isCanceled)
       throws IOException, InterruptedException {
     long positionOffset = absoluteStreamPosition - dataSpec.absoluteStreamPosition;
@@ -263,22 +271,23 @@ public final class CacheUtil {
         // 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 final class CacheUtil {
     }
   }
 
+  /*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 final class CacheAsserts {
    * @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 final class CacheAsserts {
   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);
   }
 
   /**