From 727645179b3c26e495540f4445216fd035cc7654 Mon Sep 17 00:00:00 2001 From: tonihei Date: Wed, 26 Jun 2024 09:06:20 -0700 Subject: [PATCH] Send pending updates before adding discontinuity for error When handling a playback error that originates from a future item in the playlist, we added support for jumping to that item first, ensuring the errors 'happen' for the right 'current item'. See https://github.com/androidx/media/commit/79b688ef30d4a3306d4d321ddf4464b428074ea2. However, when we add this new position discontinuity to the playback state, there may already be other position discontinuities pending from other parts of the code that executed before the error. As we can't control that in this case (because it's part of a generic try/catch block), we need to send any pending updates first before handling the new change. Issue: androidx/media#1483 #cherrypick PiperOrigin-RevId: 646968309 --- RELEASENOTES.md | 3 + .../exoplayer/ExoPlayerImplInternal.java | 2 + .../media3/exoplayer/ExoPlayerTest.java | 88 +++++++++++++++++++ 3 files changed, 93 insertions(+) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 87dee0fd54..cbdbb5b83b 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -15,6 +15,9 @@ * `Effects` in `ExoPlayer.setVideoEffect()` will receive the timestamps with the renderer offset removed ([#1098](https://github.com/androidx/media/issues/1098)). + * Fix potential `IllegalArgumentException` when handling player error that + happened while reading ahead into another playlist item + ([#1483](https://github.com/androidx/media/issues/1483)). * Transformer: * Track Selection: * Extractors: diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java index 0b2bc58010..7ea8f7372e 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java @@ -672,6 +672,8 @@ import java.util.concurrent.atomic.AtomicBoolean; queue.advancePlayingPeriod(); } MediaPeriodHolder newPlayingPeriodHolder = checkNotNull(queue.getPlayingPeriod()); + // Send already pending updates if needed before making further changes to PlaybackInfo. + maybeNotifyPlaybackInfoChanged(); playbackInfo = handlePositionDiscontinuity( newPlayingPeriodHolder.info.id, diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java index 9a3384823c..ed2ea706c2 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java @@ -14052,6 +14052,94 @@ public class ExoPlayerTest { eventsInOrder.verify(mockListener).onPlayerError(any(), any()); } + @Test + public void + rendererError_whileReadingAheadAndOverlappingWithOtherDiscontinuity_isReportedAfterMediaItemTransition() + throws Exception { + // Throw an exception as soon as we try to process a buffer for the second item. This happens + // while the player is still playing the first item. At the same time, also constantly report + // silence skip to force a situation where we have already a pending discontinuity at the point + // of handling the error. + AtomicInteger silenceSkipCount = new AtomicInteger(0); + FakeMediaClockRenderer fakeRenderer = + new FakeMediaClockRenderer(C.TRACK_TYPE_AUDIO) { + private long positionUs; + int streamChangeCount = 0; + + @Override + protected void onStreamChanged( + Format[] formats, long startPositionUs, long offsetUs, MediaPeriodId mediaPeriodId) { + positionUs = startPositionUs; + streamChangeCount++; + } + + @Override + public long getPositionUs() { + // Advance position to make playback progress. + positionUs += 10_000; + return positionUs; + } + + @Override + public boolean hasSkippedSilenceSinceLastCall() { + // Continuously report skipped silences to ensure they will overlap with other + // discontinuities like the AUTO_TRANSITION triggered after the error. + silenceSkipCount.incrementAndGet(); + return true; + } + + @Override + protected boolean shouldProcessBuffer(long bufferTimeUs, long playbackPositionUs) { + boolean shouldProcess = super.shouldProcessBuffer(bufferTimeUs, playbackPositionUs); + if (streamChangeCount == 2 && shouldProcess) { + Util.sneakyThrow( + createRendererException( + new IllegalStateException(), + /* format= */ null, + PlaybackException.ERROR_CODE_DECODING_FAILED)); + } + return shouldProcess; + } + + @Override + public void setPlaybackParameters(PlaybackParameters playbackParameters) {} + + @Override + public PlaybackParameters getPlaybackParameters() { + return PlaybackParameters.DEFAULT; + } + }; + TestExoPlayerBuilder playerBuilder = + new TestExoPlayerBuilder(context).setRenderers(fakeRenderer); + ExoPlayer player = parameterizeTestExoPlayerBuilder(playerBuilder).build(); + Player.Listener mockListener = mock(Player.Listener.class); + player.addListener(mockListener); + + player.setMediaSources( + ImmutableList.of( + new FakeMediaSource(new FakeTimeline(), ExoPlayerTestRunner.AUDIO_FORMAT), + new FakeMediaSource(new FakeTimeline(), ExoPlayerTestRunner.AUDIO_FORMAT))); + player.prepare(); + player.play(); + runUntilError(player); + int mediaItemIndexAfterError = player.getCurrentMediaItemIndex(); + player.release(); + + assertThat(mediaItemIndexAfterError).isEqualTo(1); + InOrder eventsInOrder = inOrder(mockListener); + // Verify that all expected discontinuities and the error arrived in order. + eventsInOrder + .verify(mockListener, times(silenceSkipCount.get())) + .onPositionDiscontinuity(any(), any(), eq(Player.DISCONTINUITY_REASON_SILENCE_SKIP)); + eventsInOrder + .verify(mockListener) + .onPositionDiscontinuity(any(), any(), eq(Player.DISCONTINUITY_REASON_AUTO_TRANSITION)); + eventsInOrder + .verify(mockListener) + .onMediaItemTransition(any(), eq(Player.MEDIA_ITEM_TRANSITION_REASON_AUTO)); + eventsInOrder.verify(mockListener).onPlayerError(any()); + } + @Test public void play_withReadingAheadWithNewRenderer_enablesButNotStartsNewRenderer() throws Exception {