Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PiperTTS] Initial contribution #15965

Merged
merged 12 commits into from
Feb 19, 2024

Conversation

GiviMAD
Copy link
Member

@GiviMAD GiviMAD commented Nov 26, 2023

This is a working in progress PR for integrating Piper.

It's already finished, but I let by error a println call inside the library used, so it can not be merged. I'm going to let it for next weekend so I can also give it another review.

I noticed a problem with Piper when using the low size models at short phrases, but with the medium model I have tested it works great and very fast without using too much ram.

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD GiviMAD requested a review from a team as a code owner November 26, 2023 18:08
@GiviMAD
Copy link
Member Author

GiviMAD commented Nov 26, 2023

Hey @dalgwen, if you have a chance to giving it a try, in case you notice something wrong or improvable.
BR.

@dalgwen
Copy link
Contributor

dalgwen commented Nov 27, 2023

Excellent !
I found piper-tts some weeks ago and I immediately thought that we should also have a TTS service for it. Well, it's christmas time ! Thanks.
I tested your implementation on my dev env and it works great 👍
As Mycroft cease activity, I was worried about mimic-tts. But now I can get rid of my mimic docker container in favor of this much simpler solution. I will push this on my "prod" env right now !

About the code, I have suggestions, all about cache :

  • Could you consider extending AbstractCachedTTSService instead of implementing directly TTSService ? It is a smart LRU cache to store the wav files created on disk. As piper is fast and free to use, it is not so important as it is for cloud services, but it seems nonetheless a good idea and it also has other advantages (like allowing to play concurrently on several sinks with only one call to the TTS service.) To do this, you can refer to the commit 28e6da6 for MaryTTS.
  • Do you think you can cache the json file reading in getAvailableVoices and getVoice ? These two methods are unfortunately called every time by the framework.

Thanks again

@lolodomo
Copy link
Contributor

As Mycroft cease activity

Does it mean we have to remove the binding? Or does it still work?

@dalgwen
Copy link
Contributor

dalgwen commented Nov 27, 2023

Does it mean we have to remove the binding? Or does it still work?

mimic 3 is completely offline, and the code is 100% open source and on github. So, it will continue to work and to be available.
I don't know if OpenVoice OS or Neon (old forks of Mycroft, and home for the Mycroft refugees community) have plan about it, but AFAIK there is no more development on mimic. So it is, at least, on the downard slope.
But I could be wrong, I'm no expert on the Mycroft legacy.

Piper has the support of rhasspy and Home Assistant, and many qualities, so it is IMHO the perfect choice now.

EDIT : I just realize that you may refer to the Mycroft binding ?
For the Mycroft binding, I really don't know : Open Voice OS and Neon took on the legacy. I didn't test and don't know if the binding works with those.

@dalgwen
Copy link
Contributor

dalgwen commented Nov 27, 2023

Hey @dalgwen, if you have a chance to giving it a try, in case you notice something wrong or improvable. BR.

Additionnal question : Is the service supposed (do you want the service) to work on openHAB 4.0 ?

With a 4.0.3 version, I have this :

2023-11-27 14:16:52.141 [WARN ] [org.apache.felix.fileinstall        ] - Error while starting bundle: file:/usr/share/openhab/addons/org.openhab.voice.pipertts-4.1.0-SNAPSHOT.jar
org.osgi.framework.BundleException: Could not resolve module: org.openhab.voice.pipertts [348]
  Unresolved requirement: Import-Package: com.fasterxml.jackson.databind; version="[2.15.0,3.0.0)"

        at org.eclipse.osgi.container.Module.start(Module.java:463) ~[org.eclipse.osgi-3.18.0.jar:?]
        at org.eclipse.osgi.internal.framework.EquinoxBundle.start(EquinoxBundle.java:445) ~[org.eclipse.osgi-3.18.0.jar:?]
        at org.apache.felix.fileinstall.internal.DirectoryWatcher.startBundle(DirectoryWatcher.java:1260) ~[?:?]
        at org.apache.felix.fileinstall.internal.DirectoryWatcher.startBundles(DirectoryWatcher.java:1233) ~[?:?]
        at org.apache.felix.fileinstall.internal.DirectoryWatcher.doProcess(DirectoryWatcher.java:520) ~[?:?]
        at org.apache.felix.fileinstall.internal.DirectoryWatcher.process(DirectoryWatcher.java:365) ~[?:?]
        at org.apache.felix.fileinstall.internal.DirectoryWatcher.run(DirectoryWatcher.java:316) ~[?:?]

@GiviMAD
Copy link
Member Author

GiviMAD commented Nov 28, 2023

Excellent ! I found piper-tts some weeks ago and I immediately thought that we should also have a TTS service for it. Well, it's christmas time ! Thanks. I tested your implementation on my dev env and it works great 👍 As Mycroft cease activity, I was worried about mimic-tts. But now I can get rid of my mimic docker container in favor of this much simpler solution. I will push this on my "prod" env right now !

About the code, I have suggestions, all about cache :

  • Could you consider extending AbstractCachedTTSService instead of implementing directly TTSService ? It is a smart LRU cache to store the wav files created on disk. As piper is fast and free to use, it is not so important as it is for cloud services, but it seems nonetheless a good idea and it also has other advantages (like allowing to play concurrently on several sinks with only one call to the TTS service.) To do this, you can refer to the commit 28e6da6 for MaryTTS.
  • Do you think you can cache the json file reading in getAvailableVoices and getVoice ? These two methods are unfortunately called every time by the framework.

Thanks again

Thank you for testing it. And yes make a lot of sense to apply both suggestions. I found an issue with the option for keeping the voice loaded, seems like when I keep one voice in memory and use another there is a bug in the moment of release the memory of the second one, I think I've made something wrong. I hope to find time these weekend to get the PR ready.

Additionnal question : Is the service supposed (do you want the service) to work on openHAB 4.0 ?

I think bindings of the branch 4.1.0 are not compatibles with 4.0.x versions, I'm not sure about it, I started having problems testing some bindings and moved to the milestones.

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD GiviMAD changed the title WIP [PiperTTS] Initial contribution [PiperTTS] Initial contribution Dec 5, 2023
@GiviMAD
Copy link
Member Author

GiviMAD commented Dec 5, 2023

I found an issue with the option for keeping the voice loaded, seems like when I keep one voice in memory and use another there is a bug in the moment of release the memory of the second one, I think I've made something wrong. I hope to find time these weekend to get the PR ready.

I found the problem, I incorrectly understood the piper initialize and terminate methods, I have fixed the implementation.

Either way I have changed the preloadModel operation to keep the last voice used loaded, I think it has a lot more sense implemented that way.

Also I've extended the cache class and it works nicely, and added a simple cache mechanism to avoid the json read, thank you for the comments!

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/dialog-processing-with-the-pulseaudiobinding/148191/1

GiviMAD and others added 3 commits December 23, 2023 14:00
Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: GiviMAD <GiviMAD@users.noreply.github.com>
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly some minor text comments. Also also some Exception handling improvements can be made. Half way the PiperTTSService i stopped checking, as it seems consistent and i think if you refactor it, you will fix them.
Hope this makes any sense.

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD GiviMAD requested a review from lsiepel February 18, 2024 12:55
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, finished the review, but i'm new to voice add-on's, so i think an extra set of @openhab/add-ons-maintainers eyes would be usefull

@GiviMAD
Copy link
Member Author

GiviMAD commented Feb 18, 2024

LGTM, finished the review, but i'm new to voice add-on's, so i think an extra set of @openhab/add-ons-maintainers eyes would be usefull

Thank you for the review!

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I've added few more minor comments below:

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
GiviMAD and others added 2 commits February 19, 2024 22:08
Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: GiviMAD <GiviMAD@users.noreply.github.com>
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD GiviMAD requested a review from wborn February 19, 2024 21:17
@lsiepel lsiepel merged commit 06eeff1 into openhab:main Feb 19, 2024
3 checks passed
@lsiepel lsiepel added this to the 4.2 milestone Feb 19, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Feb 19, 2024

Thanks for this contribution. Also @wborn for the extra check.

austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
* [PiperTTS] Initial contribution

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants