diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 271f2d94b9..5ba3aa7c8d 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -14,6 +14,8 @@ * OkHttp extension: Upgrade OkHttp dependency to 3.12.1. * MP3: Wider fix for issue where streams would play twice on some Samsung devices ([#4519](https://github.com/google/ExoPlayer/issues/4519)). +* Fix issue with dropped messages when releasing a `ConcatenatingMediaSource` + ([#5464](https://github.com/google/ExoPlayer/issues/5464)). ### 2.9.4 ### diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java b/library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java index 9874004ceb..7baea9979f 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/ConcatenatingMediaSource.java @@ -29,7 +29,6 @@ import com.google.android.exoplayer2.source.ShuffleOrder.DefaultShuffleOrder; import com.google.android.exoplayer2.upstream.Allocator; import com.google.android.exoplayer2.upstream.TransferListener; import com.google.android.exoplayer2.util.Assertions; -import com.google.android.exoplayer2.util.EventDispatcher; import com.google.android.exoplayer2.util.Util; import java.io.IOException; import java.util.ArrayList; @@ -37,9 +36,11 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.IdentityHashMap; import java.util.List; import java.util.Map; +import java.util.Set; /** * Concatenates multiple {@link MediaSource}s. The list of {@link MediaSource}s can be modified @@ -52,12 +53,19 @@ public class ConcatenatingMediaSource extends CompositeMediaSource mediaSourcesPublic; - @Nullable private Handler playbackThreadHandler; + + @GuardedBy("this") + private final Set pendingOnCompletionActions; + + @GuardedBy("this") + @Nullable + private Handler playbackThreadHandler; // Accessed on the playback thread only. private final List mediaSourceHolders; @@ -68,8 +76,8 @@ public class ConcatenatingMediaSource extends CompositeMediaSource pendingOnCompletionActions; + private boolean timelineUpdateScheduled; + private Set nextTimelineUpdateOnCompletionActions; private ShuffleOrder shuffleOrder; private int windowCount; private int periodCount; @@ -128,7 +136,8 @@ public class ConcatenatingMediaSource extends CompositeMediaSource(); this.mediaSourcesPublic = new ArrayList<>(); this.mediaSourceHolders = new ArrayList<>(); - this.pendingOnCompletionActions = new EventDispatcher<>(); + this.nextTimelineUpdateOnCompletionActions = new HashSet<>(); + this.pendingOnCompletionActions = new HashSet<>(); this.isAtomic = isAtomic; this.useLazyPreparation = useLazyPreparation; window = new Timeline.Window(); @@ -149,13 +158,13 @@ public class ConcatenatingMediaSource extends CompositeMediaSource mediaSources, Handler handler, Runnable actionOnCompletion) { - addPublicMediaSources(mediaSourcesPublic.size(), mediaSources, handler, actionOnCompletion); + Collection mediaSources, Handler handler, Runnable onCompletionAction) { + addPublicMediaSources(mediaSourcesPublic.size(), mediaSources, handler, onCompletionAction); } /** @@ -227,7 +236,7 @@ public class ConcatenatingMediaSource extends CompositeMediaSource mediaSources) { - addPublicMediaSources(index, mediaSources, /* handler= */ null, /* actionOnCompletion= */ null); + addPublicMediaSources(index, mediaSources, /* handler= */ null, /* onCompletionAction= */ null); } /** @@ -237,16 +246,16 @@ public class ConcatenatingMediaSource extends CompositeMediaSource mediaSources, Handler handler, - Runnable actionOnCompletion) { - addPublicMediaSources(index, mediaSources, handler, actionOnCompletion); + Runnable onCompletionAction) { + addPublicMediaSources(index, mediaSources, handler, onCompletionAction); } /** @@ -262,7 +271,7 @@ public class ConcatenatingMediaSource extends CompositeMediaSource mediaSources, @Nullable Handler handler, - @Nullable Runnable actionOnCompletion) { - Assertions.checkArgument((handler == null) == (actionOnCompletion == null)); + @Nullable Runnable onCompletionAction) { + Assertions.checkArgument((handler == null) == (onCompletionAction == null)); + Handler playbackThreadHandler = this.playbackThreadHandler; for (MediaSource mediaSource : mediaSources) { Assertions.checkNotNull(mediaSource); } @@ -535,12 +548,12 @@ public class ConcatenatingMediaSource extends CompositeMediaSource(index, mediaSourceHolders, handler, actionOnCompletion)) + .obtainMessage(MSG_ADD, new MessageData<>(index, mediaSourceHolders, callbackAction)) .sendToTarget(); - } else if (actionOnCompletion != null && handler != null) { - handler.post(actionOnCompletion); + } else if (onCompletionAction != null && handler != null) { + handler.post(onCompletionAction); } } @@ -549,16 +562,17 @@ public class ConcatenatingMediaSource extends CompositeMediaSource(fromIndex, toIndex, handler, actionOnCompletion)) + .obtainMessage(MSG_REMOVE, new MessageData<>(fromIndex, toIndex, callbackAction)) .sendToTarget(); - } else if (actionOnCompletion != null && handler != null) { - handler.post(actionOnCompletion); + } else if (onCompletionAction != null && handler != null) { + handler.post(onCompletionAction); } } @@ -567,23 +581,24 @@ public class ConcatenatingMediaSource extends CompositeMediaSource(currentIndex, newIndex, handler, actionOnCompletion)) + .obtainMessage(MSG_MOVE, new MessageData<>(currentIndex, newIndex, callbackAction)) .sendToTarget(); - } else if (actionOnCompletion != null && handler != null) { - handler.post(actionOnCompletion); + } else if (onCompletionAction != null && handler != null) { + handler.post(onCompletionAction); } } @GuardedBy("this") private void setPublicShuffleOrder( - ShuffleOrder shuffleOrder, @Nullable Handler handler, @Nullable Runnable actionOnCompletion) { - Assertions.checkArgument((handler == null) == (actionOnCompletion == null)); + ShuffleOrder shuffleOrder, @Nullable Handler handler, @Nullable Runnable onCompletionAction) { + Assertions.checkArgument((handler == null) == (onCompletionAction == null)); Handler playbackThreadHandler = this.playbackThreadHandler; if (playbackThreadHandler != null) { int size = getSize(); @@ -593,20 +608,33 @@ public class ConcatenatingMediaSource extends CompositeMediaSource(/* index= */ 0, shuffleOrder, handler, actionOnCompletion)) + new MessageData<>(/* index= */ 0, shuffleOrder, callbackAction)) .sendToTarget(); } else { this.shuffleOrder = shuffleOrder.getLength() > 0 ? shuffleOrder.cloneAndClear() : shuffleOrder; - if (actionOnCompletion != null && handler != null) { - handler.post(actionOnCompletion); + if (onCompletionAction != null && handler != null) { + handler.post(onCompletionAction); } } } + @GuardedBy("this") + @Nullable + private HandlerAndRunnable createOnCompletionAction( + @Nullable Handler handler, @Nullable Runnable runnable) { + if (handler == null || runnable == null) { + return null; + } + HandlerAndRunnable handlerAndRunnable = new HandlerAndRunnable(handler, runnable); + pendingOnCompletionActions.add(handlerAndRunnable); + return handlerAndRunnable; + } + // Internal methods. Called on the playback thread. @SuppressWarnings("unchecked") @@ -617,7 +645,7 @@ public class ConcatenatingMediaSource extends CompositeMediaSource>) Util.castNonNull(msg.obj); shuffleOrder = shuffleOrder.cloneAndInsert(addMessage.index, addMessage.customData.size()); addMediaSourcesInternal(addMessage.index, addMessage.customData); - scheduleListenerNotification(addMessage.handler, addMessage.actionOnCompletion); + scheduleTimelineUpdate(addMessage.onCompletionAction); break; case MSG_REMOVE: MessageData removeMessage = (MessageData) Util.castNonNull(msg.obj); @@ -631,29 +659,27 @@ public class ConcatenatingMediaSource extends CompositeMediaSource= fromIndex; index--) { removeMediaSourceInternal(index); } - scheduleListenerNotification(removeMessage.handler, removeMessage.actionOnCompletion); + scheduleTimelineUpdate(removeMessage.onCompletionAction); break; case MSG_MOVE: MessageData moveMessage = (MessageData) Util.castNonNull(msg.obj); shuffleOrder = shuffleOrder.cloneAndRemove(moveMessage.index, moveMessage.index + 1); shuffleOrder = shuffleOrder.cloneAndInsert(moveMessage.customData, 1); moveMediaSourceInternal(moveMessage.index, moveMessage.customData); - scheduleListenerNotification(moveMessage.handler, moveMessage.actionOnCompletion); + scheduleTimelineUpdate(moveMessage.onCompletionAction); break; case MSG_SET_SHUFFLE_ORDER: MessageData shuffleOrderMessage = (MessageData) Util.castNonNull(msg.obj); shuffleOrder = shuffleOrderMessage.customData; - scheduleListenerNotification( - shuffleOrderMessage.handler, shuffleOrderMessage.actionOnCompletion); + scheduleTimelineUpdate(shuffleOrderMessage.onCompletionAction); break; - case MSG_NOTIFY_LISTENER: - notifyListener(); + case MSG_UPDATE_TIMELINE: + updateTimelineAndScheduleOnCompletionActions(); break; case MSG_ON_COMPLETION: - EventDispatcher actionsOnCompletion = - (EventDispatcher) Util.castNonNull(msg.obj); - actionsOnCompletion.dispatch(Runnable::run); + Set actions = (Set) Util.castNonNull(msg.obj); + dispatchOnCompletionActions(actions); break; default: throw new IllegalStateException(); @@ -661,36 +687,48 @@ public class ConcatenatingMediaSource extends CompositeMediaSource actionsOnCompletion = pendingOnCompletionActions; - pendingOnCompletionActions = new EventDispatcher<>(); + private void updateTimelineAndScheduleOnCompletionActions() { + timelineUpdateScheduled = false; + Set onCompletionActions = nextTimelineUpdateOnCompletionActions; + nextTimelineUpdateOnCompletionActions = new HashSet<>(); refreshSourceInfo( new ConcatenatedTimeline( mediaSourceHolders, windowCount, periodCount, shuffleOrder, isAtomic), /* manifest= */ null); - Assertions.checkNotNull(playbackThreadHandler) - .obtainMessage(MSG_ON_COMPLETION, actionsOnCompletion) + getPlaybackThreadHandlerOnPlaybackThread() + .obtainMessage(MSG_ON_COMPLETION, onCompletionActions) .sendToTarget(); } + @SuppressWarnings("GuardedBy") + private Handler getPlaybackThreadHandlerOnPlaybackThread() { + // Write access to this value happens on the playback thread only, so playback thread reads + // don't need to be synchronized. + return Assertions.checkNotNull(playbackThreadHandler); + } + + private synchronized void dispatchOnCompletionActions( + Set onCompletionActions) { + for (HandlerAndRunnable pendingAction : onCompletionActions) { + pendingAction.dispatch(); + } + pendingOnCompletionActions.removeAll(onCompletionActions); + } + private void addMediaSourcesInternal( int index, Collection mediaSourceHolders) { for (MediaSourceHolder mediaSourceHolder : mediaSourceHolders) { @@ -787,7 +825,7 @@ public class ConcatenatingMediaSource extends CompositeMediaSource { + SourceInfoRefreshListener listener = mock(SourceInfoRefreshListener.class); + mediaSource.addMediaSources(Arrays.asList(createMediaSources(2))); + mediaSource.prepareSource(listener, /* mediaTransferListener= */ null); + mediaSource.moveMediaSource( + /* currentIndex= */ 0, + /* newIndex= */ 1, + new Handler(), + callbackCalledCondition::open); + mediaSource.releaseSource(listener); + }); + assertThat(callbackCalledCondition.block(MediaSourceTestRunner.TIMEOUT_MS)).isTrue(); + } finally { + dummyMainThread.release(); + } + } + @Test public void testPeriodCreationWithAds() throws IOException, InterruptedException { // Create concatenated media source with ad child source. @@ -973,7 +997,7 @@ public final class ConcatenatingMediaSourceTest { @Test public void testCustomCallbackBeforePreparationSetShuffleOrder() throws Exception { - Runnable runnable = Mockito.mock(Runnable.class); + Runnable runnable = mock(Runnable.class); mediaSource.setShuffleOrder( new ShuffleOrder.UnshuffledShuffleOrder(/* length= */ 0), new Handler(), runnable);