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 d102d2c637..c4c50eaa03 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 class SimpleExoPlayer extends BasePlayer 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 e8ab433d55..f09e5db812 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 class AnalyticsCollector }); } + /** + * 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 6ac6ae5847..6589b98351 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 interface AnalyticsListener { 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 interface AnalyticsListener { 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 @@ public interface AnalyticsListener { */ 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 24bf624338..d0df7a662e 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 final class ListenerSet { } 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 @@ public final class 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 final class ListenerSet { /** * 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 final class ListenerSet { 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 final class ListenerSet { * 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 final class ListenerSet { } /** - * 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 final class ListenerSet { 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 e3a625a3ce..a11961c301 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 class SimpleExoPlayerTest { 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 19ffd24899..6526b4db22 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.times; 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 class ListenerSetTest { 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() {}