From 6330d46d9eb2d653503dd172df70ab34839ad6b5 Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 4 Feb 2019 17:40:01 +0000 Subject: [PATCH] Improve housekeeping of ConcatenatingMediaSource callbacks. When calling releaseSource(), all pending messages will be removed. That means that all action-on-completion callbacks which are somewhere in flight are just dropped without being called. This change adds code to keep track of the current state of each callback to allow all of them being called when the source is released. Issue:#5464 PiperOrigin-RevId: 232312528 --- RELEASENOTES.md | 2 + .../source/ConcatenatingMediaSource.java | 266 +++++++++++------- .../source/ConcatenatingMediaSourceTest.java | 40 ++- 3 files changed, 192 insertions(+), 116 deletions(-) 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);