Add JavaScript (WebAssembly) API for PocketTTS - #3163
Conversation
dosubot
Bot
added
the
size:L
📝 WalkthroughWalkthroughAdds Pocket TTS support and per-call generation config (generateWithConfig) across C++/WASM/JS stacks, extends WASM runtime exports to expose HEAP views, adds language/hotwords fields to ASR JS configs, includes a Node.js Pocket TTS example and README instructions, and removes an unused import in a Node example. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OfflineTts
participant WASM
participant GenConfig
Client->>OfflineTts: new OfflineTts(config with pocket)
OfflineTts->>WASM: initSherpaOnnxOfflineTtsModelConfig(config)
WASM-->>OfflineTts: modelConfigPtr
Client->>GenConfig: create genConfig (referenceAudio, steps, extra)
GenConfig->>WASM: initSherpaOnnxGenerationConfig(genConfig)
WASM-->>GenConfig: genConfigPtr
Client->>OfflineTts: generateWithConfig(text, genConfig)
OfflineTts->>WASM: SherpaOnnxOfflineTtsGenerateWithConfig(text, genConfigPtr)
WASM->>WASM: synthesize using pocket model & genConfig
WASM-->>OfflineTts: audioPtr, sampleCount
OfflineTts-->>Client: Float32Array samples
Client->>GenConfig: freeSherpaOnnxGenerationConfig(genConfigPtr)
WASM-->>GenConfig: freed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
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 expands the project's capabilities by integrating PocketTTS, a zero-shot Text-to-Speech model, into its JavaScript (WebAssembly) API. The changes facilitate advanced TTS generation with configurable options and improve the underlying WebAssembly infrastructure by exposing more memory manipulation methods. This allows for greater control and flexibility when working with speech synthesis and recognition models in JavaScript environments. 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
|
There was a problem hiding this comment.
Pull request overview
This PR extends the JavaScript (WebAssembly) bindings to support PocketTTS model configuration and adds a new generateWithConfig() pathway (backed by SherpaOnnxOfflineTtsGenerateWithConfig) for zero-shot / reference-audio-driven generation. It also updates WASM build flags to expose Emscripten heap views needed by the JS wrappers, and aligns ASR JS struct packing with updated C-API structs.
Changes:
- Add PocketTTS model config packing/printing in WASM TTS + JS wrapper support.
- Introduce JS allocation/free helpers for
SherpaOnnxGenerationConfigand exposeOfflineTts.generateWithConfig(). - Update multiple WASM targets’ exported runtime methods to include
HEAP*views; update ASR JS struct packing and add a Node.js example + README docs for PocketTTS.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| wasm/vad/CMakeLists.txt | Export Emscripten heap views via EXPORTED_RUNTIME_METHODS for JS access. |
| wasm/vad-asr/CMakeLists.txt | Same heap-view export update for the vad-asr WASM build. |
| wasm/tts/sherpa-onnx-wasm-main-tts.cc | Add PocketTTS config size assertions + print support; assert SherpaOnnxGenerationConfig size. |
| wasm/tts/sherpa-onnx-tts.js | Add PocketTTS config packing + new GenerationConfig packing and generateWithConfig() API. |
| wasm/tts/CMakeLists.txt | Export new SherpaOnnxOfflineTtsGenerateWithConfig symbol + heap views for TTS WASM. |
| wasm/speech-enhancement/CMakeLists.txt | Export heap views for speech-enhancement WASM. |
| wasm/speaker-diarization/CMakeLists.txt | Export heap views for speaker-diarization WASM. |
| wasm/nodejs/sherpa-onnx-wasm-nodejs.cc | Update static_assert sizes to match updated ASR C-API structs. |
| wasm/nodejs/CMakeLists.txt | Export new TTS symbol + heap views for Node.js WASM build. |
| wasm/kws/CMakeLists.txt | Export heap views for KWS WASM. |
| wasm/asr/sherpa-onnx-asr.js | Update JS struct packing for FunASR Nano + Whisper to match updated C-API fields. |
| wasm/asr/CMakeLists.txt | Export heap views for ASR WASM. |
| nodejs-examples/test-offline-tts-pocket-en.js | New example demonstrating PocketTTS zero-shot generation using generateWithConfig(). |
| nodejs-examples/test-offline-medasr-ctc.js | Remove unused wav import. |
| nodejs-examples/README.md | Document the new PocketTTS Node.js example and model download steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| generateWithConfig(text, genConfig) { | ||
| console.log('started'); | ||
| // 1️⃣ Allocate SherpaOnnxGenerationConfig in WASM | ||
| const cfgWasm = initSherpaOnnxGenerationConfig(genConfig, this.Module); |
There was a problem hiding this comment.
Unconditional console.log('started') in generateWithConfig() will spam stdout for every synthesis. Remove it or gate behind a debug flag.
| function initSherpaOnnxGenerationConfig(config, Module) { | ||
| console.log(`here config: ${config}`); | ||
| // Allocate memory for the struct itself (size = 7 * 4 bytes + pointer sizes) |
There was a problem hiding this comment.
initSherpaOnnxGenerationConfig() contains an unconditional console.log of the config object. This is noisy in production use and the template string will log [object Object] rather than helpful content; remove it or gate behind an explicit debug flag (and use JSON.stringify if you truly need it).
| // Allocate memory for the struct itself (size = 7 * 4 bytes + pointer sizes) | ||
| // Assuming 32-bit system, each float/int32 = 4 bytes, each pointer = 4 bytes | ||
| const len = 8 * 4; // 8 fields in your struct |
There was a problem hiding this comment.
initSherpaOnnxGenerationConfig() allocates only 8*4 bytes for SherpaOnnxGenerationConfig, but the C struct has 9 fields (see SherpaOnnxGenerationConfig in c-api.h) and this function writes 9 values (ending with extra). This will write past the allocated buffer and can corrupt WASM memory; allocate 9 * 4 bytes (wasm32) and keep the field count consistent with the C struct.
| // Allocate memory for the struct itself (size = 7 * 4 bytes + pointer sizes) | |
| // Assuming 32-bit system, each float/int32 = 4 bytes, each pointer = 4 bytes | |
| const len = 8 * 4; // 8 fields in your struct | |
| // Allocate memory for the struct itself (size = 9 * 4 bytes on wasm32) | |
| // Assuming 32-bit system, each float/int32/pointer field is 4 bytes | |
| const len = 9 * 4; // 9 fields in the struct |
| // const char* extra | ||
| let extraPtr = 0; | ||
|
|
||
| if (config.extra && typeof config.extra === 'object') { | ||
| config.extra = JSON.stringify(config.extra); | ||
| } | ||
|
|
||
| Module.setValue(ptr + offset, extraPtr, 'i8*'); | ||
| offset += 4; |
There was a problem hiding this comment.
extra is never actually passed into the WASM struct: extraPtr stays 0 and no memory is allocated for the JSON/string. Serialize to a string (without mutating the caller’s object), allocate space with _malloc, write it via stringToUTF8, and store the pointer in the struct so model-specific options can work.
| silence_scale: 0.2, | ||
| speed: 1.0, | ||
| sid: 1, | ||
| reference_audio: myFloat32Array, // optional | ||
| reference_sample_rate: 16000, | ||
| reference_text: "Hello world", | ||
| num_steps: 20, | ||
| extra: { bar: "ok", foo: 0.8, foobar: 10} | ||
| }; | ||
| }; | ||
|
|
||
| */ | ||
|
|
There was a problem hiding this comment.
The commented genConfig example uses snake_case keys (e.g., silence_scale, reference_audio) and has an extra closing };, but the actual JS API in this file expects camelCase (e.g., silenceScale, referenceAudio). Update the example to match the public JS-facing config shape to avoid users passing ignored fields.
| silence_scale: 0.2, | |
| speed: 1.0, | |
| sid: 1, | |
| reference_audio: myFloat32Array, // optional | |
| reference_sample_rate: 16000, | |
| reference_text: "Hello world", | |
| num_steps: 20, | |
| extra: { bar: "ok", foo: 0.8, foobar: 10} | |
| }; | |
| }; | |
| */ | |
| silenceScale: 0.2, | |
| speed: 1.0, | |
| sid: 1, | |
| referenceAudio: myFloat32Array, // optional | |
| referenceSampleRate: 16000, | |
| referenceText: "Hello world", | |
| numSteps: 20, | |
| extra: { bar: "ok", foo: 0.8, foobar: 10 }, | |
| }; | |
| */ |
| constructor(configObj, Module) { | ||
| console.log('creating'); | ||
| console.log(configObj) | ||
| const config = initSherpaOnnxOfflineTtsConfig(configObj, Module) | ||
| console.log('config:'); | ||
| const handle = Module._SherpaOnnxCreateOfflineTts(config.ptr); | ||
| console.log('created'); | ||
|
|
There was a problem hiding this comment.
There are multiple unconditional console.log statements added in the TTS constructor. This will spam stdout for library consumers (including nodejs package usage). Remove these logs or gate them behind an explicit debug flag in the config.
| const h = this.Module._SherpaOnnxOfflineTtsGenerate( | ||
| this.handle, textPtr, config.sid, config.speed); | ||
| console.log(this.Module); | ||
|
|
There was a problem hiding this comment.
Unconditional console.log(this.Module) in generate() will print the whole Emscripten module object for every call, which is very noisy and can be expensive. Remove it or guard behind a debug flag.
|
|
||
| const tts = createOfflineTts(); | ||
| const text = | ||
| 'Today as always, men fall into two groups: slaves and free men. Whoever does not have two-thirds of his day for himself, is a slave, whatever he may be: a statesman, a businessman, an official, or a scholar.' |
There was a problem hiding this comment.
Avoid automated semicolon insertion (90% of all statements in the enclosing script have an explicit semicolon).
| 'Today as always, men fall into two groups: slaves and free men. Whoever does not have two-thirds of his day for himself, is a slave, whatever he may be: a statesman, a businessman, an official, or a scholar.' | |
| 'Today as always, men fall into two groups: slaves and free men. Whoever does not have two-thirds of his day for himself, is a slave, whatever he may be: a statesman, a businessman, an official, or a scholar.'; |
![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 support for PocketTTS in the JavaScript/WebAssembly API, complete with a new example and necessary updates to the JS wrapper and build system. It also includes some beneficial, though unrelated, enhancements for FunASR Nano and Whisper models. While the changes are largely positive, I've identified a critical memory allocation bug in wasm/tts/sherpa-onnx-tts.js that could lead to a buffer overflow, along with an issue where a configuration field is not passed correctly. Additionally, there are several leftover debugging logs and a minor syntax issue in a comment that should be addressed before merging.
| function initSherpaOnnxGenerationConfig(config, Module) { | ||
| console.log(`here config: ${config}`); | ||
| // Allocate memory for the struct itself (size = 7 * 4 bytes + pointer sizes) | ||
| // Assuming 32-bit system, each float/int32 = 4 bytes, each pointer = 4 bytes | ||
| const len = 8 * 4; // 8 fields in your struct | ||
| const ptr = Module._malloc(len); | ||
|
|
||
| let offset = 0; | ||
|
|
||
| // float silence_scale | ||
| Module.setValue(ptr + offset, config.silenceScale || 0.2, 'float'); | ||
| offset += 4; | ||
|
|
||
| // float speed | ||
| Module.setValue(ptr + offset, config.speed || 1.0, 'float'); | ||
| offset += 4; | ||
|
|
||
| // int32_t sid | ||
| Module.setValue(ptr + offset, config.sid || 0, 'i32'); | ||
| offset += 4; | ||
|
|
||
| // const float* reference_audio | ||
| let referenceAudioPtr = 0; | ||
| if (config.referenceAudio && config.referenceAudio.length > 0) { | ||
| referenceAudioPtr = Module._malloc(config.referenceAudio.length * 4); | ||
| Module.HEAPF32.set(config.referenceAudio, referenceAudioPtr / 4); | ||
| } | ||
| Module.setValue(ptr + offset, referenceAudioPtr, 'i8*'); | ||
| offset += 4; | ||
|
|
||
| // int32_t reference_audio_len | ||
| Module.setValue( | ||
| ptr + offset, config.referenceAudio ? config.referenceAudio.length : 0, | ||
| 'i32'); | ||
| offset += 4; | ||
|
|
||
| // int32_t reference_sample_rate | ||
| Module.setValue(ptr + offset, config.referenceSampleRate || 0, 'i32'); | ||
| offset += 4; | ||
|
|
||
| // const char* reference_text | ||
| let referenceTextPtr = 0; | ||
| if (config.referenceText) { | ||
| const textLen = Module.lengthBytesUTF8(config.referenceText) + 1; | ||
| referenceTextPtr = Module._malloc(textLen); | ||
| Module.stringToUTF8(config.referenceText, referenceTextPtr, textLen); | ||
| } | ||
| Module.setValue(ptr + offset, referenceTextPtr, 'i8*'); | ||
| offset += 4; | ||
|
|
||
| // int32_t num_steps | ||
| Module.setValue(ptr + offset, config.numSteps || 5, 'i32'); | ||
| offset += 4; | ||
|
|
||
| // const char* extra | ||
| let extraPtr = 0; | ||
|
|
||
| if (config.extra && typeof config.extra === 'object') { | ||
| config.extra = JSON.stringify(config.extra); | ||
| } | ||
|
|
||
| Module.setValue(ptr + offset, extraPtr, 'i8*'); | ||
| offset += 4; | ||
|
|
||
| return { | ||
| ptr, | ||
| referenceAudioPtr, | ||
| referenceTextPtr, | ||
| extraPtr, | ||
| }; | ||
| } |
There was a problem hiding this comment.
There are a few issues in initSherpaOnnxGenerationConfig:
- Critical Bug: The memory allocated for the
SherpaOnnxGenerationConfigstruct is incorrect. The C++ struct has 9 fields, requiring9 * 4 = 36bytes on a 32-bit system, but only8 * 4 = 32bytes are allocated. This will cause a buffer overflow when writing the last field. - Bug: The
extraconfiguration field is not handled correctly. The code stringifies it but doesn't allocate memory for the string in the WASM heap or pass the pointer to C++. TheextraPtris always 0. - Cleanup: There are debug
console.logstatements that should be removed.
The suggested change fixes these issues and also avoids modifying the input config object, which is a good practice.
function initSherpaOnnxGenerationConfig(config, Module) {
// The C++ struct has 9 fields.
const len = 9 * 4;
const ptr = Module._malloc(len);
let offset = 0;
// float silence_scale
Module.setValue(ptr + offset, config.silenceScale || 0.2, 'float');
offset += 4;
// float speed
Module.setValue(ptr + offset, config.speed || 1.0, 'float');
offset += 4;
// int32_t sid
Module.setValue(ptr + offset, config.sid || 0, 'i32');
offset += 4;
// const float* reference_audio
let referenceAudioPtr = 0;
if (config.referenceAudio && config.referenceAudio.length > 0) {
referenceAudioPtr = Module._malloc(config.referenceAudio.length * 4);
Module.HEAPF32.set(config.referenceAudio, referenceAudioPtr / 4);
}
Module.setValue(ptr + offset, referenceAudioPtr, 'i8*');
offset += 4;
// int32_t reference_audio_len
Module.setValue(
ptr + offset, config.referenceAudio ? config.referenceAudio.length : 0,
'i32');
offset += 4;
// int32_t reference_sample_rate
Module.setValue(ptr + offset, config.referenceSampleRate || 0, 'i32');
offset += 4;
// const char* reference_text
let referenceTextPtr = 0;
if (config.referenceText) {
const textLen = Module.lengthBytesUTF8(config.referenceText) + 1;
referenceTextPtr = Module._malloc(textLen);
Module.stringToUTF8(config.referenceText, referenceTextPtr, textLen);
}
Module.setValue(ptr + offset, referenceTextPtr, 'i8*');
offset += 4;
// int32_t num_steps
Module.setValue(ptr + offset, config.numSteps || 5, 'i32');
offset += 4;
// const char* extra
let extraPtr = 0;
if (config.extra) {
let extraStr = '';
if (typeof config.extra === 'object') {
extraStr = JSON.stringify(config.extra);
} else {
extraStr = config.extra.toString();
}
const extraLen = Module.lengthBytesUTF8(extraStr) + 1;
extraPtr = Module._malloc(extraLen);
Module.stringToUTF8(extraStr, extraPtr, extraLen);
}
Module.setValue(ptr + offset, extraPtr, 'i8*');
offset += 4;
return {
ptr,
referenceAudioPtr,
referenceTextPtr,
extraPtr,
};
}| num_steps: 20, | ||
| extra: { bar: "ok", foo: 0.8, foobar: 10} | ||
| }; | ||
| }; |
There was a problem hiding this comment.
This line contains an extra }; within a comment block. While it doesn't affect execution, it's syntactically incorrect for the commented-out code and could be confusing. It should be removed for clarity.
| console.log('creating'); | ||
| console.log(configObj) | ||
| const config = initSherpaOnnxOfflineTtsConfig(configObj, Module) | ||
| console.log('config:'); | ||
| const handle = Module._SherpaOnnxCreateOfflineTts(config.ptr); | ||
| console.log('created'); |
There was a problem hiding this comment.
These console.log statements appear to be for debugging and should be removed before merging.
const config = initSherpaOnnxOfflineTtsConfig(configObj, Module)
const handle = Module._SherpaOnnxCreateOfflineTts(config.ptr);|
|
||
| const h = this.Module._SherpaOnnxOfflineTtsGenerate( | ||
| this.handle, textPtr, config.sid, config.speed); | ||
| console.log(this.Module); |
There was a problem hiding this comment.
This console.log statement appears to be for debugging and should be removed.
| } | ||
|
|
||
| generateWithConfig(text, genConfig) { | ||
| console.log('started'); |
There was a problem hiding this comment.
This console.log statement appears to be for debugging and should be removed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@nodejs-examples/test-offline-tts-pocket-en.js`:
- Around line 32-42: The binding currently stringifies config.extra but never
allocates/copies it into WASM memory so extraPtr stays NULL; update the function
in wasm/tts/sherpa-onnx-tts.js that handles config (the same code that processes
referenceText) to: if config.extra exists, JSON.stringify it, allocate WASM
memory for the string, copy the bytes into the allocated buffer, set extraPtr to
that pointer, and ensure extraPtr is passed to the C API call (mirror the
referenceText allocation and free pattern used around the referenceText
handling).
In `@wasm/tts/sherpa-onnx-tts.js`:
- Around line 612-682: The function initSherpaOnnxGenerationConfig allocates len
= 8*4 but writes 9 fields causing a heap overflow and also never stores the
serialized extra string into WASM memory; fix by changing the struct allocation
to 9*4 (or compute fieldsCount = 9) and use that for Module._malloc, and when
config.extra exists (after JSON.stringify), allocate extraPtr =
Module._malloc(lengthBytesUTF8+1), call Module.stringToUTF8(config.extra,
extraPtr, ...), then Module.setValue(ptr + offsetForExtra, extraPtr, 'i8*') so
the extra pointer is written into the struct (refer to symbols:
initSherpaOnnxGenerationConfig, len, ptr, extraPtr, config.extra,
Module.stringToUTF8).
In `@wasm/tts/sherpa-onnx-wasm-main-tts.cc`:
- Line 35: The struct SherpaOnnxGenerationConfig is 9×4 bytes but
initSherpaOnnxGenerationConfig in the JS allocator only reserves 8×4 bytes,
causing a heap overflow when writing the extra field; fix by updating the
allocator to reserve 9 * 4 = 36 bytes (or change the native struct to 8 fields
and update the static_assert accordingly) so the extra field write is within
bounds—specifically modify initSherpaOnnxGenerationConfig to allocate 36 bytes
to match SherpaOnnxGenerationConfig (and keep
static_assert(sizeof(SherpaOnnxGenerationConfig) == 9 * 4, "") consistent with
the JS allocation if you choose the 9-field layout).
🧹 Nitpick comments (5)
wasm/tts/sherpa-onnx-tts.js (2)
596-609: Doc comment uses snake_case but the code uses camelCase.The example comment shows
silence_scale,reference_audio,reference_sample_rate,reference_text,num_steps— but the actualinitSherpaOnnxGenerationConfigreadssilenceScale,referenceAudio,referenceSampleRate,referenceText,numSteps. There's also a stray extra};on line 607.
613-613: Remove debugconsole.logstatements before merging.Lines 613, 697, 698, 700, 702, 729, and 745 contain
console.logcalls that appear to be development-time debug artifacts. These will produce noisy output in production.Also applies to: 697-702, 729-729, 745-745
nodejs-examples/test-offline-tts-pocket-en.js (1)
44-51: Missing semicolons on lines 41 and 46.Lines 41 and 46 are missing trailing semicolons. While JavaScript's ASI handles this, the rest of the file uses semicolons consistently — these two omissions break the convention.
🔧 Add missing semicolons
- extra: {max_reference_audio_len: 12} + extra: {max_reference_audio_len: 12}, };- 'Today as always, men fall into two groups: slaves and free men. Whoever does not have two-thirds of his day for himself, is a slave, whatever he may be: a statesman, a businessman, an official, or a scholar.' + 'Today as always, men fall into two groups: slaves and free men. Whoever does not have two-thirds of his day for himself, is a slave, whatever he may be: a statesman, a businessman, an official, or a scholar.';nodejs-examples/README.md (2)
63-69: Add language specifier to the fenced code block.The code block on line 63 is missing a language specifier, which was also flagged by markdownlint (MD040).
🔧 Fix
-``` +```bash curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/tts-models/sherpa-onnx-pocket-tts-int8-2026-01-26.tar.bz2
73-73: Usepythoninstead ofpython3as the fenced code block language.Standard markdown renderers recognize
python, notpython3, for syntax highlighting.🔧 Fix
-```python3 +```python
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wasm/tts/sherpa-onnx-tts.js`:
- Around line 619-625: The code uses the || operator which treats legitimate 0
values as falsy (e.g., Module.setValue calls for config.silenceScale,
config.speed and config.numSteps), so replace those defaulting expressions with
nullish coalescing (use ??) to only fall back on the default when the config
property is null or undefined; update the Module.setValue invocations that read
config.silenceScale, config.speed, config.sid and the numSteps usage to use
config.property ?? defaultValue (keep types same: 'float' for
silenceScale/speed, 'i32' for sid/numSteps) so explicit 0 values are preserved.
🧹 Nitpick comments (2)
wasm/tts/sherpa-onnx-tts.js (2)
616-616: Unused variableoffset.
offsetis declared on line 616 but never read or written to again — all struct fields are addressed via explicitptr + N * 4expressions.Proposed fix
- let offset = 0; - // float silence_scale
596-609: Stray};in the doc comment example.Line 607 has an extra
};that makes the example syntactically invalid. Minor documentation nit.Proposed fix
extra: { bar: "ok", foo: 0.8, foobar: 10} }; -};
| Module.setValue(ptr + 0 * 4, config.silenceScale || 0.2, 'float'); | ||
|
|
||
| // float speed | ||
| Module.setValue(ptr + 1 * 4, config.speed || 1.0, 'float'); | ||
|
|
||
| // int32_t sid | ||
| Module.setValue(ptr + 2 * 4, config.sid || 0, 'i32'); |
There was a problem hiding this comment.
Falsy-value gotcha: config.speed || 1.0 silently overrides an explicit 0.
Using || for defaults treats 0 as falsy. If a caller intentionally passes speed: 0 or silenceScale: 0, the fallback value is used instead. The same pattern applies to numSteps on line 653.
Proposed fix using nullish coalescing
- Module.setValue(ptr + 0 * 4, config.silenceScale || 0.2, 'float');
+ Module.setValue(ptr + 0 * 4, config.silenceScale ?? 0.2, 'float');
- Module.setValue(ptr + 1 * 4, config.speed || 1.0, 'float');
+ Module.setValue(ptr + 1 * 4, config.speed ?? 1.0, 'float');
- Module.setValue(ptr + 2 * 4, config.sid || 0, 'i32');
+ Module.setValue(ptr + 2 * 4, config.sid ?? 0, 'i32');And similarly for numSteps on line 653:
- Module.setValue(ptr + 7 * 4, config.numSteps || 5, 'i32');
+ Module.setValue(ptr + 7 * 4, config.numSteps ?? 5, 'i32');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Module.setValue(ptr + 0 * 4, config.silenceScale || 0.2, 'float'); | |
| // float speed | |
| Module.setValue(ptr + 1 * 4, config.speed || 1.0, 'float'); | |
| // int32_t sid | |
| Module.setValue(ptr + 2 * 4, config.sid || 0, 'i32'); | |
| Module.setValue(ptr + 0 * 4, config.silenceScale ?? 0.2, 'float'); | |
| // float speed | |
| Module.setValue(ptr + 1 * 4, config.speed ?? 1.0, 'float'); | |
| // int32_t sid | |
| Module.setValue(ptr + 2 * 4, config.sid ?? 0, 'i32'); |
🤖 Prompt for AI Agents
In `@wasm/tts/sherpa-onnx-tts.js` around lines 619 - 625, The code uses the ||
operator which treats legitimate 0 values as falsy (e.g., Module.setValue calls
for config.silenceScale, config.speed and config.numSteps), so replace those
defaulting expressions with nullish coalescing (use ??) to only fall back on the
default when the config property is null or undefined; update the
Module.setValue invocations that read config.silenceScale, config.speed,
config.sid and the numSteps usage to use config.property ?? defaultValue (keep
types same: 'float' for silenceScale/speed, 'i32' for sid/numSteps) so explicit
0 values are preserved.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (config.extra && typeof config.extra === 'object') { | ||
| config.extra = JSON.stringify(config.extra); | ||
|
|
||
| const extraLen = Module.lengthBytesUTF8(config.extra) + 1; | ||
| extraPtr = Module._malloc(extraLen); | ||
| Module.stringToUTF8(config.extra, extraPtr, extraLen); |
There was a problem hiding this comment.
initSherpaOnnxGenerationConfig() mutates the caller-provided config by overwriting config.extra with a JSON string, which can surprise callers if they reuse the object. Also, extra is ignored when it is already a string (the C API expects a JSON string), so passing a pre-serialized JSON string currently results in extraPtr staying 0. Please avoid mutating config (use a local variable) and support both object and string inputs for extra.
| if (config.extra && typeof config.extra === 'object') { | |
| config.extra = JSON.stringify(config.extra); | |
| const extraLen = Module.lengthBytesUTF8(config.extra) + 1; | |
| extraPtr = Module._malloc(extraLen); | |
| Module.stringToUTF8(config.extra, extraPtr, extraLen); | |
| let extraStr = null; | |
| if (config.extra) { | |
| if (typeof config.extra === 'object') { | |
| extraStr = JSON.stringify(config.extra); | |
| } else if (typeof config.extra === 'string') { | |
| extraStr = config.extra; | |
| } | |
| } | |
| if (extraStr !== null) { | |
| const extraLen = Module.lengthBytesUTF8(extraStr) + 1; | |
| extraPtr = Module._malloc(extraLen); | |
| Module.stringToUTF8(extraStr, extraPtr, extraLen); |
| generateWithConfig(text, genConfig) { | ||
| // 1️⃣ Allocate SherpaOnnxGenerationConfig in WASM | ||
| const cfgWasm = initSherpaOnnxGenerationConfig(genConfig, this.Module); | ||
|
|
||
| // 2️⃣ Allocate text in WASM | ||
| const textLen = this.Module.lengthBytesUTF8(text) + 1; |
There was a problem hiding this comment.
The step comments in generateWithConfig() use emoji (e.g., 1️⃣). Non-ASCII characters in source comments can cause issues with some tooling/encodings and make grepping harder. Consider replacing them with plain ASCII numbering (e.g., // 1. ...).
| referenceSample_rate: 16000, // used if referenceAudio is required | ||
| referenceText: "Hello world", // optional | ||
| numSteps: 5, // optional | ||
| extra: { bar: "ok", foo: 0.8, foobar: 10} | ||
| }; | ||
| }; |
There was a problem hiding this comment.
The example genConfig block is misleading: it uses referenceSample_rate (underscore) while the actual API reads referenceSampleRate, and the snippet contains an extra }; which makes it look like invalid JS. Please update the commented example to match the supported property names and valid syntax to avoid confusing users.
| referenceSample_rate: 16000, // used if referenceAudio is required | |
| referenceText: "Hello world", // optional | |
| numSteps: 5, // optional | |
| extra: { bar: "ok", foo: 0.8, foobar: 10} | |
| }; | |
| }; | |
| referenceSampleRate: 16000, // used if referenceAudio is required | |
| referenceText: "Hello world", // optional | |
| numSteps: 5, // optional | |
| extra: { bar: "ok", foo: 0.8, foobar: 10 }, | |
| }; |
| let offset = 0; | ||
|
|
There was a problem hiding this comment.
let offset = 0; is declared but never used in initSherpaOnnxGenerationConfig(). Please remove it to avoid dead code and reduce confusion (offset-based writes are used elsewhere in this file, so an unused variable here looks accidental).
| let offset = 0; |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wasm/tts/sherpa-onnx-tts.js (1)
718-736:⚠️ Potential issue | 🟡 MinorPre-existing leak:
textPtris never freed ingenerate().While not introduced by this PR,
generate()allocatestextPtron line 720 but never frees it. The newgenerateWithConfig()correctly frees itstextPtr(line 771). Consider fixing the older method for consistency.Proposed fix
const samples = new Float32Array(numSamples); for (let i = 0; i < numSamples; i++) { samples[i] = this.Module.HEAPF32[samplesPtr + i]; } this.Module._SherpaOnnxDestroyOfflineTtsGeneratedAudio(h); + this.Module._free(textPtr); return {samples: samples, sampleRate: sampleRate};
Summary by CodeRabbit
New Features
Bug Fixes
Chores