Skip to content

Commit

Permalink
Fix SimpleCache.getCachedLength rollover bug & improve test coverage
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 312266156
  • Loading branch information
@ojw28 @andrewlewis
ojw28 authored and andrewlewis committed May 29, 2020
1 parent 09025d3 commit 1d8dd76
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 24 deletions.
12 changes: 10 additions & 2 deletions library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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();
Expand Down
99 changes: 77 additions & 22 deletions library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down

0 comments on commit 1d8dd76

Please sign in to comment.