From 9982e1f15496f2c96780c874609967b682db90cc Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 14 Dec 2020 19:45:22 +0000 Subject: [PATCH] Release AnalyticsListeners after calling SimpleExoPlayer.release Currently we don't remove the AnalyticsListeners registed to SimpleExoPlayer after calling release. We didn't do this mainly because there are messages triggered as part of the release that still cause interesting events (e.g. decoderDisabled with the final counters, final dropped counts etc). However, we should fully release/remove the listeners once these pending events are delivered to: 1. Not leak listener implementations (e.g. if the listener is an Activity) 2. Ensure we don't send future events that may cause listeners to unintentionally access released or nulled variables. This could happen for example if someone calls a player method after the player was released. In addition, we can add a onPlayerReleased callback to AnalyticsListener to allow implementations to clean themselves up once all pending events are delivered. PiperOrigin-RevId: 347434344 --- .../android/exoplayer2/SimpleExoPlayer.java | 1 + .../analytics/AnalyticsCollector.java | 13 +++++ .../analytics/AnalyticsListener.java | 12 +++- .../android/exoplayer2/util/ListenerSet.java | 57 +++++++++++++------ .../exoplayer2/SimpleExoPlayerTest.java | 36 ++++++++++++ .../exoplayer2/util/ListenerSetTest.java | 29 ++++++++++ 6 files changed, 130 insertions(+), 18 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java b/library/core/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java index d102d2c6375..c4c50eaa037 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java @@ -1785,6 +1785,7 @@ public void release() { wifiLockManager.setStayAwake(false); audioFocusManager.release(); player.release(); + analyticsCollector.release(); removeSurfaceCallbacks(); if (surface != null) { if (ownsSurface) { 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 e8ab433d550..f09e5db8122 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 @@ -144,6 +144,19 @@ public void setPlayer(Player player, Looper looper) { }); } + /** + * Releases the collector. Must be called after the player for which data is collected has been + * released. + */ + public void release() { + EventTime eventTime = generateCurrentPlayerMediaPeriodEventTime(); + eventTimes.put(AnalyticsListener.EVENT_PLAYER_RELEASED, 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)); + } + /** * Updates the playback queue information used for event association. * diff --git a/library/core/src/main/java/com/google/android/exoplayer2/analytics/AnalyticsListener.java b/library/core/src/main/java/com/google/android/exoplayer2/analytics/AnalyticsListener.java index 6ac6ae5847f..6589b983519 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/analytics/AnalyticsListener.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/analytics/AnalyticsListener.java @@ -196,7 +196,8 @@ public int get(int index) { EVENT_DRM_SESSION_MANAGER_ERROR, EVENT_DRM_KEYS_RESTORED, EVENT_DRM_KEYS_REMOVED, - EVENT_DRM_SESSION_RELEASED + EVENT_DRM_SESSION_RELEASED, + EVENT_PLAYER_RELEASED, }) @interface EventFlags {} /** {@link Player#getCurrentTimeline()} changed. */ @@ -309,6 +310,8 @@ public int get(int index) { int EVENT_DRM_KEYS_REMOVED = 1034; /** A DRM session has been released. */ int EVENT_DRM_SESSION_RELEASED = 1035; + /** The player was released. */ + int EVENT_PLAYER_RELEASED = 1036; /** Time information of an event. */ final class EventTime { @@ -1013,6 +1016,13 @@ default void onDrmKeysRemoved(EventTime eventTime) {} */ default void onDrmSessionReleased(EventTime eventTime) {} + /** + * Called when the {@link Player} is released. + * + * @param eventTime The event time. + */ + default void onPlayerReleased(EventTime eventTime) {} + /** * Called after one or more events occurred. * diff --git a/library/core/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java b/library/core/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java index 24bf624338b..d0df7a662e6 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java @@ -70,8 +70,9 @@ public interface IterationFinishedEvent { } private static final int MSG_ITERATION_FINISHED = 0; + private static final int MSG_LAZY_RELEASE = 1; - private final Handler iterationFinishedHandler; + private final Handler handler; private final Supplier eventFlagsSupplier; private final IterationFinishedEvent iterationFinishedEvent; private final CopyOnWriteArraySet> listeners; @@ -113,8 +114,8 @@ private ListenerSet( queuedEvents = new ArrayDeque<>(); // It's safe to use "this" because we don't send a message before exiting the constructor. @SuppressWarnings("methodref.receiver.bound.invalid") - Handler handler = Util.createHandler(looper, this::handleIterationFinished); - iterationFinishedHandler = handler; + Handler handler = Util.createHandler(looper, this::handleMessage); + this.handler = handler; } /** @@ -165,8 +166,8 @@ public void remove(T listener) { /** * Adds an event that is sent to the listeners when {@link #flushEvents} is called. * - * @param eventFlag An integer indicating the type of the event, or {@link C#INDEX_UNSET} to not - * report this event with a flag. + * @param eventFlag An integer indicating the type of the event, or {@link C#INDEX_UNSET} to + * report this event without flag. * @param event The event. */ public void queueEvent(int eventFlag, Event event) { @@ -185,8 +186,8 @@ public void flushEvents() { if (queuedEvents.isEmpty()) { return; } - if (!iterationFinishedHandler.hasMessages(MSG_ITERATION_FINISHED)) { - iterationFinishedHandler.obtainMessage(MSG_ITERATION_FINISHED).sendToTarget(); + if (!handler.hasMessages(MSG_ITERATION_FINISHED)) { + handler.obtainMessage(MSG_ITERATION_FINISHED).sendToTarget(); } boolean recursiveFlushInProgress = !flushingEvents.isEmpty(); flushingEvents.addAll(queuedEvents); @@ -206,7 +207,7 @@ public void flushEvents() { * flushes} the event queue to notify all listeners. * * @param eventFlag An integer flag indicating the type of the event, or {@link C#INDEX_UNSET} to - * not report this event with a flag. + * report this event without flag. * @param event The event. */ public void sendEvent(int eventFlag, Event event) { @@ -215,7 +216,7 @@ public void sendEvent(int eventFlag, Event event) { } /** - * Releases the set of listeners. + * Releases the set of listeners immediately. * *

This will ensure no events are sent to any listener after this method has been called. */ @@ -227,15 +228,37 @@ public void release() { released = true; } - private boolean handleIterationFinished(Message message) { - for (ListenerHolder holder : listeners) { - holder.iterationFinished(eventFlagsSupplier, iterationFinishedEvent); - if (iterationFinishedHandler.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; + /** + * 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(eventFlagsSupplier, 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/core/src/test/java/com/google/android/exoplayer2/SimpleExoPlayerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/SimpleExoPlayerTest.java index e3a625a3ceb..a11961c301d 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/SimpleExoPlayerTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/SimpleExoPlayerTest.java @@ -15,14 +15,25 @@ */ package com.google.android.exoplayer2; +import static com.google.android.exoplayer2.robolectric.TestPlayerRunHelper.runUntilPlaybackState; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.android.exoplayer2.analytics.AnalyticsListener; +import com.google.android.exoplayer2.testutil.AutoAdvancingFakeClock; +import com.google.android.exoplayer2.testutil.ExoPlayerTestRunner; +import com.google.android.exoplayer2.testutil.FakeMediaSource; +import com.google.android.exoplayer2.testutil.FakeTimeline; +import com.google.android.exoplayer2.testutil.FakeVideoRenderer; import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; import org.junit.runner.RunWith; import org.robolectric.annotation.Config; +import org.robolectric.shadows.ShadowLooper; /** Unit test for {@link SimpleExoPlayer}. */ @RunWith(AndroidJUnit4.class) @@ -43,4 +54,29 @@ public void builder_inBackgroundThread_doesNotThrow() throws Exception { assertThat(builderThrow.get()).isNull(); } + + @Test + public void release_triggersAllPendingEventsInAnalyticsListeners() throws Exception { + SimpleExoPlayer player = + new SimpleExoPlayer.Builder( + ApplicationProvider.getApplicationContext(), + (handler, videoListener, audioListener, textOutput, metadataOutput) -> + new Renderer[] {new FakeVideoRenderer(handler, videoListener)}) + .setClock(new AutoAdvancingFakeClock()) + .build(); + AnalyticsListener listener = mock(AnalyticsListener.class); + player.addAnalyticsListener(listener); + // Do something that requires clean-up callbacks like decoder disabling. + player.setMediaSource( + new FakeMediaSource(new FakeTimeline(), ExoPlayerTestRunner.VIDEO_FORMAT)); + player.prepare(); + player.play(); + runUntilPlaybackState(player, Player.STATE_READY); + + player.release(); + ShadowLooper.runMainLooperToNextTask(); + + verify(listener).onVideoDisabled(any(), any()); + verify(listener).onPlayerReleased(any()); + } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java b/library/core/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java index 19ffd248992..6526b4db22e 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java @@ -22,6 +22,7 @@ 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; @@ -366,6 +367,34 @@ public void release_preventsRegisteringNewListeners() { verify(listener, never()).callback1(); } + @Test + public void lazyRelease_stopsForwardingEventsFromNewHandlerMessagesAndCallsReleaseCallback() { + ListenerSet listenerSet = + new ListenerSet<>(Looper.myLooper(), Flags::new, 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).iterationFinished(Flags.create(EVENT_ID_1)); + verify(listener).iterationFinished(Flags.create(EVENT_ID_1, EVENT_ID_3)); + verifyNoMoreInteractions(listener); + } + private interface TestListener { default void callback1() {}