QVAC-23295 fix[api]: address CosyVoice3 post-merge review comments - #3737
Conversation
License compliance — cleanNo new dependency license findings in this PR. Warn-only (shadow) mode — this check does not block merges yet. Updated automatically by the canonical license compliance workflow. NOTICE presence (advisory)Missing NOTICE (advisory, does not block):
|
Review StatusCurrent Status: ✅ APPROVED |
Follow-up on the merged CosyVoice3 engine (#3446), addressing four review comments in @qvac/tts-ggml: - Forward openclCacheDir to the native engine. The JS layer sent it but the addon dropped it, so CosyVoice3's Android OpenCL/Adreno GPU path recompiled its kernels every process instead of reusing the persistent cache. Add CosyvoiceConfig::openclCacheDir, read it in buildCosyvoiceConfig, and forward it to EngineOptions::opencl_cache_dir. Expose toEngineOptions as a free function so the GPU/OpenCL forwarding is unit-tested without weights. - Mark streamLeftContextTokens reserved / not yet effective. The pinned tts-cpp engine accepts but does not read it, so setting it has no effect; document it like cfmSteps in both the TS surface and CosyvoiceConfig. - Harden instruct validation. A set-but-empty or null control ({ dialect: '' }, { dialect: null }), an array, or a non-object silently degraded to zero-shot synthesis. Reject non-object/array values and validate controls by property presence rather than truthiness, with regression coverage. - Fix the stale CPU-only contract. The pinned engine now runs CosyVoice3 on the OpenCL/Adreno GPU path when nGpuLayers/useGpu request it, and the addon forwards that intent. Update CosyvoiceConfig and the README accordingly, and add GPU-forwarding tests. Also resolve stray merge-conflict markers left in the CHANGELOG.
1fe2e2a to
1da1bb8
Compare
There was a problem hiding this comment.
[medium] packages/tts-ggml/src/index.ts — line 149 (if (instruct == null) return "";)
An explicit instruct: null still silently selects zero-shot synthesis, contradicting the new validation contract. Treat only undefined as omitted, reject null, add a regression test, and regenerate index.js.
[low] packages/tts-ggml/src/index.ts — line 155 (typeof instruct !== "object" || Array.isArray(instruct))
This does not enforce the stated plain-object requirement: new Date() silently becomes zero-shot, while inherited controls can be accepted. Require an ordinary object with own control properties and test these cases.
[medium] packages/tts-ggml/addon/src/js-interface/JSAdapter.cpp — line 286 (cfg.openclCacheDir =)
The native test constructs CosyvoiceConfig directly, so it cannot catch the original JS-boundary regression. Add coverage proving a JavaScript openclCacheDir passes through buildCosyvoiceConfig into EngineOptions::opencl_cache_dir.
[medium] packages/tts-ggml/README.md — line 560 (streamLeftContextTokens)
This table still describes the option as effective for CosyVoice3 and Chatterbox, contradicting the updated declaration and nearby documentation. Mark it CosyVoice3-only and reserved/not yet effective.
[low] README.md — line 41 (Text-to-Speech)
The repository documentation rule requires both README levels to reflect changed public functionality. Update the root TTS entry to mention CosyVoice3’s opt-in Android OpenCL/Adreno offload.
Second review round on the CosyVoice3 follow-ups: - Reject an explicit `instruct: null`. Only `undefined` counts as omitted (zero-shot); `null` is now treated as a malformed value like any other non-object. - Require a plain control object. A `Date`, class instance, or other exotic object previously slipped past the object check and degraded to zero-shot, and controls could be picked up from an inherited prototype. Validate the value defensively (own-property, ordinary object) so only own control keys of a plain object are honored. - Cover the openclCacheDir JS boundary. Add a unit test proving the JS layer emits `openclCacheDir` into the native configuration params (the key the addon's buildCosyvoiceConfig reads), from both the option and config; the config -> EngineOptions::opencl_cache_dir half stays covered by test_cosyvoice_config.cpp. - README: mark `streamLeftContextTokens` CosyVoice3-only and reserved / not yet effective in the options table, and note CosyVoice3's opt-in Android OpenCL/Adreno GPU offload in the root package overview.
…ice-review-followups # Conflicts: # README.md
4 participants
Summary
Follow-up on the merged CosyVoice3 engine (#3446), addressing the four post-merge
review comments in
@qvac/tts-ggml.openclCacheDirsent by JS but dropped at the C++ boundaryCosyvoiceConfig::openclCacheDir→ read inbuildCosyvoiceConfig→ forwarded toEngineOptions::opencl_cache_dirintoEngineOptions. Without it the Android OpenCL/Adreno GPU path recompiled kernels every process.streamLeftContextTokensdocumented as effectiveindex.d.ts, andCosyvoiceConfig.hpp, mirroringcfmSteps.instructaccepted malformed values{ dialect: '' },{ dialect: null }, arrays, and non-objects silently degraded to zero-shot. Now rejected: non-object/array values throw, and controls are validated by property presence rather than truthiness.n_gpu_layers > 0on the OpenCL/Adreno path and the addon forwardsuseGPU/nGpuLayers. UpdatedCosyvoiceConfigdocs + README (CPU-only claims), and added GPU-forwarding tests.Also resolves stray merge-conflict markers left in
CHANGELOG.mdby an earlier merge.Testing
toEngineOptionsexposed as a free function so the OpenCL cache-dir and GPUintent forwarding are unit-tested without model weights
(
addon/tests/test_cosyvoice_config.cpp).test/unit/cosyvoice3.inference.test.js— instruct regression cases forempty/null/array/number and the undefined-skip precedence path. 20/20 pass;
88/88 across the other mocked engine suites.
index.js/index.d.tsregenerated fromsrc/index.ts; eslint / prettier /lunte / clang-format / clang-tidy clean.
No version bump here (kept for a separate release PR per convention).