Skip to content

Conversation

tomaszrybakiewicz
Copy link
Contributor

@tomaszrybakiewicz tomaszrybakiewicz commented Dec 16, 2022

Closes NAVAND-1001

Description

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 API and Player cancellation alongside the parent Job.

…ls 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.
@tomaszrybakiewicz tomaszrybakiewicz added the bug Defect to be fixed. label Dec 16, 2022
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that I tested this downstream in 1TAP with a SNAPSHOT and can't reproduce NAVAND-1001 🎉

@dzinad
Copy link
Contributor

dzinad commented Dec 19, 2022

It does not fix the root cause of instruction being played twice, although it does reduce the probability of such a situation.
The problem is that we first start speaking a voice instruction with language=en and MapboxVoiceAudioGuidance#1 and only then the language changes to "en-US" and we cancel everything and create MapboxVoiceAudioGuidance#2.
The issue is not reproduced because in the conditions that we test the cancellation starts at the moment when we generate voice instruction. But if the timings are less fortunate and the cancellation starts later, we may run into the situation where we start pronouncing the instruction, then cancel it and start from the beginning.
I can reproduce it by adding a delay of 700-900 here. I know that right now it sounds artificial. But what will happen when we add pre-downloading? Suppose the instruction is already in the cache. Maybe in the memory cache even. The generate will return much faster. And less delay will be needed to reproduce the situation. So we will be at risk of running into this bug again.

@dzinad
Copy link
Contributor

dzinad commented Dec 20, 2022

BTW, soon we might want to interrupt the instruction if the new one arrives (https://mapbox.atlassian.net/browse/NAVSDK-775).

@dzinad
Copy link
Contributor

dzinad commented Dec 20, 2022

I've opened a draft PR that would fix the issue that a described here: #6766.
It contains the commit from this PR so look review only the newest commit. I didn't want to push my suggestion here so I had to duplicate the commit.
@Guardiola31337 @tomaszrybakiewicz

@Guardiola31337
Copy link
Contributor

@dzinad

I've opened a draft PR that would fix the issue that a described here: #6766.
It contains the commit from this PR so look review only the newest commit. I didn't want to push my suggestion here so I had to duplicate the commit.

Looking good to me, I've tested downstream in 1TAP with a SNAPSHOT from #6766 and it's working as expected - same first voice instruction is not played twice anymore. I believe we can go ahead and continue in #6766 🚀

cc @tomaszrybakiewicz

@@ -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)
Copy link
Contributor

@Zayankovsky Zayankovsky Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you emit a flow with a single value, you can use a simple map instead of flatMapConcat.
After that you can replace two consecutive maps with one map.

@tomaszrybakiewicz
Copy link
Contributor Author

Closing in favor of #6766

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defect to be fixed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants