Skip to content

Conversation

@phamson02
Copy link

Illustrate a change that can resolve issue #230

@KoljaB
Copy link
Owner

KoljaB commented Dec 10, 2024

Thanks for this promising PR! I can’t merge it as-is, though, because of the changes to the synthesize method’s signature:

def synthesize(self, text: str, audio_queue: queue.Queue) -> bool:

This would require changes to the base class:
https://github.com/KoljaB/RealtimeTTS/blob/master/RealtimeTTS/engines/base_engine.py#L56

Plus, every other engine would need its synthesize method updated too, since all engines need the same signature. Merging this now would break support for all engines except Coqui.

Also, could you add a test Python file in the tests folder to highlight the benefits of this PR? That would help me validate the changes and see them in action.

Thanks again for your effort, much appreciated!

@phamson02
Copy link
Author

Thanks for your nice words. I know that it would break things doing it that, but wanted to open this PR to illustrate a kinda hacky way that resolve my problem.

Will come up with a small example for you to test this out later.

@phamson02
Copy link
Author

Hey @KoljaB, sorry for the delay on the promised test for this PR. I didn't realise that I only need to make some changes to your FastAPI example. Pushed it here.

@KoljaB
Copy link
Owner

KoljaB commented Dec 16, 2024

Thank you for the test. Might take me a while to review, maybe one or two weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants