From 1c557e2fd18c77243474af2f8c99fa6ea061b38a Mon Sep 17 00:00:00 2001 From: tonihei Date: Thu, 13 Apr 2023 14:42:24 +0100 Subject: [PATCH] Ensure TrackSelectionParameters overrides match existing groups The overrides specified by a MediaController may not use the exact same TrackGroup instances as known to the Player because the groups have been bundled to and from the controller. This bundling may alter the instance slightly depending on the version used on each side of the communication and the fields set (e.g. Format.metadata is not supported for bundling). This issue can be solved by creating unique track group ids for each group on the session side before bundling. On the way back, the groups in the track selection parameters can be mapped backed to their original instances based on this id. #minor-release Issue: androidx/media#296 PiperOrigin-RevId: 523986626 --- RELEASENOTES.md | 4 + .../java/androidx/media3/common/Tracks.java | 12 ++ .../media3/session/MediaSessionImpl.java | 2 +- .../media3/session/MediaSessionStub.java | 74 ++++++++++- .../session/MediaControllerListenerTest.java | 9 +- .../media3/session/MediaControllerTest.java | 116 +++++++++++++++++- .../MediaSessionAndControllerTest.java | 62 ++++++++++ 7 files changed, 272 insertions(+), 7 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index fbc1e5e6f2..631cf4cdde 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -48,6 +48,10 @@ * `void increaseDeviceVolume(int)` * `void decreaseDeviceVolume(int)` * `void setDeviceMuted(boolean, int)` + * Fix issue that `TrackSelectionOverride` instances sent from a + `MediaController` are ignored if they reference a group with + `Format.metadata` + ([#296](https://github.com/androidx/media/issues/296)). * Audio: * Fix bug where some playbacks fail when tunneling is enabled and `AudioProcessors` are active, e.g. for gapless trimming diff --git a/libraries/common/src/main/java/androidx/media3/common/Tracks.java b/libraries/common/src/main/java/androidx/media3/common/Tracks.java index 734a8306b8..a932ac6924 100644 --- a/libraries/common/src/main/java/androidx/media3/common/Tracks.java +++ b/libraries/common/src/main/java/androidx/media3/common/Tracks.java @@ -191,6 +191,18 @@ public final class Tracks implements Bundleable { return mediaTrackGroup.type; } + /** + * Copies the {@code Group} with a new {@link TrackGroup#id}. + * + * @param groupId The new {@link TrackGroup#id} + * @return The copied {@code Group}. + */ + @UnstableApi + public Group copyWithId(String groupId) { + return new Group( + mediaTrackGroup.copyWithId(groupId), adaptiveSupported, trackSupport, trackSelected); + } + @Override public boolean equals(@Nullable Object other) { if (this == other) { diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java index f37eb13947..3fac801364 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java @@ -352,7 +352,7 @@ import org.checkerframework.checker.initialization.qual.Initialized; private void dispatchOnPlayerInfoChanged( PlayerInfo playerInfo, boolean excludeTimeline, boolean excludeTracks) { - + playerInfo = sessionStub.generateAndCacheUniqueTrackGroupIds(playerInfo); List controllers = sessionStub.getConnectedControllersManager().getConnectedControllers(); for (int i = 0; i < controllers.size(); i++) { diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java index c9ec362d0f..ee33cffe39 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java @@ -72,7 +72,10 @@ import androidx.media3.common.MediaMetadata; import androidx.media3.common.PlaybackParameters; import androidx.media3.common.Player; import androidx.media3.common.Rating; +import androidx.media3.common.TrackGroup; +import androidx.media3.common.TrackSelectionOverride; import androidx.media3.common.TrackSelectionParameters; +import androidx.media3.common.Tracks; import androidx.media3.common.util.Assertions; import androidx.media3.common.util.BundleableUtil; import androidx.media3.common.util.Consumer; @@ -84,6 +87,7 @@ import androidx.media3.session.MediaSession.ControllerCb; import androidx.media3.session.MediaSession.ControllerInfo; import androidx.media3.session.MediaSession.MediaItemsWithStartPosition; import androidx.media3.session.SessionCommand.CommandCode; +import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -115,6 +119,9 @@ import java.util.concurrent.ExecutionException; private final ConnectedControllersManager connectedControllersManager; private final Set pendingControllers; + private ImmutableBiMap trackGroupIdMap; + private int nextUniqueTrackGroupIdPrefix; + public MediaSessionStub(MediaSessionImpl sessionImpl) { // Initialize members with params. this.sessionImpl = new WeakReference<>(sessionImpl); @@ -122,6 +129,7 @@ import java.util.concurrent.ExecutionException; connectedControllersManager = new ConnectedControllersManager<>(sessionImpl); // ConcurrentHashMap has a bug in APIs 21-22 that can result in lost updates. pendingControllers = Collections.synchronizedSet(new HashSet<>()); + trackGroupIdMap = ImmutableBiMap.of(); } public ConnectedControllersManager getConnectedControllersManager() { @@ -495,6 +503,7 @@ import java.util.concurrent.ExecutionException; // session/controller. PlayerWrapper playerWrapper = sessionImpl.getPlayerWrapper(); PlayerInfo playerInfo = playerWrapper.createPlayerInfoForBundling(); + playerInfo = generateAndCacheUniqueTrackGroupIds(playerInfo); ConnectionState state = new ConnectionState( MediaLibraryInfo.VERSION_INT, @@ -1493,7 +1502,11 @@ import java.util.concurrent.ExecutionException; sequenceNumber, COMMAND_SET_TRACK_SELECTION_PARAMETERS, sendSessionResultSuccess( - player -> player.setTrackSelectionParameters(trackSelectionParameters))); + player -> { + TrackSelectionParameters updatedParameters = + updateOverridesUsingUniqueTrackGroupIds(trackSelectionParameters); + player.setTrackSelectionParameters(updatedParameters); + })); } ////////////////////////////////////////////////////////////////////////////////////////////// @@ -1680,6 +1693,65 @@ import java.util.concurrent.ExecutionException; librarySessionImpl.onUnsubscribeOnHandler(controller, parentId))); } + /* package */ PlayerInfo generateAndCacheUniqueTrackGroupIds(PlayerInfo playerInfo) { + ImmutableList trackGroups = playerInfo.currentTracks.getGroups(); + ImmutableList.Builder updatedTrackGroups = ImmutableList.builder(); + ImmutableBiMap.Builder updatedTrackGroupIdMap = ImmutableBiMap.builder(); + for (int i = 0; i < trackGroups.size(); i++) { + Tracks.Group trackGroup = trackGroups.get(i); + TrackGroup mediaTrackGroup = trackGroup.getMediaTrackGroup(); + @Nullable String uniqueId = trackGroupIdMap.get(mediaTrackGroup); + if (uniqueId == null) { + uniqueId = generateUniqueTrackGroupId(mediaTrackGroup); + } + updatedTrackGroupIdMap.put(mediaTrackGroup, uniqueId); + updatedTrackGroups.add(trackGroup.copyWithId(uniqueId)); + } + trackGroupIdMap = updatedTrackGroupIdMap.buildOrThrow(); + playerInfo = playerInfo.copyWithCurrentTracks(new Tracks(updatedTrackGroups.build())); + if (playerInfo.trackSelectionParameters.overrides.isEmpty()) { + return playerInfo; + } + TrackSelectionParameters.Builder updatedTrackSelectionParameters = + playerInfo.trackSelectionParameters.buildUpon().clearOverrides(); + for (TrackSelectionOverride override : playerInfo.trackSelectionParameters.overrides.values()) { + TrackGroup trackGroup = override.mediaTrackGroup; + @Nullable String uniqueId = trackGroupIdMap.get(trackGroup); + if (uniqueId != null) { + updatedTrackSelectionParameters.addOverride( + new TrackSelectionOverride(trackGroup.copyWithId(uniqueId), override.trackIndices)); + } else { + updatedTrackSelectionParameters.addOverride(override); + } + } + return playerInfo.copyWithTrackSelectionParameters(updatedTrackSelectionParameters.build()); + } + + private TrackSelectionParameters updateOverridesUsingUniqueTrackGroupIds( + TrackSelectionParameters trackSelectionParameters) { + if (trackSelectionParameters.overrides.isEmpty()) { + return trackSelectionParameters; + } + TrackSelectionParameters.Builder updateTrackSelectionParameters = + trackSelectionParameters.buildUpon().clearOverrides(); + for (TrackSelectionOverride override : trackSelectionParameters.overrides.values()) { + TrackGroup trackGroup = override.mediaTrackGroup; + @Nullable TrackGroup originalTrackGroup = trackGroupIdMap.inverse().get(trackGroup.id); + if (originalTrackGroup != null + && override.mediaTrackGroup.length == originalTrackGroup.length) { + updateTrackSelectionParameters.addOverride( + new TrackSelectionOverride(originalTrackGroup, override.trackIndices)); + } else { + updateTrackSelectionParameters.addOverride(override); + } + } + return updateTrackSelectionParameters.build(); + } + + private String generateUniqueTrackGroupId(TrackGroup trackGroup) { + return Util.intToStringMaxRadix(nextUniqueTrackGroupIdPrefix++) + "-" + trackGroup.id; + } + /** Common interface for code snippets to handle all incoming commands from the controller. */ private interface SessionTask { T run(K sessionImpl, ControllerInfo controller, int sequenceNumber); diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java index b8d27ac58e..1eb583db5b 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java @@ -1095,15 +1095,16 @@ public class MediaControllerListenerTest { assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(initialCurrentTracksRef.get()).isEqualTo(Tracks.EMPTY); - assertThat(changedCurrentTracksFromParamRef.get()).isEqualTo(currentTracks); - assertThat(changedCurrentTracksFromGetterRef.get()).isEqualTo(currentTracks); + assertThat(changedCurrentTracksFromParamRef.get().getGroups()).hasSize(2); + assertThat(changedCurrentTracksFromGetterRef.get()) + .isEqualTo(changedCurrentTracksFromParamRef.get()); assertThat(capturedEvents).hasSize(2); assertThat(getEventsAsList(capturedEvents.get(0))).containsExactly(Player.EVENT_TRACKS_CHANGED); assertThat(getEventsAsList(capturedEvents.get(1))) .containsExactly(Player.EVENT_IS_LOADING_CHANGED); assertThat(changedCurrentTracksFromOnEvents).hasSize(2); - assertThat(changedCurrentTracksFromOnEvents.get(0)).isEqualTo(currentTracks); - assertThat(changedCurrentTracksFromOnEvents.get(1)).isEqualTo(currentTracks); + assertThat(changedCurrentTracksFromOnEvents.get(0).getGroups()).hasSize(2); + assertThat(changedCurrentTracksFromOnEvents.get(1).getGroups()).hasSize(2); // Assert that an equal instance is not re-sent over the binder. assertThat(changedCurrentTracksFromOnEvents.get(0)) .isSameInstanceAs(changedCurrentTracksFromOnEvents.get(1)); diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java index 0bf910b686..4a4c01db4f 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java @@ -42,6 +42,7 @@ import androidx.media3.common.IllegalSeekPositionException; import androidx.media3.common.MediaItem; import androidx.media3.common.MediaLibraryInfo; import androidx.media3.common.MediaMetadata; +import androidx.media3.common.Metadata; import androidx.media3.common.PlaybackException; import androidx.media3.common.PlaybackParameters; import androidx.media3.common.Player; @@ -50,6 +51,7 @@ import androidx.media3.common.Rating; import androidx.media3.common.StarRating; import androidx.media3.common.Timeline; import androidx.media3.common.TrackGroup; +import androidx.media3.common.TrackSelectionOverride; import androidx.media3.common.TrackSelectionParameters; import androidx.media3.common.Tracks; import androidx.media3.common.VideoSize; @@ -427,7 +429,7 @@ public class MediaControllerTest { assertThat(seekForwardIncrementRef.get()).isEqualTo(seekForwardIncrementMs); assertThat(maxSeekToPreviousPositionMsRef.get()).isEqualTo(maxSeekToPreviousPositionMs); assertThat(trackSelectionParametersRef.get()).isEqualTo(trackSelectionParameters); - assertThat(currentTracksRef.get()).isEqualTo(currentTracks); + assertThat(currentTracksRef.get().getGroups()).hasSize(2); assertTimelineMediaItemsEquals(timelineRef.get(), timeline); assertThat(currentMediaItemIndexRef.get()).isEqualTo(currentMediaItemIndex); assertThat(currentMediaItemRef.get()).isEqualTo(currentMediaItem); @@ -1211,6 +1213,118 @@ public class MediaControllerTest { assertThat(mediaMetadata).isEqualTo(testMediaMetadata); } + @Test + public void getCurrentTracks_hasEqualTrackGroupsForEqualGroupsInPlayer() throws Exception { + // Include metadata in Format to ensure the track group can't be fully bundled. + Tracks initialPlayerTracks = + new Tracks( + ImmutableList.of( + new Tracks.Group( + new TrackGroup( + new Format.Builder().setMetadata(new Metadata()).setId("1").build()), + /* adaptiveSupported= */ false, + /* trackSupport= */ new int[1], + /* trackSelected= */ new boolean[1]), + new Tracks.Group( + new TrackGroup( + new Format.Builder().setMetadata(new Metadata()).setId("2").build()), + /* adaptiveSupported= */ false, + /* trackSupport= */ new int[1], + /* trackSelected= */ new boolean[1]))); + Tracks updatedPlayerTracks = + new Tracks( + ImmutableList.of( + new Tracks.Group( + new TrackGroup( + new Format.Builder().setMetadata(new Metadata()).setId("2").build()), + /* adaptiveSupported= */ true, + /* trackSupport= */ new int[] {C.FORMAT_HANDLED}, + /* trackSelected= */ new boolean[] {true}), + new Tracks.Group( + new TrackGroup( + new Format.Builder().setMetadata(new Metadata()).setId("3").build()), + /* adaptiveSupported= */ false, + /* trackSupport= */ new int[1], + /* trackSelected= */ new boolean[1]))); + Bundle playerConfig = + new RemoteMediaSession.MockPlayerConfigBuilder() + .setCurrentTracks(initialPlayerTracks) + .build(); + remoteSession.setPlayer(playerConfig); + MediaController controller = controllerTestRule.createController(remoteSession.getToken()); + CountDownLatch trackChangedEvent = new CountDownLatch(1); + threadTestRule + .getHandler() + .postAndSync( + () -> + controller.addListener( + new Player.Listener() { + @Override + public void onTracksChanged(Tracks tracks) { + trackChangedEvent.countDown(); + } + })); + + Tracks initialControllerTracks = + threadTestRule.getHandler().postAndSync(controller::getCurrentTracks); + // Do something unrelated first to ensure tracks are correctly kept even after multiple updates. + remoteSession.getMockPlayer().notifyPlaybackStateChanged(Player.STATE_READY); + remoteSession.getMockPlayer().notifyTracksChanged(updatedPlayerTracks); + trackChangedEvent.await(); + Tracks updatedControllerTracks = + threadTestRule.getHandler().postAndSync(controller::getCurrentTracks); + + assertThat(initialControllerTracks.getGroups()).hasSize(2); + assertThat(updatedControllerTracks.getGroups()).hasSize(2); + assertThat(initialControllerTracks.getGroups().get(1).getMediaTrackGroup()) + .isEqualTo(updatedControllerTracks.getGroups().get(0).getMediaTrackGroup()); + } + + @Test + public void getCurrentTracksAndTrackOverrides_haveEqualTrackGroupsForEqualGroupsInPlayer() + throws Exception { + // Include metadata in Format to ensure the track group can't be fully bundled. + TrackGroup playerTrackGroupForOverride = + new TrackGroup(new Format.Builder().setMetadata(new Metadata()).setId("2").build()); + Tracks playerTracks = + new Tracks( + ImmutableList.of( + new Tracks.Group( + new TrackGroup( + new Format.Builder().setMetadata(new Metadata()).setId("1").build()), + /* adaptiveSupported= */ false, + /* trackSupport= */ new int[1], + /* trackSelected= */ new boolean[1]), + new Tracks.Group( + playerTrackGroupForOverride, + /* adaptiveSupported= */ false, + /* trackSupport= */ new int[1], + /* trackSelected= */ new boolean[1]))); + TrackSelectionParameters trackSelectionParameters = + TrackSelectionParameters.DEFAULT_WITHOUT_CONTEXT + .buildUpon() + .addOverride( + new TrackSelectionOverride(playerTrackGroupForOverride, /* trackIndex= */ 0)) + .build(); + Bundle playerConfig = + new RemoteMediaSession.MockPlayerConfigBuilder() + .setCurrentTracks(playerTracks) + .setTrackSelectionParameters(trackSelectionParameters) + .build(); + remoteSession.setPlayer(playerConfig); + MediaController controller = controllerTestRule.createController(remoteSession.getToken()); + + Tracks controllerTracks = threadTestRule.getHandler().postAndSync(controller::getCurrentTracks); + TrackSelectionParameters controllerTrackSelectionParameters = + threadTestRule.getHandler().postAndSync(controller::getTrackSelectionParameters); + + TrackGroup controllerTrackGroup = controllerTracks.getGroups().get(1).getMediaTrackGroup(); + assertThat(controllerTrackSelectionParameters.overrides) + .containsExactly( + controllerTrackGroup, + new TrackSelectionOverride(controllerTrackGroup, /* trackIndex= */ 0)); + } + @Test public void setMediaItems_setLessMediaItemsThanCurrentMediaItemIndex_masksCurrentMediaItemIndexAndStateCorrectly() diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java index c4def47aad..172f6a8dfa 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java @@ -24,13 +24,21 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS; import android.content.Context; import android.os.Handler; import android.os.Looper; +import androidx.media3.common.C; +import androidx.media3.common.Format; +import androidx.media3.common.Metadata; +import androidx.media3.common.MimeTypes; import androidx.media3.common.Player; +import androidx.media3.common.TrackGroup; +import androidx.media3.common.TrackSelectionOverride; +import androidx.media3.common.Tracks; import androidx.media3.common.util.Util; import androidx.media3.test.session.common.HandlerThreadTestRule; import androidx.media3.test.session.common.MainLooperTestRule; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.filters.LargeTest; +import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.ListenableFuture; import java.util.concurrent.CancellationException; import java.util.concurrent.CountDownLatch; @@ -248,4 +256,58 @@ public class MediaSessionAndControllerTest { player.awaitMethodCalled(MockPlayer.METHOD_PREPARE, TIMEOUT_MS); player.awaitMethodCalled(MockPlayer.METHOD_PLAY, TIMEOUT_MS); } + + @Test + public void setTrackSelectionParameters_withOverrides_matchesExpectedTrackGroupInPlayer() + throws Exception { + MockPlayer player = + new MockPlayer.Builder().setApplicationLooper(Looper.getMainLooper()).build(); + // Intentionally add metadata to the format as this can't be bundled. + Tracks.Group trackGroupInPlayer = + new Tracks.Group( + new TrackGroup( + new Format.Builder() + .setId("0") + .setSampleMimeType(MimeTypes.VIDEO_H264) + .setMetadata(new Metadata()) + .build(), + new Format.Builder() + .setId("1") + .setSampleMimeType(MimeTypes.VIDEO_H264) + .setMetadata(new Metadata()) + .build()), + /* adaptiveSupported= */ false, + /* trackSupport= */ new int[] {C.FORMAT_HANDLED, C.FORMAT_HANDLED}, + /* trackSelected= */ new boolean[] {true, false}); + player.currentTracks = new Tracks(ImmutableList.of(trackGroupInPlayer)); + MediaSession session = + sessionTestRule.ensureReleaseAfterTest( + new MediaSession.Builder(context, player).setId(TAG).build()); + MediaController controller = controllerTestRule.createController(session.getToken()); + + threadTestRule + .getHandler() + .postAndSync( + () -> + controller.setTrackSelectionParameters( + controller + .getTrackSelectionParameters() + .buildUpon() + .setOverrideForType( + new TrackSelectionOverride( + controller + .getCurrentTracks() + .getGroups() + .get(0) + .getMediaTrackGroup(), + /* trackIndex= */ 1)) + .build())); + player.awaitMethodCalled(MockPlayer.METHOD_SET_TRACK_SELECTION_PARAMETERS, TIMEOUT_MS); + + assertThat(player.trackSelectionParameters.overrides) + .containsExactly( + trackGroupInPlayer.getMediaTrackGroup(), + new TrackSelectionOverride( + trackGroupInPlayer.getMediaTrackGroup(), /* trackIndex= */ 1)); + } }