From 8f9e1ab87d4c17c6bd1d92957d99b930bc36eeb2 Mon Sep 17 00:00:00 2001 From: Antoine Jaury Date: Fri, 2 Oct 2026 16:01:35 +0200 Subject: [PATCH] fix: correct the way pause between phrases was handled --- CHANGELOG.md | 2 + data/playback/build.gradle.kts | 3 + .../player/data/PlaybackRepositoryImpl.kt | 244 +++++++++-------- .../chombev/player/data/SubtitleProgress.kt | 1 - .../player/data/PlaybackRepositoryImplTest.kt | 249 ++++++++++++++++++ 5 files changed, 392 insertions(+), 107 deletions(-) create mode 100644 data/playback/src/commonTest/kotlin/bzh/ajaury/chombev/player/data/PlaybackRepositoryImplTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index aad4aff..73fe453 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- correct the way pause between phrases was handled + ## [1.0.0-beta03] - 2026-09-30 ### Fixed diff --git a/data/playback/build.gradle.kts b/data/playback/build.gradle.kts index 4021fe4..0e6cf72 100644 --- a/data/playback/build.gradle.kts +++ b/data/playback/build.gradle.kts @@ -21,5 +21,8 @@ kotlin { implementation(projects.data.resources) implementation(projects.data.subtitle) } + commonTest.dependencies { + implementation(libs.kotlinx.coroutinesTest) + } } } diff --git a/data/playback/src/commonMain/kotlin/bzh/ajaury/chombev/player/data/PlaybackRepositoryImpl.kt b/data/playback/src/commonMain/kotlin/bzh/ajaury/chombev/player/data/PlaybackRepositoryImpl.kt index 9e40849..38c3ac9 100644 --- a/data/playback/src/commonMain/kotlin/bzh/ajaury/chombev/player/data/PlaybackRepositoryImpl.kt +++ b/data/playback/src/commonMain/kotlin/bzh/ajaury/chombev/player/data/PlaybackRepositoryImpl.kt @@ -10,6 +10,7 @@ import bzh.ajaury.chombev.player.model.PlaybackState import bzh.ajaury.chombev.player.model.PlaybackTiming import bzh.ajaury.chombev.player.model.PlayerState import bzh.ajaury.chombev.preferences.domain.PreferencesRepository +import bzh.ajaury.chombev.preferences.model.PlaybackPreferences import bzh.ajaury.chombev.resources.domain.ResourceReader import bzh.ajaury.chombev.subtitle.domain.GetCurrentSubtitleIndexUseCase import bzh.ajaury.chombev.subtitle.domain.SubtitleRepository @@ -18,23 +19,17 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.combine -import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.filter -import kotlinx.coroutines.flow.first -import kotlinx.coroutines.flow.firstOrNull import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOf -import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.map -import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import kotlin.time.Duration @@ -53,13 +48,18 @@ internal class PlaybackRepositoryImpl( ) : PlaybackRepository { // Audio operations must run on the main thread (e.g. ExoPlayer), so keep the scope on it. private val scope = CoroutineScope(SupervisorJob() + dispatcherProvider.main) - private var speedObservationJob: Job? = null + private var preferencesObservationJob: Job? = null private var phraseBreakJob: Job? = null + private var playbackPreferences = PlaybackPreferences() // Pause-then-resume of an in-progress phrase break; cancelled when the user takes over. private var breakJob: Job? = null private val isBreaking = MutableStateFlow(false) + private var reachedPhraseIndex = -1 + private var pendingPhraseStart: Duration? = null + private var pendingSeekTarget: Duration? = null + private val recordTitle = MutableStateFlow(Phrase(transcription = "")) private val subtitle = MutableStateFlow(Subtitle(emptyList())) @@ -113,45 +113,16 @@ internal class PlaybackRepositoryImpl( subtitle, currentPosition, ) { subtitle, position -> - val currentSubtitleIndex = getCurrentSubtitleIndex( - subtitle = subtitle, - position = position, - ) - - val isGoingToPlayNextPhrase = isGoingToPlayNextPhrase( - currentSubtitleIndex = currentSubtitleIndex, - subtitle = subtitle, - position = position, - ) - SubtitleProgress( subtitle = subtitle, currentPosition = position, - currentSubtitleIndex = currentSubtitleIndex, - isGoingToPlayNextPhrase = isGoingToPlayNextPhrase, + currentSubtitleIndex = getCurrentSubtitleIndex( + subtitle = subtitle, + position = position, + ), ) } - private fun isGoingToPlayNextPhrase( - currentSubtitleIndex: Int?, - subtitle: Subtitle, - position: Duration, - ): Boolean { - if (currentSubtitleIndex == null) { - return false - } - - val nextSubtitleIndex = currentSubtitleIndex + 1 - if (nextSubtitleIndex <= 0) { - return false - } - - val nextSubtitle = subtitle.lines.getOrNull(nextSubtitleIndex) - ?: return false - - return nextSubtitle.startTime < position + 2 * POSITION_POLL_INTERVAL - } - override val playerState: Flow = combine( playbackState, playbackTiming, @@ -168,19 +139,27 @@ internal class PlaybackRepositoryImpl( } override suspend fun load(record: Record) { + phraseBreakJob?.cancel() + cancelRunningBreak() + pendingPhraseStart = null + pendingSeekTarget = null + playAudio(filePath = record.audioResourcePath) subtitle.value = loadSubtitle(record = record) + reachedPhraseIndex = upcomingPhraseIndex(position = Duration.ZERO) recordTitle.value = record.title - observePlaybackSpeed() + observePlaybackPreferences() observePhraseBreaks() } - private fun observePlaybackSpeed() { - speedObservationJob?.cancel() - speedObservationJob = scope.launch { - preferencesRepository.playbackPreferences.collect { playbackPreferences -> - setSpeed(speed = playbackPreferences.speed) + private fun observePlaybackPreferences() { + preferencesObservationJob?.cancel() + preferencesObservationJob = scope.launch { + preferencesRepository.playbackPreferences.collect { preferences -> + // Cached so that reaching a phrase reacts immediately, without a suspending read. + playbackPreferences = preferences + setSpeed(speed = preferences.speed) } } } @@ -193,51 +172,98 @@ internal class PlaybackRepositoryImpl( * * Auto-pause takes precedence: while it is on, the break between phrases is ignored. * - * This only happens on automatic advancement — manual seeking and previous/next navigation are - * ignored because they cancel the running break job and never re-enter [PlaybackState.PLAYING] - * through this path. + * A phrase is considered reached as soon as a polled position is past its start minus + * [PHRASE_LOOKAHEAD], even if the poll came late and the phrase already started: in that case + * playback is rewound to the phrase start when resuming. Each phrase is only handled once. + * + * Manual seeking and previous/next navigation never trigger a break: [seekAudio] marks the + * phrase at the target position as already reached. */ - private suspend fun observePhraseBreaks() { + private fun observePhraseBreaks() { phraseBreakJob?.cancel() - cancelRunningBreakJob() - phraseBreakJob = subtitleProgress - .map { it.isGoingToPlayNextPhrase } - .distinctUntilChanged() - .filter { it } - .onEach { - if (playbackState.firstOrNull() != PlaybackState.PLAYING) return@onEach - - val playbackPreferences = preferencesRepository.playbackPreferences.first() - if (playbackPreferences.autoPause) { - audioPlayer.pause() - return@onEach + phraseBreakJob = scope.launch { + playbackState.collectLatest { state -> + if (state != PlaybackState.PLAYING) return@collectLatest + while (true) { + checkPhraseBoundary() + delay(POSITION_POLL_INTERVAL) } - - val breakDuration = playbackPreferences.breakBetweenPhrases - if (breakDuration <= Duration.ZERO) return@onEach - - insertBreakBeforePhrase(breakDuration = breakDuration) - }.launchIn(scope) - } - - private suspend fun insertBreakBeforePhrase(breakDuration: Duration) { - breakJob?.cancelAndJoin() - breakJob = scope.launch { - try { - // Pause and rewind the polling overshoot so the next phrase resumes from its very start. - isBreaking.value = true - audioPlayer.pause() - delay(breakDuration) - } finally { - // Only resume if still paused by this break; the user may have acted during it. - if (playbackState.firstOrNull() == PlaybackState.BREAKING) { - audioPlayer.play() - } - isBreaking.value = false } } } + private fun checkPhraseBoundary() { + if (isBreaking.value || audioPlayer.playbackState.value != PlaybackState.PLAYING) return + + val position = audioPlayer.currentPosition + pendingSeekTarget?.let { target -> + // Some players (e.g. AVPlayer) seek asynchronously and still report the pre-seek + // position for a while: ignore it, or it could be taken for a phrase change. + if ((position - target).absoluteValue > SEEK_TOLERANCE) return + pendingSeekTarget = null + } + + val upcomingIndex = upcomingPhraseIndex(position = position) + if (upcomingIndex <= reachedPhraseIndex) return + reachedPhraseIndex = upcomingIndex + + // There is no break before the very first phrase. + if (upcomingIndex == 0) return + val phraseStart = subtitle.value.lines[upcomingIndex].startTime + + val preferences = playbackPreferences + when { + preferences.autoPause -> { + pendingPhraseStart = phraseStart + audioPlayer.pause() + } + + preferences.breakBetweenPhrases > Duration.ZERO -> { + insertBreakBeforePhrase( + breakDuration = preferences.breakBetweenPhrases, + phraseStart = phraseStart, + ) + } + } + } + + private fun insertBreakBeforePhrase( + breakDuration: Duration, + phraseStart: Duration, + ) { + breakJob?.cancel() + pendingPhraseStart = phraseStart + isBreaking.value = true + audioPlayer.pause() + breakJob = scope.launch { + delay(breakDuration) + isBreaking.value = false + resumeAtPhraseStart(phraseStart = phraseStart) + } + } + + private fun resumeAtPhraseStart(phraseStart: Duration) { + pendingPhraseStart = null + // The phrase may have been detected a little late: rewind so it is heard from its start. + if (audioPlayer.currentPosition > phraseStart) { + seekAudio(position = phraseStart) + } + audioPlayer.play() + } + + private fun upcomingPhraseIndex(position: Duration): Int = + getCurrentSubtitleIndex( + subtitle = subtitle.value, + position = position + PHRASE_LOOKAHEAD, + ) ?: -1 + + private fun seekAudio(position: Duration) { + pendingPhraseStart = null + reachedPhraseIndex = upcomingPhraseIndex(position = position) + pendingSeekTarget = position + audioPlayer.seekTo(position) + } + private fun playAudio(filePath: String) { try { audioPlayer.load(uri = resourceReader.uri(filePath)) @@ -265,24 +291,30 @@ internal class PlaybackRepositoryImpl( } override suspend fun play() { - cancelRunningBreakJob() - audioPlayer.play() + cancelRunningBreak() + val phraseStart = pendingPhraseStart + if (phraseStart != null) { + resumeAtPhraseStart(phraseStart = phraseStart) + } else { + audioPlayer.play() + } } override suspend fun pause() { - cancelRunningBreakJob() + cancelRunningBreak() audioPlayer.pause() } override suspend fun replay() { - cancelRunningBreakJob() - audioPlayer.seekTo(Duration.ZERO) + cancelRunningBreak() + seekAudio(position = Duration.ZERO) audioPlayer.play() } override suspend fun seekTo(position: Duration) { - cancelRunningBreakJob() - audioPlayer.seekTo(position) + val wasBreaking = cancelRunningBreak() + seekAudio(position = position) + if (wasBreaking) audioPlayer.play() } private fun setSpeed(speed: Float) { @@ -310,9 +342,8 @@ internal class PlaybackRepositoryImpl( } override suspend fun seekToSentence(sentenceIndex: Int) { - cancelRunningBreakJob() val matchingSubtitleLine = subtitle.value.lines.getOrNull(sentenceIndex) ?: return - audioPlayer.seekTo(matchingSubtitleLine.startTime) + seekTo(position = matchingSubtitleLine.startTime) } private fun currentSentenceIndex(): Int? = @@ -322,32 +353,33 @@ internal class PlaybackRepositoryImpl( ) override suspend fun stop() { - breakJob?.cancel() - isBreaking.value = false + phraseBreakJob?.cancel() + cancelRunningBreak() + pendingPhraseStart = null subtitle.value = Subtitle(emptyList()) audioPlayer.stop() } override fun release() { - breakJob?.cancel() - isBreaking.value = false + phraseBreakJob?.cancel() + cancelRunningBreak() + pendingPhraseStart = null audioPlayer.release() subtitle.value = Subtitle(emptyList()) } - private suspend fun cancelRunningBreakJob() { - breakJob?.cancelAndJoin() - - // Unlock if paused by a break. - if (playbackState.value == PlaybackState.BREAKING) { - audioPlayer.play() - } - + private fun cancelRunningBreak(): Boolean { + breakJob?.cancel() + breakJob = null + val wasBreaking = isBreaking.value isBreaking.value = false + return wasBreaking } companion object { private val POSITION_POLL_INTERVAL = 50.milliseconds + private val PHRASE_LOOKAHEAD = 2 * POSITION_POLL_INTERVAL + private val SEEK_TOLERANCE = 500.milliseconds private val PREVIOUS_SENTENCE_THRESHOLD = 4.seconds } } diff --git a/data/playback/src/commonMain/kotlin/bzh/ajaury/chombev/player/data/SubtitleProgress.kt b/data/playback/src/commonMain/kotlin/bzh/ajaury/chombev/player/data/SubtitleProgress.kt index 44a280a..d125f4b 100644 --- a/data/playback/src/commonMain/kotlin/bzh/ajaury/chombev/player/data/SubtitleProgress.kt +++ b/data/playback/src/commonMain/kotlin/bzh/ajaury/chombev/player/data/SubtitleProgress.kt @@ -7,5 +7,4 @@ data class SubtitleProgress( val subtitle: Subtitle, val currentPosition: Duration, val currentSubtitleIndex: Int?, - val isGoingToPlayNextPhrase: Boolean, ) diff --git a/data/playback/src/commonTest/kotlin/bzh/ajaury/chombev/player/data/PlaybackRepositoryImplTest.kt b/data/playback/src/commonTest/kotlin/bzh/ajaury/chombev/player/data/PlaybackRepositoryImplTest.kt new file mode 100644 index 0000000..8b64d5c --- /dev/null +++ b/data/playback/src/commonTest/kotlin/bzh/ajaury/chombev/player/data/PlaybackRepositoryImplTest.kt @@ -0,0 +1,249 @@ +package bzh.ajaury.chombev.player.data + +import bzh.ajaury.chombev.core.coroutines.domain.DispatcherProvider +import bzh.ajaury.chombev.core.logging.domain.Logger +import bzh.ajaury.chombev.core.model.Phrase +import bzh.ajaury.chombev.core.model.Record +import bzh.ajaury.chombev.player.domain.AudioPlayer +import bzh.ajaury.chombev.player.model.PlaybackState +import bzh.ajaury.chombev.preferences.domain.PreferencesRepository +import bzh.ajaury.chombev.preferences.model.PlaybackPreferences +import bzh.ajaury.chombev.preferences.model.SubtitlePreferences +import bzh.ajaury.chombev.resources.domain.ResourceReader +import bzh.ajaury.chombev.subtitle.domain.GetCurrentSubtitleIndexUseCase +import bzh.ajaury.chombev.subtitle.domain.SubtitleRepository +import bzh.ajaury.chombev.subtitle.model.Subtitle +import bzh.ajaury.chombev.subtitle.model.SubtitleLine +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.time.Duration +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds + +@OptIn(ExperimentalCoroutinesApi::class) +class PlaybackRepositoryImplTest { + private val dispatcher = StandardTestDispatcher() + private val audioPlayer = FakeAudioPlayer() + private val preferences = MutableStateFlow(PlaybackPreferences(breakBetweenPhrases = 2.seconds)) + + private val repository = PlaybackRepositoryImpl( + audioPlayer = audioPlayer, + resourceReader = FakeResourceReader, + subtitleRepository = FakeSubtitleRepository, + preferencesRepository = FakePreferencesRepository(preferences), + getCurrentSubtitleIndex = GetCurrentSubtitleIndexUseCase(), + logger = FakeLogger, + dispatcherProvider = FakeDispatcherProvider(dispatcher), + ) + + @Test + fun breaks_before_the_next_phrase_when_the_poll_lands_just_before_it() = runTest(dispatcher) { + loadAndPlay() + + playTo(SECOND_PHRASE_START - 60.milliseconds) + + assertEquals(PlaybackState.PAUSED, audioPlayer.playbackState.value) + advanceTimeBy(2.seconds + 1.milliseconds) + assertEquals(PlaybackState.PLAYING, audioPlayer.playbackState.value) + assertEquals(SECOND_PHRASE_START - 60.milliseconds, audioPlayer.currentPosition) + } + + @Test + fun breaks_and_rewinds_when_the_poll_lands_after_the_next_phrase_started() = runTest(dispatcher) { + loadAndPlay() + + // A late poll (e.g. busy main thread) skips the window right before the phrase start. + playTo(SECOND_PHRASE_START + 80.milliseconds) + + assertEquals(PlaybackState.PAUSED, audioPlayer.playbackState.value) + advanceTimeBy(2.seconds + 1.milliseconds) + assertEquals(PlaybackState.PLAYING, audioPlayer.playbackState.value) + assertEquals(SECOND_PHRASE_START, audioPlayer.currentPosition) + } + + @Test + fun breaks_only_once_per_phrase() = runTest(dispatcher) { + loadAndPlay() + playTo(SECOND_PHRASE_START - 60.milliseconds) + advanceTimeBy(2.seconds + 1.milliseconds) + + playTo(SECOND_PHRASE_START + 500.milliseconds) + + assertEquals(PlaybackState.PLAYING, audioPlayer.playbackState.value) + } + + @Test + fun auto_pause_stops_and_resumes_from_the_phrase_start() = runTest(dispatcher) { + preferences.value = PlaybackPreferences(autoPause = true) + loadAndPlay() + + playTo(SECOND_PHRASE_START + 80.milliseconds) + assertEquals(PlaybackState.PAUSED, audioPlayer.playbackState.value) + + advanceTimeBy(10.seconds) + assertEquals(PlaybackState.PAUSED, audioPlayer.playbackState.value) + + repository.play() + assertEquals(PlaybackState.PLAYING, audioPlayer.playbackState.value) + assertEquals(SECOND_PHRASE_START, audioPlayer.currentPosition) + } + + @Test + fun seeking_to_the_next_sentence_does_not_break() = runTest(dispatcher) { + loadAndPlay() + playTo(1.seconds) + + repository.goToNextSentence() + runCurrent() + advanceTimeBy(200.milliseconds) + + assertEquals(PlaybackState.PLAYING, audioPlayer.playbackState.value) + } + + @Test + fun a_stale_position_after_a_backward_seek_does_not_break() = runTest(dispatcher) { + loadAndPlay() + playTo(SECOND_PHRASE_START + 1.seconds) + advanceTimeBy(3.seconds) + assertEquals(PlaybackState.PLAYING, audioPlayer.playbackState.value) + + // Like AVPlayer, the player still reports the pre-seek position for a while. + audioPlayer.asynchronousSeek = true + repository.seekToSentence(0) + advanceTimeBy(200.milliseconds) + assertEquals(PlaybackState.PLAYING, audioPlayer.playbackState.value) + + audioPlayer.completeSeek() + advanceTimeBy(200.milliseconds) + assertEquals(PlaybackState.PLAYING, audioPlayer.playbackState.value) + } + + private suspend fun TestScope.loadAndPlay() { + repository.load(record) + runCurrent() + } + + private fun TestScope.playTo(position: Duration) { + audioPlayer.position = position + advanceTimeBy(60.milliseconds) + } + + private companion object { + val SECOND_PHRASE_START = 4310.milliseconds + + val record = Record( + id = 1, + index = 0, + emoji = "", + title = Phrase(transcription = ""), + audioResourcePath = "audio", + subtitleResourcePath = "subtitle", + ) + } +} + +private class FakeAudioPlayer : AudioPlayer { + override val playbackState = MutableStateFlow(PlaybackState.IDLE) + var position: Duration = Duration.ZERO + var asynchronousSeek = false + private var pendingSeek: Duration? = null + + override val currentPosition: Duration get() = position + override val duration: Duration = 10.seconds + + override fun seekTo(position: Duration) { + if (asynchronousSeek) pendingSeek = position else this.position = position + } + + fun completeSeek() { + pendingSeek?.let { position = it } + pendingSeek = null + } + + override fun play() { + playbackState.value = PlaybackState.PLAYING + } + + override fun pause() { + playbackState.value = PlaybackState.PAUSED + } + + override fun stop() { + playbackState.value = PlaybackState.IDLE + } + + override fun release() = stop() + + override fun load(uri: String) = Unit + + override fun setSpeed(speed: Float) = Unit +} + +private object FakeResourceReader : ResourceReader { + override suspend fun read(resourcePath: String): String = "" + + override fun uri(resourcePath: String): String = resourcePath +} + +private object FakeSubtitleRepository : SubtitleRepository { + override suspend fun getSubtitle( + transcriptionContent: String, + translationContent: String?, + ): Subtitle = Subtitle( + lines = listOf( + SubtitleLine(startTime = 0.milliseconds, phrase = Phrase(transcription = "Demat deoc'h !")), + SubtitleLine(startTime = 4310.milliseconds, phrase = Phrase(transcription = "Kenavo !")), + SubtitleLine(startTime = 7350.milliseconds, phrase = Phrase(transcription = "Kenavo emberr !")), + ), + ) +} + +private class FakePreferencesRepository( + override val playbackPreferences: StateFlow, +) : PreferencesRepository { + override val subtitlePreferences: Flow = flowOf(SubtitlePreferences()) + override val onboardingCompleted: Flow = flowOf(true) + + override suspend fun setShowTranscription(enabled: Boolean) = Unit + + override suspend fun setShowTranslation(enabled: Boolean) = Unit + + override suspend fun setSubtitleTextScale(scale: Float) = Unit + + override suspend fun setShowPhoneticMarkers(enabled: Boolean) = Unit + + override suspend fun setPlaybackSpeed(speed: Float) = Unit + + override suspend fun setBreakBetweenPhrases(duration: Duration) = Unit + + override suspend fun setAutoPause(enabled: Boolean) = Unit + + override suspend fun setOnboardingCompleted() = Unit +} + +private object FakeLogger : Logger { + override fun debug(message: String, throwable: Throwable?) = Unit + + override fun info(message: String, throwable: Throwable?) = Unit + + override fun warning(message: String, throwable: Throwable?) = Unit + + override fun error(message: String, throwable: Throwable?) = Unit +} + +private class FakeDispatcherProvider(dispatcher: CoroutineDispatcher) : DispatcherProvider { + override val main = dispatcher + override val default = dispatcher + override val io = dispatcher +}