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

[mimictts] Add LRU cache #14564

Merged
merged 2 commits into from
Jul 12, 2023
Merged

[mimictts] Add LRU cache #14564

merged 2 commits into from
Jul 12, 2023

Conversation

dalgwen
Copy link
Contributor

@dalgwen dalgwen commented Mar 9, 2023

Hello,

This pull request add the common TTS cache capability to the mimic TTS Service. (MimicTTSService.java)

And also : [mimictts] Fix playing with audio servlet
It also fixes an issue with a sound not playing with servlet based sink. (AutoDeleteFileAudioStream.java)
(The system responsible for deleting a file that is not used anymore was too greedy. The sound file was deleted, detecting a closing stream, before even being read for "real")

@dalgwen dalgwen added the enhancement An enhancement or new feature for an existing add-on label Mar 9, 2023
@lolodomo
Copy link
Contributor

lolodomo commented Mar 9, 2023

Maybe your class AutoDeleteFileAudioStream could be moved into core framework to be used by different add-ons?

@dalgwen
Copy link
Contributor Author

dalgwen commented Mar 10, 2023

Hum yes, but is it really a good implementation for core ?
I designed it as a workaround (temporary), and it is not very bug proof. If the stream is not read by the sink, for whatever reason, then it won't even be deleted.

The more I think about it, the more I think it should be handled in the AudioHTTPServer implementation.
Instead of :
String serve(FixedLengthAudioStream stream, int seconds);
The method could be :
String serve(AudioStream stream, int seconds);

And then, in the AudioServlet implementation, the serve (AudioStream stream, int second) method implementation could

  • if stream is FixedAudioStream, then as proceed usual (putting the stream in a multi time enabled map for further reference)
  • if not, then using a temporary cache file, a wrapper, or whatever magic, and still putting it in the multi time enabled map.

Then we won't have to do workarounds in all TTS services for the audio sinks that only support this multi time serve (FixedLengthAudioStream stream, int second) method.

WDYT ?

@lolodomo
Copy link
Contributor

Looks like a great idea.

@lolodomo
Copy link
Contributor

@dalgwen : do you propose to implement your idea (AudioHTTPServer) ?

@lolodomo
Copy link
Contributor

lolodomo commented Mar 10, 2023

@dalgwen : I have another idea. synthesizeForCache is called by the cache handler in two cases:

  1. When the cache is enabled and a new TTS has to be added in the cache
  2. When the cache is disabled

Our problem is only with the second case. Why not distinguishing the two cases and add a new method synthesizeForDisabledCache in AbstractCachedTTSService. For this method, the TTS service would know that it has to create a file and return a FileAudioStream.
mimic and voiceRRS would then build a temporary file when this method is called.

WDYT ?

Edit: In fact, we just need to know in synthesizeForCache if the cache is enabled or disabled. I have to check if this information is accessible. No need for a new method.
Edit2: it is not available. We could add another argument for method synthesizeForCache to requestr a FileAudioStream.

lolodomo added a commit to lolodomo/openhab-core that referenced this pull request Mar 10, 2023
Related to openhab/openhab-addons#14564 (comment)

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-core that referenced this pull request Mar 10, 2023
Related to openhab/openhab-addons#14564 (comment)

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@dalgwen
Copy link
Contributor Author

dalgwen commented Mar 11, 2023

do you propose to implement your idea (AudioHTTPServer) ?

I made a try :
dalgwen/openhab-core@9b95051

With this, the servlet and the audiosinks using it do not need to restrain to FixedLengthAudioStream.
The servlet take a copy in memory if it is small (< 1MB, to avoid disk write operation), or in a temporary file (should be very rare because 1MB is, I think, larger than the majority of use cases). It then serves the copy. And the copy can be cloned and used several times.

Of course, if it not needed (if the TTS service or the cache already made a file), there is no modification.

I still have one weird behaviour : the code to delete the file when expired is called, but the file remains on disk.

@lolodomo
Copy link
Contributor

Looks like a good solution? I hope you can fix your weird behaviour with file not deleted.

@dalgwen dalgwen marked this pull request as draft March 17, 2023 15:31
@dalgwen
Copy link
Contributor Author

dalgwen commented Mar 17, 2023

I did a PR for what we discuss here.

In the meantime, I put this PR on "draft" mode

@lolodomo lolodomo mentioned this pull request Jun 18, 2023
7 tasks
@dalgwen dalgwen force-pushed the mimictts_add_cache branch from 89db2b8 to 33b4fb6 Compare June 19, 2023 23:10
@dalgwen dalgwen force-pushed the mimictts_add_cache branch from 33b4fb6 to 6c13d97 Compare July 4, 2023 21:34
@lolodomo
Copy link
Contributor

lolodomo commented Jul 7, 2023

@dalgwen : is it ready for a review ?

And simplifies code with new core capabilities (no more need to create temporary files implementing FixedLengthAudioStream)

Signed-off-by: Gwendal Roulleau <gwendal.roulleau@gmail.com>
@dalgwen dalgwen force-pushed the mimictts_add_cache branch from 6c13d97 to 3b792b9 Compare July 8, 2023 07:30
@dalgwen
Copy link
Contributor Author

dalgwen commented Jul 8, 2023

@dalgwen : is it ready for a review ?

I just force pushed after a rebase.

As this PR removes the workaround for the HTTP audio servlet, it will break compatibility with audio sink using audio servlet and still testing against FixedLengthAudioStream.
Apart of this, it is ready.

@lolodomo
Copy link
Contributor

lolodomo commented Jul 8, 2023

As this PR removes the workaround for the HTTP audio servlet, it will break compatibility with audio sink using audio servlet and still testing against FixedLengthAudioStream.
Apart of this, it is ready.

Please unmark the PR as draft in this case.
I will merge it as soon as most of audio sinks are changed.

@dalgwen dalgwen marked this pull request as ready for review July 8, 2023 13:36
Apply code review and static analysis.

Signed-off-by: Gwendal Roulleau <gwendal.roulleau@gmail.com>
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo
Copy link
Contributor

lolodomo commented Jul 8, 2023

I will wait a little (maybe end of weekend), I hope someone will merge my changes in audio sinks.
But this PR will be included in Milestone 5.

@lolodomo lolodomo merged commit 7587e0c into openhab:main Jul 12, 2023
@lolodomo lolodomo added this to the 4.0 milestone Jul 12, 2023
@dalgwen dalgwen deleted the mimictts_add_cache branch July 15, 2023 14:12
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
* [mimictts] Add LRU cache

And simplifies code with new core capabilities (no more need to create temporary files implementing FixedLengthAudioStream)

---------

Signed-off-by: Gwendal Roulleau <gwendal.roulleau@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
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants