From 3a9557c72f9f8be786a6bdecd8ff5d477d2a8354 Mon Sep 17 00:00:00 2001 From: olly Date: Wed, 2 Jan 2019 13:43:19 +0000 Subject: [PATCH] Don't pass maxCacheFileSize to CacheEvictor if real size is unknown CacheEvictor.onStartFile currently receives a length, which is the maximum size of the content that might be written. The LRU cache evictor will make sure there's sufficient space for the specified size. If the actual size of the content is unknown, CacheDataSink passes maxCacheFileSize. The current behavior isn't ideal. It seems valid for a developer to specify maxCacheFileSize=Long.MAX_VALUE if they don't want any file fragmentation in the cache. However if they then attempt to cache something with unknown length, LRU cache will try and make room for content of length Long.MAX_VALUE, and end up evicting the entire cache to do so. This change alters the logic so a length is only passed if the actual size of the content is known. In other cases C.LENGTH_UNSET is now passed. The LRU evictor will not evict until after the file is committed. Note a custom LRU evictor could still opt to evict to ensure some application specified amount of space, if that's the desired behavior. PiperOrigin-RevId: 227509525 --- .../android/exoplayer2/upstream/cache/Cache.java | 7 ++++--- .../exoplayer2/upstream/cache/CacheDataSink.java | 11 +++++++---- .../exoplayer2/upstream/cache/CacheEvictor.java | 7 ++++--- .../upstream/cache/LeastRecentlyUsedCacheEvictor.java | 7 +++++-- .../exoplayer2/upstream/cache/SimpleCache.java | 5 ++--- 5 files changed, 22 insertions(+), 15 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java index 8641746c74..f65b9b64fe 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java @@ -16,6 +16,7 @@ package com.google.android.exoplayer2.upstream.cache; import android.support.annotation.Nullable; +import com.google.android.exoplayer2.C; import java.io.File; import java.io.IOException; import java.util.NavigableSet; @@ -169,12 +170,12 @@ public interface Cache { * * @param key The cache key for the data. * @param position The starting position of the data. - * @param maxLength The maximum length of the data to be written. Used only to ensure that there - * is enough space in the cache. + * @param length The length of the data being written, or {@link C#LENGTH_UNSET} if unknown. Used + * only to ensure that there is enough space in the cache. * @return The file into which data should be written. * @throws CacheException If an error is encountered. */ - File startFile(String key, long position, long maxLength) throws CacheException; + File startFile(String key, long position, long length) throws CacheException; /** * Commits a file into the cache. Must only be called when holding a corresponding hole {@link diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSink.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSink.java index 6767548c15..8390d09843 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSink.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSink.java @@ -170,10 +170,13 @@ public final class CacheDataSink implements DataSink { } private void openNextOutputStream() throws IOException { - long maxLength = dataSpec.length == C.LENGTH_UNSET ? maxCacheFileSize - : Math.min(dataSpec.length - dataSpecBytesWritten, maxCacheFileSize); - file = cache.startFile(dataSpec.key, dataSpec.absoluteStreamPosition + dataSpecBytesWritten, - maxLength); + long length = + dataSpec.length == C.LENGTH_UNSET + ? C.LENGTH_UNSET + : Math.min(dataSpec.length - dataSpecBytesWritten, maxCacheFileSize); + file = + cache.startFile( + dataSpec.key, dataSpec.absoluteStreamPosition + dataSpecBytesWritten, length); underlyingFileOutputStream = new FileOutputStream(file); if (bufferSize > 0) { if (bufferedOutputStream == null) { diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheEvictor.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheEvictor.java index 8944b45033..dbec4b78fc 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheEvictor.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheEvictor.java @@ -15,6 +15,8 @@ */ package com.google.android.exoplayer2.upstream.cache; +import com.google.android.exoplayer2.C; + /** * Evicts data from a {@link Cache}. Implementations should call {@link Cache#removeSpan(CacheSpan)} * to evict cache entries based on their eviction policies. @@ -32,8 +34,7 @@ public interface CacheEvictor extends Cache.Listener { * @param cache The source of the event. * @param key The key being written. * @param position The starting position of the data being written. - * @param maxLength The maximum length of the data being written. + * @param length The length of the data being written, or {@link C#LENGTH_UNSET} if unknown. */ - void onStartFile(Cache cache, String key, long position, long maxLength); - + void onStartFile(Cache cache, String key, long position, long length); } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/LeastRecentlyUsedCacheEvictor.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/LeastRecentlyUsedCacheEvictor.java index 79d23dd1b0..aa40c1d2fd 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/LeastRecentlyUsedCacheEvictor.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/LeastRecentlyUsedCacheEvictor.java @@ -15,6 +15,7 @@ */ package com.google.android.exoplayer2.upstream.cache; +import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.upstream.cache.Cache.CacheException; import java.util.Comparator; import java.util.TreeSet; @@ -40,8 +41,10 @@ public final class LeastRecentlyUsedCacheEvictor implements CacheEvictor, Compar } @Override - public void onStartFile(Cache cache, String key, long position, long maxLength) { - evictCache(cache, maxLength); + public void onStartFile(Cache cache, String key, long position, long length) { + if (length != C.LENGTH_UNSET) { + evictCache(cache, length); + } } @Override diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java index 8bcf1758fa..80788b5786 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java @@ -259,8 +259,7 @@ public final class SimpleCache implements Cache { } @Override - public synchronized File startFile(String key, long position, long maxLength) - throws CacheException { + public synchronized File startFile(String key, long position, long length) throws CacheException { Assertions.checkState(!released); CachedContent cachedContent = index.get(key); Assertions.checkNotNull(cachedContent); @@ -270,7 +269,7 @@ public final class SimpleCache implements Cache { cacheDir.mkdirs(); removeStaleSpans(); } - evictor.onStartFile(this, key, position, maxLength); + evictor.onStartFile(this, key, position, length); return SimpleCacheSpan.getCacheFile( cacheDir, cachedContent.id, position, System.currentTimeMillis()); }