diff --git a/library/core/src/androidTest/java/com/google/android/exoplayer2/analytics/AnalyticsCollectorInstrumentationTest.java b/library/core/src/androidTest/java/com/google/android/exoplayer2/analytics/AnalyticsCollectorInstrumentationTest.java deleted file mode 100644 index 94f7c26725..0000000000 --- a/library/core/src/androidTest/java/com/google/android/exoplayer2/analytics/AnalyticsCollectorInstrumentationTest.java +++ /dev/null @@ -1,189 +0,0 @@ -/* - * Copyright 2021 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package com.google.android.exoplayer2.analytics; - -import static com.google.common.truth.Truth.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.argThat; -import static org.mockito.Mockito.inOrder; -import static org.mockito.Mockito.same; -import static org.mockito.Mockito.spy; - -import android.app.Instrumentation; -import androidx.test.ext.junit.runners.AndroidJUnit4; -import androidx.test.platform.app.InstrumentationRegistry; -import com.google.android.exoplayer2.C; -import com.google.android.exoplayer2.ExoPlayer; -import com.google.android.exoplayer2.MediaItem; -import com.google.android.exoplayer2.PlaybackException; -import com.google.android.exoplayer2.Player; -import com.google.android.exoplayer2.Renderer; -import com.google.android.exoplayer2.SimpleExoPlayer; -import com.google.android.exoplayer2.util.Clock; -import com.google.android.exoplayer2.util.ConditionVariable; -import java.util.concurrent.atomic.AtomicLong; -import java.util.concurrent.atomic.AtomicReference; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.ArgumentCaptor; -import org.mockito.InOrder; - -/** Instrumentation tests for the {@link AnalyticsCollector} */ -@RunWith(AndroidJUnit4.class) -public class AnalyticsCollectorInstrumentationTest { - - /** - * This is a regression test against [internal ref: b/195396384]. The root cause of the regression - * was an introduction of an additional {@link AnalyticsListener#onEvents} callback inside {@link - * AnalyticsCollector} for callbacks that are queued during {@link Player#release}. The additional - * callback was called before {@link AnalyticsListener#onPlayerReleased} but its associated event - * real-time timestamps were greater than the real-time timestamp of {@link - * AnalyticsListener#onPlayerReleased}. As a result, the {@link AnalyticsListener#onEvents} that - * contained {@link AnalyticsListener#EVENT_PLAYER_RELEASED} had a real-time timestamp that was - * smaller than the timestamps of the previously forwarded events. That broke the {@link - * PlaybackStatsListener} which assumed that real-time event timestamps are always increasing. - * - *

The regression was easily reproduced in the demo app with a {@link PlaybackStatsListener} - * attached to the player and pressing back while the app was playing a video. That would make the - * app call {@link Player#release} while the player was playing, and it would throw an exception - * from the {@link PlaybackStatsListener}. - * - *

This test starts playback of an item and calls {@link Player#release} while the player is - * playing. The test uses a custom {@link Renderer} that queues a callback to be handled after - * {@link Player#release} completes. The test asserts that {@link - * AnalyticsListener#onPlayerReleased} is called after any callbacks queued during {@link - * Player#release} and its real-time timestamp is greater that the timestamps of the other - * callbacks. - */ - @Test - public void releasePlayer_whilePlaying_onPlayerReleasedIsForwardedLast() throws Exception { - AtomicLong playerReleaseTimeMs = new AtomicLong(C.TIME_UNSET); - AtomicReference playerError = new AtomicReference<>(); - AtomicReference player = new AtomicReference<>(); - ConditionVariable playerReleasedEventArrived = new ConditionVariable(); - AnalyticsListener analyticsListener = - spy(new TestAnalyticsListener(playerReleasedEventArrived)); - Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation(); - instrumentation.runOnMainSync( - () -> { - player.set(new ExoPlayer.Builder(instrumentation.getContext()).build()); - player - .get() - .addListener( - new Player.Listener() { - @Override - public void onPlayerError(PlaybackException error) { - playerError.set(error); - player.get().release(); - } - }); - player.get().addAnalyticsListener(analyticsListener); - player.get().setMediaItem(MediaItem.fromUri("asset:///media/mp4/preroll-5s.mp4")); - player.get().prepare(); - player.get().play(); - }); - waitUntilPosition(player.get(), /* positionMs= */ 1000); - instrumentation.runOnMainSync( - () -> { - player.get().release(); - playerReleaseTimeMs.set(Clock.DEFAULT.elapsedRealtime()); - }); - playerReleasedEventArrived.block(); - - assertThat(playerError.get()).isNull(); - assertThat(playerReleaseTimeMs).isNotEqualTo(C.TIME_UNSET); - InOrder inOrder = inOrder(analyticsListener); - ArgumentCaptor onVideoDecoderReleasedArgumentCaptor = - ArgumentCaptor.forClass(AnalyticsListener.EventTime.class); - inOrder - .verify(analyticsListener) - .onVideoDecoderReleased(onVideoDecoderReleasedArgumentCaptor.capture(), any()); - assertThat(onVideoDecoderReleasedArgumentCaptor.getValue().realtimeMs) - .isGreaterThan(playerReleaseTimeMs.get()); - inOrder - .verify(analyticsListener) - .onEvents( - same(player.get()), - argThat(events -> events.contains(AnalyticsListener.EVENT_VIDEO_DECODER_RELEASED))); - ArgumentCaptor onPlayerReleasedArgumentCaptor = - ArgumentCaptor.forClass(AnalyticsListener.EventTime.class); - inOrder.verify(analyticsListener).onPlayerReleased(onPlayerReleasedArgumentCaptor.capture()); - assertThat(onPlayerReleasedArgumentCaptor.getAllValues()).hasSize(1); - assertThat(onPlayerReleasedArgumentCaptor.getValue().realtimeMs) - .isGreaterThan(onVideoDecoderReleasedArgumentCaptor.getValue().realtimeMs); - inOrder - .verify(analyticsListener) - .onEvents( - same(player.get()), - argThat(events -> events.contains(AnalyticsListener.EVENT_PLAYER_RELEASED))); - } - - private static void waitUntilPosition(SimpleExoPlayer simpleExoPlayer, long positionMs) - throws Exception { - ConditionVariable conditionVariable = new ConditionVariable(); - InstrumentationRegistry.getInstrumentation() - .runOnMainSync( - () -> { - simpleExoPlayer.addListener( - new Player.Listener() { - @Override - public void onPlayerError(PlaybackException error) { - conditionVariable.open(); - } - }); - simpleExoPlayer - .createMessage((messageType, payload) -> conditionVariable.open()) - .setPosition(positionMs) - .send(); - }); - conditionVariable.block(); - } - - /** - * An {@link AnalyticsListener} that blocks the thread on {@link - * AnalyticsListener#onVideoDecoderReleased} for at least {@code 1ms} and unblocks a {@link - * ConditionVariable} when an {@link AnalyticsListener#EVENT_PLAYER_RELEASED} arrives. The class - * is public and non-final so we can use it with {@link org.mockito.Mockito#spy}. - */ - public static class TestAnalyticsListener implements AnalyticsListener { - private final ConditionVariable playerReleasedEventArrived; - - public TestAnalyticsListener(ConditionVariable playerReleasedEventArrived) { - this.playerReleasedEventArrived = playerReleasedEventArrived; - } - - @Override - public void onVideoDecoderReleased(EventTime eventTime, String decoderName) { - // Sleep for 1 ms so that the elapsedRealtime when the subsequent events - // are greater by at least 1 ms. - long startTimeMs = Clock.DEFAULT.elapsedRealtime(); - try { - while (startTimeMs + 1 > Clock.DEFAULT.elapsedRealtime()) { - Thread.sleep(1); - } - } catch (InterruptedException e) { - throw new IllegalStateException(e); - } - } - - @Override - public void onEvents(Player player, Events events) { - if (events.contains(AnalyticsListener.EVENT_PLAYER_RELEASED)) { - playerReleasedEventArrived.open(); - } - } - } -} diff --git a/library/core/src/test/java/com/google/android/exoplayer2/analytics/AnalyticsCollectorTest.java b/library/core/src/test/java/com/google/android/exoplayer2/analytics/AnalyticsCollectorTest.java index 654e010eae..0753464f04 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/analytics/AnalyticsCollectorTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/analytics/AnalyticsCollectorTest.java @@ -1985,37 +1985,59 @@ public final class AnalyticsCollectorTest { } @Test - public void release_withCallbacksArrivingAfterRelease_onPlayerReleasedForwardedLast() { - FakeClock fakeClock = new FakeClock(/* initialTimeMs= */ 0, /* isAutoAdvancing= */ false); - AnalyticsCollector analyticsCollector = new AnalyticsCollector(fakeClock); + public void release_withCallbacksArrivingAfterRelease_onPlayerReleasedForwardedLast() + throws Exception { + FakeClock fakeClock = new FakeClock(/* initialTimeMs= */ 0, /* isAutoAdvancing= */ true); SimpleExoPlayer simpleExoPlayer = - new ExoPlayer.Builder(ApplicationProvider.getApplicationContext()).buildExoPlayer(); - analyticsCollector.setPlayer(simpleExoPlayer, Looper.myLooper()); - AnalyticsListener analyticsListener = mock(AnalyticsListener.class); - analyticsCollector.addListener(analyticsListener); + new TestExoPlayerBuilder(ApplicationProvider.getApplicationContext()) + .setClock(fakeClock) + .build(); + AnalyticsListener analyticsListener = + spy( + new AnalyticsListener() { + @Override + public void onVideoDisabled(EventTime eventTime, DecoderCounters decoderCounters) { + // Add delay in callback to test whether event timestamp and release timestamp are + // in the correct order. + fakeClock.advanceTime(1); + } + }); + simpleExoPlayer.addAnalyticsListener(analyticsListener); - // Simulate Player.release(): events arrive to the analytics collector after it's been released. - analyticsCollector.release(); - fakeClock.advanceTime(/* timeDiffMs= */ 1); - analyticsCollector.onDroppedFrames(/* count= */ 1, /* elapsedMs= */ 1); - fakeClock.advanceTime(/* timeDiffMs= */ 1); + // Prepare with media to ensure video renderer is enabled. + simpleExoPlayer.setMediaSource( + new FakeMediaSource(new FakeTimeline(), ExoPlayerTestRunner.VIDEO_FORMAT)); + simpleExoPlayer.prepare(); + TestPlayerRunHelper.runUntilPlaybackState(simpleExoPlayer, Player.STATE_READY); + + // Release and add delay on releasing thread to verify timestamps of events. + simpleExoPlayer.release(); + long releaseTimeMs = fakeClock.currentTimeMillis(); + fakeClock.advanceTime(1); ShadowLooper.idleMainLooper(); + // Verify video disable events and release events arrived in order. + ArgumentCaptor videoDisabledEventTime = + ArgumentCaptor.forClass(AnalyticsListener.EventTime.class); + ArgumentCaptor releasedEventTime = + ArgumentCaptor.forClass(AnalyticsListener.EventTime.class); InOrder inOrder = inOrder(analyticsListener); + inOrder.verify(analyticsListener).onVideoDisabled(videoDisabledEventTime.capture(), any()); inOrder .verify(analyticsListener) - .onDroppedVideoFrames(argThat(eventTime -> eventTime.realtimeMs == 1), eq(1), eq(1L)); - inOrder - .verify(analyticsListener) - .onEvents( - same(simpleExoPlayer), argThat(events -> events.contains(EVENT_DROPPED_VIDEO_FRAMES))); - inOrder - .verify(analyticsListener) - .onPlayerReleased(argThat(eventTime -> eventTime.realtimeMs == 2)); + .onEvents(same(simpleExoPlayer), argThat(events -> events.contains(EVENT_VIDEO_DISABLED))); + inOrder.verify(analyticsListener).onPlayerReleased(releasedEventTime.capture()); inOrder .verify(analyticsListener) .onEvents(same(simpleExoPlayer), argThat(events -> events.contains(EVENT_PLAYER_RELEASED))); - inOrder.verifyNoMoreInteractions(); + + // Verify order of timestamps of these events. + // This verification is needed as a regression test against [internal ref: b/195396384]. The + // root cause of the regression was an onPlayerReleased timestamp that was less than the + // previously reported timestamps for other events triggered as part of the release. + long videoDisableTimeMs = videoDisabledEventTime.getValue().realtimeMs; + assertThat(videoDisableTimeMs).isGreaterThan(releaseTimeMs); + assertThat(releasedEventTime.getValue().realtimeMs).isGreaterThan(videoDisableTimeMs); } private static TestAnalyticsListener runAnalyticsTest(MediaSource mediaSource) throws Exception {