From 05d5096b3a7160d7a9bffbcc47eead2a9ca96970 Mon Sep 17 00:00:00 2001 From: Jianyong Xiao Date: Mon, 19 Mar 2018 01:34:35 -0700 Subject: [PATCH 1/2] Provide an option to skip file descriptor sync in CacheDataSink --- .../offline/DownloaderConstructorHelper.java | 2 +- .../upstream/cache/CacheDataSink.java | 14 ++++-- .../upstream/cache/CacheDataSinkFactory.java | 2 +- .../upstream/cache/CacheDataSource.java | 2 +- .../upstream/cache/CacheDataSourceTest.java | 48 ++++++++++++++----- .../upstream/cache/CacheDataSourceTest2.java | 2 +- 6 files changed, 51 insertions(+), 19 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java index 9ef9366397..5e088d81b1 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java @@ -93,7 +93,7 @@ public final class DownloaderConstructorHelper { } else { DataSink cacheWriteDataSink = cacheWriteDataSinkFactory != null ? cacheWriteDataSinkFactory.createDataSink() - : new CacheDataSink(cache, CacheDataSource.DEFAULT_MAX_CACHE_FILE_SIZE); + : new CacheDataSink(cache, CacheDataSource.DEFAULT_MAX_CACHE_FILE_SIZE, false); DataSource upstream = upstreamDataSourceFactory.createDataSource(); upstream = priorityTaskManager == null ? upstream : new PriorityDataSource(upstream, priorityTaskManager, C.PRIORITY_DOWNLOAD); 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 57f5a6ad93..b3c2def7d1 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 @@ -43,6 +43,7 @@ public final class CacheDataSink implements DataSink { private final Cache cache; private final long maxCacheFileSize; private final int bufferSize; + private final boolean skipFDSync; private DataSpec dataSpec; private File file; @@ -70,9 +71,10 @@ public final class CacheDataSink implements DataSink { * @param maxCacheFileSize The maximum size of a cache file, in bytes. If the sink is opened for * a {@link DataSpec} whose size exceeds this value, then the data will be fragmented into * multiple cache files. + * @param skipFDSync Skip file descriptor sync when closing current output stream. */ - public CacheDataSink(Cache cache, long maxCacheFileSize) { - this(cache, maxCacheFileSize, DEFAULT_BUFFER_SIZE); + public CacheDataSink(Cache cache, long maxCacheFileSize, boolean skipFDSync) { + this(cache, maxCacheFileSize, DEFAULT_BUFFER_SIZE, skipFDSync); } /** @@ -82,11 +84,13 @@ public final class CacheDataSink implements DataSink { * multiple cache files. * @param bufferSize The buffer size in bytes for writing to a cache file. A zero or negative * value disables buffering. + * @param skipFDSync Skip file descriptor sync when closing current output stream. */ - public CacheDataSink(Cache cache, long maxCacheFileSize, int bufferSize) { + public CacheDataSink(Cache cache, long maxCacheFileSize, int bufferSize, boolean skipFDSync) { this.cache = Assertions.checkNotNull(cache); this.maxCacheFileSize = maxCacheFileSize; this.bufferSize = bufferSize; + this.skipFDSync = skipFDSync; } @Override @@ -170,7 +174,9 @@ public final class CacheDataSink implements DataSink { boolean success = false; try { outputStream.flush(); - underlyingFileOutputStream.getFD().sync(); + if (!skipFDSync) { + underlyingFileOutputStream.getFD().sync(); + } success = true; } finally { Util.closeQuietly(outputStream); diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java index 0b9ab66508..15608e9493 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java @@ -44,7 +44,7 @@ public final class CacheDataSinkFactory implements DataSink.Factory { @Override public DataSink createDataSink() { - return new CacheDataSink(cache, maxCacheFileSize, bufferSize); + return new CacheDataSink(cache, maxCacheFileSize, bufferSize, false); } } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java index c0927eb7db..3ec08cb381 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java @@ -157,7 +157,7 @@ public final class CacheDataSource implements DataSource { */ public CacheDataSource(Cache cache, DataSource upstream, @Flags int flags, long maxCacheFileSize) { - this(cache, upstream, new FileDataSource(), new CacheDataSink(cache, maxCacheFileSize), + this(cache, upstream, new FileDataSource(), new CacheDataSink(cache, maxCacheFileSize, false), flags, null); } 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 09be138abe..d68cbead7e 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 @@ -79,29 +79,49 @@ public final class CacheDataSourceTest { @Test public void testCacheAndRead() throws Exception { - assertCacheAndRead(false, false); + assertCacheAndRead(false, false, false); } @Test public void testCacheAndReadUnboundedRequest() throws Exception { - assertCacheAndRead(true, false); + assertCacheAndRead(true, false, false); } @Test public void testCacheAndReadUnknownLength() throws Exception { - assertCacheAndRead(false, true); + assertCacheAndRead(false, true, false); } @Test public void testCacheAndReadUnboundedRequestUnknownLength() throws Exception { - assertCacheAndRead(true, true); + assertCacheAndRead(true, true, false); + } + + @Test + public void testCacheAndReadSkipFDSync() throws Exception { + assertCacheAndRead(false, false, false); + } + + @Test + public void testCacheAndReadUnboundedRequestSkipFDSync() throws Exception { + assertCacheAndRead(true, false, false); + } + + @Test + public void testCacheAndReadUnknownLengthSkipFDSync() throws Exception { + assertCacheAndRead(false, true, false); + } + + @Test + public void testCacheAndReadUnboundedRequestUnknownLengthSkipFDSync() throws Exception { + assertCacheAndRead(true, true, false); } @Test public void testUnsatisfiableRange() throws Exception { // Bounded request but the content length is unknown. This forces all data to be cached but not // the length - assertCacheAndRead(false, true); + assertCacheAndRead(false, true, false); // Now do an unbounded request. This will read all of the data from cache and then try to read // more from upstream which will cause to a 416 so CDS will store the length. @@ -347,10 +367,11 @@ public final class CacheDataSourceTest { cacheDataSource.close(); } - private void assertCacheAndRead(boolean unboundedRequest, boolean simulateUnknownLength) + private void assertCacheAndRead(boolean unboundedRequest, boolean simulateUnknownLength, + boolean skipFDSync) throws IOException { // Read all data from upstream and write to cache - CacheDataSource cacheDataSource = createCacheDataSource(false, simulateUnknownLength); + CacheDataSource cacheDataSource = createCacheDataSource(false, simulateUnknownLength, skipFDSync); assertReadDataContentLength(cacheDataSource, unboundedRequest, simulateUnknownLength); // Just read from cache @@ -391,14 +412,19 @@ public final class CacheDataSourceTest { private CacheDataSource createCacheDataSource(boolean setReadException, boolean simulateUnknownLength) { - return createCacheDataSource(setReadException, simulateUnknownLength, - CacheDataSource.FLAG_BLOCK_ON_CACHE); + return createCacheDataSource(setReadException, simulateUnknownLength, false); } private CacheDataSource createCacheDataSource(boolean setReadException, - boolean simulateUnknownLength, @CacheDataSource.Flags int flags) { + boolean simulateUnknownLength, boolean skipFDSync) { + return createCacheDataSource(setReadException, simulateUnknownLength, + CacheDataSource.FLAG_BLOCK_ON_CACHE, skipFDSync); + } + + private CacheDataSource createCacheDataSource(boolean setReadException, + boolean simulateUnknownLength, @CacheDataSource.Flags int flags, boolean skipFDSync) { return createCacheDataSource(setReadException, simulateUnknownLength, flags, - new CacheDataSink(cache, MAX_CACHE_FILE_SIZE)); + new CacheDataSink(cache, MAX_CACHE_FILE_SIZE, skipFDSync)); } private CacheDataSource createCacheDataSource(boolean setReadException, diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest2.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest2.java index 1e6febd8a9..918e54d2c9 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest2.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest2.java @@ -166,7 +166,7 @@ public final class CacheDataSourceTest2 { ? new AesCipherDataSource(Util.getUtf8Bytes(secretKey), file) : file; // Sink and cipher - CacheDataSink cacheSink = new CacheDataSink(cache, EXO_CACHE_MAX_FILESIZE); + CacheDataSink cacheSink = new CacheDataSink(cache, EXO_CACHE_MAX_FILESIZE, false); byte[] scratch = new byte[3897]; DataSink cacheWriteDataSink = useAesEncryption ? new AesCipherDataSink(Util.getUtf8Bytes(secretKey), cacheSink, scratch) : cacheSink; From 75f8711bd95992e69681af379ae5cff1b11a1822 Mon Sep 17 00:00:00 2001 From: Jianyong Xiao Date: Fri, 23 Mar 2018 01:55:17 -0700 Subject: [PATCH 2/2] code review comments --- .../offline/DownloaderConstructorHelper.java | 2 +- .../upstream/cache/CacheDataSink.java | 28 +++++++++++++------ .../upstream/cache/CacheDataSinkFactory.java | 2 +- .../upstream/cache/CacheDataSource.java | 2 +- .../upstream/cache/CacheDataSourceTest.java | 24 ++++++++-------- .../upstream/cache/CacheDataSourceTest2.java | 2 +- 6 files changed, 36 insertions(+), 24 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java index 5e088d81b1..9ef9366397 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java @@ -93,7 +93,7 @@ public final class DownloaderConstructorHelper { } else { DataSink cacheWriteDataSink = cacheWriteDataSinkFactory != null ? cacheWriteDataSinkFactory.createDataSink() - : new CacheDataSink(cache, CacheDataSource.DEFAULT_MAX_CACHE_FILE_SIZE, false); + : new CacheDataSink(cache, CacheDataSource.DEFAULT_MAX_CACHE_FILE_SIZE); DataSource upstream = upstreamDataSourceFactory.createDataSource(); upstream = priorityTaskManager == null ? upstream : new PriorityDataSource(upstream, priorityTaskManager, C.PRIORITY_DOWNLOAD); 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 b3c2def7d1..61c14272cb 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 @@ -43,7 +43,7 @@ public final class CacheDataSink implements DataSink { private final Cache cache; private final long maxCacheFileSize; private final int bufferSize; - private final boolean skipFDSync; + private final boolean syncFileDescriptor; private DataSpec dataSpec; private File file; @@ -71,10 +71,22 @@ public final class CacheDataSink implements DataSink { * @param maxCacheFileSize The maximum size of a cache file, in bytes. If the sink is opened for * a {@link DataSpec} whose size exceeds this value, then the data will be fragmented into * multiple cache files. - * @param skipFDSync Skip file descriptor sync when closing current output stream. */ - public CacheDataSink(Cache cache, long maxCacheFileSize, boolean skipFDSync) { - this(cache, maxCacheFileSize, DEFAULT_BUFFER_SIZE, skipFDSync); + public CacheDataSink(Cache cache, long maxCacheFileSize) { + this(cache, maxCacheFileSize, DEFAULT_BUFFER_SIZE, true); + } + + /** + * Constructs a CacheDataSink using the {@link #DEFAULT_BUFFER_SIZE}. + * + * @param cache The cache into which data should be written. + * @param maxCacheFileSize The maximum size of a cache file, in bytes. If the sink is opened for + * a {@link DataSpec} whose size exceeds this value, then the data will be fragmented into + * multiple cache files. + * @param syncFileDescriptor Skip file descriptor sync when closing current output stream. + */ + public CacheDataSink(Cache cache, long maxCacheFileSize, boolean syncFileDescriptor) { + this(cache, maxCacheFileSize, DEFAULT_BUFFER_SIZE, syncFileDescriptor); } /** @@ -84,13 +96,13 @@ public final class CacheDataSink implements DataSink { * multiple cache files. * @param bufferSize The buffer size in bytes for writing to a cache file. A zero or negative * value disables buffering. - * @param skipFDSync Skip file descriptor sync when closing current output stream. + * @param syncFileDescriptor Sync file descriptor when closing current output stream. */ - public CacheDataSink(Cache cache, long maxCacheFileSize, int bufferSize, boolean skipFDSync) { + public CacheDataSink(Cache cache, long maxCacheFileSize, int bufferSize, boolean syncFileDescriptor) { this.cache = Assertions.checkNotNull(cache); this.maxCacheFileSize = maxCacheFileSize; this.bufferSize = bufferSize; - this.skipFDSync = skipFDSync; + this.syncFileDescriptor = syncFileDescriptor; } @Override @@ -174,7 +186,7 @@ public final class CacheDataSink implements DataSink { boolean success = false; try { outputStream.flush(); - if (!skipFDSync) { + if (syncFileDescriptor) { underlyingFileOutputStream.getFD().sync(); } success = true; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java index 15608e9493..93791f1c12 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java @@ -44,7 +44,7 @@ public final class CacheDataSinkFactory implements DataSink.Factory { @Override public DataSink createDataSink() { - return new CacheDataSink(cache, maxCacheFileSize, bufferSize, false); + return new CacheDataSink(cache, maxCacheFileSize, bufferSize, true); } } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java index 3ec08cb381..c0927eb7db 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java @@ -157,7 +157,7 @@ public final class CacheDataSource implements DataSource { */ public CacheDataSource(Cache cache, DataSource upstream, @Flags int flags, long maxCacheFileSize) { - this(cache, upstream, new FileDataSource(), new CacheDataSink(cache, maxCacheFileSize, false), + this(cache, upstream, new FileDataSource(), new CacheDataSink(cache, maxCacheFileSize), flags, null); } 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 d68cbead7e..374fb61c17 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 @@ -79,22 +79,22 @@ public final class CacheDataSourceTest { @Test public void testCacheAndRead() throws Exception { - assertCacheAndRead(false, false, false); + assertCacheAndRead(false, false, true); } @Test public void testCacheAndReadUnboundedRequest() throws Exception { - assertCacheAndRead(true, false, false); + assertCacheAndRead(true, false, true); } @Test public void testCacheAndReadUnknownLength() throws Exception { - assertCacheAndRead(false, true, false); + assertCacheAndRead(false, true, true); } @Test public void testCacheAndReadUnboundedRequestUnknownLength() throws Exception { - assertCacheAndRead(true, true, false); + assertCacheAndRead(true, true, true); } @Test @@ -121,7 +121,7 @@ public final class CacheDataSourceTest { public void testUnsatisfiableRange() throws Exception { // Bounded request but the content length is unknown. This forces all data to be cached but not // the length - assertCacheAndRead(false, true, false); + assertCacheAndRead(false, true, true); // Now do an unbounded request. This will read all of the data from cache and then try to read // more from upstream which will cause to a 416 so CDS will store the length. @@ -368,10 +368,10 @@ public final class CacheDataSourceTest { } private void assertCacheAndRead(boolean unboundedRequest, boolean simulateUnknownLength, - boolean skipFDSync) + boolean syncFD) throws IOException { // Read all data from upstream and write to cache - CacheDataSource cacheDataSource = createCacheDataSource(false, simulateUnknownLength, skipFDSync); + CacheDataSource cacheDataSource = createCacheDataSource(false, simulateUnknownLength, syncFD); assertReadDataContentLength(cacheDataSource, unboundedRequest, simulateUnknownLength); // Just read from cache @@ -412,19 +412,19 @@ public final class CacheDataSourceTest { private CacheDataSource createCacheDataSource(boolean setReadException, boolean simulateUnknownLength) { - return createCacheDataSource(setReadException, simulateUnknownLength, false); + return createCacheDataSource(setReadException, simulateUnknownLength, true); } private CacheDataSource createCacheDataSource(boolean setReadException, - boolean simulateUnknownLength, boolean skipFDSync) { + boolean simulateUnknownLength, boolean syncFD) { return createCacheDataSource(setReadException, simulateUnknownLength, - CacheDataSource.FLAG_BLOCK_ON_CACHE, skipFDSync); + CacheDataSource.FLAG_BLOCK_ON_CACHE, syncFD); } private CacheDataSource createCacheDataSource(boolean setReadException, - boolean simulateUnknownLength, @CacheDataSource.Flags int flags, boolean skipFDSync) { + boolean simulateUnknownLength, @CacheDataSource.Flags int flags, boolean syncFD) { return createCacheDataSource(setReadException, simulateUnknownLength, flags, - new CacheDataSink(cache, MAX_CACHE_FILE_SIZE, skipFDSync)); + new CacheDataSink(cache, MAX_CACHE_FILE_SIZE, syncFD)); } private CacheDataSource createCacheDataSource(boolean setReadException, diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest2.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest2.java index 918e54d2c9..1e6febd8a9 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest2.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest2.java @@ -166,7 +166,7 @@ public final class CacheDataSourceTest2 { ? new AesCipherDataSource(Util.getUtf8Bytes(secretKey), file) : file; // Sink and cipher - CacheDataSink cacheSink = new CacheDataSink(cache, EXO_CACHE_MAX_FILESIZE, false); + CacheDataSink cacheSink = new CacheDataSink(cache, EXO_CACHE_MAX_FILESIZE); byte[] scratch = new byte[3897]; DataSink cacheWriteDataSink = useAesEncryption ? new AesCipherDataSink(Util.getUtf8Bytes(secretKey), cacheSink, scratch) : cacheSink;