From 2e9ed51503ba491a19f605e6994fa5839633c74f Mon Sep 17 00:00:00 2001 From: olly Date: Mon, 27 Apr 2020 10:58:58 +0100 Subject: [PATCH] Use Executor instead of ExecutorService for parallel downloads - Executor is a superclass of ExecutorService, so this is arguably a little more flexible. - It removes the need to use null for direct execution, because Runnable::run is a direct executor that can be trivially used instead. - Removing the error-prone "cannot be a direct executor" restriction in the parallel version of SegmentDownloader requires not relying on the futures returned from ExecutorService.submit() anyway. Issue: #5978 PiperOrigin-RevId: 308586620 --- library/core/proguard-rules.txt | 6 ++-- .../offline/DefaultDownloaderFactory.java | 29 +++++++------------ .../offline/ProgressiveDownloader.java | 12 ++++---- .../exoplayer2/offline/SegmentDownloader.java | 14 ++++----- .../source/dash/offline/DashDownloader.java | 14 ++++----- .../source/hls/offline/HlsDownloader.java | 15 ++++------ .../smoothstreaming/offline/SsDownloader.java | 15 ++++------ 7 files changed, 43 insertions(+), 62 deletions(-) diff --git a/library/core/proguard-rules.txt b/library/core/proguard-rules.txt index 1014dc0de43..cbeb74cf6ce 100644 --- a/library/core/proguard-rules.txt +++ b/library/core/proguard-rules.txt @@ -51,15 +51,15 @@ # Constructors accessed via reflection in DefaultDownloaderFactory -dontnote com.google.android.exoplayer2.source.dash.offline.DashDownloader -keepclassmembers class com.google.android.exoplayer2.source.dash.offline.DashDownloader { - (android.net.Uri, java.util.List, com.google.android.exoplayer2.upstream.cache.CacheDataSource.Factory, java.util.concurrent.ExecutorService); + (android.net.Uri, java.util.List, com.google.android.exoplayer2.upstream.cache.CacheDataSource.Factory, java.util.concurrent.Executor); } -dontnote com.google.android.exoplayer2.source.hls.offline.HlsDownloader -keepclassmembers class com.google.android.exoplayer2.source.hls.offline.HlsDownloader { - (android.net.Uri, java.util.List, com.google.android.exoplayer2.upstream.cache.CacheDataSource.Factory, java.util.concurrent.ExecutorService); + (android.net.Uri, java.util.List, com.google.android.exoplayer2.upstream.cache.CacheDataSource.Factory, java.util.concurrent.Executor); } -dontnote com.google.android.exoplayer2.source.smoothstreaming.offline.SsDownloader -keepclassmembers class com.google.android.exoplayer2.source.smoothstreaming.offline.SsDownloader { - (android.net.Uri, java.util.List, com.google.android.exoplayer2.upstream.cache.CacheDataSource.Factory, java.util.concurrent.ExecutorService); + (android.net.Uri, java.util.List, com.google.android.exoplayer2.upstream.cache.CacheDataSource.Factory, java.util.concurrent.Executor); } # Constructors accessed via reflection in DefaultMediaSourceFactory and DownloadHelper diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloaderFactory.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloaderFactory.java index 619456a2011..0b7434c339f 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloaderFactory.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloaderFactory.java @@ -20,7 +20,7 @@ import com.google.android.exoplayer2.upstream.cache.CacheDataSource; import java.lang.reflect.Constructor; import java.util.List; -import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executor; /** * Default {@link DownloaderFactory}, supporting creation of progressive, DASH, HLS and @@ -71,7 +71,7 @@ public class DefaultDownloaderFactory implements DownloaderFactory { } private final CacheDataSource.Factory cacheDataSourceFactory; - @Nullable private final ExecutorService executorService; + private final Executor executor; /** * Creates an instance. @@ -80,7 +80,7 @@ public class DefaultDownloaderFactory implements DownloaderFactory { * downloads will be written. */ public DefaultDownloaderFactory(CacheDataSource.Factory cacheDataSourceFactory) { - this(cacheDataSourceFactory, /* executorService= */ null); + this(cacheDataSourceFactory, Runnable::run); } /** @@ -88,16 +88,14 @@ public DefaultDownloaderFactory(CacheDataSource.Factory cacheDataSourceFactory) * * @param cacheDataSourceFactory A {@link CacheDataSource.Factory} for the cache into which * downloads will be written. - * @param executorService An {@link ExecutorService} used to make requests for media being - * downloaded. Must not be a direct executor, but may be {@code null} if each download should - * make its requests directly on its own thread. Providing an {@link ExecutorService} that - * uses multiple threads will speed up download tasks that can be split into smaller parts for - * parallel execution. + * @param executor An {@link Executor} used to make requests for media being downloaded. Providing + * an {@link Executor} that uses multiple threads will speed up download tasks that can be + * split into smaller parts for parallel execution. */ public DefaultDownloaderFactory( - CacheDataSource.Factory cacheDataSourceFactory, @Nullable ExecutorService executorService) { + CacheDataSource.Factory cacheDataSourceFactory, Executor executor) { this.cacheDataSourceFactory = cacheDataSourceFactory; - this.executorService = executorService; + this.executor = executor; } @Override @@ -105,7 +103,7 @@ public Downloader createDownloader(DownloadRequest request) { switch (request.type) { case DownloadRequest.TYPE_PROGRESSIVE: return new ProgressiveDownloader( - request.uri, request.customCacheKey, cacheDataSourceFactory, executorService); + request.uri, request.customCacheKey, cacheDataSourceFactory, executor); case DownloadRequest.TYPE_DASH: return createDownloader(request, DASH_DOWNLOADER_CONSTRUCTOR); case DownloadRequest.TYPE_HLS: @@ -117,10 +115,6 @@ public Downloader createDownloader(DownloadRequest request) { } } - // Checker framework has no way of knowing whether arguments to newInstance can be null or not, - // and so opts to be conservative and assumes they cannot. In this case executorService can be - // nullable, so we suppress the warning. - @SuppressWarnings("nullness:argument.type.incompatible") private Downloader createDownloader( DownloadRequest request, @Nullable Constructor constructor) { if (constructor == null) { @@ -128,7 +122,7 @@ private Downloader createDownloader( } try { return constructor.newInstance( - request.uri, request.streamKeys, cacheDataSourceFactory, executorService); + request.uri, request.streamKeys, cacheDataSourceFactory, executor); } catch (Exception e) { throw new RuntimeException("Failed to instantiate downloader for: " + request.type, e); } @@ -139,8 +133,7 @@ private static Constructor getDownloaderConstructor(Class< try { return clazz .asSubclass(Downloader.class) - .getConstructor( - Uri.class, List.class, CacheDataSource.Factory.class, ExecutorService.class); + .getConstructor(Uri.class, List.class, CacheDataSource.Factory.class, Executor.class); } catch (NoSuchMethodException e) { // The downloader is present, but the expected constructor is missing. throw new RuntimeException("Downloader constructor missing", e); diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/ProgressiveDownloader.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/ProgressiveDownloader.java index 1bae4c3655d..ecdd1748bbf 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/ProgressiveDownloader.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/ProgressiveDownloader.java @@ -23,7 +23,7 @@ import com.google.android.exoplayer2.upstream.cache.CacheUtil; import com.google.android.exoplayer2.util.PriorityTaskManager; import java.io.IOException; -import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicBoolean; /** A downloader for progressive media streams. */ @@ -44,7 +44,7 @@ public final class ProgressiveDownloader implements Downloader { */ public ProgressiveDownloader( Uri uri, @Nullable String customCacheKey, CacheDataSource.Factory cacheDataSourceFactory) { - this(uri, customCacheKey, cacheDataSourceFactory, /* executorService= */ null); + this(uri, customCacheKey, cacheDataSourceFactory, Runnable::run); } /** @@ -53,17 +53,15 @@ public ProgressiveDownloader( * indexing. May be null. * @param cacheDataSourceFactory A {@link CacheDataSource.Factory} for the cache into which the * download will be written. - * @param executorService An {@link ExecutorService} used to make requests for the media being - * downloaded. Must not be a direct executor, but may be {@code null} if the requests should - * be made directly from the thread that calls {@link #download(ProgressListener)}. In the - * future, providing an {@link ExecutorService} that uses multiple threads may speed up the + * @param executor An {@link Executor} used to make requests for the media being downloaded. In + * the future, providing an {@link Executor} that uses multiple threads may speed up the * download by allowing parts of it to be executed in parallel. */ public ProgressiveDownloader( Uri uri, @Nullable String customCacheKey, CacheDataSource.Factory cacheDataSourceFactory, - @Nullable ExecutorService executorService) { + Executor executor) { dataSpec = new DataSpec.Builder() .setUri(uri) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/SegmentDownloader.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/SegmentDownloader.java index cd4e67863b2..f7b2fc7ffd4 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/SegmentDownloader.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/SegmentDownloader.java @@ -33,7 +33,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; -import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicBoolean; /** @@ -70,7 +70,7 @@ public int compareTo(Segment other) { private final DataSpec manifestDataSpec; private final ArrayList streamKeys; private final CacheDataSource.Factory cacheDataSourceFactory; - @Nullable private final ExecutorService executorService; + private final Executor executor; private final AtomicBoolean isCanceled; /** @@ -79,21 +79,19 @@ public int compareTo(Segment other) { * If empty, all streams are downloaded. * @param cacheDataSourceFactory A {@link CacheDataSource.Factory} for the cache into which the * download will be written. - * @param executorService An {@link ExecutorService} used to make requests for the media being - * downloaded. Must not be a direct executor, but may be {@code null} if the requests should - * be made directly from the thread that calls {@link #download(ProgressListener)}. Providing - * an {@link ExecutorService} that uses multiple threads will speed up the download by + * @param executor An {@link Executor} used to make requests for the media being downloaded. + * Providing an {@link Executor} that uses multiple threads will speed up the download by * allowing parts of it to be executed in parallel. */ public SegmentDownloader( Uri manifestUri, List streamKeys, CacheDataSource.Factory cacheDataSourceFactory, - @Nullable ExecutorService executorService) { + Executor executor) { this.manifestDataSpec = getCompressibleDataSpec(manifestUri); this.streamKeys = new ArrayList<>(streamKeys); this.cacheDataSourceFactory = cacheDataSourceFactory; - this.executorService = executorService; + this.executor = executor; isCanceled = new AtomicBoolean(); } diff --git a/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/offline/DashDownloader.java b/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/offline/DashDownloader.java index a694fea4338..8629c919d04 100644 --- a/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/offline/DashDownloader.java +++ b/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/offline/DashDownloader.java @@ -38,7 +38,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executor; /** * A downloader for DASH streams. @@ -74,7 +74,7 @@ public final class DashDownloader extends SegmentDownloader { */ public DashDownloader( Uri manifestUri, List streamKeys, CacheDataSource.Factory cacheDataSourceFactory) { - this(manifestUri, streamKeys, cacheDataSourceFactory, /* executorService= */ null); + this(manifestUri, streamKeys, cacheDataSourceFactory, Runnable::run); } /** @@ -83,18 +83,16 @@ public DashDownloader( * download. If empty, all representations are downloaded. * @param cacheDataSourceFactory A {@link CacheDataSource.Factory} for the cache into which the * download will be written. - * @param executorService An {@link ExecutorService} used to make requests for the media being - * downloaded. Must not be a direct executor, but may be {@code null} if the requests should - * be made directly from the thread that calls {@link #download(ProgressListener)}. Providing - * an {@link ExecutorService} that uses multiple threads will speed up the download by + * @param executor An {@link Executor} used to make requests for the media being downloaded. + * Providing an {@link Executor} that uses multiple threads will speed up the download by * allowing parts of it to be executed in parallel. */ public DashDownloader( Uri manifestUri, List streamKeys, CacheDataSource.Factory cacheDataSourceFactory, - @Nullable ExecutorService executorService) { - super(manifestUri, streamKeys, cacheDataSourceFactory, executorService); + Executor executor) { + super(manifestUri, streamKeys, cacheDataSourceFactory, executor); } @Override diff --git a/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/offline/HlsDownloader.java b/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/offline/HlsDownloader.java index 5d5bffbfdde..cd8651deff9 100644 --- a/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/offline/HlsDownloader.java +++ b/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/offline/HlsDownloader.java @@ -16,7 +16,6 @@ package com.google.android.exoplayer2.source.hls.offline; import android.net.Uri; -import androidx.annotation.Nullable; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.offline.SegmentDownloader; import com.google.android.exoplayer2.offline.StreamKey; @@ -33,7 +32,7 @@ import java.util.ArrayList; import java.util.HashSet; import java.util.List; -import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executor; /** * A downloader for HLS streams. @@ -69,7 +68,7 @@ public final class HlsDownloader extends SegmentDownloader { */ public HlsDownloader( Uri playlistUri, List streamKeys, CacheDataSource.Factory cacheDataSourceFactory) { - this(playlistUri, streamKeys, cacheDataSourceFactory, /* executorService= */ null); + this(playlistUri, streamKeys, cacheDataSourceFactory, Runnable::run); } /** @@ -78,18 +77,16 @@ public HlsDownloader( * download. If empty, all renditions are downloaded. * @param cacheDataSourceFactory A {@link CacheDataSource.Factory} for the cache into which the * download will be written. - * @param executorService An {@link ExecutorService} used to make requests for the media being - * downloaded. Must not be a direct executor, but may be {@code null} if the requests should - * be made directly from the thread that calls {@link #download(ProgressListener)}. Providing - * an {@link ExecutorService} that uses multiple threads will speed up the download by + * @param executor An {@link Executor} used to make requests for the media being downloaded. + * Providing an {@link Executor} that uses multiple threads will speed up the download by * allowing parts of it to be executed in parallel. */ public HlsDownloader( Uri playlistUri, List streamKeys, CacheDataSource.Factory cacheDataSourceFactory, - @Nullable ExecutorService executorService) { - super(playlistUri, streamKeys, cacheDataSourceFactory, executorService); + Executor executor) { + super(playlistUri, streamKeys, cacheDataSourceFactory, executor); } @Override diff --git a/library/smoothstreaming/src/main/java/com/google/android/exoplayer2/source/smoothstreaming/offline/SsDownloader.java b/library/smoothstreaming/src/main/java/com/google/android/exoplayer2/source/smoothstreaming/offline/SsDownloader.java index 64d3e438685..dc141a212f6 100644 --- a/library/smoothstreaming/src/main/java/com/google/android/exoplayer2/source/smoothstreaming/offline/SsDownloader.java +++ b/library/smoothstreaming/src/main/java/com/google/android/exoplayer2/source/smoothstreaming/offline/SsDownloader.java @@ -16,7 +16,6 @@ package com.google.android.exoplayer2.source.smoothstreaming.offline; import android.net.Uri; -import androidx.annotation.Nullable; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.offline.SegmentDownloader; import com.google.android.exoplayer2.offline.StreamKey; @@ -31,7 +30,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executor; /** * A downloader for SmoothStreaming streams. @@ -68,7 +67,7 @@ public final class SsDownloader extends SegmentDownloader { */ public SsDownloader( Uri manifestUri, List streamKeys, CacheDataSource.Factory cacheDataSourceFactory) { - this(manifestUri, streamKeys, cacheDataSourceFactory, /* executorService= */ null); + this(manifestUri, streamKeys, cacheDataSourceFactory, Runnable::run); } /** @@ -77,18 +76,16 @@ public SsDownloader( * If empty, all streams are downloaded. * @param cacheDataSourceFactory A {@link CacheDataSource.Factory} for the cache into which the * download will be written. - * @param executorService An {@link ExecutorService} used to make requests for the media being - * downloaded. Must not be a direct executor, but may be {@code null} if the requests should - * be made directly from the thread that calls {@link #download(ProgressListener)}. Providing - * an {@link ExecutorService} that uses multiple threads will speed up the download by + * @param executor An {@link Executor} used to make requests for the media being downloaded. + * Providing an {@link Executor} that uses multiple threads will speed up the download by * allowing parts of it to be executed in parallel. */ public SsDownloader( Uri manifestUri, List streamKeys, CacheDataSource.Factory cacheDataSourceFactory, - @Nullable ExecutorService executorService) { - super(SsUtil.fixManifestUri(manifestUri), streamKeys, cacheDataSourceFactory, executorService); + Executor executor) { + super(SsUtil.fixManifestUri(manifestUri), streamKeys, cacheDataSourceFactory, executor); } @Override