NAVAND-552: predownload voice instructions #6547
Conversation
Codecov Report
|
dzinad
force-pushed
the
NAVAND-552-dd-predownloading2
branch
from
e330118 to
116f136
Compare
dzinad
force-pushed
the
NAVAND-552-dd-predownloading2
branch
from
116f136 to
b1b8c24
Compare
Guardiola31337
left a comment
Guardiola31337
left a comment
There was a problem hiding this comment.
Left a couple of initial comments. Will continue reviewing the implementation details thoroughly.
| fun onCreated(mapboxNavigation: MapboxNavigation) | ||
| } | ||
|
|
||
| @Deprecated("Used to keep track of MapboxNavigation instances created via deprecated methods") |
There was a problem hiding this comment.
This is weird, why are we adding a new @Deprecated object? 🤔
There was a problem hiding this comment.
I have to register RoutesObserver and RouteProgressObserver. And there are multiple ways of creating MapboxNavigation :
- constructor
MapboxNavigationProvider(deprecated)MapboxNavigationApp
The "correct" way of observingMapboxNavigationis usingMapboxNavigationAppbut the other ways are widely used and I want the feature to work for all the users. SoLegacyMapboxNavigationInstanceHoldertracks allMapboxNavigationinstances. Not sure if the naming is very good but I wanted to emphasize by this that theMapboxNavigationAppis the appropriate way to do it andLegacyMapboxNavigationInstanceHolderis just for us to support other creation methods.
There was a problem hiding this comment.
LegacyMapboxNavigationInstanceHolder is an internal object. User aren't supposed to use it anyway, are they?
There was a problem hiding this comment.
They aren't. It's just a note for us. I can just add a comment or sth instead of deprecating it.
But actually I'm thinking that it should not be deprecated at all. We still expose a valid way of creating MapboxNavigation via constructor and some customers use it. So it's not really "legacy", it's more like "everything including legacy". I just don't like it that we have MapboxNavigationApp and this new way of observing. But since the new one is internal, it's not that big a deal.
There was a problem hiding this comment.
LegacyMapboxNavigationInstanceHolder is definitely not an improvement to MapboxNavigationApp though, in my opinion.
MapboxNavigationCreateObserver is also redundant.. It is the same as MapboxNavigationObserver.onAttached.
There was a problem hiding this comment.
have to register RoutesObserver
This is not a reason to require an automatic link. It can be done after MapboxNavigation is constructed
| 3 * 60, | ||
| 0.5 |
There was a problem hiding this comment.
Where are these magic numbers coming from? 🤔
There was a problem hiding this comment.
These are just the defaults. Switched to using named constants.
dzinad
force-pushed
the
NAVAND-552-dd-predownloading2
branch
from
b1b8c24 to
41e81a7
Compare
|
Noting that Ktlint is failing https://app.circleci.com/pipelines/github/mapbox/mapbox-navigation-android/22526/workflows/3a278636-caea-4a82-8eb0-11b8eda6b597/jobs/119244 👀 |
|
I'm pretty sure I'll be redoing some stuff here so I don't see any point in fixing ktlint just yet. :) |
Just flagging ✌️ |
dzinad
force-pushed
the
NAVAND-552-dd-predownloading2
branch
from
41e81a7 to
660d8ec
Compare
| mockkObject(LegacyMapboxNavigationInstanceHolder) { | ||
| every { LegacyMapboxNavigationInstanceHolder.peek() } returns mockk(relaxed = true) | ||
| createMapboxNavigation() | ||
| verify(exactly = 0) { | ||
| LegacyMapboxNavigationInstanceHolder.onCreated(any()) | ||
| } | ||
| } |
There was a problem hiding this comment.
will it test the same?
| mockkObject(LegacyMapboxNavigationInstanceHolder) { | |
| every { LegacyMapboxNavigationInstanceHolder.peek() } returns mockk(relaxed = true) | |
| createMapboxNavigation() | |
| verify(exactly = 0) { | |
| LegacyMapboxNavigationInstanceHolder.onCreated(any()) | |
| } | |
| } | |
| createMapboxNavigation() | |
| createMapboxNavigation() |
There was a problem hiding this comment.
How? There is not verification that onCreated was not called the second time.
| mockkObject(LegacyMapboxNavigationInstanceHolder) { | ||
| createMapboxNavigation() | ||
| mapboxNavigation.onDestroy() | ||
| verify { LegacyMapboxNavigationInstanceHolder.onDestroyed() } | ||
| } |
There was a problem hiding this comment.
What do you think is more important for us:
MapboxNavigationcallsLegacyMapboxNavigationInstanceHolder.onDestroyed()during destroy- User can create a new instance of
MapboxNavigationwhen the old one is destroyed
?
Second statement seems more important to me, so I would test it instead
| mockkObject(LegacyMapboxNavigationInstanceHolder) { | |
| createMapboxNavigation() | |
| mapboxNavigation.onDestroy() | |
| verify { LegacyMapboxNavigationInstanceHolder.onDestroyed() } | |
| } | |
| createMapboxNavigation() | |
| mapboxNavigation.onDestroy() | |
| createMapboxNavigation() |
There was a problem hiding this comment.
Your test would have passed even without the LegacyMapboxNavigationInstanceHolder...
What I want to test here is that LegacyMapboxNavigationInstanceHolder will be cleared. And what happens when it's cleared is tested in LegacyMapboxNavigationInstanceHolderTest.
| @Test | ||
| fun millis() { | ||
| val tolerance = 100 | ||
| val diff = abs(System.currentTimeMillis() - Time.SystemImpl.millis()) | ||
| assertTrue(diff < tolerance) | ||
| } | ||
|
|
||
| @Test | ||
| fun nanoTime() { | ||
| val tolerance = 100000000 | ||
| val diff = abs(System.nanoTime() - Time.SystemImpl.nanoTime()) | ||
| assertTrue(diff < tolerance) | ||
| } |
There was a problem hiding this comment.
Why do you need those tests?
libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxSpeechLoader.kt
Show resolved
Hide resolved
| synchronized(downloadedInstructionsLock) { | ||
| return typeAndAnnouncement in downloadedInstructions | ||
| } |
There was a problem hiding this comment.
In which scenario this lock can help you?
It seems that there is a long time window during downloading of a voice instruction when the same voice instructions could be requested many times. I'm wondering if that locking makes sense 🤔
There was a problem hiding this comment.
I guess this lock just to synchronise access from many threads: thread pool from default dispatches + tilestore thread.
Does it make sense to use some concurrent collections like ConcurrentHashSet ?
There was a problem hiding this comment.
Anyway it seems that this check won't help if the same instruction will be requested many times before the first one is downloaded
There was a problem hiding this comment.
I guess this lock just to synchronise access from many threads: thread pool from default dispatches + tilestore thread.
Yes.
Does it make sense to use some concurrent collections like ConcurrentHashSet ?
It does, but there is no such collection. :) We could work that around by using ConcurrentHashMap but I don't see why it's better than just a lock. If you see any reasons for it, I can easily change it.
Anyway it seems that this check won't help if the same instruction will be requested many times before the first one is downloaded
It won't. But on the Core SDK they won't start multiple identical requests. I don't want to update the collection right after the request has started because what if it does not succeed? Then we would like to retry.
| private fun predownload(typeAndAnnouncement: TypeAndAnnouncement) { | ||
| try { | ||
| val request = createRequest(typeAndAnnouncement) | ||
| var id: Long? = null | ||
| id = resourceLoader.load(request) { result -> | ||
| id?.let { currentRequests.remove(it) } | ||
| // tilestore thread | ||
| if (result.isValue) { | ||
| synchronized(downloadedInstructionsLock) { | ||
| downloadedInstructions.add(typeAndAnnouncement) | ||
| } | ||
| } | ||
| } | ||
| currentRequests.add(id) | ||
| } catch (ex: Throwable) { | ||
| logE("Failed to download instruction '$typeAndAnnouncement': ${ex.localizedMessage}") | ||
| } | ||
| } |
There was a problem hiding this comment.
You wouldn't need to keep currentRequests if predownload was suspend function like this:
| private fun predownload(typeAndAnnouncement: TypeAndAnnouncement) { | |
| try { | |
| val request = createRequest(typeAndAnnouncement) | |
| var id: Long? = null | |
| id = resourceLoader.load(request) { result -> | |
| id?.let { currentRequests.remove(it) } | |
| // tilestore thread | |
| if (result.isValue) { | |
| synchronized(downloadedInstructionsLock) { | |
| downloadedInstructions.add(typeAndAnnouncement) | |
| } | |
| } | |
| } | |
| currentRequests.add(id) | |
| } catch (ex: Throwable) { | |
| logE("Failed to download instruction '$typeAndAnnouncement': ${ex.localizedMessage}") | |
| } | |
| } | |
| private suspend fun predownload(typeAndAnnouncement: TypeAndAnnouncement) { | |
| try { | |
| suspendCancellableCoroutine<Unit> { continuation -> | |
| val request = createRequest(typeAndAnnouncement) | |
| var id: Long? = null | |
| id = resourceLoader.load(request) { result -> | |
| // tilestore thread | |
| if (result.isValue) { | |
| continuation.resume(Unit) | |
| } | |
| } | |
| continuation.invokeOnCancellation { | |
| resourceLoader.cancel(id) | |
| } | |
| } | |
| synchronized(downloadedInstructionsLock) { | |
| downloadedInstructions.add(typeAndAnnouncement) | |
| } | |
| } catch (ex: Throwable) { | |
| logE("Failed to download instruction '$typeAndAnnouncement': ${ex.localizedMessage}") | |
| } | |
| } |
There was a problem hiding this comment.
But then the requests won't be run in parallel?
| val typeAndAnnouncement = VoiceInstructionsParser.parse(voiceInstruction) | ||
| .getValueOrElse { throw it } | ||
| val request = createRequest(typeAndAnnouncement).apply { | ||
| networkRestriction = NetworkRestriction.DISALLOW_ALL | ||
| } | ||
| val response = resourceLoader.load(request) |
There was a problem hiding this comment.
How the predownloaded instruction are reused here? Does tilestore return result immediately if the instruction has been already downloaded?
There was a problem hiding this comment.
Yes, it returns the cached value. And the NetworkRestriction.DISALLOW_ALL makes sure it won't try to download it.
There was a problem hiding this comment.
what if downloading is in progress? Will it wait or return an error?
| init { | ||
| VoiceInstructionsPredownloadHub.register(speechLoader) | ||
| } |
There was a problem hiding this comment.
As I understand you did in this way to make predownloading enabled for all users by default. Our components responsible for speech finds instance of MapboxNavigation, listen for route progress, and download voice instructions under the hood.
This approach has its benefits, and I guess the main benefit is that it works out of the box for all users.
But there are also some downsides:
SpeeachApiused to have a very straight forward api: user asks it to generate voice instruction - it generates. It may be surprising for users and the SDK developers that there are some work which is going on under the hood.- Those hidden access of mapbox navigation instance seems like a cyclic dependency which makes it harder to understand data flow between navigation core and voice generation: some data comes from mapbox navigation via users and some via hidden access of mapbox navigation.
Maybe we can implement predownloading without hidden dependencies so that it's works explicitly for the users and the SDK developers.
What if speech API provide predownload function + the SDK provide a separate component which analyses route progress and triggers callback where user asks speech api to predownload some instructions. In this case it will be clear for user how the predownloading happens.
We can additionally provide a higher level API, where all the components are connected and works out of the box.
For me this decision seems like a tradeoff between easy to integrate vs easy to support/troubleshoot. I would picked the latter.
@dzinad , did you consider similar approaches to the suggested one? What was behind your decision to pick the approach which you have picked?
There was a problem hiding this comment.
Capturing from an internal discussion.
Providing a solution for all users so that they don't have to do anything on their end is the main reason why I chose this approach. Predownloading instructions fixes a bug when voice instructions are being played too late with low connectivity and it makes sense to fix the bug for everyone and not say: "do this and then the bug will be gone".
We've also discussed an alternative approach. We don't provide a solution under the hood, however, we integrate pre-downloading for users that use high-level APIs and for low-level API users we add a MapboxSpeechApi#predownload function and a way of getting instructions that should be predownloaded (for example, via a new observer).
I like this approach as well because it fits into the current NavSDK paradigm (Drop-In vs low-level APIs) but that wouldn't fix the bug for all users under the hood. And I would really like that.
It would be nice to hear more opinions on this discussion. @abhishek1508 @tomaszrybakiewicz @kmadsen @Guardiola31337
There was a problem hiding this comment.
The DropInUi component exists for audio guidance with MapboxAudioGuidance. If we introduce a new top level api, it would be something like MapboxAudioGuidanceOptions where you could specify details of preloading - I do not think we should add options yet.
The rest of the pre-downloaded code would be lower level. I like the idea of adding a function like MapboxSpeechApi#preload, make it experimental because we may iterate on parameters. Have you decided how to deal with memory/disk constraints? And then the MapboxSpeechApi#generate function would be able to continue to work.
It may be worth looking at image caching libraries for inspiration because the implementation details have been iterated on (with strong opinions) for years. For example, maybe there should be a LRU cache but has voice instructions instead of images? Load it until the cache is full or there are no more instructions for the route. https://github.com/square/picasso/blob/master/picasso/src/main/java/com/squareup/picasso3/PlatformLruCache.kt
There was a problem hiding this comment.
The DropInUi component exists for audio guidance with MapboxAudioGuidance. If we introduce a new top level api, it would be something like MapboxAudioGuidanceOptions where you could specify details of preloading - I do not think we should add options yet.
I don't think we should introduce new API, I think that MapboxAudioGuidance should support pre-downloading out of the box.
The rest of the pre-downloaded code would be lower level. I like the idea of adding a function like MapboxSpeechApi#preload, make it experimental because we may iterate on parameters. Have you decided how to deal with memory/disk constraints? And then the MapboxSpeechApi#generate function would be able to continue to work.
For now caching is handled by Core SDK.
But I'm especially worried about not fixing an issue with playing voice instruction too late out of the box if we add new API. The whole idea came from the fact that there is a bug. Any thoughts on that?
It may be worth looking at image caching libraries for inspiration because the implementation details have been iterated on (with strong opinions) for years. For example, maybe there should be a LRU cache but has voice instructions instead of images? Load it until the cache is full or there are no more instructions for the route. https://github.com/square/picasso/blob/master/picasso/src/main/java/com/squareup/picasso3/PlatformLruCache.kt
There's been a Review Forum (Week 32) discussion on which approach we should choose for implementing pre-downloading. And I thought we'd come up with an overall plan. Maybe I was wrong but it seemed to me that at the moment what was left is implementation and PR review would be mostly to discuss implementation details and not change the approach in general. Of course there are hundreds of ways it can be implemented and everyone might have their own opinion on what is best. If there is a specific reason why you think that previously chosen approach is bad, let's close this PR and start from the beginning with another round of discussion.
There was a problem hiding this comment.
But I'm especially worried about not fixing an issue with playing voice instruction too late out of the box if we add new API. The whole idea came from the fact that there is a bug. Any thoughts on that?
It will, but users will have to adopt new API which predownloads voice instructions
There was a problem hiding this comment.
There are a lot of things said here that are being missed in translation. We're in agreement on many things that are considered not in agreement.
When looking at the implementation, the only thing I actually disagree with is adding a mechanism that hooks into the MapboxNavigation constructor, that is not needed and introduces awkward objects (e.g., LegacyMapboxNavigationInstanceHolder)
There was a problem hiding this comment.
I'm also not seeing logic for managing or defining the limits for the preloaded instructions.
How do you know a super long route will not take up all of the disk space? Also how do you clear up the free space?
There was a problem hiding this comment.
There are a lot of things said here that are being missed in translation. We're in agreement on many things that are considered not in agreement.
Is there any specific sentence of mine you didn't fully understand? Please share because I'm asking for your opinions in order to have a constructive discussion and that will not be possible if we don't understand each other well.
There was a problem hiding this comment.
Kind of hard to explain, your responses here #6547 (comment) were not in line with the points i was trying to make. So I feel like my statements were not clear.
I'm seeing from other comments that we are all in line, so I'm thinking it's all good. I must have missed Vadzim suggestion, because that is also what I was suggesting.
In regards to memory constraints and cleaning up instructions, we can move that discussion to this thread #6547 (comment)
There was a problem hiding this comment.
The second iteration that takes into account the discussion above can be found here: #6771
...re/src/main/java/com/mapbox/navigation/core/internal/LegacyMapboxNavigationInstanceHolder.kt
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private fun predownload(typeAndAnnouncement: TypeAndAnnouncement) { |
There was a problem hiding this comment.
How does the system cleanup unplayed voice instructions?
There was a problem hiding this comment.
The same way it cleans up played instructions. :)
We have a mechanism of cleaning them but we only clean the files that were created by the Nav SDK during generate method.
I'll ask Core SDK but will it change anything? I mean the only difference with what we have now on main branch is that for un unfinished route there will be slightly more instructions in the cache.
There was a problem hiding this comment.
Nice, generate will clean up played instructions.
Is it possible for the preloading instructions, to cause disk or out of memory issues? For example, the route preloads 10GB of instructions but only 1GB of of space is available (exaggerating actual space values)
There was a problem hiding this comment.
created by the Nav SDK during generate method.
Also curious, unplayed instructions will only be cleared when generate is called? generate may not be called, right?
There was a problem hiding this comment.
generate will not clean up played instructions. It will create a file (see this line) that will be cleaned by user's callback that they pass to MapboxVoiceInstructionsPlayer#play. Predownloading does not pass through this line, therefore this file will not be created.
However, there is also cache handled by the Core SDK. And it is not cleared by us. Neither now, nor on main. And there is no way right now to trigger some specific file removal. 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.
A mechanism to clear the cache might be part of the OP plan for 2023.
...
By default the quota is unlimited, but there is logic to evict resources if the disk space is low. So for removal to happen, either a quota must have been set an exceeded or the disk space must be running low.
4 participants
No description provided.