From 047310b31d8c58b9e98c167e029e5c5ebabdf64a Mon Sep 17 00:00:00 2001 From: Marc Baechinger Date: Fri, 30 Sep 2022 17:32:37 +0000 Subject: [PATCH] Merge pull request #10570 from Artemych:fix/progressive_downloader_infinite_loop PiperOrigin-RevId: 472475124 (cherry picked from commit cec6f045ea216b2cb54075d3c8503c0923b06802) --- RELEASENOTES.md | 4 ++ .../offline/ProgressiveDownloader.java | 32 +++++++------ .../offline/ProgressiveDownloaderTest.java | 47 +++++++++++++++++++ 3 files changed, 68 insertions(+), 15 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 4a91a46a51..8cdadc2dcf 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -11,6 +11,10 @@ ([#10057](https://github.com/google/ExoPlayer/issues/10057)). * Limit parallel download removals to 1 to avoid excessive thread creation ([#10458](https://github.com/google/ExoPlayer/issues/10458)). +* Downloads: + * Fix potential infinite loop in `ProgressiveDownloader` caused by + simultaneous download and playback with the same `PriorityTaskManager` + ([#10570](https://github.com/google/ExoPlayer/pull/10570)). * Audio: * Adds `AudioOffloadListener.onExperimentalOffloadedPlayback` for the AudioTrack offload state. diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/offline/ProgressiveDownloader.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/offline/ProgressiveDownloader.java index ffdc3c1387..db974fc5ba 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/offline/ProgressiveDownloader.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/offline/ProgressiveDownloader.java @@ -15,6 +15,8 @@ */ package androidx.media3.exoplayer.offline; +import static androidx.media3.common.util.Assertions.checkNotNull; + import androidx.annotation.Nullable; import androidx.media3.common.C; import androidx.media3.common.MediaItem; @@ -90,26 +92,26 @@ public final class ProgressiveDownloader implements Downloader { public void download(@Nullable ProgressListener progressListener) throws IOException, InterruptedException { this.progressListener = progressListener; - downloadRunnable = - new RunnableFutureTask() { - @Override - protected Void doWork() throws IOException { - cacheWriter.cache(); - return null; - } - - @Override - protected void cancelWork() { - cacheWriter.cancel(); - } - }; - if (priorityTaskManager != null) { priorityTaskManager.add(C.PRIORITY_DOWNLOAD); } try { boolean finished = false; while (!finished && !isCanceled) { + // Recreate downloadRunnable on each loop iteration to avoid rethrowing a previous error. + downloadRunnable = + new RunnableFutureTask() { + @Override + protected Void doWork() throws IOException { + cacheWriter.cache(); + return null; + } + + @Override + protected void cancelWork() { + cacheWriter.cancel(); + } + }; if (priorityTaskManager != null) { priorityTaskManager.proceed(C.PRIORITY_DOWNLOAD); } @@ -132,7 +134,7 @@ public final class ProgressiveDownloader implements Downloader { } finally { // If the main download thread was interrupted as part of cancelation, then it's possible that // the runnable is still doing work. We need to wait until it's finished before returning. - downloadRunnable.blockUntilFinished(); + checkNotNull(downloadRunnable).blockUntilFinished(); if (priorityTaskManager != null) { priorityTaskManager.remove(C.PRIORITY_DOWNLOAD); } diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/offline/ProgressiveDownloaderTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/offline/ProgressiveDownloaderTest.java index ce9db0ae8e..48923cd434 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/offline/ProgressiveDownloaderTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/offline/ProgressiveDownloaderTest.java @@ -19,7 +19,9 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; import android.net.Uri; +import androidx.media3.common.C; import androidx.media3.common.MediaItem; +import androidx.media3.common.PriorityTaskManager; import androidx.media3.common.util.Util; import androidx.media3.database.DatabaseProvider; import androidx.media3.datasource.DataSource; @@ -126,6 +128,51 @@ public class ProgressiveDownloaderTest { assertThat(progressListener.bytesDownloaded).isEqualTo(1024); } + @Test + public void download_afterPriorityTooLow_succeeds() throws Exception { + PriorityTaskManager priorityTaskManager = new PriorityTaskManager(); + AtomicBoolean hasSetPlaybackPriority = new AtomicBoolean(/* initialValue= */ false); + Uri uri = Uri.parse("test:///test.mp4"); + // Fake data which briefly sets the playback priority while downloading for the first time. + FakeDataSet data = new FakeDataSet(); + data.newData(uri) + .appendReadAction( + () -> { + if (!hasSetPlaybackPriority.getAndSet(true)) { + // This only interrupts the download the next time the DataSource checks for the + // priority, so delay the removal of the playback priority slightly. As we can't + // check when the DataSource throws the PriorityTooLowException exactly, we need to + // use an arbitrary delay. + priorityTaskManager.add(C.PRIORITY_PLAYBACK); + new Thread( + () -> { + try { + Thread.sleep(200); + } catch (InterruptedException e) { + // Ignore. + } + priorityTaskManager.remove(C.PRIORITY_PLAYBACK); + }) + .start(); + } + }) + .appendReadData(2_000_000); + DataSource.Factory upstreamDataSource = new FakeDataSource.Factory().setFakeDataSet(data); + MediaItem mediaItem = MediaItem.fromUri(uri); + CacheDataSource.Factory cacheDataSourceFactory = + new CacheDataSource.Factory() + .setCache(downloadCache) + .setUpstreamDataSourceFactory(upstreamDataSource) + .setUpstreamPriorityTaskManager(priorityTaskManager); + ProgressiveDownloader downloader = new ProgressiveDownloader(mediaItem, cacheDataSourceFactory); + TestProgressListener progressListener = new TestProgressListener(); + + // Download expected to finish (despite the interruption by the higher playback priority). + downloader.download(progressListener); + + assertThat(progressListener.bytesDownloaded).isEqualTo(2_000_000); + } + private static final class TestProgressListener implements Downloader.ProgressListener { public long bytesDownloaded;