From 29af6899feebdb4e348cd50ea5eb595af059771a Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 23 Aug 2019 10:18:26 +0100 Subject: [PATCH] Move playback error into PlaybackInfo. The error is closely related to the playback state IDLE and should be updated in sync with the state to prevent unexpected event ordering and/or keeping the error after re-preparation. Issue:#5407 PiperOrigin-RevId: 265014630 --- .../android/exoplayer2/ExoPlayerImpl.java | 33 ++++++++++------- .../exoplayer2/ExoPlayerImplInternal.java | 29 +++++++++------ .../android/exoplayer2/PlaybackInfo.java | 36 +++++++++++++++++++ .../analytics/AnalyticsCollector.java | 5 +-- .../exoplayer2/MediaPeriodQueueTest.java | 1 + 5 files changed, 77 insertions(+), 27 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java index cacdaec02ed..38d66e5cbc3 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java @@ -74,7 +74,6 @@ private int pendingSetPlaybackParametersAcks; private PlaybackParameters playbackParameters; private SeekParameters seekParameters; - @Nullable private ExoPlaybackException playbackError; // Playback information when there is no pending seek/set source operation. private PlaybackInfo playbackInfo; @@ -202,13 +201,12 @@ public int getPlaybackState() { @Override @Nullable public ExoPlaybackException getPlaybackError() { - return playbackError; + return playbackInfo.playbackError; } @Override public void retry() { - if (mediaSource != null - && (playbackError != null || playbackInfo.playbackState == Player.STATE_IDLE)) { + if (mediaSource != null && playbackInfo.playbackState == Player.STATE_IDLE) { prepare(mediaSource, /* resetPosition= */ false, /* resetState= */ false); } } @@ -220,11 +218,13 @@ public void prepare(MediaSource mediaSource) { @Override public void prepare(MediaSource mediaSource, boolean resetPosition, boolean resetState) { - playbackError = null; this.mediaSource = mediaSource; PlaybackInfo playbackInfo = getResetPlaybackInfo( - resetPosition, resetState, /* playbackState= */ Player.STATE_BUFFERING); + resetPosition, + resetState, + /* resetError= */ true, + /* playbackState= */ Player.STATE_BUFFERING); // Trigger internal prepare first before updating the playback info and notifying external // listeners to ensure that new operations issued in the listener notifications reach the // player after this prepare. The internal player can't change the playback info immediately @@ -381,13 +381,13 @@ public void setForegroundMode(boolean foregroundMode) { @Override public void stop(boolean reset) { if (reset) { - playbackError = null; mediaSource = null; } PlaybackInfo playbackInfo = getResetPlaybackInfo( /* resetPosition= */ reset, /* resetState= */ reset, + /* resetError= */ reset, /* playbackState= */ Player.STATE_IDLE); // Trigger internal stop first before updating the playback info and notifying external // listeners to ensure that new operations issued in the listener notifications reach the @@ -415,6 +415,7 @@ public void release() { getResetPlaybackInfo( /* resetPosition= */ false, /* resetState= */ false, + /* resetError= */ false, /* playbackState= */ Player.STATE_IDLE); } @@ -572,11 +573,6 @@ public Timeline getCurrentTimeline() { case ExoPlayerImplInternal.MSG_PLAYBACK_PARAMETERS_CHANGED: handlePlaybackParameters((PlaybackParameters) msg.obj, /* operationAck= */ msg.arg1 != 0); break; - case ExoPlayerImplInternal.MSG_ERROR: - ExoPlaybackException playbackError = (ExoPlaybackException) msg.obj; - this.playbackError = playbackError; - notifyListeners(listener -> listener.onPlayerError(playbackError)); - break; default: throw new IllegalStateException(); } @@ -635,7 +631,10 @@ private void handlePlaybackInfo( } private PlaybackInfo getResetPlaybackInfo( - boolean resetPosition, boolean resetState, @Player.State int playbackState) { + boolean resetPosition, + boolean resetState, + boolean resetError, + @Player.State int playbackState) { if (resetPosition) { maskingWindowIndex = 0; maskingPeriodIndex = 0; @@ -659,6 +658,7 @@ private PlaybackInfo getResetPlaybackInfo( startPositionUs, contentPositionUs, playbackState, + resetError ? null : playbackInfo.playbackError, /* isLoading= */ false, resetState ? TrackGroupArray.EMPTY : playbackInfo.trackGroups, resetState ? emptyTrackSelectorResult : playbackInfo.trackSelectorResult, @@ -728,6 +728,7 @@ private static final class PlaybackInfoUpdate implements Runnable { private final @Player.TimelineChangeReason int timelineChangeReason; private final boolean seekProcessed; private final boolean playbackStateChanged; + private final boolean playbackErrorChanged; private final boolean timelineChanged; private final boolean isLoadingChanged; private final boolean trackSelectorResultChanged; @@ -752,6 +753,9 @@ public PlaybackInfoUpdate( this.seekProcessed = seekProcessed; this.playWhenReady = playWhenReady; playbackStateChanged = previousPlaybackInfo.playbackState != playbackInfo.playbackState; + playbackErrorChanged = + previousPlaybackInfo.playbackError != playbackInfo.playbackError + && playbackInfo.playbackError != null; timelineChanged = previousPlaybackInfo.timeline != playbackInfo.timeline; isLoadingChanged = previousPlaybackInfo.isLoading != playbackInfo.isLoading; trackSelectorResultChanged = @@ -770,6 +774,9 @@ public void run() { listenerSnapshot, listener -> listener.onPositionDiscontinuity(positionDiscontinuityReason)); } + if (playbackErrorChanged) { + invokeAll(listenerSnapshot, listener -> listener.onPlayerError(playbackInfo.playbackError)); + } if (trackSelectorResultChanged) { trackSelector.onSelectionActivated(playbackInfo.trackSelectorResult.info); invokeAll( diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java index 53c381961e8..a6e8c679a8f 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java @@ -61,7 +61,6 @@ // External messages public static final int MSG_PLAYBACK_INFO_CHANGED = 0; public static final int MSG_PLAYBACK_PARAMETERS_CHANGED = 1; - public static final int MSG_ERROR = 2; // Internal messages private static final int MSG_PREPARE = 0; @@ -374,19 +373,19 @@ public boolean handleMessage(Message msg) { maybeNotifyPlaybackInfoChanged(); } catch (ExoPlaybackException e) { Log.e(TAG, "Playback error.", e); - eventHandler.obtainMessage(MSG_ERROR, e).sendToTarget(); stopInternal( /* forceResetRenderers= */ true, /* resetPositionAndState= */ false, /* acknowledgeStop= */ false); + playbackInfo = playbackInfo.copyWithPlaybackError(e); maybeNotifyPlaybackInfoChanged(); } catch (IOException e) { Log.e(TAG, "Source error.", e); - eventHandler.obtainMessage(MSG_ERROR, ExoPlaybackException.createForSource(e)).sendToTarget(); stopInternal( /* forceResetRenderers= */ false, /* resetPositionAndState= */ false, /* acknowledgeStop= */ false); + playbackInfo = playbackInfo.copyWithPlaybackError(ExoPlaybackException.createForSource(e)); maybeNotifyPlaybackInfoChanged(); } catch (RuntimeException | OutOfMemoryError e) { Log.e(TAG, "Internal runtime error.", e); @@ -394,11 +393,11 @@ public boolean handleMessage(Message msg) { e instanceof OutOfMemoryError ? ExoPlaybackException.createForOutOfMemoryError((OutOfMemoryError) e) : ExoPlaybackException.createForUnexpected((RuntimeException) e); - eventHandler.obtainMessage(MSG_ERROR, error).sendToTarget(); stopInternal( /* forceResetRenderers= */ true, /* resetPositionAndState= */ false, /* acknowledgeStop= */ false); + playbackInfo = playbackInfo.copyWithPlaybackError(error); maybeNotifyPlaybackInfoChanged(); } return true; @@ -436,7 +435,11 @@ private void maybeNotifyPlaybackInfoChanged() { private void prepareInternal(MediaSource mediaSource, boolean resetPosition, boolean resetState) { pendingPrepareCount++; resetInternal( - /* resetRenderers= */ false, /* releaseMediaSource= */ true, resetPosition, resetState); + /* resetRenderers= */ false, + /* releaseMediaSource= */ true, + resetPosition, + resetState, + /* resetError= */ true); loadControl.onPrepared(); this.mediaSource = mediaSource; setState(Player.STATE_BUFFERING); @@ -688,7 +691,8 @@ private void seekToInternal(SeekPosition seekPosition) throws ExoPlaybackExcepti /* resetRenderers= */ false, /* releaseMediaSource= */ false, /* resetPosition= */ true, - /* resetState= */ false); + /* resetState= */ false, + /* resetError= */ true); } else { // Execute the seek in the current media periods. long newPeriodPositionUs = periodPositionUs; @@ -834,7 +838,8 @@ private void stopInternal( /* resetRenderers= */ forceResetRenderers || !foregroundMode, /* releaseMediaSource= */ true, /* resetPosition= */ resetPositionAndState, - /* resetState= */ resetPositionAndState); + /* resetState= */ resetPositionAndState, + /* resetError= */ resetPositionAndState); playbackInfoUpdate.incrementPendingOperationAcks( pendingPrepareCount + (acknowledgeStop ? 1 : 0)); pendingPrepareCount = 0; @@ -847,7 +852,8 @@ private void releaseInternal() { /* resetRenderers= */ true, /* releaseMediaSource= */ true, /* resetPosition= */ true, - /* resetState= */ true); + /* resetState= */ true, + /* resetError= */ false); loadControl.onReleased(); setState(Player.STATE_IDLE); internalPlaybackThread.quit(); @@ -861,7 +867,8 @@ private void resetInternal( boolean resetRenderers, boolean releaseMediaSource, boolean resetPosition, - boolean resetState) { + boolean resetState, + boolean resetError) { handler.removeMessages(MSG_DO_SOME_WORK); rebuffering = false; mediaClock.stop(); @@ -924,6 +931,7 @@ private void resetInternal( startPositionUs, contentPositionUs, playbackInfo.playbackState, + resetError ? null : playbackInfo.playbackError, /* isLoading= */ false, resetState ? TrackGroupArray.EMPTY : playbackInfo.trackGroups, resetState ? emptyTrackSelectorResult : playbackInfo.trackSelectorResult, @@ -1382,7 +1390,8 @@ private void handleSourceInfoRefreshEndedPlayback() { /* resetRenderers= */ false, /* releaseMediaSource= */ false, /* resetPosition= */ true, - /* resetState= */ false); + /* resetState= */ false, + /* resetError= */ true); } /** diff --git a/library/core/src/main/java/com/google/android/exoplayer2/PlaybackInfo.java b/library/core/src/main/java/com/google/android/exoplayer2/PlaybackInfo.java index e9b99acd778..9d2a3b54590 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/PlaybackInfo.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/PlaybackInfo.java @@ -16,6 +16,7 @@ package com.google.android.exoplayer2; import androidx.annotation.CheckResult; +import androidx.annotation.Nullable; import com.google.android.exoplayer2.source.MediaSource.MediaPeriodId; import com.google.android.exoplayer2.source.TrackGroupArray; import com.google.android.exoplayer2.trackselection.TrackSelectorResult; @@ -51,6 +52,8 @@ public final long contentPositionUs; /** The current playback state. One of the {@link Player}.STATE_ constants. */ @Player.State public final int playbackState; + /** The current playback error, or null if this is not an error state. */ + @Nullable public final ExoPlaybackException playbackError; /** Whether the player is currently loading. */ public final boolean isLoading; /** The currently available track groups. */ @@ -93,6 +96,7 @@ public static PlaybackInfo createDummy( startPositionUs, /* contentPositionUs= */ C.TIME_UNSET, Player.STATE_IDLE, + /* playbackError= */ null, /* isLoading= */ false, TrackGroupArray.EMPTY, emptyTrackSelectorResult, @@ -124,6 +128,7 @@ public PlaybackInfo( long startPositionUs, long contentPositionUs, @Player.State int playbackState, + @Nullable ExoPlaybackException playbackError, boolean isLoading, TrackGroupArray trackGroups, TrackSelectorResult trackSelectorResult, @@ -136,6 +141,7 @@ public PlaybackInfo( this.startPositionUs = startPositionUs; this.contentPositionUs = contentPositionUs; this.playbackState = playbackState; + this.playbackError = playbackError; this.isLoading = isLoading; this.trackGroups = trackGroups; this.trackSelectorResult = trackSelectorResult; @@ -194,6 +200,7 @@ public PlaybackInfo copyWithNewPosition( positionUs, periodId.isAd() ? contentPositionUs : C.TIME_UNSET, playbackState, + playbackError, isLoading, trackGroups, trackSelectorResult, @@ -217,6 +224,7 @@ public PlaybackInfo copyWithTimeline(Timeline timeline) { startPositionUs, contentPositionUs, playbackState, + playbackError, isLoading, trackGroups, trackSelectorResult, @@ -240,6 +248,31 @@ public PlaybackInfo copyWithPlaybackState(int playbackState) { startPositionUs, contentPositionUs, playbackState, + playbackError, + isLoading, + trackGroups, + trackSelectorResult, + loadingMediaPeriodId, + bufferedPositionUs, + totalBufferedDurationUs, + positionUs); + } + + /** + * Copies playback info with a playback error. + * + * @param playbackError The error. See {@link #playbackError}. + * @return Copied playback info with the playback error. + */ + @CheckResult + public PlaybackInfo copyWithPlaybackError(@Nullable ExoPlaybackException playbackError) { + return new PlaybackInfo( + timeline, + periodId, + startPositionUs, + contentPositionUs, + playbackState, + playbackError, isLoading, trackGroups, trackSelectorResult, @@ -263,6 +296,7 @@ public PlaybackInfo copyWithIsLoading(boolean isLoading) { startPositionUs, contentPositionUs, playbackState, + playbackError, isLoading, trackGroups, trackSelectorResult, @@ -288,6 +322,7 @@ public PlaybackInfo copyWithTrackInfo( startPositionUs, contentPositionUs, playbackState, + playbackError, isLoading, trackGroups, trackSelectorResult, @@ -311,6 +346,7 @@ public PlaybackInfo copyWithLoadingMediaPeriodId(MediaPeriodId loadingMediaPerio startPositionUs, contentPositionUs, playbackState, + playbackError, isLoading, trackGroups, trackSelectorResult, 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 825424ae043..1ccd4ffc840 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 @@ -465,10 +465,7 @@ public final void onShuffleModeEnabledChanged(boolean shuffleModeEnabled) { @Override public final void onPlayerError(ExoPlaybackException error) { - EventTime eventTime = - error.type == ExoPlaybackException.TYPE_SOURCE - ? generateLoadingMediaPeriodEventTime() - : generatePlayingMediaPeriodEventTime(); + EventTime eventTime = generateLastReportedPlayingMediaPeriodEventTime(); for (AnalyticsListener listener : listeners) { listener.onPlayerError(eventTime, error); } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java b/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java index 3c6c4462ca7..afcce904e93 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java @@ -359,6 +359,7 @@ private void setupTimeline(long initialPositionUs, long... adGroupTimesUs) { /* startPositionUs= */ 0, /* contentPositionUs= */ 0, Player.STATE_READY, + /* playbackError= */ null, /* isLoading= */ false, /* trackGroups= */ null, /* trackSelectorResult= */ null,