From edd3a3f3492cd5df3ddc3a287dac82f61da2a439 Mon Sep 17 00:00:00 2001 From: tianyifeng Date: Wed, 24 Jul 2024 12:38:42 -0700 Subject: [PATCH] Fix bug where BasePreloadManager.Listener invokes from incorrect thread The DefaultPreloadManagerTest didn't to catch this because we use main looper as the preload looper in the tests. This CL also improves the tests by assigning the preload looper with one that corresponds to a different thread. PiperOrigin-RevId: 655664189 --- .../source/preload/BasePreloadManager.java | 51 ++++++------ .../preload/DefaultPreloadManagerTest.java | 83 ++++++++++++------- 2 files changed, 81 insertions(+), 53 deletions(-) diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/preload/BasePreloadManager.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/preload/BasePreloadManager.java index 6657ffb15a..d8e218f856 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/preload/BasePreloadManager.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/preload/BasePreloadManager.java @@ -72,13 +72,12 @@ public abstract class BasePreloadManager { } private final Object lock; - private final Looper looper; protected final Comparator rankingDataComparator; private final TargetPreloadStatusControl targetPreloadStatusControl; private final MediaSource.Factory mediaSourceFactory; private final ListenerSet listeners; private final Map mediaItemMediaSourceHolderMap; - private final Handler startPreloadingHandler; + private final Handler applicationHandler; @GuardedBy("lock") private final PriorityQueue sourceHolderPriorityQueue; @@ -92,13 +91,13 @@ public abstract class BasePreloadManager { TargetPreloadStatusControl targetPreloadStatusControl, MediaSource.Factory mediaSourceFactory) { lock = new Object(); - looper = Util.getCurrentOrMainLooper(); + applicationHandler = Util.createHandlerForCurrentOrMainLooper(); this.rankingDataComparator = rankingDataComparator; this.targetPreloadStatusControl = targetPreloadStatusControl; this.mediaSourceFactory = mediaSourceFactory; - listeners = new ListenerSet<>(looper, Clock.DEFAULT, (listener, flags) -> {}); + listeners = + new ListenerSet<>(applicationHandler.getLooper(), Clock.DEFAULT, (listener, flags) -> {}); mediaItemMediaSourceHolderMap = new HashMap<>(); - startPreloadingHandler = Util.createHandlerForCurrentOrMainLooper(); sourceHolderPriorityQueue = new PriorityQueue<>(); } @@ -257,35 +256,39 @@ public abstract class BasePreloadManager { /** Called when the given {@link MediaSource} completes preloading. */ protected final void onPreloadCompleted(MediaSource source) { - listeners.sendEvent( - /* eventFlag= */ C.INDEX_UNSET, listener -> listener.onCompleted(source.getMediaItem())); - maybeAdvanceToNextSource(source); + applicationHandler.post( + () -> { + listeners.sendEvent( + /* eventFlag= */ C.INDEX_UNSET, + listener -> listener.onCompleted(source.getMediaItem())); + maybeAdvanceToNextSource(source); + }); } /** Called when an error occurs. */ protected final void onPreloadError(PreloadException error, MediaSource source) { - listeners.sendEvent(/* eventFlag= */ C.INDEX_UNSET, listener -> listener.onError(error)); - maybeAdvanceToNextSource(source); + applicationHandler.post( + () -> { + listeners.sendEvent(/* eventFlag= */ C.INDEX_UNSET, listener -> listener.onError(error)); + maybeAdvanceToNextSource(source); + }); } /** Called when the given {@link MediaSource} has been skipped before completing preloading. */ protected final void onPreloadSkipped(MediaSource source) { - maybeAdvanceToNextSource(source); + applicationHandler.post(() -> maybeAdvanceToNextSource(source)); } private void maybeAdvanceToNextSource(MediaSource preloadingSource) { - startPreloadingHandler.post( - () -> { - synchronized (lock) { - if (sourceHolderPriorityQueue.isEmpty() - || checkNotNull(sourceHolderPriorityQueue.peek()).mediaSource != preloadingSource) { - return; - } - do { - sourceHolderPriorityQueue.poll(); - } while (!sourceHolderPriorityQueue.isEmpty() && !maybeStartPreloadNextSource()); - } - }); + synchronized (lock) { + if (sourceHolderPriorityQueue.isEmpty() + || checkNotNull(sourceHolderPriorityQueue.peek()).mediaSource != preloadingSource) { + return; + } + do { + sourceHolderPriorityQueue.poll(); + } while (!sourceHolderPriorityQueue.isEmpty() && !maybeStartPreloadNextSource()); + } } /** @@ -372,7 +375,7 @@ public abstract class BasePreloadManager { } private void verifyApplicationThread() { - if (Looper.myLooper() != looper) { + if (Looper.myLooper() != applicationHandler.getLooper()) { throw new IllegalStateException("Preload manager is accessed on the wrong thread."); } } diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/preload/DefaultPreloadManagerTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/preload/DefaultPreloadManagerTest.java index 15b20fb2b0..ddeab4e781 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/preload/DefaultPreloadManagerTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/preload/DefaultPreloadManagerTest.java @@ -29,6 +29,7 @@ import static org.robolectric.Shadows.shadowOf; import android.content.Context; import android.net.Uri; +import android.os.HandlerThread; import android.os.Looper; import androidx.annotation.Nullable; import androidx.media3.common.AdPlaybackState; @@ -186,7 +187,7 @@ public class DefaultPreloadManagerTest { } @Test - public void invalidate_withoutSettingCurrentPlayingIndex_sourcesPreloadedToTargetStatusesInOrder() + public void invalidate_withoutSettingCurrentPlayingIndex_sourcesPreloadedToTargetStatusInOrder() throws Exception { ArrayList targetPreloadStatusControlCallStates = new ArrayList<>(); AtomicInteger currentPlayingItemIndex = new AtomicInteger(); @@ -202,6 +203,8 @@ public class DefaultPreloadManagerTest { ProgressiveMediaSource.Factory mediaSourceFactory = new ProgressiveMediaSource.Factory( new DefaultDataSource.Factory(ApplicationProvider.getApplicationContext())); + HandlerThread preloadThread = new HandlerThread("preload"); + preloadThread.start(); DefaultPreloadManager preloadManager = new DefaultPreloadManager( targetPreloadStatusControl, @@ -210,7 +213,7 @@ public class DefaultPreloadManagerTest { bandwidthMeter, rendererCapabilitiesListFactory, allocator, - Util.getCurrentOrMainLooper()); + preloadThread.getLooper()); TestPreloadManagerListener preloadManagerListener = new TestPreloadManagerListener(); preloadManager.addListener(preloadManagerListener); MediaItem.Builder mediaItemBuilder = new MediaItem.Builder(); @@ -240,10 +243,12 @@ public class DefaultPreloadManagerTest { assertThat(preloadManagerListener.onCompletedMediaItemRecords) .containsExactly(mediaItem0, mediaItem1, mediaItem2) .inOrder(); + + preloadThread.quit(); } @Test - public void invalidate_withSettingCurrentPlayingIndex_sourcesPreloadedToTargetStatusesInOrder() + public void invalidate_withSettingCurrentPlayingIndex_sourcesPreloadedToTargetStatusInOrder() throws Exception { ArrayList targetPreloadStatusControlCallStates = new ArrayList<>(); AtomicInteger currentPlayingItemIndex = new AtomicInteger(); @@ -259,6 +264,8 @@ public class DefaultPreloadManagerTest { ProgressiveMediaSource.Factory mediaSourceFactory = new ProgressiveMediaSource.Factory( new DefaultDataSource.Factory(ApplicationProvider.getApplicationContext())); + HandlerThread preloadThread = new HandlerThread("preload"); + preloadThread.start(); DefaultPreloadManager preloadManager = new DefaultPreloadManager( targetPreloadStatusControl, @@ -267,7 +274,7 @@ public class DefaultPreloadManagerTest { bandwidthMeter, rendererCapabilitiesListFactory, allocator, - Util.getCurrentOrMainLooper()); + preloadThread.getLooper()); TestPreloadManagerListener preloadManagerListener = new TestPreloadManagerListener(); preloadManager.addListener(preloadManagerListener); MediaItem.Builder mediaItemBuilder = new MediaItem.Builder(); @@ -299,10 +306,13 @@ public class DefaultPreloadManagerTest { assertThat(preloadManagerListener.onCompletedMediaItemRecords) .containsExactly(mediaItem2, mediaItem1, mediaItem0) .inOrder(); + + preloadThread.quit(); } @Test - public void invalidate_sourceHandedOverToPlayerDuringPreloading_continuesPreloadingNextSource() { + public void invalidate_sourceHandedOverToPlayerDuringPreloading_continuesPreloadingNextSource() + throws Exception { ArrayList targetPreloadStatusControlCallStates = new ArrayList<>(); TargetPreloadStatusControl targetPreloadStatusControl = rankingData -> { @@ -310,6 +320,8 @@ public class DefaultPreloadManagerTest { return new DefaultPreloadManager.Status(STAGE_SOURCE_PREPARED); }; FakeMediaSourceFactory fakeMediaSourceFactory = new FakeMediaSourceFactory(); + HandlerThread preloadThread = new HandlerThread("preload"); + preloadThread.start(); DefaultPreloadManager preloadManager = new DefaultPreloadManager( targetPreloadStatusControl, @@ -318,7 +330,7 @@ public class DefaultPreloadManagerTest { bandwidthMeter, rendererCapabilitiesListFactory, allocator, - Util.getCurrentOrMainLooper()); + preloadThread.getLooper()); TestPreloadManagerListener preloadManagerListener = new TestPreloadManagerListener(); preloadManager.addListener(preloadManagerListener); MediaItem.Builder mediaItemBuilder = new MediaItem.Builder(); @@ -341,14 +353,17 @@ public class DefaultPreloadManagerTest { (source, timeline) -> {}, bandwidthMeter.getTransferListener(), PlayerId.UNSET); wrappedMediaSource0.setAllowPreparation(true); wrappedMediaSource1.setAllowPreparation(true); - shadowOf(Looper.getMainLooper()).idle(); + runMainLooperUntil(() -> preloadManagerListener.onCompletedMediaItemRecords.size() == 1); assertThat(targetPreloadStatusControlCallStates).containsExactly(0, 1).inOrder(); assertThat(preloadManagerListener.onCompletedMediaItemRecords).containsExactly(mediaItem1); + + preloadThread.quit(); } @Test - public void invalidate_beforePreloadCompletedForLastInvalidate_preloadRespectsToLatestOrder() { + public void invalidate_beforePreloadCompletedForLastInvalidate_preloadRespectsToLatestOrder() + throws Exception { ArrayList targetPreloadStatusControlCallStates = new ArrayList<>(); TargetPreloadStatusControl targetPreloadStatusControl = rankingData -> { @@ -356,6 +371,8 @@ public class DefaultPreloadManagerTest { return new DefaultPreloadManager.Status(STAGE_SOURCE_PREPARED); }; FakeMediaSourceFactory fakeMediaSourceFactory = new FakeMediaSourceFactory(); + HandlerThread preloadThread = new HandlerThread("preload"); + preloadThread.start(); DefaultPreloadManager preloadManager = new DefaultPreloadManager( targetPreloadStatusControl, @@ -364,7 +381,7 @@ public class DefaultPreloadManagerTest { bandwidthMeter, rendererCapabilitiesListFactory, allocator, - Util.getCurrentOrMainLooper()); + preloadThread.getLooper()); TestPreloadManagerListener preloadManagerListener = new TestPreloadManagerListener(); preloadManager.addListener(preloadManagerListener); MediaItem.Builder mediaItemBuilder = new MediaItem.Builder(); @@ -387,7 +404,7 @@ public class DefaultPreloadManagerTest { preloadManager.invalidate(); wrappedMediaSource0.setAllowPreparation(true); - shadowOf(Looper.getMainLooper()).idle(); + runMainLooperUntil(() -> preloadManagerListener.onCompletedMediaItemRecords.size() == 1); assertThat(targetPreloadStatusControlCallStates).containsExactly(0, 1).inOrder(); assertThat(preloadManagerListener.onCompletedMediaItemRecords).containsExactly(mediaItem0); @@ -398,29 +415,36 @@ public class DefaultPreloadManagerTest { // Simulate the delay of the preparation of wrappedMediaSource1, which was triggered at the // first call of invalidate(). This is expected to result in nothing, as the whole flow of - // preloading should respect the priority order triggered by the latest call of invalidate(). + // preloading should respect the priority order triggered by the latest call of invalidate(), + // which will be verified by the order of items in + // preloadManagerListener.onCompletedMediaItemRecords. wrappedMediaSource1.setAllowPreparation(true); - shadowOf(Looper.getMainLooper()).idle(); - assertThat(preloadManagerListener.onCompletedMediaItemRecords).isEmpty(); wrappedMediaSource2.setAllowPreparation(true); - shadowOf(Looper.getMainLooper()).idle(); + runMainLooperUntil(() -> preloadManagerListener.onCompletedMediaItemRecords.size() == 3); assertThat(targetPreloadStatusControlCallStates).containsExactly(2, 1, 0).inOrder(); assertThat(preloadManagerListener.onCompletedMediaItemRecords) .containsExactly(mediaItem2, mediaItem1, mediaItem0) .inOrder(); + + preloadThread.quit(); } @Test - public void invalidate_provideNullTargetPreloadStatus_sourcesSkippedForPreload() { + public void invalidate_provideNullTargetPreloadStatus_sourcesSkippedForPreload() + throws Exception { ArrayList targetPreloadStatusControlCallStates = new ArrayList<>(); TargetPreloadStatusControl targetPreloadStatusControl = rankingData -> { targetPreloadStatusControlCallStates.add(rankingData); - return null; + return (rankingData == 0) + ? null + : new DefaultPreloadManager.Status(STAGE_SOURCE_PREPARED); }; ProgressiveMediaSource.Factory mediaSourceFactory = new ProgressiveMediaSource.Factory( new DefaultDataSource.Factory(ApplicationProvider.getApplicationContext())); + HandlerThread preloadThread = new HandlerThread("preload"); + preloadThread.start(); DefaultPreloadManager preloadManager = new DefaultPreloadManager( targetPreloadStatusControl, @@ -429,7 +453,7 @@ public class DefaultPreloadManagerTest { bandwidthMeter, rendererCapabilitiesListFactory, allocator, - Util.getCurrentOrMainLooper()); + preloadThread.getLooper()); TestPreloadManagerListener preloadManagerListener = new TestPreloadManagerListener(); preloadManager.addListener(preloadManagerListener); MediaItem.Builder mediaItemBuilder = new MediaItem.Builder(); @@ -443,24 +467,21 @@ public class DefaultPreloadManagerTest { .setMediaId("mediaId1") .setUri(Uri.parse("asset://android_asset/media/mp4/sample.mp4")) .build(); - MediaItem mediaItem2 = - mediaItemBuilder - .setMediaId("mediaId2") - .setUri(Uri.parse("asset://android_asset/media/mp4/sample.mp4")) - .build(); preloadManager.add(mediaItem0, /* rankingData= */ 0); preloadManager.add(mediaItem1, /* rankingData= */ 1); - preloadManager.add(mediaItem2, /* rankingData= */ 2); preloadManager.invalidate(); - shadowOf(Looper.getMainLooper()).idle(); + runMainLooperUntil(() -> preloadManagerListener.onCompletedMediaItemRecords.size() == 1); - assertThat(targetPreloadStatusControlCallStates).containsExactly(0, 1, 2); - assertThat(preloadManagerListener.onCompletedMediaItemRecords).isEmpty(); + assertThat(targetPreloadStatusControlCallStates).containsExactly(0, 1); + assertThat(preloadManagerListener.onCompletedMediaItemRecords).containsExactly(mediaItem1); + + preloadThread.quit(); } @Test - public void invalidate_sourceHasPreloadException_continuesPreloadingNextSource() { + public void invalidate_sourceHasPreloadException_continuesPreloadingNextSource() + throws Exception { ArrayList targetPreloadStatusControlCallStates = new ArrayList<>(); TargetPreloadStatusControl targetPreloadStatusControl = rankingData -> { @@ -519,6 +540,8 @@ public class DefaultPreloadManagerTest { return mediaSource; } }; + HandlerThread preloadThread = new HandlerThread("preload"); + preloadThread.start(); DefaultPreloadManager preloadManager = new DefaultPreloadManager( targetPreloadStatusControl, @@ -527,20 +550,22 @@ public class DefaultPreloadManagerTest { bandwidthMeter, rendererCapabilitiesListFactory, allocator, - Util.getCurrentOrMainLooper()); + preloadThread.getLooper()); TestPreloadManagerListener preloadManagerListener = new TestPreloadManagerListener(); preloadManager.addListener(preloadManagerListener); preloadManager.add(mediaItem0, /* rankingData= */ 0); preloadManager.add(mediaItem1, /* rankingData= */ 1); preloadManager.invalidate(); - shadowOf(Looper.getMainLooper()).idle(); + runMainLooperUntil(() -> preloadManagerListener.onCompletedMediaItemRecords.size() == 1); assertThat(targetPreloadStatusControlCallStates).containsExactly(0, 1).inOrder(); assertThat(Iterables.getOnlyElement(preloadManagerListener.onErrorPreloadExceptionRecords)) .hasCauseThat() .isEqualTo(causeException); assertThat(preloadManagerListener.onCompletedMediaItemRecords).containsExactly(mediaItem1); + + preloadThread.quit(); } @Test