Repository navigation
PySpeechModule - #1120
PySpeechModule#1120jsett wants to merge 9 commits into
Conversation
Better not take the burden of renaming, indeed :) My pypi account login is |
| :name: speechd-conf-adv | ||
|
|
||
| Timeout 0 | ||
| AddModule "kokoropy" "/home/user/src/kokoro_speechd_module/kokoro_module.py" "kokoropy.conf" |
There was a problem hiding this comment.
Mmm, no, modifying the speechd.conf configuration file is not the now-recommended way of adding a speechd module. One should rather just put it in the right place, which can be /usr/libexec/speech-dispatcher-modules/sd_kokoro and its configuration in /etc/speech-dispatcher/modules/kokoro.conf for system-wide installation, or ~/.local/libexec/speech-dispatcher-modules/sd_kokoro and ~/.config/speech-dispatcher/modules/kokoro.conf for per-user installation.
| pip install --upgrade pip | ||
| pip install pySpeechModule soundfile | ||
|
|
||
| You need to `download <https://github.com/jsett/pySpeechModule/raw/refs/heads/main/docs/source/deep_learning.wav>`_ and make sure to name it ``deep_learning.wav``. |
There was a problem hiding this comment.
This should get fixed into the speechd/ url that we will have
| :name: speechd-conf-quick | ||
|
|
||
| Timeout 0 | ||
| AddModule "dummypy" "/home/user/src/my_first_speechd_module/dummy.py" "dummypy.conf" |
There was a problem hiding this comment.
As mentioned above, better document the now-recommended "ship two files" approach.
| logging.debug(f"Ahead: {self._ahead}") | ||
| self.action_queue.put({"command": "speak", "val": item}) | ||
| # sleep if we have too much audio generated, only do this is max_ahead and min_ahead are set. | ||
| if (hasattr(self, "max_ahead") and hasattr(self, "min_ahead")): |
There was a problem hiding this comment.
Is this "ahead" management really needed?
My concern is that it was requested that speech-dispatcher not only be able to play the speech, but also allow some clients to retrieve the generated speech, and in that case we do not want to wait.
| self.execution_worker.set_callback(callback) | ||
| self.execution_worker.start() | ||
| self.action_worker.start() | ||
| self._callback._set_init(self.action_worker.queue, self) |
There was a problem hiding this comment.
To properly respect the protocol, we'd rather want to call the initialization and configuration after having received INIT from the server (and return any error, for proper logging)
|
|
||
| .. code-block:: python | ||
|
|
||
| [{"mark": "<mark>", "text": "<text>"}] |
There was a problem hiding this comment.
But then this loses all ssml tags except the marks? This should be documented as being just a helper for syntheses that do not know about ssml but can manage prosody over several pieces.
Ideally the underlying synthesis should be just given the ssml without any pre-parsing, so it can interpret whatever the speechd client requested.
| def speak(self, ssml): | ||
| try: | ||
| # parse the ssml into chunk of text each with a mark. | ||
| for item in parse_ssml(ssml): |
There was a problem hiding this comment.
Mmm, but then we give only pieces of text to the underlying synthesis?
The issue is that screen readers which want fine-grain marking for on-screen speech progression will e.g. put marks between each word. Each word is then fed to the underlying synthesis separately, which can make the synthesis not produce any prosody at all, which will degrade the quality a lot.
I'd say better document using strip_ssml, which will by default bring good prosody, and add documentation to explain about managing marks vs supporting prosody. Some synthesis may be fed with several pieces with global prosody management, some others not, it really depends on the synth. Ideally synths would be fed with the complete ssml...
| - Pico | ||
| - Piper | ||
| - Swift | ||
| - Kitten |
There was a problem hiding this comment.
This PR does not add Kitten?
daxmawal
left a comment
daxmawal
left a comment
There was a problem hiding this comment.
I compared this with PR #1104.
The main overlap is SSML stripping. module_strip_ssml() and strip_ssml() share the same goal, but their behavior differs regarding incomplete tags and unknown entities. We could reuse the tests from #1104, add cases involving bytes, and retain the pySpeechModule implementation.
The dictionaries representing voices already include the SPDVoice fields: name, language, and variant. They seem suitable as-is.
Regarding audio, AudioTrack in #1104 stored the data along with fields describing the format. I left a comment asking whether bits and num_channels would still be useful, or if we wanted to stick with the current format: 16-bit little-endian mono.
PR #1104 also added constants for message types. These commands are recognized here, but their type is not passed to speak().
| root = ET.fromstring(ssml_string) | ||
| except ET.ParseError: | ||
| # Wrap fragments in a temporary root node to ensure valid parsing | ||
| root = ET.fromstring(f"<root>{ssml_string}</root>") |
There was a problem hiding this comment.
| root = ET.fromstring(f"<root>{ssml_string}</root>") | |
| if isinstance(ssml_string, bytes): | |
| wrapped = b"<root>" + ssml_string + b"</root>" | |
| else: | |
| wrapped = f"<root>{ssml_string}</root>" | |
| root = ET.fromstring(wrapped) |
strip_ssml(b"Hello") returns "b'Hello'" instead of "Hello". The suggestion fixes that.
The tests from #1104 also cover incomplete tags and unknown entities, which now raise ParseError. Could we reuse those tests, add bytes cases, and decide whether to keep the old behaviour ?
There was a problem hiding this comment.
it'd probably be better to cope with parsing errors indeed. Better produce some audio than just an error. For blind people, that can make a difference between some oddities and a completely useless machine.
| if (line == "SPEAK\n"): | ||
| self._speak() | ||
| elif (line == "SOUND_ICON\n"): | ||
| self._speak() | ||
| elif (line == "CHAR\n"): | ||
| self._speak() | ||
| elif (line == "KEY\n"): | ||
| self._speak() |
There was a problem hiding this comment.
Would it be useful to pass the message type to speak() too ? #1104 added constants for these types, and passing the type would let modules handle characters, keys and sound icons separately.
There was a problem hiding this comment.
Yes, we want to pass the message type, just like in the C API.
| # anything else is a fatal error. | ||
| raise Exception("Audio must be list or ndarray.") | ||
|
|
||
| stdout.write(f"705-bits=16\n") |
There was a problem hiding this comment.
I was wondering if the bits and num_channels fields from #1104's AudioTrack could still be useful here.
There was a problem hiding this comment.
We do want to be able to let the module specify the encoding, indeed. I'd rather not have a default value, better make sure that module writers know they have to specify it properly.
3 participants
This a framework for creating speechd modules in python, along with documentation for using it, and a github workflow for building/publishing to PyPi.
Additional notes: For PyPi we can either recreate with a new name, or I think I can move it to a new owner(which I need a user name for the new owner). If we recreate it a number of different places will need to be updated with the new name. We will also need to change the trusted publishing repo for PyPi so that github can publish. For readthedocs I did not see a easy way to move it. It will almost certainly need to be recreated with a new name and then the code will need to be updated with that name.