From 1d8dd763f0bd3f0f54707860129d125908ce35a6 Mon Sep 17 00:00:00 2001 From: olly Date: Tue, 19 May 2020 14:19:56 +0100 Subject: [PATCH] Fix SimpleCache.getCachedLength rollover bug & improve test coverage PiperOrigin-RevId: 312266156 --- .../upstream/cache/CachedContent.java | 12 ++- .../upstream/cache/SimpleCacheTest.java | 99 ++++++++++++++----- 2 files changed, 87 insertions(+), 24 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContent.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContent.java index 7abb9b3896d..b6a55c8da48 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContent.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContent.java @@ -15,8 +15,10 @@ */ package com.google.android.exoplayer2.upstream.cache; +import static com.google.android.exoplayer2.util.Assertions.checkArgument; +import static com.google.android.exoplayer2.util.Assertions.checkState; + import androidx.annotation.Nullable; -import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.Log; import java.io.File; import java.util.TreeSet; @@ -115,12 +117,18 @@ public SimpleCacheSpan getSpan(long position) { * @return the length of the cached or not cached data block length. */ public long getCachedBytesLength(long position, long length) { + checkArgument(position >= 0); + checkArgument(length >= 0); SimpleCacheSpan span = getSpan(position); if (span.isHoleSpan()) { // We don't have a span covering the start of the queried region. return -Math.min(span.isOpenEnded() ? Long.MAX_VALUE : span.length, length); } long queryEndPosition = position + length; + if (queryEndPosition < 0) { + // The calculation rolled over (length is probably Long.MAX_VALUE). + queryEndPosition = Long.MAX_VALUE; + } long currentEndPosition = span.position + span.length; if (currentEndPosition < queryEndPosition) { for (SimpleCacheSpan next : cachedSpans.tailSet(span, false)) { @@ -151,7 +159,7 @@ public long getCachedBytesLength(long position, long length) { */ public SimpleCacheSpan setLastTouchTimestamp( SimpleCacheSpan cacheSpan, long lastTouchTimestamp, boolean updateFile) { - Assertions.checkState(cachedSpans.remove(cacheSpan)); + checkState(cachedSpans.remove(cacheSpan)); File file = cacheSpan.file; if (updateFile) { File directory = file.getParentFile(); 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 4d9a936c4e7..8294dee3836 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 @@ -284,38 +284,93 @@ public void testEncryptedIndexLostKey() throws Exception { } @Test - public void testGetCachedLength() throws Exception { + public void getCachedLength_noCachedContent_returnsNegativeMaxHoleLength() { SimpleCache simpleCache = getSimpleCache(); - CacheSpan cacheSpan = simpleCache.startReadWrite(KEY_1, 0); - - // No cached bytes, returns -'length' - assertThat(simpleCache.getCachedLength(KEY_1, 0, 100)).isEqualTo(-100); - - // Position value doesn't affect the return value - assertThat(simpleCache.getCachedLength(KEY_1, 20, 100)).isEqualTo(-100); - addCache(simpleCache, KEY_1, 0, 15); - - // Returns the length of a single span - assertThat(simpleCache.getCachedLength(KEY_1, 0, 100)).isEqualTo(15); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 0, /* length= */ 100)) + .isEqualTo(-100); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 0, /* length= */ Long.MAX_VALUE)) + .isEqualTo(-Long.MAX_VALUE); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ 100)) + .isEqualTo(-100); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ Long.MAX_VALUE)) + .isEqualTo(-Long.MAX_VALUE); + } - // Value is capped by the 'length' - assertThat(simpleCache.getCachedLength(KEY_1, 0, 10)).isEqualTo(10); + @Test + public void getCachedLength_returnsNegativeHoleLength() throws Exception { + SimpleCache simpleCache = getSimpleCache(); + CacheSpan cacheSpan = simpleCache.startReadWrite(KEY_1, /* position= */ 0); + addCache(simpleCache, KEY_1, /* position= */ 50, /* length= */ 50); + simpleCache.releaseHoleSpan(cacheSpan); - addCache(simpleCache, KEY_1, 15, 35); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 0, /* length= */ 100)) + .isEqualTo(-50); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 0, /* length= */ Long.MAX_VALUE)) + .isEqualTo(-50); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ 100)) + .isEqualTo(-30); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ Long.MAX_VALUE)) + .isEqualTo(-30); + } - // Returns the length of two adjacent spans - assertThat(simpleCache.getCachedLength(KEY_1, 0, 100)).isEqualTo(50); + @Test + public void getCachedLength_returnsCachedLength() throws Exception { + SimpleCache simpleCache = getSimpleCache(); + CacheSpan cacheSpan = simpleCache.startReadWrite(KEY_1, /* position= */ 0); + addCache(simpleCache, KEY_1, /* position= */ 0, /* length= */ 50); + simpleCache.releaseHoleSpan(cacheSpan); - addCache(simpleCache, KEY_1, 60, 10); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 0, /* length= */ 100)) + .isEqualTo(50); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 0, /* length= */ Long.MAX_VALUE)) + .isEqualTo(50); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ 100)) + .isEqualTo(30); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ Long.MAX_VALUE)) + .isEqualTo(30); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ 15)) + .isEqualTo(15); + } - // Not adjacent span doesn't affect return value - assertThat(simpleCache.getCachedLength(KEY_1, 0, 100)).isEqualTo(50); + @Test + public void getCachedLength_withMultipleAdjacentSpans_returnsCachedLength() throws Exception { + SimpleCache simpleCache = getSimpleCache(); + CacheSpan cacheSpan = simpleCache.startReadWrite(KEY_1, /* position= */ 0); + addCache(simpleCache, KEY_1, /* position= */ 0, /* length= */ 25); + addCache(simpleCache, KEY_1, /* position= */ 25, /* length= */ 25); + simpleCache.releaseHoleSpan(cacheSpan); - // Returns length of hole up to the next cached span - assertThat(simpleCache.getCachedLength(KEY_1, 55, 100)).isEqualTo(-5); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 0, /* length= */ 100)) + .isEqualTo(50); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 0, /* length= */ Long.MAX_VALUE)) + .isEqualTo(50); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ 100)) + .isEqualTo(30); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ Long.MAX_VALUE)) + .isEqualTo(30); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ 15)) + .isEqualTo(15); + } + @Test + public void getCachedLength_withMultipleNonAdjacentSpans_returnsCachedLength() throws Exception { + SimpleCache simpleCache = getSimpleCache(); + CacheSpan cacheSpan = simpleCache.startReadWrite(KEY_1, /* position= */ 0); + addCache(simpleCache, KEY_1, /* position= */ 0, /* length= */ 10); + addCache(simpleCache, KEY_1, /* position= */ 15, /* length= */ 35); simpleCache.releaseHoleSpan(cacheSpan); + + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 0, /* length= */ 100)) + .isEqualTo(10); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 0, /* length= */ Long.MAX_VALUE)) + .isEqualTo(10); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ 100)) + .isEqualTo(30); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ Long.MAX_VALUE)) + .isEqualTo(30); + assertThat(simpleCache.getCachedLength(KEY_1, /* position= */ 20, /* length= */ 15)) + .isEqualTo(15); } /* Tests https://github.com/google/ExoPlayer/issues/3260 case. */