From e8c15b4e8b2cfaf6dab16823236b93b4a7607594 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Tue, 12 Jul 2022 13:27:23 +0100 Subject: [PATCH] Correct the playing status logic. (#367) --- media-data/api/current.api | 8 +- .../media/data/MediaItemPositionMapper.kt | 41 ++++++ .../media/data/PlayerRepositoryImpl.kt | 37 +---- .../media/data/PlayerStateMapper.kt | 40 ++++-- .../horologist/media/data/FakeStatePlayer.kt | 129 ++++++++++++++++++ .../media/data/MediaItemPositionMapperTest.kt | 68 +++++++++ .../media/data/PlayerRepositoryImplTest.kt | 78 +---------- .../media/data/PlayerStateMapperTest.kt | 48 +++++++ 8 files changed, 334 insertions(+), 115 deletions(-) create mode 100644 media-data/src/main/java/com/google/android/horologist/media/data/MediaItemPositionMapper.kt create mode 100644 media-data/src/test/java/com/google/android/horologist/media/data/FakeStatePlayer.kt create mode 100644 media-data/src/test/java/com/google/android/horologist/media/data/MediaItemPositionMapperTest.kt create mode 100644 media-data/src/test/java/com/google/android/horologist/media/data/PlayerStateMapperTest.kt diff --git a/media-data/api/current.api b/media-data/api/current.api index 8123ea3e0a..31b9325126 100644 --- a/media-data/api/current.api +++ b/media-data/api/current.api @@ -20,6 +20,11 @@ package com.google.android.horologist.media.data { field public static final com.google.android.horologist.media.data.MediaItemMapper INSTANCE; } + public final class MediaItemPositionMapper { + method public com.google.android.horologist.media.model.MediaItemPosition? map(androidx.media3.common.Player? player); + field public static final com.google.android.horologist.media.data.MediaItemPositionMapper INSTANCE; + } + public final class PlayerRepositoryImpl implements java.io.Closeable com.google.android.horologist.media.repository.PlayerRepository { ctor public PlayerRepositoryImpl(); method public void addMediaItem(com.google.android.horologist.media.model.MediaItem mediaItem); @@ -68,7 +73,8 @@ package com.google.android.horologist.media.data { } public final class PlayerStateMapper { - method public com.google.android.horologist.media.model.PlayerState map(@androidx.media3.common.Player.State int media3PlayerState); + method public boolean affectsState(androidx.media3.common.Player.Events events); + method public com.google.android.horologist.media.model.PlayerState map(androidx.media3.common.Player player); field public static final com.google.android.horologist.media.data.PlayerStateMapper INSTANCE; } diff --git a/media-data/src/main/java/com/google/android/horologist/media/data/MediaItemPositionMapper.kt b/media-data/src/main/java/com/google/android/horologist/media/data/MediaItemPositionMapper.kt new file mode 100644 index 0000000000..4073bb1810 --- /dev/null +++ b/media-data/src/main/java/com/google/android/horologist/media/data/MediaItemPositionMapper.kt @@ -0,0 +1,41 @@ +/* + * Copyright 2022 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 + * + * https://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.horologist.media.data + +import androidx.media3.common.C +import androidx.media3.common.Player +import com.google.android.horologist.media.model.MediaItemPosition +import kotlin.time.Duration.Companion.milliseconds + +/** + * Maps a [Media3 player][Player] position into a [MediaItemPosition]. + */ +public object MediaItemPositionMapper { + public fun map(player: Player?): MediaItemPosition? { + return if (player == null || player.currentMediaItem == null) { + null + } else if (player.duration == C.TIME_UNSET) { + MediaItemPosition.UnknownDuration(player.currentPosition.milliseconds) + } else { + MediaItemPosition.create( + current = player.currentPosition.milliseconds, + // Ensure progress is max 100%, even given faulty media metadata + duration = (player.duration.coerceAtLeast(player.currentPosition)).milliseconds + ) + } + } +} diff --git a/media-data/src/main/java/com/google/android/horologist/media/data/PlayerRepositoryImpl.kt b/media-data/src/main/java/com/google/android/horologist/media/data/PlayerRepositoryImpl.kt index 6513bb0a3d..8d83edc34f 100644 --- a/media-data/src/main/java/com/google/android/horologist/media/data/PlayerRepositoryImpl.kt +++ b/media-data/src/main/java/com/google/android/horologist/media/data/PlayerRepositoryImpl.kt @@ -17,7 +17,6 @@ package com.google.android.horologist.media.data import android.util.Log -import androidx.media3.common.C import androidx.media3.common.PlaybackParameters import androidx.media3.common.Player import com.google.android.horologist.media.model.Command @@ -29,7 +28,6 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import java.io.Closeable import kotlin.time.Duration -import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.seconds import kotlin.time.DurationUnit import kotlin.time.toDuration @@ -97,21 +95,16 @@ public class PlayerRepositoryImpl : PlayerRepository, Closeable { if (events.contains(Player.EVENT_MEDIA_ITEM_TRANSITION)) { _currentMediaItem.value = player.currentMediaItem?.let(MediaItemMapper::map) + updatePosition() } // Reason for handling these events here, instead of using individual callbacks - // (onIsLoadingChanged, onIsPlayingChanged, onPlaybackStateChanged): + // (onIsLoadingChanged, onIsPlayingChanged, onPlaybackStateChanged, etc): // - The listener intends to use multiple state values that are reported through // separate callbacks together, or in combination with Player getter methods // Reference: // https://exoplayer.dev/listening-to-player-events.html#individual-callbacks-vs-onevents - if (events.containsAny( - Player.EVENT_IS_LOADING_CHANGED, - Player.EVENT_IS_PLAYING_CHANGED, - Player.EVENT_PLAYBACK_STATE_CHANGED, - Player.EVENT_PLAY_WHEN_READY_CHANGED - ) - ) { + if (PlayerStateMapper.affectsState(events)) { updateState(player) } } @@ -130,13 +123,7 @@ public class PlayerRepositoryImpl : PlayerRepository, Closeable { * [Player.getPlaybackState] and [Player.getPlayWhenReady] properties. */ private fun updateState(player: Player) { - _currentState.value = if ((player.isPlaying || player.isLoading) && player.playWhenReady) { - PlayerState.Playing - } else if (player.isLoading) { - PlayerState.Loading - } else { - PlayerStateMapper.map(player.playbackState) - } + _currentState.value = PlayerStateMapper.map(player) Log.d(TAG, "Player state changed to ${_currentState.value}") } @@ -357,27 +344,13 @@ public class PlayerRepositoryImpl : PlayerRepository, Closeable { * Updating roughly once a second while activity is foregrounded is appropriate. */ public fun updatePosition() { - player.value.updatePosition() + _mediaItemPosition.value = MediaItemPositionMapper.map(player.value) } public fun setPlaybackSpeed(speed: Float) { player.value?.setPlaybackSpeed(speed) } - private fun Player?.updatePosition() { - _mediaItemPosition.value = if (this == null) { - null - } else if (duration == C.TIME_UNSET) { - MediaItemPosition.UnknownDuration(currentPosition.milliseconds) - } else { - MediaItemPosition.create( - current = currentPosition.milliseconds, - // Ensure progress is max 100%, even given faulty media metadata - duration = (duration.coerceAtLeast(currentPosition)).milliseconds - ) - } - } - private fun checkNotClosed() { check(!closed) { "Player is already closed." } } diff --git a/media-data/src/main/java/com/google/android/horologist/media/data/PlayerStateMapper.kt b/media-data/src/main/java/com/google/android/horologist/media/data/PlayerStateMapper.kt index 58dd14dff7..3841d4cca9 100644 --- a/media-data/src/main/java/com/google/android/horologist/media/data/PlayerStateMapper.kt +++ b/media-data/src/main/java/com/google/android/horologist/media/data/PlayerStateMapper.kt @@ -20,16 +20,38 @@ import androidx.media3.common.Player import com.google.android.horologist.media.model.PlayerState /** - * Maps a [Media3 player state][Player.State] into a [PlayerState]. + * Maps a [Media3 player][Player] into a [PlayerState]. */ public object PlayerStateMapper { - - public fun map(@Player.State media3PlayerState: Int): PlayerState = - when (media3PlayerState) { - Player.STATE_IDLE -> PlayerState.Idle - Player.STATE_BUFFERING -> PlayerState.Loading - Player.STATE_READY -> PlayerState.Ready - Player.STATE_ENDED -> PlayerState.Ended - else -> throw IllegalArgumentException("Invalid media3 player state: $media3PlayerState") + public fun map(player: Player): PlayerState { + return if (( + player.playbackState == Player.STATE_BUFFERING || + player.playbackState == Player.STATE_READY + ) && + player.playWhenReady + ) { + PlayerState.Playing + } else if (player.isLoading) { + PlayerState.Loading + } else { + map(player.playbackState) } + } + + private fun map(@Player.State media3PlayerState: Int): PlayerState = when (media3PlayerState) { + Player.STATE_IDLE -> PlayerState.Idle + Player.STATE_BUFFERING -> PlayerState.Loading + Player.STATE_READY -> PlayerState.Ready + Player.STATE_ENDED -> PlayerState.Ended + else -> throw IllegalArgumentException("Invalid media3 player state: $media3PlayerState") + } + + public fun affectsState(events: Player.Events): Boolean { + return events.containsAny( + Player.EVENT_IS_LOADING_CHANGED, + Player.EVENT_IS_PLAYING_CHANGED, + Player.EVENT_PLAYBACK_STATE_CHANGED, + Player.EVENT_PLAY_WHEN_READY_CHANGED, + ) + } } diff --git a/media-data/src/test/java/com/google/android/horologist/media/data/FakeStatePlayer.kt b/media-data/src/test/java/com/google/android/horologist/media/data/FakeStatePlayer.kt new file mode 100644 index 0000000000..8a823fdc95 --- /dev/null +++ b/media-data/src/test/java/com/google/android/horologist/media/data/FakeStatePlayer.kt @@ -0,0 +1,129 @@ +/* + * Copyright 2022 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 + * + * https://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.horologist.media.data + +import androidx.media3.common.AdPlaybackState +import androidx.media3.common.C +import androidx.media3.common.FlagSet +import androidx.media3.common.MediaItem +import androidx.media3.common.Player +import androidx.media3.common.Player.Listener +import androidx.media3.common.Timeline +import androidx.media3.test.utils.FakeTimeline +import androidx.media3.test.utils.StubPlayer +import com.google.common.collect.ImmutableList + +class FakeStatePlayer( + var _currentPosition: Long = 0L, + var _duration: Long = C.TIME_UNSET, + var _playbackState: Int = STATE_IDLE, + var _playWhenReady: Boolean = false, + var _currentMediaItem: MediaItem? = null +) : StubPlayer() { + private val listeners = mutableListOf() + + override fun addListener(listener: Player.Listener) { + listeners.add(listener) + super.addListener(listener) + } + + override fun getCurrentPosition(): Long = _currentPosition + + override fun getDuration(): Long { + return _duration + } + + override fun getPlaybackState(): Int = _playbackState + + override fun setPlayWhenReady(playWhenReady: Boolean) { + _playWhenReady = playWhenReady + } + + override fun getPlayWhenReady(): Boolean = _playWhenReady + + override fun getCurrentTimeline(): Timeline { + val currentMediaItem = _currentMediaItem + if (currentMediaItem == null) { + return FakeTimeline() + } else { + return FakeTimeline( + FakeTimeline.TimelineWindowDefinition( + /* periodCount= */ 1, + /* id= */ 1, + /* isSeekable= */ true, + /* isDynamic= */ false, + /* isLive= */ false, + /* isPlaceholder= */ false, + /* durationUs= */ 1000 * C.MICROS_PER_SECOND, + /* defaultPositionUs= */ 2 * C.MICROS_PER_SECOND, + /* windowOffsetInFirstPeriodUs= */ 123456789, + ImmutableList.of(AdPlaybackState.NONE), + currentMediaItem + ) + ) + } + } + + override fun getCurrentMediaItemIndex(): Int = 0 + + fun overridePosition( + currentPosition: Long = 0L, + duration: Long = C.TIME_UNSET, + currentMediaItem: MediaItem? = null + ) { + _currentPosition = currentPosition + _duration = duration + _currentMediaItem = currentMediaItem + + listeners.forEach { + it.onEvents( + this, + Player.Events( + FlagSet.Builder().addAll( + EVENT_MEDIA_ITEM_TRANSITION, + EVENT_MEDIA_ITEM_TRANSITION, + EVENT_MEDIA_METADATA_CHANGED + ).build() + ) + ) + } + } + + fun overrideState( + playbackState: Int = STATE_IDLE, + playWhenReady: Boolean = false, + reason: @Player.PlayWhenReadyChangeReason Int = PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST + ) { + _playbackState = playbackState + _playWhenReady = playWhenReady + + listeners.forEach { + it.onEvents( + this, + Player.Events( + FlagSet.Builder().addAll( + EVENT_PLAYBACK_STATE_CHANGED, + EVENT_PLAY_WHEN_READY_CHANGED, + EVENT_MEDIA_ITEM_TRANSITION + ).build() + ) + ) + it.onPlayWhenReadyChanged(_playWhenReady, reason) + it.onPlaybackStateChanged(_playbackState) + } + } +} diff --git a/media-data/src/test/java/com/google/android/horologist/media/data/MediaItemPositionMapperTest.kt b/media-data/src/test/java/com/google/android/horologist/media/data/MediaItemPositionMapperTest.kt new file mode 100644 index 0000000000..2edc621aaf --- /dev/null +++ b/media-data/src/test/java/com/google/android/horologist/media/data/MediaItemPositionMapperTest.kt @@ -0,0 +1,68 @@ +/* + * Copyright 2022 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 + * + * https://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.horologist.media.data + +import androidx.media3.common.C +import com.google.android.horologist.media.model.MediaItemPosition +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import kotlin.time.Duration.Companion.milliseconds + +class MediaItemPositionMapperTest { + val fakeStatePlayer = FakeStatePlayer() + + @Test + fun `check position calculations null`() { + val position = MediaItemPositionMapper.map(null) + assertThat(position).isNull() + } + + @Test + fun `check position calculations unknown duration`() { + fakeStatePlayer.overridePosition( + currentPosition = 10L, + duration = C.TIME_UNSET + ) + val position = + MediaItemPositionMapper.map(fakeStatePlayer) as MediaItemPosition.UnknownDuration + assertThat(position.current).isEqualTo(10.milliseconds) + } + + @Test + fun `check position calculations past end`() { + fakeStatePlayer.overridePosition( + currentPosition = 100L, + duration = 99L + ) + val position = + MediaItemPositionMapper.map(fakeStatePlayer) as MediaItemPosition.KnownDuration + assertThat(position.current).isEqualTo(100.milliseconds) + assertThat(position.duration).isEqualTo(100.milliseconds) + } + + @Test + fun `check position calculations during`() { + fakeStatePlayer.overridePosition( + currentPosition = 100L, + duration = 1000L + ) + val position = + MediaItemPositionMapper.map(fakeStatePlayer) as MediaItemPosition.KnownDuration + assertThat(position.current).isEqualTo(100.milliseconds) + assertThat(position.duration).isEqualTo(1000.milliseconds) + } +} diff --git a/media-data/src/test/java/com/google/android/horologist/media/data/PlayerRepositoryImplTest.kt b/media-data/src/test/java/com/google/android/horologist/media/data/PlayerRepositoryImplTest.kt index 8917720303..3328083a9c 100644 --- a/media-data/src/test/java/com/google/android/horologist/media/data/PlayerRepositoryImplTest.kt +++ b/media-data/src/test/java/com/google/android/horologist/media/data/PlayerRepositoryImplTest.kt @@ -18,8 +18,6 @@ package com.google.android.horologist.media.data import android.content.Context import android.os.Looper.getMainLooper -import androidx.media3.common.C -import androidx.media3.common.ForwardingPlayer import androidx.media3.common.Player import androidx.media3.test.utils.TestExoPlayerBuilder import androidx.media3.test.utils.robolectric.TestPlayerRunHelper.playUntilPosition @@ -324,9 +322,7 @@ class PlayerRepositoryImplTest { fun `given seek increment when getSeekBackIncrement then correct value is returned`() { // given val seekIncrement = 1234L - val player = TestExoPlayerBuilder(context) - .setSeekBackIncrementMs(seekIncrement) - .build() + val player = TestExoPlayerBuilder(context).setSeekBackIncrementMs(seekIncrement).build() sut.connect(player) {} // when @@ -340,9 +336,7 @@ class PlayerRepositoryImplTest { fun `given seek increment when getSeekForwardIncrement then correct value is returned`() { // given val seekIncrement = 1234L - val player = TestExoPlayerBuilder(context) - .setSeekForwardIncrementMs(seekIncrement) - .build() + val player = TestExoPlayerBuilder(context).setSeekForwardIncrementMs(seekIncrement).build() sut.connect(player) {} // when @@ -543,7 +537,7 @@ class PlayerRepositoryImplTest { assertThat(sut.playbackSpeed.value).isEqualTo(1f) assertThat(sut.shuffleModeEnabled.value).isFalse() assertThat(sut.player.value).isSameInstanceAs(player) - assertThat(sut.mediaItemPosition.value).isNull() + assertThat(sut.mediaItemPosition.value).isEqualTo(MediaItemPosition.UnknownDuration(0.seconds)) assertThat(sut.availableCommands.value).containsExactlyElementsIn( listOf(Command.PlayPause, Command.SetShuffle) ) @@ -598,7 +592,7 @@ class PlayerRepositoryImplTest { assertThat(sut.playbackSpeed.value).isEqualTo(1f) assertThat(sut.shuffleModeEnabled.value).isFalse() assertThat(sut.player.value).isSameInstanceAs(player) - assertThat(sut.mediaItemPosition.value).isNull() + assertThat(sut.mediaItemPosition.value).isEqualTo(MediaItemPosition.UnknownDuration(0.seconds)) assertThat(sut.availableCommands.value).containsExactlyElementsIn( listOf(Command.PlayPause, Command.SetShuffle) ) @@ -872,8 +866,7 @@ class PlayerRepositoryImplTest { assertThat(sut.mediaItemPosition.value).isInstanceOf(MediaItemPosition.KnownDuration::class.java) val expectedMediaItemPosition = MediaItemPosition.create(current = 1020.milliseconds, duration = 1022.milliseconds) - val actualMediaItemPosition = - sut.mediaItemPosition.value as MediaItemPosition.KnownDuration + val actualMediaItemPosition = sut.mediaItemPosition.value as MediaItemPosition.KnownDuration // TODO these checks can be simplified to `assertThat().isEqualTo()` once horologist implements equals for MediaItemPosition.KnownDuration assertThat(actualMediaItemPosition.current).isEqualTo(expectedMediaItemPosition.current) assertThat(actualMediaItemPosition.duration).isEqualTo(expectedMediaItemPosition.duration) @@ -926,67 +919,6 @@ class PlayerRepositoryImplTest { ) } - private fun buildFakePositionPlayer( - _currentPosition: Long = 0L, - _duration: Long = C.TIME_UNSET - ): Player { - val player = TestExoPlayerBuilder(context).build() - return object : ForwardingPlayer(player) { - override fun getCurrentPosition(): Long { - return _currentPosition - } - - override fun getDuration(): Long { - return _duration - } - } - } - - @Test - fun `position is null when player is not set`() { - sut.updatePosition() - - assertThat(sut.mediaItemPosition.value).isNull() - } - - @Test - fun `position is handled when duration is unknown`() { - val player = buildFakePositionPlayer(10L, C.TIME_UNSET) - - sut.connect(player, onClose = {}) - sut.updatePosition() - - val position = sut.mediaItemPosition.value as MediaItemPosition.UnknownDuration - - assertThat(position.current).isEqualTo(10.milliseconds) - } - - @Test - fun `position is corrected when beyond duration`() { - val player = buildFakePositionPlayer(100L, 99L) - - sut.connect(player, onClose = {}) - sut.updatePosition() - - val position = sut.mediaItemPosition.value as MediaItemPosition.KnownDuration - - assertThat(position.current).isEqualTo(100.milliseconds) - assertThat(position.duration).isEqualTo(100.milliseconds) - } - - @Test - fun `position is sensible in normal case`() { - val player = buildFakePositionPlayer(100L, 1000L) - - sut.connect(player, onClose = {}) - sut.updatePosition() - - val position = sut.mediaItemPosition.value as MediaItemPosition.KnownDuration - - assertThat(position.current).isEqualTo(100.milliseconds) - assertThat(position.duration).isEqualTo(1000.milliseconds) - } - private fun getDummyMediaItem() = MediaItem( id = "id", uri = "uri", diff --git a/media-data/src/test/java/com/google/android/horologist/media/data/PlayerStateMapperTest.kt b/media-data/src/test/java/com/google/android/horologist/media/data/PlayerStateMapperTest.kt new file mode 100644 index 0000000000..0e4a6aca5f --- /dev/null +++ b/media-data/src/test/java/com/google/android/horologist/media/data/PlayerStateMapperTest.kt @@ -0,0 +1,48 @@ +/* + * Copyright 2022 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 + * + * https://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.horologist.media.data + +import androidx.media3.common.Player +import com.google.android.horologist.media.model.PlayerState +import com.google.common.truth.Truth.assertThat +import org.junit.Test + +class PlayerStateMapperTest { + val fakeStatePlayer = FakeStatePlayer() + + @Test + fun `check playback state while playing`() { + fakeStatePlayer.overrideState( + playbackState = Player.STATE_READY, + playWhenReady = true + ) + val state = PlayerStateMapper.map(fakeStatePlayer) + + assertThat(state).isEqualTo(PlayerState.Playing) + } + + @Test + fun `check playback state while buffering`() { + fakeStatePlayer.overrideState( + playbackState = Player.STATE_BUFFERING, + playWhenReady = true + ) + val state = PlayerStateMapper.map(fakeStatePlayer) + + assertThat(state).isEqualTo(PlayerState.Playing) + } +}