From 913441ce23ce877423868d243bcc67411425702c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Muller?= Date: Tue, 23 Apr 2024 12:57:06 +0200 Subject: [PATCH] Mitigate missing aspect ratio issue --- .../pillarbox/player/PlayerCallbackFlow.kt | 21 +++++--- .../pillarbox/player/extension/VideoSize.kt | 8 ++-- .../player/TestPlayerCallbackFlow.kt | 48 +++++++++++++++++++ .../player/extension/VideoSizeTest.kt | 29 ++++------- .../ui/widget/player/PlayerSurface.kt | 2 +- 5 files changed, 75 insertions(+), 33 deletions(-) diff --git a/pillarbox-player/src/main/java/ch/srgssr/pillarbox/player/PlayerCallbackFlow.kt b/pillarbox-player/src/main/java/ch/srgssr/pillarbox/player/PlayerCallbackFlow.kt index 6f6f7ffba..1739896fd 100644 --- a/pillarbox-player/src/main/java/ch/srgssr/pillarbox/player/PlayerCallbackFlow.kt +++ b/pillarbox-player/src/main/java/ch/srgssr/pillarbox/player/PlayerCallbackFlow.kt @@ -5,7 +5,6 @@ package ch.srgssr.pillarbox.player import androidx.media3.common.C -import androidx.media3.common.Format import androidx.media3.common.MediaItem import androidx.media3.common.MediaMetadata import androidx.media3.common.PlaybackException @@ -17,6 +16,7 @@ import androidx.media3.common.TrackSelectionParameters import androidx.media3.common.Tracks import androidx.media3.common.VideoSize import ch.srgssr.pillarbox.player.asset.Chapter +import ch.srgssr.pillarbox.player.extension.computeAspectRatioOrNull import ch.srgssr.pillarbox.player.extension.getChapterAtPosition import ch.srgssr.pillarbox.player.extension.getCurrentMediaItems import ch.srgssr.pillarbox.player.extension.getPlaybackSpeed @@ -28,6 +28,7 @@ import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.callbackFlow +import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.map @@ -295,10 +296,16 @@ fun Player.videoSizeAsFlow(): Flow = callbackFlow { * * @param defaultAspectRatio The aspect ratio when the video size is unknown, or for audio content. */ +@OptIn(ExperimentalCoroutinesApi::class) fun Player.getAspectRatioAsFlow(defaultAspectRatio: Float): Flow { - return getCurrentTracksAsFlow() - .map { it.getVideoAspectRatioOrElse(defaultAspectRatio) } - .distinctUntilChanged() + return combine( + getCurrentTracksAsFlow(), + videoSizeAsFlow(), + ) { currentTracks, videoSize -> + currentTracks.getVideoAspectRatioOrNull() + ?: videoSize.computeAspectRatioOrNull() + ?: defaultAspectRatio + }.distinctUntilChanged() } /** @@ -403,11 +410,11 @@ private suspend fun ProducerScope.addPlayerListener(player: Player, liste } } -private fun Tracks.getVideoAspectRatioOrElse(defaultAspectRatio: Float): Float { +private fun Tracks.getVideoAspectRatioOrNull(): Float? { val format = videoTracks.find { it.isSelected }?.format - return if (format == null || format.height <= 0 || format.width == Format.NO_VALUE) { - defaultAspectRatio + return if (format == null || format.height <= 0 || format.width <= 0) { + null } else { format.width * format.pixelWidthHeightRatio / format.height.toFloat() } diff --git a/pillarbox-player/src/main/java/ch/srgssr/pillarbox/player/extension/VideoSize.kt b/pillarbox-player/src/main/java/ch/srgssr/pillarbox/player/extension/VideoSize.kt index 7040b3cb3..096b3313d 100644 --- a/pillarbox-player/src/main/java/ch/srgssr/pillarbox/player/extension/VideoSize.kt +++ b/pillarbox-player/src/main/java/ch/srgssr/pillarbox/player/extension/VideoSize.kt @@ -8,12 +8,10 @@ import android.util.Rational import androidx.media3.common.VideoSize /** - * Compute aspect ratio, return [unknownAspectRatioValue] if aspect ratio can't be computed. - * - * @param unknownAspectRatioValue + * Compute the aspect ratio, return `null` if the aspect ratio can't be computed. */ -fun VideoSize.computeAspectRatio(unknownAspectRatioValue: Float): Float { - return if (height == 0 || width == 0) unknownAspectRatioValue else width * this.pixelWidthHeightRatio / height +fun VideoSize.computeAspectRatioOrNull(): Float? { + return if (height == 0 || width == 0) null else width * this.pixelWidthHeightRatio / height } /** diff --git a/pillarbox-player/src/test/java/ch/srgssr/pillarbox/player/TestPlayerCallbackFlow.kt b/pillarbox-player/src/test/java/ch/srgssr/pillarbox/player/TestPlayerCallbackFlow.kt index 8fa3c6dbe..4a8fbe3b2 100644 --- a/pillarbox-player/src/test/java/ch/srgssr/pillarbox/player/TestPlayerCallbackFlow.kt +++ b/pillarbox-player/src/test/java/ch/srgssr/pillarbox/player/TestPlayerCallbackFlow.kt @@ -432,6 +432,54 @@ class TestPlayerCallbackFlow { } } + @Test + fun `get aspect ratio as flow, video tracks no video size, has video size`() = runTest { + val videoTracks = Tracks( + listOf( + createTrackGroup( + selectedIndex = 1, + createVideoFormat("v1", width = Format.NO_VALUE, height = Format.NO_VALUE), + createVideoFormat("v2", width = Format.NO_VALUE, height = Format.NO_VALUE), + createVideoFormat("v3", width = Format.NO_VALUE, height = Format.NO_VALUE), + ) + ) + ) + + val fakePlayer = PlayerListenerCommander(player) + + fakePlayer.getAspectRatioAsFlow(0f).test { + fakePlayer.onVideoSizeChanged(VideoSize(1920, 1080)) + fakePlayer.onTracksChanged(videoTracks) + + assertEquals(0f, awaitItem()) + assertEquals(16 / 9f, awaitItem()) + ensureAllEventsConsumed() + } + } + + @Test + fun `get aspect ratio as flow, video tracks no video size, no video size`() = runTest { + val videoTracks = Tracks( + listOf( + createTrackGroup( + selectedIndex = 1, + createVideoFormat("v1", width = Format.NO_VALUE, height = Format.NO_VALUE), + createVideoFormat("v2", width = Format.NO_VALUE, height = Format.NO_VALUE), + createVideoFormat("v3", width = Format.NO_VALUE, height = Format.NO_VALUE), + ) + ) + ) + + val fakePlayer = PlayerListenerCommander(player) + + fakePlayer.getAspectRatioAsFlow(0f).test { + fakePlayer.onTracksChanged(videoTracks) + + assertEquals(0f, awaitItem()) + ensureAllEventsConsumed() + } + } + @Test fun `get aspect ratio as flow, video tracks without selection`() = runTest { val videoTracksWithoutSelection = Tracks( diff --git a/pillarbox-player/src/test/java/ch/srgssr/pillarbox/player/extension/VideoSizeTest.kt b/pillarbox-player/src/test/java/ch/srgssr/pillarbox/player/extension/VideoSizeTest.kt index 838a73a40..73983a014 100644 --- a/pillarbox-player/src/test/java/ch/srgssr/pillarbox/player/extension/VideoSizeTest.kt +++ b/pillarbox-player/src/test/java/ch/srgssr/pillarbox/player/extension/VideoSizeTest.kt @@ -10,71 +10,60 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import org.junit.runner.RunWith import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertNull @RunWith(AndroidJUnit4::class) class VideoSizeTest { @Test fun `computeAspectRatio unknown video size`() { val input = VideoSize.UNKNOWN - val expectedAspectRatio = 16f / 9 - val unknownAspectRatio = 16f / 9 - assertEquals(expectedAspectRatio, input.computeAspectRatio(unknownAspectRatio)) + assertNull(input.computeAspectRatioOrNull()) } @Test fun `computeAspectRatio with height set to 0`() { val input = VideoSize(0, 100) - val expectedAspectRatio = 16f / 9 - val unknownAspectRatio = 16f / 9 - assertEquals(expectedAspectRatio, input.computeAspectRatio(unknownAspectRatio)) + assertNull(input.computeAspectRatioOrNull()) } @Test fun `computeAspectRatio with width set to 0`() { val input = VideoSize(240, 0) - val expectedAspectRatio = 16f / 9 - val unknownAspectRatio = 16f / 9 - assertEquals(expectedAspectRatio, input.computeAspectRatio(unknownAspectRatio)) + assertNull(input.computeAspectRatioOrNull()) } @Test fun `computeAspectRatio with width and height set to 0`() { val input = VideoSize(0, 0) - val expectedAspectRatio = 16f / 9 - val unknownAspectRatio = 16f / 9 - assertEquals(expectedAspectRatio, input.computeAspectRatio(unknownAspectRatio)) + assertNull(input.computeAspectRatioOrNull()) } @Test fun `computeAspectRatio with a 16-9 aspect ratio`() { val input = VideoSize(1920, 1080) val expectedAspectRatio = 16f / 9 - val unknownAspectRatio = 1f - assertEquals(expectedAspectRatio, input.computeAspectRatio(unknownAspectRatio)) + assertEquals(expectedAspectRatio, input.computeAspectRatioOrNull()) } @Test fun `computeAspectRatio with a 9-16 aspect ratio`() { val input = VideoSize(1080, 1920) val expectedAspectRatio = 9f / 16 - val unknownAspectRatio = 1f - assertEquals(expectedAspectRatio, input.computeAspectRatio(unknownAspectRatio)) + assertEquals(expectedAspectRatio, input.computeAspectRatioOrNull()) } @Test fun `computeAspectRatio with a 4-3 aspect ratio`() { val input = VideoSize(800, 600) val expectedAspectRatio = 4f / 3 - val unknownAspectRatio = 1f - assertEquals(expectedAspectRatio, input.computeAspectRatio(unknownAspectRatio)) + assertEquals(expectedAspectRatio, input.computeAspectRatioOrNull()) } @Test fun `computeAspectRatio with a square aspect ratio`() { val input = VideoSize(500, 500) val expectedAspectRatio = 1f - val unknownAspectRatio = 0f - assertEquals(expectedAspectRatio, input.computeAspectRatio(unknownAspectRatio)) + assertEquals(expectedAspectRatio, input.computeAspectRatioOrNull()) } @Test diff --git a/pillarbox-ui/src/main/java/ch/srgssr/pillarbox/ui/widget/player/PlayerSurface.kt b/pillarbox-ui/src/main/java/ch/srgssr/pillarbox/ui/widget/player/PlayerSurface.kt index 84ac093d1..b3a658ab1 100644 --- a/pillarbox-ui/src/main/java/ch/srgssr/pillarbox/ui/widget/player/PlayerSurface.kt +++ b/pillarbox-ui/src/main/java/ch/srgssr/pillarbox/ui/widget/player/PlayerSurface.kt @@ -54,7 +54,7 @@ fun PlayerSurface( displayDebugView: Boolean = false, surfaceContent: @Composable (BoxScope.() -> Unit)? = { ExoPlayerSubtitleView(player = player) }, ) { - var lastKnownVideoAspectRatio by remember { mutableFloatStateOf(defaultAspectRatio ?: 0f) } + var lastKnownVideoAspectRatio by remember { mutableFloatStateOf(defaultAspectRatio ?: 1f) } val videoAspectRatio by player.getAspectRatioAsState(defaultAspectRatio = lastKnownVideoAspectRatio) // If the media has tracks, but no video tracks, we reset the aspect ratio to 0