Add async js API for tts generate. - #3133
Conversation
dosubot
Bot
added
the
size:L
Summary of ChangesHello @csukuangfj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Node.js binding for Sherpa-Onnx's Text-to-Speech functionality by introducing an asynchronous generation API. This allows applications to perform TTS synthesis in a non-blocking manner, providing real-time progress updates and the ability to cancel ongoing operations. The changes ensure a more responsive and interactive user experience by integrating asynchronous patterns across the C++ backend, the Node.js wrapper, and an updated example. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds asynchronous non-streaming TTS: Node wrapper gets OfflineTts.generateAsync with onProgress and cancellation semantics; C++ addon switches callback flags to atomic, refines cancellation handling and cleans processed callback entries; example updated to use the new async API. (≤50 words) Changes
Sequence DiagramsequenceDiagram
participant App as JavaScript App
participant Wrapper as Node.js Wrapper
participant Native as C++ Addon
participant Worker as Worker Thread
App->>Wrapper: generateAsync({ onProgress })
Wrapper->>Native: addon.offlineTtsGenerateAsync(wrappedCallback)
Native->>Worker: enqueue generation task
loop per audio chunk
Worker->>Native: emit progress event
Native->>Wrapper: call wrappedCallback(progress)
Wrapper->>App: invoke user onProgress(progress)
App-->>Wrapper: return value (continue or 0/false)
Wrapper-->>Native: return canonicalized boolean
Native->>Native: set cancelled if return is 0/false
alt cancelled
Native-->>Worker: stop processing
else
Worker->>Worker: continue processing
end
end
Worker->>Native: generation finished
Native-->>Wrapper: return GeneratedAudio
Wrapper-->>App: resolve Promise with GeneratedAudio
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an asynchronous generateAsync API for TTS in the Node.js addon, which is a great feature for non-blocking operations. The implementation includes a progress callback mechanism.
My review has identified a critical thread safety issue in the C++ addon that could lead to data races and undefined behavior. I've also suggested an improvement to the JavaScript example file to make its behavior less confusing for users.
| for (auto it = _this->data_list_.begin(); | ||
| it != _this->data_list_.end();) { | ||
| if ((*it)->processed) { | ||
| delete *it; | ||
| it = _this->data_list_.erase(it); | ||
| } else { | ||
| ++it; | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a data race on the data_list_ member. The TtsGenerateWorker::Execute method runs on a worker thread, while InvokeJsCallback is executed on the main Node.js event loop thread.
- The worker thread reads
(*it)->processedhere andd->cancelledin the next loop. - The main thread writes to
data->processedanddata->cancelledinInvokeJsCallback.
This concurrent read/write from different threads without synchronization is a data race and can lead to undefined behavior.
To fix this, the processed and cancelled members of the TtsCallbackData struct should be declared as std::atomic<bool>.
// In non-streaming-tts.cc
#include <atomic>
struct TtsCallbackData {
std::vector<float> samples;
float progress;
std::atomic<bool> processed = {false};
std::atomic<bool> cancelled = {false};
};With this change, the concurrent access will be thread-safe.
| // Return 0 or false to cancel, any other value to continue | ||
| return false; |
There was a problem hiding this comment.
The example currently cancels the TTS generation immediately after the first chunk by returning false from the onProgress callback. This can be confusing for users who run the example expecting to get a fully generated audio file.
To make the example more intuitive, I suggest changing it to continue generation by default, and improving the comment to clarify how cancellation works.
// Return true to continue generation.
// You can return false or 0 to cancel the generation.
return true;![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/non-streaming-tts.cc`:
- Around line 430-438: The TtsCallbackData fields processed and cancelled are
written on the JS thread inside the TSFN callback and read on the worker thread
in Execute, causing a data race; fix it by changing TtsCallbackData::processed
and TtsCallbackData::cancelled to std::atomic<bool> (or wrap accesses with a
mutex) and update all reads/writes in the TSFN callback and the Execute method
to use atomic operations (or lock/unlock the mutex) so synchronization is
enforced for TtsCallbackData, ensuring thread-safe visibility between the TSFN
callback and Execute.
In `@nodejs-addon-examples/test_tts_non_streaming_kitten_en.js`:
- Around line 50-57: The onProgress callback currently returns false (in
onProgress), which per the API cancels generation and yields incomplete audio;
change the callback to return a truthy value (for example return true) or remove
the explicit return so generation continues; update the onProgress
implementation where samples and progress are handled to return true instead of
false.
🧹 Nitpick comments (1)
scripts/node-addon-api/lib/non-streaming-tts.js (1)
27-43: JSDoc nested comment workaround is fragile.The escaped
*\/inside the JSDoc block is a workaround for nested comments, but some documentation generators may not handle this correctly. Consider extracting the callback type to a separate@typedeffor cleaner documentation.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Improvements