Add C++ runtime for Whisper with Qualcomm NPU using QNN. - #3699
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds QNN Whisper config support, a new offline QNN Whisper decoder implementation, factory and build wiring, and export workflow renames for Whisper QNN artifacts. ChangesWhisper QNN Integration
Sequence Diagram(s)sequenceDiagram
participant App
participant OfflineRecognizerImpl
participant OfflineWhisperModelQnn
participant QnnEncoderModel
participant QnnDecoderModel
App->>OfflineRecognizerImpl: Create(config, provider="qnn")
OfflineRecognizerImpl->>OfflineWhisperModelQnn: construct(config)
OfflineWhisperModelQnn->>QnnEncoderModel: Init encoder
OfflineWhisperModelQnn->>QnnDecoderModel: Init decoder
OfflineWhisperModelQnn-->>OfflineRecognizerImpl: ready
App->>OfflineRecognizerImpl: Decode(audio)
OfflineRecognizerImpl->>OfflineWhisperModelQnn: Run(features)
OfflineWhisperModelQnn->>QnnEncoderModel: RunEncoder(mel)
QnnEncoderModel-->>OfflineWhisperModelQnn: cross-KV tensors
loop token generation
OfflineWhisperModelQnn->>QnnDecoderModel: RunDecoder(token, KV cache, mask)
QnnDecoderModel-->>OfflineWhisperModelQnn: logits
end
OfflineWhisperModelQnn-->>OfflineRecognizerImpl: transcript result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
dosubot
Bot
added
the
size:XL
![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 adds support for the Whisper model using the Qualcomm Neural Network (QNN) backend in sherpa-onnx, including configuration validation and the core model implementation. The review feedback highlights several critical safety improvements to prevent potential crashes and undefined behavior. Specifically, it is recommended to add bounds checks on the logits vector during language detection, verify tensor sizes before updating the KV cache or transposing encoder outputs, ensure shape vectors are not empty during decoder initialization, validate that cross_k_shape has at least three dimensions, and confirm that feat_dim_ is greater than zero to avoid a division-by-zero error.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const auto &all_lang_ids = GetAllWhisperLanguageTokenIds(); | ||
| int32_t lang_id = all_lang_ids[0]; | ||
| float this_logit = logits[lang_id]; | ||
|
|
||
| for (int32_t i = 1; i != static_cast<int32_t>(all_lang_ids.size()); ++i) { | ||
| int32_t id = all_lang_ids[i]; | ||
| float p = logits[id]; | ||
|
|
||
| if (p > this_logit) { | ||
| this_logit = p; | ||
| lang_id = id; | ||
| } | ||
| } |
There was a problem hiding this comment.
In DetectLanguage, the code directly accesses logits[lang_id] and logits[id] using language token IDs from GetAllWhisperLanguageTokenIds(). However, if the loaded model has a smaller vocabulary size (e.g., a pruned or custom model) such that logits.size() is less than the language token IDs (which can be up to 50357), this will cause an out-of-bounds vector access and crash the application.
Please add a safety check to ensure that we only query token IDs that are within the bounds of the logits vector.
const auto &all_lang_ids = GetAllWhisperLanguageTokenIds();
int32_t lang_id = -1;
float this_logit = 0.0f;
for (int32_t id : all_lang_ids) {
if (id >= static_cast<int32_t>(logits.size())) {
continue;
}
float p = logits[id];
if (lang_id == -1 || p > this_logit) {
this_logit = p;
lang_id = id;
}
}
if (lang_id == -1) {
SHERPA_ONNX_LOGE("No valid language token found in logits (logits size: %d)",
static_cast<int32_t>(logits.size()));
SHERPA_ONNX_EXIT(-1);
}| auto delta_k = | ||
| decoder_model_->GetOutputTensorData(decoder_this_self_k_[i]); | ||
| float *self_k = self_kv_data + (i * 2) * self_kv_size_; | ||
| for (size_t r = 0; r != delta_k.size(); ++r) { | ||
| self_k[r * self_kv_stride_ + offset] += delta_k[r]; | ||
| } | ||
|
|
||
| // Update self_v | ||
| auto delta_v = | ||
| decoder_model_->GetOutputTensorData(decoder_this_self_v_[i]); | ||
| float *self_v = self_kv_data + (i * 2 + 1) * self_kv_size_; | ||
| for (size_t r = 0; r != delta_v.size(); ++r) { | ||
| self_v[r * self_kv_stride_ + offset] += delta_v[r]; | ||
| } |
There was a problem hiding this comment.
In UpdateSelfKvCache, the code retrieves delta_k and delta_v from the decoder model and copies their elements into self_k and self_v assuming their size matches n_text_state_. If a mismatched or invalid model is loaded where the output tensor size of decoder_this_self_k_ or decoder_this_self_v_ is larger than n_text_state_, this loop will write out of bounds of the allocated self_kv buffer, leading to memory corruption or crashes.
Please add a defensive check to verify that the sizes of delta_k and delta_v match n_text_state_ before copying.
auto delta_k =
decoder_model_->GetOutputTensorData(decoder_this_self_k_[i]);
if (delta_k.size() != static_cast<size_t>(n_text_state_)) {
SHERPA_ONNX_LOGE("Mismatched self_k size: expected %d, got %d",
n_text_state_, static_cast<int32_t>(delta_k.size()));
SHERPA_ONNX_EXIT(-1);
}
float *self_k = self_kv_data + (i * 2) * self_kv_size_;
for (size_t r = 0; r != delta_k.size(); ++r) {
self_k[r * self_kv_stride_ + offset] += delta_k[r];
}
// Update self_v
auto delta_v =
decoder_model_->GetOutputTensorData(decoder_this_self_v_[i]);
if (delta_v.size() != static_cast<size_t>(n_text_state_)) {
SHERPA_ONNX_LOGE("Mismatched self_v size: expected %d, got %d",
n_text_state_, static_cast<int32_t>(delta_v.size()));
SHERPA_ONNX_EXIT(-1);
}
float *self_v = self_kv_data + (i * 2 + 1) * self_kv_size_;
for (size_t r = 0; r != delta_v.size(); ++r) {
self_v[r * self_kv_stride_ + offset] += delta_v[r];
}| auto cross_k = encoder_model_->GetOutputTensorData(encoder_cross_k_[i]); | ||
| auto cross_v = encoder_model_->GetOutputTensorData(encoder_cross_v_[i]); | ||
|
|
||
| cross_kv[i * 2] = | ||
| Transpose(cross_k.data(), num_out_frames_, n_text_state_); | ||
| cross_kv[i * 2 + 1] = | ||
| Transpose(cross_v.data(), num_out_frames_, n_text_state_); |
There was a problem hiding this comment.
In the encoder output processing loop, Transpose is called on cross_k.data() and cross_v.data() with dimensions num_out_frames_ and n_text_state_. If the model output tensor size does not match num_out_frames_ * n_text_state_, Transpose will perform an out-of-bounds read, leading to undefined behavior or crashes.
Please add a defensive check to verify that the sizes of cross_k and cross_v match the expected dimensions before transposing.
auto cross_k = encoder_model_->GetOutputTensorData(encoder_cross_k_[i]);
auto cross_v = encoder_model_->GetOutputTensorData(encoder_cross_v_[i]);
if (cross_k.size() != static_cast<size_t>(num_out_frames_ * n_text_state_)) {
SHERPA_ONNX_LOGE("Mismatched cross_k size for layer %d: expected %d, got %d",
i, num_out_frames_ * n_text_state_, static_cast<int32_t>(cross_k.size()));
SHERPA_ONNX_EXIT(-1);
}
if (cross_v.size() != static_cast<size_t>(num_out_frames_ * n_text_state_)) {
SHERPA_ONNX_LOGE("Mismatched cross_v size for layer %d: expected %d, got %d",
i, num_out_frames_ * n_text_state_, static_cast<int32_t>(cross_v.size()));
SHERPA_ONNX_EXIT(-1);
}
cross_kv[i * 2] =
Transpose(cross_k.data(), num_out_frames_, n_text_state_);
cross_kv[i * 2 + 1] =
Transpose(cross_v.data(), num_out_frames_, n_text_state_);| std::vector<int32_t> mask_shape = decoder_model_->TensorShape(mask_name); | ||
| mask_size_ = mask_shape[0]; | ||
|
|
||
| std::string logits_name = prefix_ + "_logits"; | ||
| if (!decoder_model_->HasTensor(logits_name)) { | ||
| SHERPA_ONNX_LOGE("Decoder does not have output tensor '%s'", | ||
| logits_name.c_str()); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } | ||
|
|
||
| std::vector<int32_t> logits_shape = | ||
| decoder_model_->TensorShape(logits_name); | ||
| vocab_size_ = logits_shape.back(); |
There was a problem hiding this comment.
In PostInitDecoder, the code retrieves mask_shape and logits_shape from the decoder model and accesses their elements without checking if the shape vectors are empty. If a malformed or incompatible model is loaded, TensorShape might return an empty vector, and accessing mask_shape[0] or logits_shape.back() will cause undefined behavior or crashes.
Please add safety checks to ensure these shape vectors are not empty before accessing their elements.
std::vector<int32_t> mask_shape = decoder_model_->TensorShape(mask_name);
if (mask_shape.empty()) {
SHERPA_ONNX_LOGE("Decoder mask shape is empty");
SHERPA_ONNX_EXIT(-1);
}
mask_size_ = mask_shape[0];
std::string logits_name = prefix_ + "_logits";
if (!decoder_model_->HasTensor(logits_name)) {
SHERPA_ONNX_LOGE("Decoder does not have output tensor '%s'",
logits_name.c_str());
SHERPA_ONNX_EXIT(-1);
}
std::vector<int32_t> logits_shape =
decoder_model_->TensorShape(logits_name);
if (logits_shape.empty()) {
SHERPA_ONNX_LOGE("Decoder logits shape is empty");
SHERPA_ONNX_EXIT(-1);
}
vocab_size_ = logits_shape.back();| std::vector<int32_t> cross_k_shape = | ||
| encoder_model_->TensorShape("cross_k_0"); | ||
|
|
||
| num_out_frames_ = cross_k_shape[1]; | ||
| n_text_state_ = cross_k_shape[2]; |
There was a problem hiding this comment.
In PostInitEncoder, the code retrieves cross_k_shape and accesses its elements at indices 1 and 2 without verifying that the shape vector has at least 3 dimensions. If the shape vector is smaller, this will cause an out-of-bounds access and crash.
Please add a check to ensure cross_k_shape has at least 3 dimensions.
std::vector<int32_t> cross_k_shape =
encoder_model_->TensorShape("cross_k_0");
if (cross_k_shape.size() < 3) {
SHERPA_ONNX_LOGE("Expected cross_k_0 to have at least 3 dimensions");
SHERPA_ONNX_EXIT(-1);
}
num_out_frames_ = cross_k_shape[1];
n_text_state_ = cross_k_shape[2];| num_frames_ = mel_shape[1]; | ||
| feat_dim_ = mel_shape[2]; |
There was a problem hiding this comment.
In PostInitEncoder, after extracting num_frames_ and feat_dim_ from mel_shape, there is no check to ensure they are positive. If feat_dim_ is 0, a division-by-zero crash (SIGFPE) will occur in Run when computing num_frames = features.size() / feat_dim_.
Please add a check to ensure num_frames_ and feat_dim_ are greater than 0.
num_frames_ = mel_shape[1];
feat_dim_ = mel_shape[2];
if (num_frames_ <= 0 || feat_dim_ <= 0) {
SHERPA_ONNX_LOGE("Invalid encoder mel input shape: [%d, %d, %d]",
mel_shape[0], num_frames_, feat_dim_);
SHERPA_ONNX_EXIT(-1);
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/export-whisper-qnn.yaml:
- Line 413: The workflow shell step that builds and uses the d variable from
matrix values needs hardening because unquoted `${{ matrix.* }}` content is
later expanded through `$d`, which can trigger shell metacharacter
interpretation. Update the affected shell script blocks that assign and consume
`d` in the export-whisper-qnn workflow so the matrix-derived values are safely
quoted/escaped before any shell use, and ensure all subsequent references to `d`
preserve that quoting behavior.
In `@sherpa-onnx/csrc/offline-recognizer-impl.cc`:
- Around line 210-214: The Whisper dispatch in OfflineRecognizerImpl is too
broad and sends any non-empty whisper.encoder config into
OfflineWhisperModelQnn, even when the model is not actually a QNN artifact.
Update the Create() branching in OfflineRecognizerImpl to use the same
QNN-artifact check as OfflineWhisperModelConfig::Validate()—only dispatch to
OfflineRecognizerWhisperTplImpl<OfflineWhisperModelQnn> when the encoder/decoder
are QNN .so files or whisper.qnn_config.context_binary is set—so normal ONNX
Whisper configs do not enter the QNN path.
In `@sherpa-onnx/csrc/qnn/offline-whisper-model-qnn.cc`:
- Around line 65-71: The Manager-based ctor in OfflineWhisperModelQnn::Impl is
currently an always-failing stub, but OfflineRecognizerImpl::Create(Manager *,
...) now routes QNN Whisper through it. Either implement real Manager-backed
model loading in this constructor or remove/gate the Manager overload so the
factory rejects Android/OHOS asset-manager usage explicitly instead of aborting
after dispatch; use the OfflineRecognizerImpl::Create and Impl(Manager *, const
OfflineModelConfig &) symbols to locate the path.
- Around line 430-434: Validate the tensor ranks before indexing shapes in the
QNN Whisper model init path, since `cross_k_shape[1]`, `cross_k_shape[2]`,
`mask_shape[0]`, and `logits_shape.back()` can otherwise access out of bounds on
mismatched artifacts. Add explicit rank checks in the same style as the existing
`mel_shape` validation inside `OfflineWhisperModelQnn` before reading these
dimensions, and fail with a clear validation error if the encoder/decoder tensor
shapes do not match expectations. Apply the same safeguard around the related
shape reads in the later init block that uses `mask_shape` and `logits_shape`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c22c89c0-b439-41e6-b4d7-cc9b76848a0c
📒 Files selected for processing (7)
.github/workflows/export-whisper-qnn.yamlsherpa-onnx/csrc/CMakeLists.txtsherpa-onnx/csrc/offline-recognizer-impl.ccsherpa-onnx/csrc/offline-whisper-model-config.ccsherpa-onnx/csrc/offline-whisper-model-config.hsherpa-onnx/csrc/qnn/offline-whisper-model-qnn.ccsherpa-onnx/csrc/qnn/offline-whisper-model-qnn.h
| echo "collect results" | ||
|
|
||
| d=sherpa-onnx-qnn-${{ matrix.soc }}-binary-${{ matrix.model_name }} | ||
| d=sherpa-onnx-qnn-${{ matrix.soc }}-binary-whisper-${{ matrix.model_name }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Quote templated matrix values before shell expansion.
Line 413 and Lines 430-432 interpolate ${{ matrix.* }} into bash variables that are later used unquoted ($d). This can allow shell metacharacter expansion and break or execute unintended commands if matrix values change upstream.
Suggested hardening diff
- d=sherpa-onnx-qnn-${{ matrix.soc }}-binary-whisper-${{ matrix.model_name }}
- mkdir -p $d
- mkdir -p $d/test_wavs
- cp -v non_quant/binary/*.bin $d/
- cp -v $model_dir/tokens.txt $d
- cp -v $model_dir/*.wav $d/test_wavs
+ d="sherpa-onnx-qnn-${{ matrix.soc }}-binary-whisper-${{ matrix.model_name }}"
+ mkdir -p "$d"
+ mkdir -p "$d/test_wavs"
+ cp -v non_quant/binary/*.bin "$d"/
+ cp -v "$model_dir"/tokens.txt "$d"
+ cp -v "$model_dir"/*.wav "$d"/test_wavs
@@
- d=sherpa-onnx-qnn-whisper-${{ matrix.model_name }}-linux-x64
+ d="sherpa-onnx-qnn-whisper-${{ matrix.model_name }}-linux-x64"
elif [[ $p == aarch64-android ]]; then
- d=sherpa-onnx-qnn-whisper-${{ matrix.model_name }}-android-aarch64
+ d="sherpa-onnx-qnn-whisper-${{ matrix.model_name }}-android-aarch64"Also applies to: 430-432
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 413-413: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 413-413: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 413-413: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 413-413: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/export-whisper-qnn.yaml at line 413, The workflow shell
step that builds and uses the d variable from matrix values needs hardening
because unquoted `${{ matrix.* }}` content is later expanded through `$d`, which
can trigger shell metacharacter interpretation. Update the affected shell script
blocks that assign and consume `d` in the export-whisper-qnn workflow so the
matrix-derived values are safely quoted/escaped before any shell use, and ensure
all subsequent references to `d` preserve that quoting behavior.
Source: Linters/SAST tools
| } else if (!config.model_config.whisper.encoder.empty() || | ||
| !config.model_config.whisper.qnn_config.context_binary | ||
| .empty()) { | ||
| return std::make_unique< | ||
| OfflineRecognizerWhisperTplImpl<OfflineWhisperModelQnn>>(config); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate QNN Whisper dispatch on actual QNN artifacts.
These branches instantiate OfflineWhisperModelQnn for any non-empty whisper.encoder, but OfflineWhisperModelConfig::Validate() only recognizes Whisper-as-QNN when the encoder/decoder are .so or whisper.qnn_config.context_binary is set. With provider == "qnn" and a normal ONNX encoder path, Create() routes into the QNN codepath and OfflineWhisperModelQnn then tries to load that ONNX file as a QNN model library. Reuse the same QNN-artifact predicate here so unsupported configs fail at dispatch instead of during init.
Also applies to: 576-580
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sherpa-onnx/csrc/offline-recognizer-impl.cc` around lines 210 - 214, The
Whisper dispatch in OfflineRecognizerImpl is too broad and sends any non-empty
whisper.encoder config into OfflineWhisperModelQnn, even when the model is not
actually a QNN artifact. Update the Create() branching in OfflineRecognizerImpl
to use the same QNN-artifact check as OfflineWhisperModelConfig::Validate()—only
dispatch to OfflineRecognizerWhisperTplImpl<OfflineWhisperModelQnn> when the
encoder/decoder are QNN .so files or whisper.qnn_config.context_binary is set—so
normal ONNX Whisper configs do not enter the QNN path.
Source: Learnings
| template <typename Manager> | ||
| Impl(Manager *mgr, const OfflineModelConfig &config) : config_(config) { | ||
| SHERPA_ONNX_LOGE( | ||
| "Please copy all files from assets to SD card and set assetManager to " | ||
| "null"); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove or gate the unsupported manager overload.
This ctor always exits, but the new OfflineRecognizerImpl::Create(Manager *, ...) branch now selects it for QNN Whisper. Any Android/OHOS asset-manager path will abort immediately after successful factory dispatch. Either implement manager-backed loading here or reject this case in the factory with an explicit unsupported-path message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sherpa-onnx/csrc/qnn/offline-whisper-model-qnn.cc` around lines 65 - 71, The
Manager-based ctor in OfflineWhisperModelQnn::Impl is currently an
always-failing stub, but OfflineRecognizerImpl::Create(Manager *, ...) now
routes QNN Whisper through it. Either implement real Manager-backed model
loading in this constructor or remove/gate the Manager overload so the factory
rejects Android/OHOS asset-manager usage explicitly instead of aborting after
dispatch; use the OfflineRecognizerImpl::Create and Impl(Manager *, const
OfflineModelConfig &) symbols to locate the path.
| std::vector<int32_t> cross_k_shape = | ||
| encoder_model_->TensorShape("cross_k_0"); | ||
|
|
||
| num_out_frames_ = cross_k_shape[1]; | ||
| n_text_state_ = cross_k_shape[2]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate tensor ranks before indexing these shapes.
cross_k_shape[1], cross_k_shape[2], mask_shape[0], and logits_shape.back() assume the encoder/decoder artifacts are structurally correct. A mismatched model/context pair will turn that into out-of-bounds access during init instead of a clean validation error. Mirror the existing mel_shape rank check before reading these indices.
Also applies to: 464-476
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sherpa-onnx/csrc/qnn/offline-whisper-model-qnn.cc` around lines 430 - 434,
Validate the tensor ranks before indexing shapes in the QNN Whisper model init
path, since `cross_k_shape[1]`, `cross_k_shape[2]`, `mask_shape[0]`, and
`logits_shape.back()` can otherwise access out of bounds on mismatched
artifacts. Add explicit rank checks in the same style as the existing
`mel_shape` validation inside `OfflineWhisperModelQnn` before reading these
dimensions, and fail with a clear validation error if the encoder/decoder tensor
shapes do not match expectations. Apply the same safeguard around the related
shape reads in the later init block that uses `mask_shape` and `logits_shape`.
1 participant
Usage
See https://k2-fsa.github.io/sherpa/onnx/qnn/run-executables-on-your-phone-binary.html
now run it on your phone:
Output logs:
We use SM8850 in the above example. You can select more models for different SOC from
https://github.com/k2-fsa/sherpa-onnx/releases/tag/asr-models-qnn-binary-2
Summary by CodeRabbit
New Features
Bug Fixes
.sopresence.