diff --git a/RELEASENOTES.md b/RELEASENOTES.md index a7931f9009..1699f2c09b 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -6,6 +6,8 @@ ([#1583](https://github.com/google/ExoPlayer/issues/1583)). * MPEG-TS: Use random access indicators to minimize the need for `FLAG_ALLOW_NON_IDR_KEYFRAMES`. +* Downloading: Reduce time taken to remove downloads + ([#5136](https://github.com/google/ExoPlayer/issues/5136)). * MP3: * Use the true bitrate for constant-bitrate MP3 seeking. * Fix issue where streams would play twice on some Samsung devices 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 a769e9acac..b5b5dc64e6 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 @@ -82,7 +82,7 @@ public interface Cache { * Releases the cache. This method must be called when the cache is no longer required. The cache * must not be used after calling this method. */ - void release() throws CacheException; + void release(); /** * Registers a listener to listen for changes to a given key. diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java index 3bcfac5053..43e6730844 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java @@ -16,6 +16,7 @@ package com.google.android.exoplayer2.upstream.cache; import android.util.SparseArray; +import android.util.SparseBooleanArray; import com.google.android.exoplayer2.upstream.cache.Cache.CacheException; import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.AtomicFile; @@ -41,6 +42,7 @@ import javax.crypto.CipherOutputStream; import javax.crypto.NoSuchPaddingException; import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.SecretKeySpec; +import org.checkerframework.checker.nullness.compatqual.NullableType; /** Maintains the index of cached content. */ /*package*/ class CachedContentIndex { @@ -52,7 +54,30 @@ import javax.crypto.spec.SecretKeySpec; private static final int FLAG_ENCRYPTED_INDEX = 1; private final HashMap keyToContent; - private final SparseArray idToKey; + /** + * Maps assigned ids to their corresponding keys. Also contains (id -> null) entries for ids that + * have been removed from the index since it was last stored. This prevents reuse of these ids, + * which is necessary to avoid clashes that could otherwise occur as a result of the sequence: + * + *

[1] (key1, id1) is removed from the in-memory index ... the index is not stored to disk ... + * [2] id1 is reused for a different key2 ... the index is not stored to disk ... [3] A file for + * key2 is partially written using a path corresponding to id1 ... the process is killed before + * the index is stored to disk ... [4] The index is read from disk, causing the partially written + * file to be incorrectly associated to key1 + * + *

By avoiding id reuse in step [2], a new id2 will be used instead. Step [4] will then delete + * the partially written file because the index does not contain an entry for id2. + * + *

When the index is next stored (id -> null) entries are removed, making the ids eligible for + * reuse. + */ + private final SparseArray<@NullableType String> idToKey; + /** + * Tracks ids for which (id -> null) entries are present in idToKey, so that they can be removed + * efficiently when the index is next stored. + */ + private final SparseBooleanArray removedIds; + private final AtomicFile atomicFile; private final Cipher cipher; private final SecretKeySpec secretKeySpec; @@ -104,6 +129,7 @@ import javax.crypto.spec.SecretKeySpec; } keyToContent = new HashMap<>(); idToKey = new SparseArray<>(); + removedIds = new SparseBooleanArray(); atomicFile = new AtomicFile(new File(cacheDir, FILE_NAME)); } @@ -124,6 +150,12 @@ import javax.crypto.spec.SecretKeySpec; } writeFile(); changed = false; + // Make ids that were removed since the index was last stored eligible for re-use. + int removedIdCount = removedIds.size(); + for (int i = 0; i < removedIdCount; i++) { + idToKey.remove(removedIds.keyAt(i)); + } + removedIds.clear(); } /** @@ -168,8 +200,11 @@ import javax.crypto.spec.SecretKeySpec; CachedContent cachedContent = keyToContent.get(key); if (cachedContent != null && cachedContent.isEmpty() && !cachedContent.isLocked()) { keyToContent.remove(key); - idToKey.remove(cachedContent.id); changed = true; + // Keep an entry in idToKey to stop the id from being reused until the index is next stored. + idToKey.put(cachedContent.id, /* value= */ null); + // Track that the entry should be removed from idToKey when the index is next stored. + removedIds.put(cachedContent.id, /* value= */ true); } } 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 ca2983c891..ab60be2b4b 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 @@ -146,13 +146,16 @@ public final class SimpleCache implements Cache { } @Override - public synchronized void release() throws CacheException { + public synchronized void release() { if (released) { return; } listeners.clear(); + removeStaleSpans(); try { - removeStaleSpansAndCachedContents(); + index.store(); + } catch (CacheException e) { + Log.e(TAG, "Storing index file failed", e); } finally { unlockFolder(cacheDir); released = true; @@ -265,7 +268,7 @@ public final class SimpleCache implements Cache { if (!cacheDir.exists()) { // For some reason the cache directory doesn't exist. Make a best effort to create it. cacheDir.mkdirs(); - removeStaleSpansAndCachedContents(); + removeStaleSpans(); } evictor.onStartFile(this, key, position, maxLength); return SimpleCacheSpan.getCacheFile( @@ -311,9 +314,9 @@ public final class SimpleCache implements Cache { } @Override - public synchronized void removeSpan(CacheSpan span) throws CacheException { + public synchronized void removeSpan(CacheSpan span) { Assertions.checkState(!released); - removeSpan(span, true); + removeSpanInternal(span); } @Override @@ -379,7 +382,7 @@ public final class SimpleCache implements Cache { if (span.isCached && !span.file.exists()) { // The file has been deleted from under us. It's likely that other files will have been // deleted too, so scan the whole in-memory representation. - removeStaleSpansAndCachedContents(); + removeStaleSpans(); continue; } return span; @@ -431,27 +434,21 @@ public final class SimpleCache implements Cache { notifySpanAdded(span); } - private void removeSpan(CacheSpan span, boolean removeEmptyCachedContent) throws CacheException { + private void removeSpanInternal(CacheSpan span) { CachedContent cachedContent = index.get(span.key); if (cachedContent == null || !cachedContent.removeSpan(span)) { return; } totalSpace -= span.length; - try { - if (removeEmptyCachedContent) { - index.maybeRemove(cachedContent.key); - index.store(); - } - } finally { - notifySpanRemoved(span); - } + index.maybeRemove(cachedContent.key); + notifySpanRemoved(span); } /** * Scans all of the cached spans in the in-memory representation, removing any for which files no * longer exist. */ - private void removeStaleSpansAndCachedContents() throws CacheException { + private void removeStaleSpans() { ArrayList spansToBeRemoved = new ArrayList<>(); for (CachedContent cachedContent : index.getAll()) { for (CacheSpan span : cachedContent.getSpans()) { @@ -461,11 +458,8 @@ public final class SimpleCache implements Cache { } } for (int i = 0; i < spansToBeRemoved.size(); i++) { - // Remove span but not CachedContent to prevent multiple index.store() calls. - removeSpan(spansToBeRemoved.get(i), false); + removeSpanInternal(spansToBeRemoved.get(i)); } - index.removeEmpty(); - index.store(); } private void notifySpanRemoved(CacheSpan span) { 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 55d05eb7d4..2285a68062 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 @@ -489,11 +489,7 @@ public final class CacheDataSourceTest { NavigableSet cachedSpans = cache.getCachedSpans(expectedCacheKey); for (CacheSpan cachedSpan : cachedSpans) { if (cachedSpan.position >= halfDataLength) { - try { - cache.removeSpan(cachedSpan); - } catch (Cache.CacheException e) { - // do nothing - } + cache.removeSpan(cachedSpan); } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java index a4e444386a..15bbb8c108 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java @@ -47,6 +47,7 @@ import org.robolectric.RuntimeEnvironment; public class SimpleCacheTest { private static final String KEY_1 = "key1"; + private static final String KEY_2 = "key2"; private File cacheDir; @@ -152,6 +153,40 @@ public class SimpleCacheTest { assertCachedDataReadCorrect(cacheSpan2); } + @Test + public void testReloadCacheWithoutRelease() throws Exception { + SimpleCache simpleCache = getSimpleCache(); + + // Write data for KEY_1. + CacheSpan cacheSpan1 = simpleCache.startReadWrite(KEY_1, 0); + addCache(simpleCache, KEY_1, 0, 15); + simpleCache.releaseHoleSpan(cacheSpan1); + // Write and remove data for KEY_2. + CacheSpan cacheSpan2 = simpleCache.startReadWrite(KEY_2, 0); + addCache(simpleCache, KEY_2, 0, 15); + simpleCache.releaseHoleSpan(cacheSpan2); + simpleCache.removeSpan(simpleCache.getCachedSpans(KEY_2).first()); + + // Don't release the cache. This means the index file wont have been written to disk after the + // data for KEY_2 was removed. Move the cache instead, so we can reload it without failing the + // folder locking check. + File cacheDir2 = Util.createTempFile(RuntimeEnvironment.application, "ExoPlayerTest"); + cacheDir2.delete(); + cacheDir.renameTo(cacheDir2); + + // Reload the cache from its new location. + simpleCache = new SimpleCache(cacheDir2, new NoOpCacheEvictor()); + + // Read data back for KEY_1. + CacheSpan cacheSpan3 = simpleCache.startReadWrite(KEY_1, 0); + assertCachedDataReadCorrect(cacheSpan3); + + // Check the entry for KEY_2 was removed when the cache was reloaded. + assertThat(simpleCache.getCachedSpans(KEY_2)).isEmpty(); + + Util.recursiveDelete(cacheDir2); + } + @Test public void testEncryptedIndex() throws Exception { byte[] key = "Bar12345Bar12345".getBytes(C.UTF8_NAME); // 128 bit key