Supply phonetic road names to AVSpeechSynthesizer #624

1ec5 · 2017-09-18T11:54:45Z

This change provides AVSpeechSynthesizer with the same phonetic names that #622 provides Polly with. This way the AVSpeechSynthesizer fallback remains consistent with Polly in terms of its pronunciation of difficult names. The closer the two voices’ pronunciation, the less jarring it is when the fallback kicks in.

AVSpeechSynthesizer doesn’t support SSML; instead, on iOS 10 and above, it uses the native NSAttributedString type to hold attributes such as pronunciations. It turns out that the Alex voice (#612) does not support attributed strings, but it manages to correctly pronounce just about all the roads that are currently tagged with pronunciations anyways.

~~Depends on #621, mapbox/mapbox-directions-swift#174, and Project-OSRM/osrm-text-instructions.swift#39.~~

/cc @frederoni @bsudekum

bsudekum · 2017-09-18T15:53:25Z

@1ec5 does it make sense to add this just for AVSpeechSynthesizer if in theory all users will be using polly via #617?

1ec5 · 2017-09-18T16:59:38Z

in theory all users will be using polly via #617

The fallback exists for a few reasons, including:

The API may not be reachable at the time it’s needed. Since instructions are time-critical, a fallback needs to kick in locally. Google Maps takes a different approach of preloading some common utterances locally and omitting details like road names that are more variable. I think it’s more helpful to have a different voice kick in than to leave the user hanging.
Some of the languages this library is localized into are supported by AVSpeechSynthesizer but not Polly.

frederoni · 2017-09-26T19:09:04Z

MapboxNavigation/PollyVoiceController.swift

            return
        }

        guard let url = awsTask.result else {
-            super.speak(fallbackText, error: "No polly response")
+            print("No polly response")
+            speakWithoutPolly(for: routeProgress, userDistance: userDistance)
            return
        }



Should we cancel the task before creating a new one?

Good idea, but I think we should address this issue in #661. Once that PR lands, I’ll rebase and do likewise here.

frederoni · 2017-09-26T19:10:10Z

MapboxNavigation/PollyVoiceController.swift

@@ -150,13 +152,14 @@ public class PollyVoiceController: RouteVoiceController {
                        audioPlayer.volume = strongSelf.volume
                        audioPlayer.play()


the return value of .play() is discardable but I think we should observe it.

#661 takes care of responding to the return value.

bsudekum · 2017-11-16T01:21:23Z

@1ec5 is this still actionable?

1ec5 · 2017-11-16T22:08:17Z

It would still be desirable to ensure that the AVSpeechSynthesizer fallback can handle phonetic names. AVSpeechSynthesizer isn’t going away; after all, Polly is optional, and it doesn’t support most of the languages we do, like Chinese.

For AVSpeechSynthesizer to apply the phonetic names as string attributes, this library currently depends on OSRMTextInstructions.swift, hence all the merge conflicts. Relying on OSRMTextInstructions.swift would create some inconsistencies between the instructions spoken by Polly and those spoken by AVSpeechSynthesizer, due to extra processing we’re doing server-side outside of OSRM Text Instructions. A better solution would be for the Directions API to indicate the ranges within the plain-text instruction that should be pronounced a certain way. We can track that issue internally.

I’ve also been leaving this PR around in order to track a refactoring of PollyVoiceController and RouteVoiceController, but I had you carry it out in #800.

1ec5 · 2017-11-17T00:29:31Z

A better solution would be for the Directions API to indicate the ranges within the plain-text instruction that should be pronounced a certain way. We can track that issue internally.

In the meantime, RouteVoiceController can search for the current route step’s names inside the plain-text instruction and apply the step’s phoneticNames to those substrings inside an attributed string. The assumption would be that we wouldn’t encounter a single instruction that contains the same name pronounced two different ways. Hopefully the road name is always distinguishable from other instruction text.

1ec5 · 2017-12-01T09:36:26Z

Ready for review.

frederoni · 2017-12-04T12:21:47Z

This implementation currently relies on RouteStepFormatter which is getting removed in #767.
Would it make sense to wait for 767 to land and adapt this PR afterward?

bsudekum · 2017-12-04T16:56:57Z

Would it make sense to wait for 767 to land and adapt this PR afterward?

Yeah let's do that.

1ec5 · 2017-12-05T00:31:18Z

Yes, we should wait until after #767 lands. This PR still includes the modifications to RouteStepFormatter mainly so that the project can still build. But the cruxt of the change is in RouteVoiceController.

Build speech strings as attributed strings in order to apply an IPA notation attribute to road names when spoken by AVSpeechSynthesizer.

1ec5 · 2017-12-06T22:01:39Z

All set.

bsudekum · 2017-12-06T22:03:01Z

MapboxNavigation/RouteVoiceController.swift

        if Locale.preferredLocalLanguageCountryCode == "en-US" {
-            utterance.voice = AVSpeechSynthesisVoice(identifier: AVSpeechSynthesisVoiceIdentifierAlex)
+            // Alex can’t handle attributed text.


1ec5 added feature New feature request. op-ex Refactoring, Tech Debt or any other operational excellence work. topic: instructions topic: voice ⚠️ DO NOT MERGE PR should not be merged! labels Sep 18, 2017

1ec5 self-assigned this Sep 18, 2017

1ec5 requested a review from frederoni September 18, 2017 11:54

1ec5 added blocked: upstream and removed ⚠️ DO NOT MERGE PR should not be merged! labels Sep 18, 2017

1ec5 force-pushed the 1ec5-ipa-attribute branch from b098d1f to d08c3fd Compare September 18, 2017 22:59

1ec5 removed the blocked: upstream label Sep 18, 2017

1ec5 force-pushed the 1ec5-ipa-attribute branch 2 times, most recently from e5e710b to 9b8ad92 Compare September 19, 2017 18:28

frederoni reviewed Sep 26, 2017

View reviewed changes

1ec5 force-pushed the 1ec5-ipa-attribute branch from 9b8ad92 to 2097f6d Compare September 28, 2017 18:39

1ec5 mentioned this pull request Sep 28, 2017

If audioPlayer is playing, don't also play fallback voice #661

Merged

1ec5 mentioned this pull request Oct 12, 2017

Use voice instructions from server #614

Merged

3 tasks

1ec5 mentioned this pull request Nov 7, 2017

Expose delegate method for failed voice instructions #800

Merged

2 tasks

1ec5 force-pushed the 1ec5-ipa-attribute branch from 2097f6d to 9448372 Compare December 1, 2017 09:35

1ec5 requested a review from bsudekum December 1, 2017 09:35

Supply phonetic road names to AVSpeechSynthesizer

1c5e549

Build speech strings as attributed strings in order to apply an IPA notation attribute to road names when spoken by AVSpeechSynthesizer.

1ec5 force-pushed the 1ec5-ipa-attribute branch from 9448372 to 1c5e549 Compare December 6, 2017 22:01

bsudekum reviewed Dec 6, 2017

View reviewed changes

bsudekum approved these changes Dec 6, 2017

View reviewed changes

1ec5 merged commit 9047a41 into master Dec 7, 2017

1ec5 deleted the 1ec5-ipa-attribute branch December 7, 2017 06:05

1ec5 mentioned this pull request Dec 7, 2017

Fix crash adding pronunciation to instructions #918

Merged

1ec5 mentioned this pull request Aug 8, 2022

SystemSpeechSynthesizer should use SSML on iOS 16 #4057

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supply phonetic road names to AVSpeechSynthesizer #624

Supply phonetic road names to AVSpeechSynthesizer #624

1ec5 commented Sep 18, 2017 •

edited

Loading

bsudekum commented Sep 18, 2017

1ec5 commented Sep 18, 2017 •

edited

Loading

frederoni Sep 26, 2017

1ec5 Sep 28, 2017

frederoni Sep 26, 2017

1ec5 Sep 28, 2017

bsudekum commented Nov 16, 2017

1ec5 commented Nov 16, 2017

1ec5 commented Nov 17, 2017

1ec5 commented Dec 1, 2017

frederoni commented Dec 4, 2017

bsudekum commented Dec 4, 2017

1ec5 commented Dec 5, 2017

1ec5 commented Dec 6, 2017

bsudekum Dec 6, 2017

		@@ -150,13 +152,14 @@ public class PollyVoiceController: RouteVoiceController {
		audioPlayer.volume = strongSelf.volume
		audioPlayer.play()

Supply phonetic road names to AVSpeechSynthesizer #624

Supply phonetic road names to AVSpeechSynthesizer #624

Conversation

1ec5 commented Sep 18, 2017 • edited Loading

bsudekum commented Sep 18, 2017

1ec5 commented Sep 18, 2017 • edited Loading

frederoni Sep 26, 2017

Choose a reason for hiding this comment

1ec5 Sep 28, 2017

Choose a reason for hiding this comment

frederoni Sep 26, 2017

Choose a reason for hiding this comment

1ec5 Sep 28, 2017

Choose a reason for hiding this comment

bsudekum commented Nov 16, 2017

1ec5 commented Nov 16, 2017

1ec5 commented Nov 17, 2017

1ec5 commented Dec 1, 2017

frederoni commented Dec 4, 2017

bsudekum commented Dec 4, 2017

1ec5 commented Dec 5, 2017

1ec5 commented Dec 6, 2017

bsudekum Dec 6, 2017

Choose a reason for hiding this comment

1ec5 commented Sep 18, 2017 •

edited

Loading

1ec5 commented Sep 18, 2017 •

edited

Loading