NAVAND-552: predownload voice instructions

[dzinad]

Iteration 2 of #6547.

[codecov]

Codecov Report

Merging #6771 (1086a78) into main (6fc4e70) will increase coverage by 0.10%.
The diff coverage is 97.08%.

❗ Current head 1086a78 differs from pull request most recent head 594011f. Consider uploading reports for the commit 594011f to get more accurate results

@@             Coverage Diff              @@
##               main    #6771      +/-   ##
============================================
+ Coverage     72.64%   72.74%   +0.10%     
- Complexity     5564     5607      +43     
============================================
  Files           781      782       +1     
  Lines         30116    30227     +111     
  Branches       3561     3574      +13     
============================================
+ Hits          21878    21989     +111     
- Misses         6810     6812       +2     
+ Partials       1428     1426       -2     

Impacted Files	Coverage Δ
.../java/com/mapbox/navigation/utils/internal/Time.kt	0.00% <0.00%> (ø)
...mapbox/navigation/ui/voice/api/VoiceApiProvider.kt	0.00% <ø> (ø)
...box/navigation/ui/voice/api/MapboxAudioGuidance.kt	86.44% <100.00%> (+1.69%)	⬆️
.../mapbox/navigation/ui/voice/api/MapboxSpeechApi.kt	94.11% <100.00%> (+6.88%)	⬆️
...ox/navigation/ui/voice/api/MapboxSpeechProvider.kt	86.04% <100.00%> (+4.46%)	⬆️
...m/mapbox/navigation/ui/voice/api/MapboxVoiceApi.kt	94.73% <100.00%> (-5.27%)	⬇️
...navigation/ui/voice/api/VoiceInstructionsParser.kt	100.00% <100.00%> (ø)
...gation/ui/voice/api/VoiceInstructionsPrefetcher.kt	100.00% <100.00%> (ø)
...tion/ui/voice/internal/MapboxAudioGuidanceVoice.kt	100.00% <100.00%> (ø)
... and 4 more

[VysotskiVadim]

With the current API user can:

1. Register anything what implements RoutesObserver and RouteProgressObserver as a VoiceInstructionsTrigger. It's getting challenging to change observers from which predownload logic takes data as users could start using this registration for a different purpose.
2. Call VoiceInstructionsDownloadTrigger#onRouteProgressChange and VoiceInstructionsDownloadTrigger#onRoutesChanged that also limit our abilities in future changes as if we need more data for the new algorithm, we will have to support old usage despite the fact it's not right.

What do you think about hiding those implementation details from the user so that users can't use the API incorrectly + we are able to change it in the future? What if you implement public API as MapboxNavigationObserver with a single callback predownloadVoiceInstruction? This way API surface will be less, i.e. users has less changes to use it wrong + we have more freedom modifying it in the future.

Suggested change

	/**
	* Subscribes voice instructions trigger observer for all the necessary updates.
	*/
	@ExperimentalPreviewMapboxNavigationAPI
	fun <T> registerVoiceInstructionsTriggerObserver(
	observer: T
	) where T : RoutesObserver, T : RouteProgressObserver {
	registerRoutesObserver(observer)
	registerRouteProgressObserver(observer)
	}
	/**
	* Unsubscribes voice instructions trigger observer from all the updates it was subsribed for.
	*/
	@ExperimentalPreviewMapboxNavigationAPI
	fun <T> unregisterVoiceInstructionsTriggerObserver(
	observer: T
	) where T : RoutesObserver, T : RouteProgressObserver {
	unregisterRoutesObserver(observer)
	unregisterRouteProgressObserver(observer)
	}

[VysotskiVadim]

Oh, basically the same as https://github.com/mapbox/mapbox-navigation-android/pull/6771/files#r1056539683
I am not resolving the conversation because it describes motivation for the same change from a different perspective. But let's discuss it in the Kyle's comment as he suggested it first.

[VysotskiVadim]

I still don't understand purpose of those tests 🙂
Do you think that something could go wrong in the implementation:

override fun millis(): Long = System.currentTimeMillis()

?

[dzinad]

"Anything that can go wrong will go wrong". :)
I'm not saying these are very important tests. It's more of a habit: I try to test all my code when possible.
Idk, you can simply copy-paste something carelessly and end up with an implementation like:

override fun millis(): Long = System. nanoTime()

Again, not very probable but since it's just a unit test I think it can't hurt.

[kmadsen]

agreeing with vadzim here. these tests are not needed.

It is also weird to make this a unit test, when they are really instrumentation tests. They are testing the time system calls. With unit tests, you mock deterministic values.

This unit test does not give any value, and it's essentially what the tests are doing if they are unit tests.

every { System.nanoTime } returns 100L
every { Time.SystemImpl.nanoTime() } returns 100L

assertEquals(System.nanoTime, Time.SystemImpl.nanoTime())

[kmadsen]

If you want to unit test the logic of SystemImpl, it would look something like this

@Test
fun `nanoTime returns System.nanoTime`() {
  every { System.nanoTime() } returns 3999999999L

  assertEquals(3999999999L, SystemImpl.nanoTime())
}

@Test
fun `millis returns System.currentTimeMillis`() {
  every { SystemClock.currentTimeMillis() } returns 3999L

  assertEquals(3999L, SystemImpl.millis())
}

@Test
fun `seconds converts System.currentTimeMillis to seconds`() {
  every { SystemClock.currentTimeMillis() } returns 3999L

  assertEquals(3, SystemImpl.seconds())
}

The tests make it more obvious, what to expect.

[dzinad]

Sure, that would be better but System class can't be mocked unfortunately. That's why I chose this approach.
But if you are strongly against it, I'll remove the tests.

[kmadsen]

There were reasons for us to prefer SystemClock over the System calls. They are actually different clocks and nav native uses SystemClock for monotonic location timestamps. Was there a reason you were preferring the System clock? I'm looking for discussions but found this comment https://github.com/mapbox/navigation-sdks/issues/636#issuecomment-691239489

Also, SystemClock has some existing tests that are mocking it

mapbox-navigation-android/libnavigation-core/src/test/java/com/mapbox/navigation/core/replay/history/MapboxReplayerTest.kt

Lines 576 to 592 in ce11135

	@Before
	fun setup() {
	mockkStatic(SystemClock::class)
	every { SystemClock.elapsedRealtimeNanos() } returns deviceElapsedTimeNanos
	}
	@After
	fun teardown() {
	unmockkObject(SystemClock.elapsedRealtimeNanos())
	}
	private fun advanceTimeMillis(advanceMillis: Long) {
	deviceElapsedTimeNanos += TimeUnit.MILLISECONDS.toNanos(advanceMillis)
	every { SystemClock.elapsedRealtimeNanos() } returns deviceElapsedTimeNanos
	coroutineRule.testDispatcher.advanceTimeBy(advanceMillis)
	}
	}

[kmadsen]

but System class can't be mocked unfortunately

IMO this is a good reason to not add any logic to the this SystemImpl. Have a wrapper class that can be mockked, and make other classes have the tested logic. (we should also rename SystemImpl if we plan to use this more, like IClock or ClockWrapper).

[dzinad]

I didn't know there was a preference of SystemClock over System. Both can be encountered across the project code base. Here I only need it to check the time difference. There's absolutely no need to think it with NN.
I've switched to SystemClock elapsed time here. I think it makes sense.

[Zayankovsky]

According to SystemClock docs the main difference between System.nanoTime and SystemClock.elapsedRealtime(Nanos) is that the latter includes time spent in power saving modes, so it may be more preferable.

[VysotskiVadim]

We had this topic during the first iteration, but I still haven't got how it works. Sorry, I'm asking it again 🙂
How are the predownloaded instructions cleaned up if user interrupts the ride before the downloaded instructions are played?

For example:

1. User sets a different route
2. Reroute happened
3. OS killed the navigation application

[dzinad]

TLDR: basically they won't be cleaned. Just the way the played ones won't be cleaned.
We only clean the files that we create ourselves. However, we don't clean any caches on TileStore side. And we can't. Capturing from a Slack thread:

Resource cache is not cleared explicitly. TileStore has a unified quota for tiles and resources and will evict individual items when the quota is exceeded. Resources (including tiles) with the closest expiration date are removed until the quota is not exceeded anymore.

[VysotskiVadim]

Just the way the played ones won't be cleaned.

I thought this cleans played voice instructions, doesn't it?

[dzinad]

It only cleans the files created here:

mapbox-navigation-android/libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxVoiceApi.kt

Line 27 in ca5276b

val file = speechFileProvider.generateVoiceFileFrom(blob.inputStream())

Predownloading does not create this file.

[VysotskiVadim]

Hm, interesting 🤔
What if we have a long route, with predictive cache controller enabled (downloads tiles for the whole route ahead), and a tile store quota which is not enough to fit voice instructions and tiles for the whole route? Can downloaded voice instructions be evicted in favour of tiles before they are played? How do you think, is that scenario possible?

[dzinad]

Can downloaded voice instructions be evicted in favour of tiles before they are played?

Right now not because they are being downloaded on-demand. After this PR - well, I think that is theoretically possible. I know that they have cache clearing mechanism planned but it's not available yet and I don't think that this is expected very soon.
Thanks for your observation, I think it's important so I'll need to understand it better. I'll talk to the Core SDK team again and run some tests.

[dzinad]

You're right. I was able to reproduce it in an easier way: if I set a small quota (TileStore#setOptions), some voice instructions may be removed because of the new ones, although the old ones have not yet been played.

[dzinad]

The other problem is that we won't know when the resource was removed from file cache by Core SDK so we might think it's downloaded although it's not.
I'll try to think of a way to handle it.

[dzinad]

I've added a separate commit to address this issue.

[VysotskiVadim]

Continuation of: #6547 (comment)

the Core SDK they won't start multiple identical requests.

Will it download again the instruction which was already downloaded?

[dzinad]

No, it will return the one from cache.

[VysotskiVadim]

why do you need downloadedInstructions then? Core SDK already handles everything, isn't it?

[dzinad]

It does. It's just an optimization on our side to avoid forming requests and accessing TileStore when not needed. And that might happen quite often because of the way the predownloading is triggered. Example:

1. trigger download of [A, B, C, D]
2. ...
3. trigger download of [D, E, F, G]
4. ...
5. trigger download of [F, G, H, I]

Triggering is based on timer and with the default settings the instructions to predownload will overlap often enough. And the settings are such because we want to avoid starting the download too late.

[VysotskiVadim]

Continuation of: #6547 (comment)

But then the requests won't be run in parallel?

They will be in parallel if you run new coroutine per voice instruction:

launch {
    predownload(typeAndAnnouncement: TypeAndAnnouncement)
}

[dzinad]

Oh, I see what you mean. Yes, I don't need currentRequests then, thanks.

[tomaszrybakiewicz]

@dzinad Tons of great ideas here. However, we should only push a little work on developers to make this work.

The integration of this feature seems too complicated:

• developers need to instantiate a new class with the correct SpeachApi instance
• remember to attach/detach to the MapboxNavigaiton instance
• pay close attention to calling correct SpeachApi methods

This setup is too brittle and introduces too many areas where things can go wrong. Let's simplify it by hiding the VoiceInstructionsPrefetcher class and aim to make the integration as easy as flipping a boolean flag or setting a load strategy(SingleInstructionLoadStrategy, PrefetchInstructionsLoadStrategy etc.) via MapboxSpeechApiOptions that will control how instructions are loaded.

[dzinad]

This setup is too brittle and introduces too many areas where things can go wrong. Let's simplify it by hiding the VoiceInstructionsPrefetcher class and aim to make the integration as easy as flipping a boolean flag or setting a load strategy(SingleInstructionLoadStrategy, PrefetchInstructionsLoadStrategy etc.) via MapboxSpeechApiOptions that will control how instructions are loaded.

The initial solution (#6547) did not require developers to do anything.
But it was discussed that doing something to enable predownloading is expected if you use low-level APIs. I think that the current solution fits quite well into the low-level API paradigm that we have.
You have instances of voiceInstructionsPlayer, speechApi, voiceInstructionsPlayerCallback, speechCallback, voiceInstructionsObserver. You bind them together to observe, play and cleanup the instructions and you register some of them as specific observers. That's exactly the way with the new voiceInstructionsPrefetcher.
We could go your way with a strategy or something. It's a nice way. But I don't see why it's much better for the current voice instructions approach that we have (reasons described above).

I'd also like to point out that additional work is required only if you use low-level API. If you use MapboxAudioGuidance, you're all set.

[github-actions]

Changelog

Features

Bug fixes and improvements

• Introduced VoiceInstructionsPrefetcher MapboxSpeechAPI#generatePredownloaded to use predownloaded voice instructions instead of downloading them on demand. Example usage can be found in the examples directory ( see MapboxVoiceActivity). #dd
• Enabled voice instructions predownloading for those who use MapboxAudioGuidance. #dd
• Fixed an issue where with low connectivity voice instruction might have been played too late for those who use MapboxAudioGuidance. If you use MapboxSpeechAPI directly, switch to voice instructions predownloading as described above if you encounter said issue. #dd
• Improved NavigationView camera behavior to go back into overview state if routes change during route preview state. #6840

Known issues ⚠️

Other changes

Android Auto Changelog

Features

Bug fixes and improvements

[dzinad]

I did a final round of testing.
@kmadsen @VysotskiVadim @Zayankovsky if you want to finish your review or rereview, please do. I'd like to merge it into this week's alpha since it's quite a big change so I want it properly tested.