Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Fix loading Mimic phonemes from cache #2871

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Mar 22, 2021

Description

This PR fixes loading cached phonemes when using Mimic1. Noticed this when testing the cache-sync PR.

The PR does 2 things:

  • Make mimic 1 return phonemes in a similar way as Mimic2. This is an improvement in 2 ways, the string conversion gets pre-calculated, saving resources and the code becomes more similar to Mimic2.
  • Make the PhonemeFile no longer save non-json-encodable as string. SInce this can't be loaded there's no reason allowing to save in this format

How to test

Switch to mimic1 as TTS, ask mycroft to say hello two times to use the cache and ensure it works.

Contributor license agreement signed?

CLA [ Yes ]

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Mar 22, 2021
@forslund forslund added the Type: Bug - quick Bug fixes that are quick to review and the implications of the change are clear and contained. label Mar 22, 2021
@forslund forslund changed the title Fix loading Mimic phonemes Fix loading Mimic phonemes from cache Mar 22, 2021
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling krisgesling removed their request for review March 23, 2021 07:28
@chrisveilleux
Copy link
Member

I am not sure I understand this PR. Consider:

import json
s = "[[1, 2], [3, 4]]"
json.loads(s)
[[1, 2], [3, 4]]

There should be no exception raised when these phoneme files are parsed by the built-in JSON parser. Why write our own?

@forslund
Copy link
Collaborator Author

Mimic 1 did not have a json deserializable phoneme format. and there is no guarantee other tts's has it either.

  • The first commit makes Mimic1's phoneme data json serializable and deserializable
  • There is currently no constraint that the phoneme data should be json serializable / deserializable so the second commit handles the case where the phoneme data is not a structure that can be serialized / deserialized.

If we add the constraint that the phoneme's returned from the get_tts() method should be json serializable we can drop the second commit of these.

@chrisveilleux
Copy link
Member

I think I misunderstood the change when I first read it. Thanks for the clarification.

We should be consistent about the way we handle the phonemes. The comment about how some TTS engines will handle parsing themselves is telling. We should either handle the parsing inside the PhonemeFile object or outside of it. Right now we are doing both.

@forslund
Copy link
Collaborator Author

I can agree with that. Phoneme data will vary between TTS engines so previously the parsing of phoneme files were upto each TTS class, now with the PhonemeFile class that is sort of not a thing anymore.

I wonder if we instead of phonemes should cache the final visemes. Those are standardized while visemes are per TTS. It's weird to cache a pseudo format in between the raw format and the viseme.

Previously it made sense to cache the raw data since cache from previous runs were available and if a change in viseme encoding were made old cached files would work but nowadays we seem to clear the cache on startup. (also the cache object doesn't do any syncing from disk)

Drawback of doing this change would be that the mimic2-persistent cache will need to be rebuilt.

@forslund
Copy link
Collaborator Author

@chrisveilleux so what do you say? should I drop the second Commit in the PR and just doing the mimic1 phoneme format change? I would like to get something in place so that mimic1 functionality is restored.

forslund added 2 commits April 5, 2021 08:28
This makes the phonemes json de/encodable like mimic2
String format can't be loaded so it shouldn't be written either
@forslund forslund force-pushed the bugfix/mimic-phonemes branch from 335c84b to 344999d Compare April 5, 2021 06:45
@forslund
Copy link
Collaborator Author

forslund commented Apr 5, 2021

I changed the last commit to instead of allowing strings to be loaded, we disallow non-json encodable objects to be saved in the first place.

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@chrisveilleux chrisveilleux merged commit 1ba9eda into MycroftAI:dev Apr 13, 2021
@forslund forslund deleted the bugfix/mimic-phonemes branch April 13, 2021 20:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Type: Bug - quick Bug fixes that are quick to review and the implications of the change are clear and contained.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants