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
google · Apr 27, 2020 · 2e9ed51 · 2e9ed51
1 parent 008d38d
commit 2e9ed51
Show file tree

Hide file tree

Showing 7 changed files with 43 additions and 62 deletions.
diff --git 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 {
-  <init>(android.net.Uri, java.util.List, com.google.android.exoplayer2.upstream.cache.CacheDataSource.Factory, java.util.concurrent.ExecutorService);
+  <init>(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 {
-  <init>(android.net.Uri, java.util.List, com.google.android.exoplayer2.upstream.cache.CacheDataSource.Factory, java.util.concurrent.ExecutorService);
+  <init>(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 {
-  <init>(android.net.Uri, java.util.List, com.google.android.exoplayer2.upstream.cache.CacheDataSource.Factory, java.util.concurrent.ExecutorService);
+  <init>(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/...ry/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloaderFactory.java b/...ry/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,32 +80,30 @@ public class DefaultDownloaderFactory implements DownloaderFactory {
    *     downloads will be written.
    */
   public DefaultDownloaderFactory(CacheDataSource.Factory cacheDataSourceFactory) {
-    this(cacheDataSourceFactory, /* executorService= */ null);
+    this(cacheDataSourceFactory, Runnable::run);
   }
 
   /**
    * Creates an instance.
    *
    * @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
   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,18 +115,14 @@ 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<? extends Downloader> constructor) {
     if (constructor == null) {
       throw new IllegalStateException("Module missing for: " + request.type);
     }
     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<? extends Downloader> 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
@@ -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
@@ -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<StreamKey> 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<StreamKey> 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/.../dash/src/main/java/com/google/android/exoplayer2/source/dash/offline/DashDownloader.java b/.../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<DashManifest> {
    */
   public DashDownloader(
       Uri manifestUri, List<StreamKey> 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<StreamKey> streamKeys,
       CacheDataSource.Factory cacheDataSourceFactory,
-      @Nullable ExecutorService executorService) {
-    super(manifestUri, streamKeys, cacheDataSourceFactory, executorService);
+      Executor executor) {
+    super(manifestUri, streamKeys, cacheDataSourceFactory, executor);
   }
 
   @Override

diff --git a/...ary/hls/src/main/java/com/google/android/exoplayer2/source/hls/offline/HlsDownloader.java b/...ary/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<HlsPlaylist> {
    */
   public HlsDownloader(
       Uri playlistUri, List<StreamKey> 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<StreamKey> streamKeys,
       CacheDataSource.Factory cacheDataSourceFactory,
-      @Nullable ExecutorService executorService) {
-    super(playlistUri, streamKeys, cacheDataSourceFactory, executorService);
+      Executor executor) {
+    super(playlistUri, streamKeys, cacheDataSourceFactory, executor);
   }
 
   @Override

diff --git a/.../main/java/com/google/android/exoplayer2/source/smoothstreaming/offline/SsDownloader.java b/.../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<SsManifest> {
    */
   public SsDownloader(
       Uri manifestUri, List<StreamKey> 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<StreamKey> streamKeys,
       CacheDataSource.Factory cacheDataSourceFactory,
-      @Nullable ExecutorService executorService) {
-    super(SsUtil.fixManifestUri(manifestUri), streamKeys, cacheDataSourceFactory, executorService);
+      Executor executor) {
+    super(SsUtil.fixManifestUri(manifestUri), streamKeys, cacheDataSourceFactory, executor);
   }
 
   @Override