fix: Make useGPU flag more meaningful

[ishanvohra2]

🎯 What problem does this PR solve?

• The useGPU flag was plumbed from JS into the tts-ggml C++ addon but did not produce an observable backend change in all paths. With useGPU:false and no explicit nGpuLayers, n_gpu_layers was left at the tts-cpp library default — so a CPU request could silently still run on GPU.
• useGPU and nGpuLayers could disagree (e.g. useGPU:false + nGpuLayers:99); nGpuLayers silently won, giving the user the opposite backend they asked for.
• C++-only callers couldn't distinguish "unspecified" from "explicitly false" for useGPU because readOptionalBool collapsed both to false.

📝 How does it solve it?

• useGpu is now tri-state on the C++ side. ChatterboxConfig::useGpu and SupertonicConfig::useGpu are std::optional<bool>. JSAdapter::readOptionalBool returns std::optional<bool> so absence and explicit-false are distinguishable.
• Explicit useGPU always produces an explicit n_gpu_layers. In ChatterboxModel::toEngineOptions / SupertonicModel::toEngineOptions, useGpu=true → n_gpu_layers=99, useGpu=false → n_gpu_layers=0. No reliance on the tts-cpp library default.
• Conflict rejection (engine-agnostic). A single conflict check at the top of index.js::_validateConfig rejects useGPU + nGpuLayers mismatches for both Chatterbox and Supertonic before any engine-specific GPU policy fires. ChatterboxModel::validateConfig and SupertonicModel::validateConfig mirror the same check as defense-in-depth for direct C++ callers.
• != 0 predicate for "wants GPU", so a future llama.cpp-style nGpuLayers: -1 ("offload all layers") is treated correctly and doesn't falsely trip the conflict gate against useGPU:true.
• CPU-pin smoke coverage. gpu-smoke.test.js adds:
  • Chatterbox useGPU:false → asserts backendDevice=0, backendId=0.
  • Supertonic useGPU:false → asserts backendDevice=0, backendId=0.

🧪 How was it tested?

• bare test/integration/gpu-smoke.test.js on darwin/Apple M2 — all CPU + GPU smoke entries pass.

• Manual examples/chatterbox-tts.js run with useGPU:true → log shows ggml_metal_* initialization, Metal kernels compiled, stats backendDevice:1, backendId:1, RTF:0.72.

• Manual perf comparison on M2 (Chatterbox, ~30-word prompt, fixed seed):

  Run	backendDevice	backendId	totalTime	RTF
  useGPU:true	1 (GPU)	1 (Metal)	8.81s	0.997
  useGPU:false	0 (CPU)	0 (CPU)	27.47s	3.039

  → 3.05x speedup, confirms the flag is meaningful end-to-end. (One-shot manual artifact, not a CI gate — perf tests don't belong in the smoke suite.)

• C++ unit tests (test_chatterbox_config.cpp, test_supertonic_config.cpp) updated for the new std::optional<bool> shape.

💥 Breaking Changes

Behavioural change for callers that previously passed useGPU:false without nGpuLayers and were silently still running on GPU due to the tts-cpp default — they will now correctly run on CPU.

Callers that pass conflicting useGPU + nGpuLayers (e.g. useGPU:false + nGpuLayers:99) now throw at construction instead of silently picking one.

BEFORE:

new TTSGgml({
  files: { /* … */ },
  config: { useGPU: false },
  nGpuLayers: 99
})
// silently ran on GPU (nGpuLayers wins)

AFTER:

new TTSGgml({
  files: { /* … */ },
  config: { useGPU: false },
  nGpuLayers: 99
})
// throws: "tts-ggml: useGPU=false conflicts with nGpuLayers=99. …"

[ogad-tether]

Review

Thanks for tightening this up — promoting useGpu to tri-state on the C++ side is the right call, and turning explicit useGPU into an explicit n_gpu_layers removes a real foot-gun. Reasoning, docstrings, and the M2 perf table in the description are all great.

A few things below: one description/test discrepancy worth fixing, plus a couple of smaller follow-ups.

🔴 PR description vs diff: perf-delta test is described but not in the diff

The "How was it tested?" section claims gpu-smoke.test.js now also covers:

Chatterbox perf delta: same prompt+seed, useGPU:true vs false, must show a ≥1.10x speedup (relax via QVAC_TTS_GPU_SMOKE_RELAX=1).

But packages/tts-ggml/test/integration/gpu-smoke.test.js in this branch only adds two CPU smoke tests (Chatterbox CPU smoke and Supertonic CPU smoke) — no perf-delta test, no use of QVAC_TTS_GPU_SMOKE_RELAX for a perf gate, and grep -n "perf\|speedup\|1\.10" on the file comes back empty. Could you either:

• add the perf-delta test you described (it's the one that would actually have caught the original silent-GPU regression on a runner without backend-stat plumbing), or
• update the PR body to match what was actually added (CPU smoke only)?

The 3.05× M2 result in the table is great signal but it's a one-shot manual run, not a CI gate.

🟡 Supertonic + useGPU:false + nGpuLayers:99: error message is misleading

In packages/tts-ggml/index.js:305-318 the Supertonic path computes:

const wantsGpu =
  this._config.useGPU === true ||
  (this._nGpuLayers != null && this._nGpuLayers > 0)
if (wantsGpu) {
  throw new Error('tts-ggml: GPU execution is not supported by the Supertonic engine yet …')
}

If a caller passes { config: { useGPU: false }, nGpuLayers: 99 }, wantsGpu is true (because nGpuLayers > 0) and they get the "GPU not supported" message — even though they explicitly asked for CPU. That's the exact "silently the opposite backend" footgun this PR is trying to kill, just sliding into a confusing error instead of silent behavior.

Two options:

1. Run the JS conflict check (the one you added at index.js:323-336) for Supertonic too, before the wantsGpu throw, so the user sees the precise "useGPU=false conflicts with nGpuLayers=99" message.
2. At minimum, reword the Supertonic error to mention both fields when both were passed.

🟡 C++ Supertonic conflict check is effectively unreachable from JS

SupertonicModel.cpp:90-101 duplicates the conflict check the JS layer now has, but the only Supertonic paths that survive index.js are useGPU ∈ {undefined, false} and nGpuLayers ∈ {undefined, 0} — no combination of those can trip the C++ conflict. It's defensible as defense-in-depth for direct C++ callers, but a one-liner comment saying so would save the next reader a head-scratch. Same logic in Chatterbox is reachable, so this is Supertonic-specific.

🟢 Nits

• Negative nGpuLayers: the conflict checks (JS + both C++ validateConfig) use layers > 0 to derive layersWantGpu. llama.cpp historically treats n_gpu_layers = -1 as "offload all layers"; if tts-cpp inherits that, useGPU:true + nGpuLayers:-1 would falsely raise a conflict. Worth a quick check against the tts-cpp API — if -1 is meaningful upstream, layers != 0 is the safer predicate.
• Logic flow in index.js (index.js:322-340): the nested } else { if (...) ... if (...) ... } reads a little awkwardly. Could be flattened to two else ifs without losing meaning. Cosmetic.
• Error message punctuation: Chatterbox C++ message says ". Either drop one of the two, or make them agree …" while Supertonic C++ message says ". Either drop one of the two, or make them agree." (no parenthetical hint). Worth aligning so users get the same actionable suffix in both engines.
• Tri-state docstring on ChatterboxConfig.useGpu is excellent. Consider mirroring the same multi-line comment onto SupertonicConfig.useGpu — right now Supertonic just has std::optional<bool> useGpu; with no explanation, and a C++-only caller reading the header won't know the contract.

✅ What I like

• readOptionalBool cleanly returns std::optional<bool> — and confirmed grep shows useGPU is its only caller, so the signature change is safe.
• C++ unit tests updated in lockstep (EXPECT_FALSE(cfg.useGpu.has_value())).
• The strict CPU assertion (backendDevice=0, backendId=0) is the right contract — exactly counters the original "silently still on GPU" failure mode.
• Reuse of loadSupertonicTTS / ensureSupertonicModel from existing test utils, no new download plumbing.
• Migration is breaking only in the "you were getting the opposite of what you asked for" direction, which is correct.

Verdict

Holding off on approval pending the perf-delta test reconciliation (the meaningful one) and ideally the Supertonic conflict-message tweak. The rest are nits and can be addressed in follow-ups.

[github-actions]

Tier-based Approval Status

**PR Tier:** TIER1

**Current Status:** ❌ PENDING

**Requirements:**
- 1 Team Member approval ✅ (1/1)
- 1 Team Lead OR Management approval ❌ (0/1)



---
*This comment is automatically updated when reviews change.*

[ishanvohra2]

Review

Thanks for tightening this up — promoting useGpu to tri-state on the C++ side is the right call, and turning explicit useGPU into an explicit n_gpu_layers removes a real foot-gun. Reasoning, docstrings, and the M2 perf table in the description are all great.

A few things below: one description/test discrepancy worth fixing, plus a couple of smaller follow-ups.

🔴 PR description vs diff: perf-delta test is described but not in the diff

The "How was it tested?" section claims gpu-smoke.test.js now also covers:

Chatterbox perf delta: same prompt+seed, useGPU:true vs false, must show a ≥1.10x speedup (relax via QVAC_TTS_GPU_SMOKE_RELAX=1).

But packages/tts-ggml/test/integration/gpu-smoke.test.js in this branch only adds two CPU smoke tests (Chatterbox CPU smoke and Supertonic CPU smoke) — no perf-delta test, no use of QVAC_TTS_GPU_SMOKE_RELAX for a perf gate, and grep -n "perf\|speedup\|1\.10" on the file comes back empty. Could you either:

• add the perf-delta test you described (it's the one that would actually have caught the original silent-GPU regression on a runner without backend-stat plumbing), or
• update the PR body to match what was actually added (CPU smoke only)?

The 3.05× M2 result in the table is great signal but it's a one-shot manual run, not a CI gate.

🟡 Supertonic + useGPU:false + nGpuLayers:99: error message is misleading

In packages/tts-ggml/index.js:305-318 the Supertonic path computes:

const wantsGpu =
  this._config.useGPU === true ||
  (this._nGpuLayers != null && this._nGpuLayers > 0)
if (wantsGpu) {
  throw new Error('tts-ggml: GPU execution is not supported by the Supertonic engine yet …')
}

If a caller passes { config: { useGPU: false }, nGpuLayers: 99 }, wantsGpu is true (because nGpuLayers > 0) and they get the "GPU not supported" message — even though they explicitly asked for CPU. That's the exact "silently the opposite backend" footgun this PR is trying to kill, just sliding into a confusing error instead of silent behavior.

Two options:

1. Run the JS conflict check (the one you added at index.js:323-336) for Supertonic too, before the wantsGpu throw, so the user sees the precise "useGPU=false conflicts with nGpuLayers=99" message.
2. At minimum, reword the Supertonic error to mention both fields when both were passed.

🟡 C++ Supertonic conflict check is effectively unreachable from JS

SupertonicModel.cpp:90-101 duplicates the conflict check the JS layer now has, but the only Supertonic paths that survive index.js are useGPU ∈ {undefined, false} and nGpuLayers ∈ {undefined, 0} — no combination of those can trip the C++ conflict. It's defensible as defense-in-depth for direct C++ callers, but a one-liner comment saying so would save the next reader a head-scratch. Same logic in Chatterbox is reachable, so this is Supertonic-specific.

🟢 Nits

• Negative nGpuLayers: the conflict checks (JS + both C++ validateConfig) use layers > 0 to derive layersWantGpu. llama.cpp historically treats n_gpu_layers = -1 as "offload all layers"; if tts-cpp inherits that, useGPU:true + nGpuLayers:-1 would falsely raise a conflict. Worth a quick check against the tts-cpp API — if -1 is meaningful upstream, layers != 0 is the safer predicate.
• Logic flow in index.js (index.js:322-340): the nested } else { if (...) ... if (...) ... } reads a little awkwardly. Could be flattened to two else ifs without losing meaning. Cosmetic.
• Error message punctuation: Chatterbox C++ message says ". Either drop one of the two, or make them agree …" while Supertonic C++ message says ". Either drop one of the two, or make them agree." (no parenthetical hint). Worth aligning so users get the same actionable suffix in both engines.
• Tri-state docstring on ChatterboxConfig.useGpu is excellent. Consider mirroring the same multi-line comment onto SupertonicConfig.useGpu — right now Supertonic just has std::optional<bool> useGpu; with no explanation, and a C++-only caller reading the header won't know the contract.

✅ What I like

• readOptionalBool cleanly returns std::optional<bool> — and confirmed grep shows useGPU is its only caller, so the signature change is safe.
• C++ unit tests updated in lockstep (EXPECT_FALSE(cfg.useGpu.has_value())).
• The strict CPU assertion (backendDevice=0, backendId=0) is the right contract — exactly counters the original "silently still on GPU" failure mode.
• Reuse of loadSupertonicTTS / ensureSupertonicModel from existing test utils, no new download plumbing.
• Migration is breaking only in the "you were getting the opposite of what you asked for" direction, which is correct.

Verdict

Holding off on approval pending the perf-delta test reconciliation (the meaningful one) and ideally the Supertonic conflict-message tweak. The rest are nits and can be addressed in follow-ups.

Review

Thanks for tightening this up — promoting useGpu to tri-state on the C++ side is the right call, and turning explicit useGPU into an explicit n_gpu_layers removes a real foot-gun. Reasoning, docstrings, and the M2 perf table in the description are all great.

A few things below: one description/test discrepancy worth fixing, plus a couple of smaller follow-ups.

🔴 PR description vs diff: perf-delta test is described but not in the diff

The "How was it tested?" section claims gpu-smoke.test.js now also covers:

Chatterbox perf delta: same prompt+seed, useGPU:true vs false, must show a ≥1.10x speedup (relax via QVAC_TTS_GPU_SMOKE_RELAX=1).

But packages/tts-ggml/test/integration/gpu-smoke.test.js in this branch only adds two CPU smoke tests (Chatterbox CPU smoke and Supertonic CPU smoke) — no perf-delta test, no use of QVAC_TTS_GPU_SMOKE_RELAX for a perf gate, and grep -n "perf\|speedup\|1\.10" on the file comes back empty. Could you either:

• add the perf-delta test you described (it's the one that would actually have caught the original silent-GPU regression on a runner without backend-stat plumbing), or
• update the PR body to match what was actually added (CPU smoke only)?

The 3.05× M2 result in the table is great signal but it's a one-shot manual run, not a CI gate.

🟡 Supertonic + useGPU:false + nGpuLayers:99: error message is misleading

In packages/tts-ggml/index.js:305-318 the Supertonic path computes:

const wantsGpu =
  this._config.useGPU === true ||
  (this._nGpuLayers != null && this._nGpuLayers > 0)
if (wantsGpu) {
  throw new Error('tts-ggml: GPU execution is not supported by the Supertonic engine yet …')
}

If a caller passes { config: { useGPU: false }, nGpuLayers: 99 }, wantsGpu is true (because nGpuLayers > 0) and they get the "GPU not supported" message — even though they explicitly asked for CPU. That's the exact "silently the opposite backend" footgun this PR is trying to kill, just sliding into a confusing error instead of silent behavior.

Two options:

1. Run the JS conflict check (the one you added at index.js:323-336) for Supertonic too, before the wantsGpu throw, so the user sees the precise "useGPU=false conflicts with nGpuLayers=99" message.
2. At minimum, reword the Supertonic error to mention both fields when both were passed.

🟡 C++ Supertonic conflict check is effectively unreachable from JS

SupertonicModel.cpp:90-101 duplicates the conflict check the JS layer now has, but the only Supertonic paths that survive index.js are useGPU ∈ {undefined, false} and nGpuLayers ∈ {undefined, 0} — no combination of those can trip the C++ conflict. It's defensible as defense-in-depth for direct C++ callers, but a one-liner comment saying so would save the next reader a head-scratch. Same logic in Chatterbox is reachable, so this is Supertonic-specific.

🟢 Nits

• Negative nGpuLayers: the conflict checks (JS + both C++ validateConfig) use layers > 0 to derive layersWantGpu. llama.cpp historically treats n_gpu_layers = -1 as "offload all layers"; if tts-cpp inherits that, useGPU:true + nGpuLayers:-1 would falsely raise a conflict. Worth a quick check against the tts-cpp API — if -1 is meaningful upstream, layers != 0 is the safer predicate.
• Logic flow in index.js (index.js:322-340): the nested } else { if (...) ... if (...) ... } reads a little awkwardly. Could be flattened to two else ifs without losing meaning. Cosmetic.
• Error message punctuation: Chatterbox C++ message says ". Either drop one of the two, or make them agree …" while Supertonic C++ message says ". Either drop one of the two, or make them agree." (no parenthetical hint). Worth aligning so users get the same actionable suffix in both engines.
• Tri-state docstring on ChatterboxConfig.useGpu is excellent. Consider mirroring the same multi-line comment onto SupertonicConfig.useGpu — right now Supertonic just has std::optional<bool> useGpu; with no explanation, and a C++-only caller reading the header won't know the contract.

✅ What I like

• readOptionalBool cleanly returns std::optional<bool> — and confirmed grep shows useGPU is its only caller, so the signature change is safe.
• C++ unit tests updated in lockstep (EXPECT_FALSE(cfg.useGpu.has_value())).
• The strict CPU assertion (backendDevice=0, backendId=0) is the right contract — exactly counters the original "silently still on GPU" failure mode.
• Reuse of loadSupertonicTTS / ensureSupertonicModel from existing test utils, no new download plumbing.
• Migration is breaking only in the "you were getting the opposite of what you asked for" direction, which is correct.

Verdict

Holding off on approval pending the perf-delta test reconciliation (the meaningful one) and ideally the Supertonic conflict-message tweak. The rest are nits and can be addressed in follow-ups.

Resolved all comments.

For the first one - removed the performance delta part from PR description.