From 5b2b882e81f9a78c0715e5abff7073309836eb9a Mon Sep 17 00:00:00 2001 From: christosts Date: Wed, 4 Aug 2021 12:55:00 +0100 Subject: [PATCH] Change how AnalyticsCollector releases listeners The AnalyticsCollector releases listeners lazily so that listener callbacks triggered on the application looper after SimpleExoPlayer.release() are still handled. The change in ListenerSet to post the onEvents callback on the front of the application looper changed (correctly) how onEvents are propagated, however this made the AnalyticsCollector deliver onEvents with out-of-order EventTimes. This change fixes AnalyticsCollector to trigger onPlayerReleased() and the matching onEvents() event in the correct order. #minor-release PiperOrigin-RevId: 388668739 --- .../android/exoplayer2/util/ListenerSet.java | 37 ++++--------------- .../exoplayer2/util/ListenerSetTest.java | 29 --------------- .../analytics/AnalyticsCollector.java | 11 +++++- 3 files changed, 16 insertions(+), 61 deletions(-) diff --git a/library/common/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java b/library/common/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java index 43d57f988bc..7f2ce9759a4 100644 --- a/library/common/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java +++ b/library/common/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java @@ -67,7 +67,6 @@ public interface IterationFinishedEvent { } private static final int MSG_ITERATION_FINISHED = 0; - private static final int MSG_LAZY_RELEASE = 1; private final Clock clock; private final HandlerWrapper handler; @@ -220,37 +219,15 @@ public void release() { released = true; } - /** - * Releases the set of listeners after all already scheduled {@link Looper} messages were able to - * trigger final events. - * - *

After the specified released callback event, no other events are sent to a listener. - * - * @param releaseEventFlag An integer flag indicating the type of the release event, or {@link - * C#INDEX_UNSET} to report this event without a flag. - * @param releaseEvent The release event. - */ - public void lazyRelease(int releaseEventFlag, Event releaseEvent) { - handler.obtainMessage(MSG_LAZY_RELEASE, releaseEventFlag, 0, releaseEvent).sendToTarget(); - } - private boolean handleMessage(Message message) { - if (message.what == MSG_ITERATION_FINISHED) { - for (ListenerHolder holder : listeners) { - holder.iterationFinished(iterationFinishedEvent); - if (handler.hasMessages(MSG_ITERATION_FINISHED)) { - // The invocation above triggered new events (and thus scheduled a new message). We need - // to stop here because this new message will take care of informing every listener about - // the new update (including the ones already called here). - break; - } + for (ListenerHolder holder : listeners) { + holder.iterationFinished(iterationFinishedEvent); + if (handler.hasMessages(MSG_ITERATION_FINISHED)) { + // The invocation above triggered new events (and thus scheduled a new message). We need + // to stop here because this new message will take care of informing every listener about + // the new update (including the ones already called here). + break; } - } else if (message.what == MSG_LAZY_RELEASE) { - int releaseEventFlag = message.arg1; - @SuppressWarnings("unchecked") - Event releaseEvent = (Event) message.obj; - sendEvent(releaseEventFlag, releaseEvent); - release(); } return true; } diff --git a/library/common/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java b/library/common/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java index f98f9f09653..d350e99c32a 100644 --- a/library/common/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java +++ b/library/common/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java @@ -22,7 +22,6 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; -import android.os.Handler; import android.os.Looper; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.exoplayer2.C; @@ -367,34 +366,6 @@ public void release_preventsRegisteringNewListeners() { verify(listener, never()).callback1(); } - @Test - public void lazyRelease_stopsForwardingEventsFromNewHandlerMessagesAndCallsReleaseCallback() { - ListenerSet listenerSet = - new ListenerSet<>(Looper.myLooper(), Clock.DEFAULT, TestListener::iterationFinished); - TestListener listener = mock(TestListener.class); - listenerSet.add(listener); - - // In-line event before release. - listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1); - // Message triggering event sent before release. - new Handler().post(() -> listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1)); - // Lazy release with release callback. - listenerSet.lazyRelease(EVENT_ID_3, TestListener::callback3); - // In-line event after release. - listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1); - // Message triggering event sent after release. - new Handler().post(() -> listenerSet.sendEvent(EVENT_ID_2, TestListener::callback2)); - ShadowLooper.runMainLooperToNextTask(); - - // Verify all events are delivered except for the one triggered by the message sent after the - // lazy release. - verify(listener, times(3)).callback1(); - verify(listener).callback3(); - verify(listener, times(2)).iterationFinished(createFlagSet(EVENT_ID_1)); - verify(listener).iterationFinished(createFlagSet(EVENT_ID_3)); - verifyNoMoreInteractions(listener); - } - private interface TestListener { default void callback1() {} diff --git a/library/core/src/main/java/com/google/android/exoplayer2/analytics/AnalyticsCollector.java b/library/core/src/main/java/com/google/android/exoplayer2/analytics/AnalyticsCollector.java index 5e4682ef46c..00d75472026 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/analytics/AnalyticsCollector.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/analytics/AnalyticsCollector.java @@ -16,6 +16,7 @@ package com.google.android.exoplayer2.analytics; import static com.google.android.exoplayer2.util.Assertions.checkNotNull; +import static com.google.android.exoplayer2.util.Assertions.checkStateNotNull; import android.os.Looper; import android.util.SparseArray; @@ -50,6 +51,7 @@ import com.google.android.exoplayer2.upstream.BandwidthMeter; import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.Clock; +import com.google.android.exoplayer2.util.HandlerWrapper; import com.google.android.exoplayer2.util.ListenerSet; import com.google.android.exoplayer2.util.Util; import com.google.android.exoplayer2.video.VideoRendererEventListener; @@ -82,6 +84,7 @@ public class AnalyticsCollector private ListenerSet listeners; private @MonotonicNonNull Player player; + private @MonotonicNonNull HandlerWrapper handler; private boolean isSeeking; /** @@ -131,6 +134,7 @@ public void setPlayer(Player player, Looper looper) { Assertions.checkState( this.player == null || mediaPeriodQueueTracker.mediaPeriodQueue.isEmpty()); this.player = checkNotNull(player); + handler = clock.createHandler(looper, null); listeners = listeners.copy( looper, @@ -146,10 +150,13 @@ public void setPlayer(Player player, Looper looper) { public void release() { EventTime eventTime = generateCurrentPlayerMediaPeriodEventTime(); eventTimes.put(AnalyticsListener.EVENT_PLAYER_RELEASED, eventTime); + sendEvent( + eventTime, + AnalyticsListener.EVENT_PLAYER_RELEASED, + listener -> listener.onPlayerReleased(eventTime)); // Release listeners lazily so that all events that got triggered as part of player.release() // are still delivered to all listeners. - listeners.lazyRelease( - AnalyticsListener.EVENT_PLAYER_RELEASED, listener -> listener.onPlayerReleased(eventTime)); + checkStateNotNull(handler).post(() -> listeners.release()); } /**