Repository navigation
feat: Add PocketTTS cache & seed support to Node.js Addon and WASM APIs - #3206
Conversation
…nt generation Introduce a thread-safe LRU cache for voice embeddings to skip redundant Mimi encoder runs: compute a sampled hash of reference audio, return cached Ort tensor on hit, and store embeddings on miss. Expose voice_embedding_cache_capacity through model config, C API and Go wrapper, and add CLI flag pocket-voice-embedding-cache-capacity. Add deterministic NormalDataGenerator constructor (seed-aware) and use the generation 'seed' option to produce reproducible noise; preserve original thread-local RNG when seed < 0. Update config ToString and defaults, add necessary headers and logging/timing for cache hits/misses.
Several fixes and improvements across offline TTS and utilities: - c-api: minor formatting cleanup when copying segment timestamps; handle pocket voice_embedding_cache_capacity explicitly (use provided non-negative value or default to 50). - normal-data-generator: switch to an instance-level std::mt19937 seeded in the constructor for deterministic mode and use it in Fill(); added <random> and rng_ member. - offline-tts-pocket-impl.h: strengthen cache key by mixing in sample rate and length to reduce collisions; create an owned Ort tensor via AllocatorWithDefaultOptions and CreateTensor<float>(), then copy cached data into the tensor to avoid potential use-after-free. - offline-tts-pocket-model-config.cc: update help text to note that 0 disables caching and add validation to reject negative voice_embedding_cache_capacity with an error log. These changes improve determinism, correctness of cached tensor lifetimes, and robustness of the cache key and configuration validation.
Replace manual product loop with std::accumulate + std::multiplies for computing tensor element count. Change Cache::Put to take vectors by value and std::move them into the map (both on update and insert) to avoid unnecessary copies and improve performance.
- Robustify voice embedding cache key: include `max_reference_audio_len` and hash all reference audio samples to prevent collisions. - Optimization: Switch cache hit/miss logs from ERROR to INFO and make timing logic conditional on debug mode. - Refactor: Merge [NormalDataGenerator] constructors and remove unused `<strstream>` header.
Co-authored-by: Fangjun Kuang <csukuangfj@gmail.com>
Co-authored-by: Fangjun Kuang <csukuangfj@gmail.com>
Co-authored-by: Fangjun Kuang <csukuangfj@gmail.com>
…d seed support example for Pocket TTS - Add voiceEmbeddingCacheCapacity to Flutter/Dart OfflineTtsPocketModelConfig - Update FFI bindings in sherpa_onnx_bindings.dart - Update all Pocket TTS examples (Dart, Go, C) to demonstrate new parameters - Document voice_embedding_cache_capacity usage in C API example
dosubot
Bot
added
the
size:M
Summary of ChangesHello @ramishi, 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 integrates recently introduced PocketTTS features into the Node.js Addon and WebAssembly interfaces. It provides users with greater control over the PocketTTS engine by exposing parameters for managing voice embedding cache and ensuring deterministic audio output through a configurable seed. 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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Pocket TTS config field Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 successfully exposes voice_embedding_cache_capacity and seed configurations for PocketTTS to the Node.js Addon and WebAssembly APIs. The changes are well-implemented across the C++ addon, WASM wrapper, and example files. I've added a couple of minor suggestions to replace magic numbers with constants to improve code maintainability. Overall, great work!
| c.voice_embedding_cache_capacity = | ||
| o.Get("voiceEmbeddingCacheCapacity").As<Napi::Number>().Int32Value(); | ||
| } else { | ||
| c.voice_embedding_cache_capacity = 50; |
There was a problem hiding this comment.
The magic number 50 is used as a default value. It would be better to define it as a named constant to improve readability and maintainability, especially since this value is used as a default in multiple places. For example, you could define constexpr int32_t kDefaultVoiceEmbeddingCacheCapacity = 50; at a suitable scope and use it here.
| Module.setValue(ptr + 6 * 4, buffer + offset, 'i8*'); | ||
| offset += tokenScoresJsonLen; | ||
|
|
||
| Module.setValue(ptr + 7 * 4, config.voiceEmbeddingCacheCapacity !== undefined ? config.voiceEmbeddingCacheCapacity : 50, 'i32'); |
There was a problem hiding this comment.
The magic number 50 is used as a default value here and also on line 488. It's recommended to define it as a constant at a higher scope (e.g., top of the file) to avoid magic numbers and improve maintainability. For example: const DEFAULT_VOICE_EMBEDDING_CACHE_CAPACITY = 50;.
![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
🧹 Nitpick comments (1)
wasm/tts/sherpa-onnx-wasm-main-tts.cc (1)
94-101:MyPrintdoesn't log the newvoice_embedding_cache_capacityfieldAll other numeric fields of their respective configs are printed. Omitting the new field makes it harder to confirm the value was read correctly during debug sessions.
🔍 Proposed addition
fprintf(stdout, "token_scores_json: %s\n", pocket->token_scores_json); + fprintf(stdout, "voice_embedding_cache_capacity: %d\n", + pocket->voice_embedding_cache_capacity);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wasm/tts/sherpa-onnx-wasm-main-tts.cc` around lines 94 - 101, The MyPrint block that logs pocket TTS config is missing the numeric field voice_embedding_cache_capacity; update the printout in the same place that emits lm_flow/lm_main/encoder/etc. (the fprintf calls for pocket->lm_flow, pocket->encoder, etc.) to also log pocket->voice_embedding_cache_capacity so the value is visible during debugging (use the same stdout logging style and numeric format as the other numeric config fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/non-streaming-tts.cc`:
- Around line 216-221: The assignment for c.voice_embedding_cache_capacity uses
o.Has("voiceEmbeddingCacheCapacity") but lacks an IsNumber() check, so
non-numeric inputs can cause Int32Value() to produce 0; update the block that
reads o.Get("voiceEmbeddingCacheCapacity") to first check
o.Get("voiceEmbeddingCacheCapacity").IsNumber() (mirroring
SHERPA_ONNX_ASSIGN_ATTR_INT32 and the debug field handling) and only call
.As<Napi::Number>().Int32Value() when true, otherwise set
c.voice_embedding_cache_capacity to the default 50.
In `@wasm/tts/sherpa-onnx-tts.js`:
- Line 409: Replace the current undefined check on
config.voiceEmbeddingCacheCapacity with a nullish coalescing fallback so null
values don't get treated as valid; update the Module.setValue call that writes
the 7th offset (Module.setValue(ptr + 7 * 4, config.voiceEmbeddingCacheCapacity
!== undefined ? config.voiceEmbeddingCacheCapacity : 50, 'i32')) to use
config.voiceEmbeddingCacheCapacity ?? 50 instead so any null or undefined will
default to 50.
---
Nitpick comments:
In `@wasm/tts/sherpa-onnx-wasm-main-tts.cc`:
- Around line 94-101: The MyPrint block that logs pocket TTS config is missing
the numeric field voice_embedding_cache_capacity; update the printout in the
same place that emits lm_flow/lm_main/encoder/etc. (the fprintf calls for
pocket->lm_flow, pocket->encoder, etc.) to also log
pocket->voice_embedding_cache_capacity so the value is visible during debugging
(use the same stdout logging style and numeric format as the other numeric
config fields).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/non-streaming-tts.ccnodejs-addon-examples/test_tts_non_streaming_pocket_en.jsnodejs-addon-examples/test_tts_non_streaming_pocket_en_async.jsnodejs-examples/test-offline-tts-pocket-en.jsscripts/node-addon-api/lib/types.jswasm/tts/sherpa-onnx-tts.jswasm/tts/sherpa-onnx-wasm-main-tts.cc
| if (o.Has("voiceEmbeddingCacheCapacity")) { | ||
| c.voice_embedding_cache_capacity = | ||
| o.Get("voiceEmbeddingCacheCapacity").As<Napi::Number>().Int32Value(); | ||
| } else { | ||
| c.voice_embedding_cache_capacity = 50; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file around the mentioned lines
fd "non-streaming-tts.cc" -t f --exec cat -n {} \; | sed -n '210,260p'Repository: k2-fsa/sherpa-onnx
Length of output: 1974
🏁 Script executed:
# Also search for the SHERPA_ONNX_ASSIGN_ATTR_INT32 macro definition
fd -e h -e cc -e cpp | xargs grep -l "SHERPA_ONNX_ASSIGN_ATTR_INT32" | head -5Repository: k2-fsa/sherpa-onnx
Length of output: 415
🏁 Script executed:
# Search for the macro definition specifically
rg "SHERPA_ONNX_ASSIGN_ATTR_INT32" --type cpp --type h -A 3Repository: k2-fsa/sherpa-onnx
Length of output: 15535
Add IsNumber() guard before .As<Napi::Number>().Int32Value()
The code at lines 216–221 only checks o.Has("voiceEmbeddingCacheCapacity") but does not verify the value is actually a number. The SHERPA_ONNX_ASSIGN_ATTR_INT32 macro (used throughout this file) includes an explicit IsNumber() check, as does the debug field handling at lines 245–252. Without this guard, passing a non-number (e.g. a string "50") causes .As<Napi::Number>().Int32Value() to fail silently and return 0, disabling the LRU cache instead of falling back to the intended default of 50.
Proposed fix
- if (o.Has("voiceEmbeddingCacheCapacity")) {
+ if (o.Has("voiceEmbeddingCacheCapacity") &&
+ o.Get("voiceEmbeddingCacheCapacity").IsNumber()) {
c.voice_embedding_cache_capacity =
o.Get("voiceEmbeddingCacheCapacity").As<Napi::Number>().Int32Value();
} else {
c.voice_embedding_cache_capacity = 50;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/non-streaming-tts.cc`
around lines 216 - 221, The assignment for c.voice_embedding_cache_capacity uses
o.Has("voiceEmbeddingCacheCapacity") but lacks an IsNumber() check, so
non-numeric inputs can cause Int32Value() to produce 0; update the block that
reads o.Get("voiceEmbeddingCacheCapacity") to first check
o.Get("voiceEmbeddingCacheCapacity").IsNumber() (mirroring
SHERPA_ONNX_ASSIGN_ATTR_INT32 and the debug field handling) and only call
.As<Napi::Number>().Int32Value() when true, otherwise set
c.voice_embedding_cache_capacity to the default 50.
| Module.setValue(ptr + 6 * 4, buffer + offset, 'i8*'); | ||
| offset += tokenScoresJsonLen; | ||
|
|
||
| Module.setValue(ptr + 7 * 4, config.voiceEmbeddingCacheCapacity !== undefined ? config.voiceEmbeddingCacheCapacity : 50, 'i32'); |
There was a problem hiding this comment.
!== undefined doesn't guard against null — use nullish coalescing
null !== undefined is true in JavaScript, so if a caller explicitly sets voiceEmbeddingCacheCapacity: null, Module.setValue receives null which coerces to 0 (disabling the cache silently). The idiomatic fix is ??.
🛡️ Proposed fix
- Module.setValue(ptr + 7 * 4, config.voiceEmbeddingCacheCapacity !== undefined ? config.voiceEmbeddingCacheCapacity : 50, 'i32');
+ Module.setValue(ptr + 7 * 4, config.voiceEmbeddingCacheCapacity ?? 50, 'i32');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wasm/tts/sherpa-onnx-tts.js` at line 409, Replace the current undefined check
on config.voiceEmbeddingCacheCapacity with a nullish coalescing fallback so null
values don't get treated as valid; update the Module.setValue call that writes
the 7th offset (Module.setValue(ptr + 7 * 4, config.voiceEmbeddingCacheCapacity
!== undefined ? config.voiceEmbeddingCacheCapacity : 50, 'i32')) to use
config.voiceEmbeddingCacheCapacity ?? 50 instead so any null or undefined will
default to 50.
csukuangfj
left a comment
csukuangfj
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
Left a minor comment.
csukuangfj
left a comment
csukuangfj
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
2 participants
This PR exposes the recently added voice_embedding_cache_capacity and seed configurations for PocketTTS to the Node.js Addon API and WebAssembly JavaScript wrappers. #3197 3197
Summary by CodeRabbit