From 71cb2469132d81ede53e554f1e6be0d0ca6eaa0e Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 10 Jan 2025 05:57:09 -0800 Subject: [PATCH] Fix bug where changing index does not reevaluate derived metadata When the current MediaMetadata is automatically derived from the MediaItem and Tracks, the result may change when state.buildUpon() is used and new input data is provided (e.g. new current index). To avoid reusing the previously derived MediaMetadata, we need to reset it to null and re-evaluate it on every call to build() Issue: androidx/media#1940 PiperOrigin-RevId: 714021424 --- RELEASENOTES.md | 4 ++ .../media3/common/SimpleBasePlayer.java | 7 +- .../media3/common/SimpleBasePlayerTest.java | 68 +++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 30804ef597..4b7e3bbda7 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -3,6 +3,10 @@ ### Unreleased changes * Common Library: + * Fix bug in `SimpleBasePlayer` where setting a new + `currentMediaItemIndex` in `State` after `setPlaylist` with `null` + `MediaMetadata` does not reevaluate the metadata + ([#1940](https://github.com/androidx/media/issues/1940)). * ExoPlayer: * Add experimental 'ExoPlayer' pre-warming support for playback using `MediaCodecVideoRenderer`. `DefaultRenderersFactory` can be configured diff --git a/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java b/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java index 8a486c4637..15388a38b4 100644 --- a/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java +++ b/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java @@ -220,7 +220,7 @@ public abstract class SimpleBasePlayer extends BasePlayer { this.playlist = ((PlaylistTimeline) state.timeline).playlist; } else { this.currentTracks = state.currentTracks; - this.currentMetadata = state.currentMetadata; + this.currentMetadata = state.usesDerivedMediaMetadata ? null : state.currentMetadata; } this.playlistMetadata = state.playlistMetadata; this.currentMediaItemIndex = state.currentMediaItemIndex; @@ -956,9 +956,12 @@ public abstract class SimpleBasePlayer extends BasePlayer { */ public final long discontinuityPositionMs; + private final boolean usesDerivedMediaMetadata; + private State(Builder builder) { Tracks currentTracks = builder.currentTracks; MediaMetadata currentMetadata = builder.currentMetadata; + boolean usesDerivedMediaMetadata = false; if (builder.timeline.isEmpty()) { checkArgument( builder.playbackState == Player.STATE_IDLE @@ -1014,6 +1017,7 @@ public abstract class SimpleBasePlayer extends BasePlayer { getCombinedMediaMetadata( builder.timeline.getWindow(mediaItemIndex, new Timeline.Window()).mediaItem, checkNotNull(currentTracks)); + usesDerivedMediaMetadata = true; } } if (builder.playerError != null) { @@ -1090,6 +1094,7 @@ public abstract class SimpleBasePlayer extends BasePlayer { this.hasPositionDiscontinuity = builder.hasPositionDiscontinuity; this.positionDiscontinuityReason = builder.positionDiscontinuityReason; this.discontinuityPositionMs = builder.discontinuityPositionMs; + this.usesDerivedMediaMetadata = usesDerivedMediaMetadata; } /** Returns a {@link Builder} pre-populated with the current state values. */ diff --git a/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java b/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java index aa37200c7d..6abc494983 100644 --- a/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java +++ b/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java @@ -148,6 +148,74 @@ public class SimpleBasePlayerTest { assertThat(newState.hashCode()).isEqualTo(state.hashCode()); } + @Test + public void stateBuildUpon_withExplicitTimeline_isEqual() { + MediaMetadata mediaMetadata = new MediaMetadata.Builder().setTitle("title").build(); + Tracks tracks = + new Tracks( + ImmutableList.of( + new Tracks.Group( + new TrackGroup(new Format.Builder().build()), + /* adaptiveSupported= */ true, + /* trackSupport= */ new int[] {C.FORMAT_HANDLED}, + /* trackSelected= */ new boolean[] {true}))); + State state = + new State.Builder() + .setPlaylist(new FakeTimeline(/* windowCount= */ 3), tracks, mediaMetadata) + .build(); + + State newState = state.buildUpon().build(); + + assertThat(newState).isEqualTo(state); + assertThat(newState.hashCode()).isEqualTo(state.hashCode()); + } + + @Test + public void stateBuildUpon_withExplicitTimelineAndNewCurrentIndex_reevalutesMediaMetadata() { + Timeline timeline = + new FakeTimeline( + new FakeTimeline.TimelineWindowDefinition( + /* periodCount= */ 1, + /* id= */ 0, + /* isSeekable= */ true, + /* isDynamic= */ true, + /* isLive= */ true, + /* isPlaceholder= */ false, + /* durationUs= */ 1000, + /* defaultPositionUs= */ 0, + /* windowOffsetInFirstPeriodUs= */ 0, + ImmutableList.of(AdPlaybackState.NONE), + new MediaItem.Builder() + .setMediaId("1") + .setMediaMetadata(new MediaMetadata.Builder().setArtist("artist1").build()) + .build()), + new FakeTimeline.TimelineWindowDefinition( + /* periodCount= */ 1, + /* id= */ 1, + /* isSeekable= */ true, + /* isDynamic= */ true, + /* isLive= */ true, + /* isPlaceholder= */ false, + /* durationUs= */ 1000, + /* defaultPositionUs= */ 0, + /* windowOffsetInFirstPeriodUs= */ 0, + ImmutableList.of(AdPlaybackState.NONE), + new MediaItem.Builder() + .setMediaId("2") + .setMediaMetadata(new MediaMetadata.Builder().setArtist("artist2").build()) + .build())); + State state = + new State.Builder() + .setPlaylist(timeline, Tracks.EMPTY, /* currentMetadata= */ null) + .setCurrentMediaItemIndex(0) + .build(); + + State newState = state.buildUpon().setCurrentMediaItemIndex(1).build(); + + assertThat(newState.currentMetadata) + .isEqualTo(new MediaMetadata.Builder().setArtist("artist2").build()); + } + @Test public void mediaItemDataBuildUpon_build_isEqual() { SimpleBasePlayer.MediaItemData mediaItemData =