From 915b7d633a06216155f9a92e806d8fef35c9e186 Mon Sep 17 00:00:00 2001 From: Tomasz Rybakiewicz Date: Wed, 14 Dec 2022 15:11:45 -0500 Subject: [PATCH 1/2] Updated MapboxAudioGuidanceVoice to use coroutines to synchronize calls to both MapboxSpeechApi and MapboxVoiceInstructionsPlayer. As a result, MapboxAudioGuidanceVoice will wait until the player finishes playing the previous announcement before attempting to play the next one. Using coroutines also allows for API and Player cancellation alongside the parent Job. --- .../ui/voice/api/MapboxAudioGuidance.kt | 4 +- .../internal/MapboxAudioGuidanceVoice.kt | 70 ++++++++++--------- .../voice/TestMapboxAudioGuidanceServices.kt | 13 ++-- .../impl/MapboxAudioGuidanceVoiceTest.kt | 63 ++++++++++++----- 4 files changed, 90 insertions(+), 60 deletions(-) diff --git a/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidance.kt b/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidance.kt index d001fc90355..029a87c7142 100644 --- a/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidance.kt +++ b/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidance.kt @@ -27,6 +27,7 @@ import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flatMapConcat import kotlinx.coroutines.flow.flatMapLatest +import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.updateAndGet import kotlinx.coroutines.launch @@ -147,7 +148,8 @@ internal constructor( .filter { it.voiceInstructions != lastPlayedInstructions } .flatMapConcat { lastPlayedInstructions = it.voiceInstructions - audioGuidance.speak(it.voiceInstructions) + val announcement = audioGuidance.speak(it.voiceInstructions) + flowOf(announcement) } .map { speechAnnouncement -> internalStateFlow.updateAndGet { diff --git a/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/internal/MapboxAudioGuidanceVoice.kt b/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/internal/MapboxAudioGuidanceVoice.kt index 9bfa672ddbc..b6e13c1443e 100644 --- a/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/internal/MapboxAudioGuidanceVoice.kt +++ b/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/internal/MapboxAudioGuidanceVoice.kt @@ -1,20 +1,13 @@ package com.mapbox.navigation.ui.voice.internal import com.mapbox.api.directions.v5.models.VoiceInstructions -import com.mapbox.bindgen.Expected -import com.mapbox.navigation.ui.base.util.MapboxNavigationConsumer import com.mapbox.navigation.ui.voice.api.MapboxSpeechApi import com.mapbox.navigation.ui.voice.api.MapboxVoiceInstructionsPlayer import com.mapbox.navigation.ui.voice.model.SpeechAnnouncement -import com.mapbox.navigation.ui.voice.model.SpeechError -import com.mapbox.navigation.ui.voice.model.SpeechValue -import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.channels.awaitClose -import kotlinx.coroutines.channels.onFailure -import kotlinx.coroutines.channels.onSuccess -import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.callbackFlow -import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.withContext +import kotlin.coroutines.resume /** * Controls voice guidance for the car. @@ -26,35 +19,44 @@ class MapboxAudioGuidanceVoice( private val mapboxSpeechApi: MapboxSpeechApi, private val mapboxVoiceInstructionsPlayer: MapboxVoiceInstructionsPlayer ) { - fun speak(voiceInstructions: VoiceInstructions?): Flow { + /** + * Load and play [SpeechAnnouncement]. + * This method will suspend until announcement finishes playback. + */ + suspend fun speak(voiceInstructions: VoiceInstructions?): SpeechAnnouncement? { return if (voiceInstructions != null) { - speechFlow(voiceInstructions) + val announcement = mapboxSpeechApi.generate(voiceInstructions) + try { + mapboxVoiceInstructionsPlayer.play(announcement) + announcement + } finally { + withContext(NonCancellable) { + mapboxSpeechApi.clean(announcement) + } + } } else { mapboxSpeechApi.cancel() mapboxVoiceInstructionsPlayer.clear() - flowOf(null) + null } } - @OptIn(ExperimentalCoroutinesApi::class) - private fun speechFlow(voiceInstructions: VoiceInstructions): Flow = - callbackFlow { - val speechCallback = - MapboxNavigationConsumer> { value -> - val speechAnnouncement = value.value?.announcement ?: value.error!!.fallback - mapboxVoiceInstructionsPlayer.play(speechAnnouncement) { - mapboxSpeechApi.clean(it) - trySend(speechAnnouncement).onSuccess { - close() - }.onFailure { - close() - } - } - } - mapboxSpeechApi.generate(voiceInstructions, speechCallback) - awaitClose { - mapboxSpeechApi.cancel() - mapboxVoiceInstructionsPlayer.clear() - } + private suspend fun MapboxSpeechApi.generate( + instructions: VoiceInstructions + ): SpeechAnnouncement = suspendCancellableCoroutine { cont -> + generate(instructions) { value -> + val announcement = value.value?.announcement ?: value.error!!.fallback + cont.resume(announcement) } + cont.invokeOnCancellation { cancel() } + } + + private suspend fun MapboxVoiceInstructionsPlayer.play( + announcement: SpeechAnnouncement + ): SpeechAnnouncement = suspendCancellableCoroutine { cont -> + play(announcement) { + cont.resume(announcement) + } + cont.invokeOnCancellation { clear() } + } } diff --git a/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/TestMapboxAudioGuidanceServices.kt b/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/TestMapboxAudioGuidanceServices.kt index ede31eef73a..8f694ec2c70 100644 --- a/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/TestMapboxAudioGuidanceServices.kt +++ b/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/TestMapboxAudioGuidanceServices.kt @@ -8,13 +8,13 @@ import com.mapbox.navigation.ui.voice.internal.MapboxVoiceInstructionsState import com.mapbox.navigation.ui.voice.internal.impl.MapboxAudioGuidanceServices import com.mapbox.navigation.ui.voice.model.SpeechAnnouncement import io.mockk.Runs +import io.mockk.coEvery import io.mockk.every import io.mockk.just import io.mockk.mockk import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.flowOf -import kotlinx.coroutines.flow.onEach class TestMapboxAudioGuidanceServices( private val deviceLanguage: String = "en" @@ -34,7 +34,7 @@ class TestMapboxAudioGuidanceServices( } private val mapboxAudioGuidanceVoice = mockk { - every { speak(any()) } answers { + coEvery { speak(any()) } coAnswers { val voiceInstructions = firstArg() val speechAnnouncement: SpeechAnnouncement? = voiceInstructions?.let { mockk { @@ -42,12 +42,11 @@ class TestMapboxAudioGuidanceServices( every { ssmlAnnouncement } returns it.ssmlAnnouncement() } } - flowOf(speechAnnouncement).onEach { - if (it != null) { - // Simulate a real speech announcement by delaying the TestCoroutineScope - delay(SPEECH_ANNOUNCEMENT_DELAY_MS) - } + if (speechAnnouncement != null) { + // Simulate a real speech announcement by delaying the TestCoroutineScope + delay(SPEECH_ANNOUNCEMENT_DELAY_MS) } + speechAnnouncement } } diff --git a/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/internal/impl/MapboxAudioGuidanceVoiceTest.kt b/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/internal/impl/MapboxAudioGuidanceVoiceTest.kt index 00b1da07f51..3c8c2d90e62 100644 --- a/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/internal/impl/MapboxAudioGuidanceVoiceTest.kt +++ b/libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/internal/impl/MapboxAudioGuidanceVoiceTest.kt @@ -15,7 +15,8 @@ import io.mockk.every import io.mockk.mockk import io.mockk.verify import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.flow.collect +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch import org.junit.Assert.assertEquals import org.junit.Rule import org.junit.Test @@ -27,28 +28,28 @@ class MapboxAudioGuidanceVoiceTest { val coroutineRule = MainCoroutineRule() private val speechApi = mockk(relaxUnitFun = true) - private val voiceInstructionsPlayer = mockk(relaxUnitFun = true) - private val carAppAudioGuidanceVoice = MapboxAudioGuidanceVoice( + private val voiceInstructionsPlayer = mockk(relaxed = true) + private val sut = MapboxAudioGuidanceVoice( speechApi, voiceInstructionsPlayer ) @Test - fun `voice instruction should be played as SpeechAnnouncement`() = coroutineRule.runBlockingTest { - mockSuccessfulSpeechApi() - mockSuccessfulVoiceInstructionsPlayer() + fun `voice instruction should be played as SpeechAnnouncement`() = + coroutineRule.runBlockingTest { + mockSuccessfulSpeechApi() + mockSuccessfulVoiceInstructionsPlayer() - val voiceInstructions = mockk { - every { announcement() } returns "Turn right on Market" - } - carAppAudioGuidanceVoice.speak(voiceInstructions).collect { speechAnnouncement -> + val voiceInstructions = mockk { + every { announcement() } returns "Turn right on Market" + } + val speechAnnouncement = sut.speak(voiceInstructions) assertEquals("Turn right on Market", speechAnnouncement!!.announcement) } - } @Test fun `null should clean up the api and player`() = coroutineRule.runBlockingTest { - carAppAudioGuidanceVoice.speak(null).collect() + sut.speak(null) verify { speechApi.cancel() } verify { voiceInstructionsPlayer.clear() } @@ -70,18 +71,44 @@ class MapboxAudioGuidanceVoiceTest { val voiceInstructions = mockk { every { announcement() } returns "This message fails" } - carAppAudioGuidanceVoice.speak(voiceInstructions).collect { speechAnnouncement -> - assertEquals("Turn right on Market", speechAnnouncement!!.announcement) - } + val speechAnnouncement = sut.speak(voiceInstructions) + assertEquals("Turn right on Market", speechAnnouncement!!.announcement) } + @Test + fun `should wait until previous instruction finishes playback before playing next one`() = + coroutineRule.runBlockingTest { + mockSuccessfulSpeechApi() + every { voiceInstructionsPlayer.play(any(), any()) } answers { + launch { + val speechAnnouncement = firstArg() + delay(1000) // simulate 1 second announcement playback duration + secondArg>() + .accept(speechAnnouncement) + } + Unit + } + + val played = mutableListOf() + launch { + listOf( + VoiceInstructions.builder().announcement("A").build(), + VoiceInstructions.builder().announcement("B").build() + ).forEach { + val announcement = sut.speak(it) // suspend until playback finishes + played.add(announcement) + } + } + advanceTimeBy(1500) // advance time to 50% of announcement B playback time + + assertEquals(1, played.size) + } + private fun mockSuccessfulSpeechApi() { every { speechApi.generate(any(), any()) } answers { val announcementArg = firstArg().announcement() val speechValue = mockk { - every { announcement } returns mockk { - every { announcement } returns announcementArg!! - } + every { announcement } returns SpeechAnnouncement.Builder(announcementArg!!).build() } val consumer = secondArg>>() consumer.accept(ExpectedFactory.createValue(speechValue)) From 22804e69b55d59c0b437dc8007f9b5cff0094992 Mon Sep 17 00:00:00 2001 From: Dzina Dybouskaya Date: Wed, 21 Dec 2022 16:43:26 +0300 Subject: [PATCH 2/2] NAVAND-1001: use immediate dispatcher to guarantee that language will change before voice instructions --- CHANGELOG.md | 1 + .../ui/voice/api/MapboxAudioGuidance.kt | 25 ++++++++----------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ada4f316fd2..461ebfc05b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ Mapbox welcomes participation and contributions from everyone. - Fixed standalone `MapboxManeuverView` appearance when the app also integrates Drop-In UI. [#6774](https://github.com/mapbox/mapbox-navigation-android/pull/6774) - Introduced `NavigationViewListener.onSpeedInfoClicked` that would be triggered when `MapboxSpeedInfoView` is clicked upon. [#6770](https://github.com/mapbox/mapbox-navigation-android/pull/6770) - Each newly instantiated MapboxRouteArrowView class will initialize the layers with the provided options on the first render call. Previously this would only be done if the layers hadn't already been initialized. [#6466](https://github.com/mapbox/mapbox-navigation-android/pull/6466) +- Fixed an issue where the first voice instruction might have been played twice. [#6766](https://github.com/mapbox/mapbox-navigation-android/pull/6766) ## Mapbox Navigation SDK 2.10.0-rc.1 - 16 December, 2022 ### Changelog diff --git a/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidance.kt b/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidance.kt index 029a87c7142..b438d9ef4ad 100644 --- a/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidance.kt +++ b/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidance.kt @@ -14,7 +14,6 @@ import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.flow.Flow @@ -25,9 +24,7 @@ import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.first -import kotlinx.coroutines.flow.flatMapConcat import kotlinx.coroutines.flow.flatMapLatest -import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.updateAndGet import kotlinx.coroutines.launch @@ -123,7 +120,7 @@ internal constructor( /** * Top level flow that will switch based on the language and muted state. */ - @OptIn(ExperimentalCoroutinesApi::class, FlowPreview::class) + @OptIn(ExperimentalCoroutinesApi::class) private fun audioGuidanceFlow( mapboxNavigation: MapboxNavigation ): Flow { @@ -146,18 +143,15 @@ internal constructor( } else { voiceInstructions .filter { it.voiceInstructions != lastPlayedInstructions } - .flatMapConcat { + .map { lastPlayedInstructions = it.voiceInstructions val announcement = audioGuidance.speak(it.voiceInstructions) - flowOf(announcement) - } - .map { speechAnnouncement -> - internalStateFlow.updateAndGet { + internalStateFlow.updateAndGet { state -> MapboxAudioGuidanceState( - isPlayable = it.isPlayable, - isMuted = it.isMuted, - voiceInstructions = it.voiceInstructions, - speechAnnouncement = speechAnnouncement + isPlayable = state.isPlayable, + isMuted = state.isMuted, + voiceInstructions = state.voiceInstructions, + speechAnnouncement = announcement ) } } @@ -195,7 +189,10 @@ internal constructor( * Construct an instance without registering to [MapboxNavigationApp]. */ @JvmStatic - fun create() = MapboxAudioGuidance(MapboxAudioGuidanceServices(), Dispatchers.Main) + fun create() = MapboxAudioGuidance( + MapboxAudioGuidanceServices(), + Dispatchers.Main.immediate + ) /** * Get the registered instance or create one and register it to [MapboxNavigationApp].