-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
Voight Kampff Integration Test Succeeded (Results) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, couple more things I just noticed while testing it out.
It would also be great if you can squash the commits into a single "Add Festival TTS engine". This helps to keep the commit history of mycroft-core cleaner, making it much easier to know what each commit is related to.
If you're not familiar with git it sounds scary but, here's a nice overview of how to do it. If you run into any trouble let us know.
https://stackabuse.com/git-squash-multiple-commits-in-to-one-commit/
mycroft/tts/festival_tts.py
Outdated
|
||
def execute(self, sentence, ident=None, listen=False): | ||
|
||
encoding = self.config.get('encoding') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to set 'utf8' as the default here so that by default we don't convert the text:
encoding = self.config.get('encoding', 'utf8')
Wondering if we also need a TTS specific lang
config?
Looking at Festival it seems that they use the full names of languages eg "English" so self.lang
will likely never be accurate. Could still provide it as a default:
lang = self.config.get('lang', self.lang)
So a users config might then look like:
{
"tts": {
"module": "festival",
"festival": {
"lang": "italian"
}
}
}
mycroft/tts/festival_tts.py
Outdated
cmd += " | festival --tts --language " + self.lang | ||
|
||
self.begin_audio() | ||
subprocess.call(cmd, shell=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shell=True
has some security considerations so ideally we can avoid using it. Unfortunately you can't use a simple pipe character without it.
One option would be to split each command into it's own call and pipe using subprocesses built in method. Eg:
text = subprocess.Popen(('echo', sentence), stdout=subprocess.PIPE)
if encoding != 'utf8':
convert_cmd = ('iconv', '-f', 'utf8', '-t', encoding)
converted_text = subprocess.Popen(convert_cmd,
stdin=text.stdout,
stdout=subprocess.PIPE)
text.wait()
text = converted_text
tts_cmd = ('festival', '--tts', '--language', lang)
self.begin_audio()
subprocess.call(tts_cmd, stdin=text.stdout)
text.wait()
self.end_audio(listen)
Voight Kampff Integration Test Succeeded (Results) |
Hi, @krisgesling Thanks for your help. I've tried my poor skills with git. I hope It's good enough now. Regards |
Voight Kampff Integration Test Succeeded (Results) |
1 similar comment
Voight Kampff Integration Test Succeeded (Results) |
This looks perfect, nice work! |
Description
This PR adds Festival TTS engine suport #2640
How to test
Just install festival tts engine and voices (Catalan is a good example). And set mycroft settings:
Contributor license agreement signed?
Yes, I do.