Repository navigation
Add C++ QNN support for Zipformer CTC models. - #2809
Conversation
dosubot
Bot
added
the
size:L
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds QNN-backed Zipformer-CTC support and wiring: new QNN model and recognizer implementations, QnnConfig plumbing into Zipformer config and JNI/Android layers, scoped ParseOptions for per-model QNN settings, removal of some file-existence checks, and Convert() linkage changed to external. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Factory as OfflineRecognizer::Create
participant ZipRec as OfflineRecognizerZipformerCtcQnnImpl
participant QNN as OfflineZipformerCtcModelQnn
participant Decoder as OfflineCtcGreedySearchDecoderRknn
participant Stream as OfflineStream
User->>Factory: Create(config)
alt config includes zipformer_ctc + provider == "qnn"
Factory->>ZipRec: construct(config)
ZipRec->>QNN: initialize model (context/lib)
ZipRec->>Decoder: create greedy decoder
end
Factory-->>User: recognizer
User->>ZipRec: CreateStream()
ZipRec-->>User: Stream
User->>ZipRec: DecodeStreams([Stream])
loop per stream
ZipRec->>Stream: read features
ZipRec->>QNN: Run(features)
QNN-->>ZipRec: log-probs
ZipRec->>Decoder: decode(log-probs)
Decoder-->>ZipRec: OfflineCtcDecoderResult
ZipRec->>ZipRec: Convert(...) -> OfflineRecognitionResult
ZipRec->>Stream: store result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 enhances the sherpa-onnx project by adding comprehensive C++ QNN support for Zipformer CTC models. This integration allows for efficient, hardware-accelerated inference of these models, particularly beneficial for deployment on devices leveraging Qualcomm's NPUs. The changes involve introducing new QNN-specific model and recognizer implementations, updating build configurations, and refining model loading and validation processes to seamlessly incorporate QNN capabilities. Highlights
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
|
![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 Zipformer CTC models with the QNN backend. The changes include adding new implementation files for the QNN recognizer and model, integrating them into the existing factory methods, and updating the build configuration. The overall approach is sound and follows the existing structure for other model types. I've identified a few areas for improvement: a potentially confusing condition in the recognizer implementation that seems to be a copy-paste artifact, a hardcoded value that should be read from configuration, and an unfriendly error message and application exit when attempting to load a QNN model from Android assets. Addressing these points will improve code clarity, correctness, and developer experience.
| 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.
The constructor for loading from a Manager (like Android's AAssetManager) immediately calls SHERPA_ONNX_EXIT(-1), which will crash the application. While not supporting loading from assets for QNN models is a valid design choice, crashing the app is not user-friendly. Furthermore, the error message is not very helpful for a developer. It should clearly state that loading from assets is not supported for QNN models and suggest using file paths instead. A more informative and less imperative error message would improve the developer experience.
Impl(Manager *mgr, const OfflineModelConfig &config) : config_(config) {
SHERPA_ONNX_LOGE(
"Loading QNN models from assets is not supported. "
"Please provide absolute paths to model files on the file system.");
SHERPA_ONNX_EXIT(-1);
}| if (!symbol_table_.Contains("<blk>") && | ||
| !symbol_table_.Contains("<eps>") && | ||
| !symbol_table_.Contains("<blank>") && | ||
| config_.model_config.omnilingual.model.empty()) { | ||
| // for omnilingual asr, its blank id is 0 | ||
| SHERPA_ONNX_LOGE( | ||
| "We expect that tokens.txt contains " | ||
| "the symbol <blk> or <eps> or <blank> and its ID."); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } |
There was a problem hiding this comment.
The condition config_.model_config.omnilingual.model.empty() and the associated comment seem to be a copy-paste from a more generic CTC recognizer. Since this implementation is specific to Zipformer CTC models, this check is confusing and irrelevant. It would be clearer to remove the check for the omnilingual model. The logic should just verify that one of the blank symbols exists in the symbol table.
if (!symbol_table_.Contains("<blk>") &&
!symbol_table_.Contains("<eps>") &&
!symbol_table_.Contains("<blank>")) {
SHERPA_ONNX_LOGE(
"We expect that tokens.txt contains "
"the symbol <blk> or <eps> or <blank> and its ID.");
SHERPA_ONNX_EXIT(-1);
}| auto result = | ||
| decoder_->Decode(log_probs.data(), num_out_frames, vocab_size); | ||
|
|
||
| int32_t frame_shift_ms = 10; |
There was a problem hiding this comment.
The frame_shift_ms is hardcoded to 10. This might lead to incorrect timestamps if the feature configuration is changed to use a different frame shift. It's better to use the value from config_.feat_config.frame_shift_ms to ensure correctness and respect the user's configuration.
int32_t frame_shift_ms = config_.feat_config.frame_shift_ms;![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
sherpa-onnx/csrc/offline-recognizer-ctc-impl.h (1)
26-29: Consider explicit forward declaration or namespace encapsulation.Changing the
Convertfunction from static to external linkage allows it to be used by the new QNN Zipformer CTC implementation, which is good for code reuse. However, this now exposes the function as part of the public interface without explicit declaration.Consider adding a forward declaration in a common header if this function is intended to be part of the public API, or document that it's an internal utility shared across implementation units.
sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.h (1)
7-10: Include<cstdint>so the header is self-contained forint32_tThis header uses
int32_tbut does not include<cstdint>. Relying on transitive includes fromoffline-model-config.hcan be brittle.You can make the header self-contained with:
#include <memory> #include <vector> -#include "sherpa-onnx/csrc/offline-model-config.h" +#include <cstdint> + +#include "sherpa-onnx/csrc/offline-model-config.h"Also applies to: 29-30
sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.cc (1)
74-80: Clarify unsupported Manager-based construction and silence the unused-parameter warning
Impl(Manager *mgr, const OfflineModelConfig &config)currently just logs an error and exits, andmgris unused. That’s fine if you really don’t want to support asset/resource manager–based loading for this QNN model yet, but it’s worth making this intent explicit and avoiding the warning.For example:
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"); + (void)mgr; // Manager-based loading is not supported for this model. + SHERPA_ONNX_LOGE( + "OfflineZipformerCtcModelQnn does not support loading from assets/" + "raw resources. Please copy all files to a filesystem path and pass " + "assetManager/resourceManager as null."); SHERPA_ONNX_EXIT(-1); }This keeps current behavior but removes ambiguity and the unused-parameter warning.
Also applies to: 237-245
sherpa-onnx/csrc/qnn/offline-recognizer-zipformer-ctc-qnn-impl.h (1)
101-108: Use or removefeat_dimand consider a light sanity check on feature layout
feat_dimis read but not used:int32_t feat_dim = s->FeatureDim(); std::vector<float> f = s->GetFrames();If you don’t plan to use it, you can drop the variable. Alternatively, it’s a good place for a cheap consistency check between the stream and model expectations, e.g.:
- void DecodeStream(OfflineStream *s) const { - int32_t feat_dim = s->FeatureDim(); - std::vector<float> f = s->GetFrames(); + void DecodeStream(OfflineStream *s) const { + int32_t feat_dim = s->FeatureDim(); + std::vector<float> f = s->GetFrames(); + + if (!f.empty() && + static_cast<int32_t>(f.size()) % feat_dim != 0) { + SHERPA_ONNX_LOGE( + "Feature vector size (%d) is not divisible by feat_dim (%d).", + static_cast<int32_t>(f.size()), feat_dim); + SHERPA_ONNX_EXIT(-1); + }If you prefer not to add the check, at least removing
feat_dimavoids an unused-variable warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
sherpa-onnx/csrc/CMakeLists.txt(1 hunks)sherpa-onnx/csrc/offline-recognizer-ctc-impl.h(1 hunks)sherpa-onnx/csrc/offline-recognizer-impl.cc(3 hunks)sherpa-onnx/csrc/offline-sense-voice-model-config.cc(1 hunks)sherpa-onnx/csrc/offline-zipformer-ctc-model-config.cc(2 hunks)sherpa-onnx/csrc/offline-zipformer-ctc-model-config.h(1 hunks)sherpa-onnx/csrc/qnn-config.cc(1 hunks)sherpa-onnx/csrc/qnn/offline-recognizer-zipformer-ctc-qnn-impl.h(1 hunks)sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.cc(1 hunks)sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.h(1 hunks)sherpa-onnx/jni/offline-recognizer.cc(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T04:18:47.981Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:18:47.981Z
Learning: In sherpa-onnx Java API, the native library names in Core.java (WIN_NATIVE_LIBRARY_NAME = "sherpa-onnx-jni.dll", UNIX_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.so", MACOS_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.dylib") are copied directly from the compiled binary filenames and should not be changed to match other libraries' naming conventions.
Applied to files:
sherpa-onnx/csrc/qnn-config.ccsherpa-onnx/csrc/CMakeLists.txt
📚 Learning: 2025-08-06T04:23:50.237Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:23:50.237Z
Learning: The sherpa-onnx JNI library files are stored in Hugging Face repository at https://huggingface.co/csukuangfj/sherpa-onnx-libs under versioned directories like jni/1.12.7/, and the actual Windows JNI library filename is "sherpa-onnx-jni.dll" as defined in Core.java constants.
Applied to files:
sherpa-onnx/csrc/offline-recognizer-impl.ccsherpa-onnx/csrc/CMakeLists.txt
🧬 Code graph analysis (7)
sherpa-onnx/csrc/offline-sense-voice-model-config.cc (1)
sherpa-onnx/csrc/offline-zipformer-ctc-model-config.cc (1)
p(19-19)
sherpa-onnx/csrc/offline-recognizer-ctc-impl.h (4)
sherpa-onnx/csrc/offline-recognizer-paraformer-impl.h (1)
Convert(25-81)sherpa-onnx/csrc/offline-recognizer-transducer-impl.h (1)
Convert(33-115)sherpa-onnx/csrc/offline-recognizer-fire-red-asr-impl.h (1)
Convert(26-45)sherpa-onnx/csrc/offline-recognizer-moonshine-impl.h (1)
Convert(26-45)
sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.h (3)
sherpa-onnx/csrc/offline-zipformer-ctc-model-config.h (1)
sherpa_onnx(12-32)sherpa-onnx/csrc/qnn/offline-recognizer-zipformer-ctc-qnn-impl.h (1)
sherpa_onnx(23-127)sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.cc (16)
OfflineZipformerCtcModelQnn(213-215)OfflineZipformerCtcModelQnn(218-220)OfflineZipformerCtcModelQnn(222-222)OfflineZipformerCtcModelQnn(238-239)OfflineZipformerCtcModelQnn(243-244)Run(224-227)Run(224-225)features(82-108)features(82-82)VocabSize(229-231)VocabSize(229-229)SubsamplingFactor(233-235)SubsamplingFactor(233-233)Impl(33-72)Impl(33-33)Impl(75-80)
sherpa-onnx/jni/offline-recognizer.cc (3)
nodejs-addon-examples/test_asr_non_streaming_paraformer_itn.js (1)
config(6-22)nodejs-addon-examples/test_asr_non_streaming_wenet_ctc.js (1)
config(6-22)nodejs-addon-examples/test_asr_non_streaming_zipformer_ctc.js (1)
config(6-20)
sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.cc (1)
sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.h (1)
OfflineZipformerCtcModelQnn(14-35)
sherpa-onnx/csrc/offline-zipformer-ctc-model-config.cc (2)
sherpa-onnx/csrc/offline-sense-voice-model-config.cc (3)
Register(15-29)Register(15-15)p(26-26)sherpa-onnx/csrc/qnn-config.cc (2)
Register(15-32)Register(15-15)
sherpa-onnx/csrc/qnn/offline-recognizer-zipformer-ctc-qnn-impl.h (4)
sherpa-onnx/csrc/offline-recognizer-ctc-impl.h (2)
Convert(26-316)- `` (186-192)
sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.h (1)
OfflineZipformerCtcModelQnn(14-35)sherpa-onnx/csrc/offline-recognizer-impl.cc (8)
OfflineRecognizerImpl(567-618)OfflineRecognizerImpl(621-671)OfflineRecognizerImpl(700-701)OfflineRecognizerImpl(708-709)ApplyInverseTextNormalization(673-684)ApplyInverseTextNormalization(673-674)ApplyHomophoneReplacer(686-693)ApplyHomophoneReplacer(686-687)sherpa-onnx/csrc/rknn/offline-ctc-greedy-search-decoder-rknn.h (1)
OfflineCtcGreedySearchDecoderRknn(14-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: ubuntu-latest Debug shared tts-OFF
- GitHub Check: ubuntu-latest Release static tts-OFF
- GitHub Check: ubuntu-latest Release shared tts-OFF
- GitHub Check: ubuntu-latest Release static tts-ON
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: ubuntu-24.04 3.11
- GitHub Check: rknn shared ON
- GitHub Check: ubuntu-24.04 3.12
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: ubuntu-24.04 3.9
- GitHub Check: ubuntu-24.04 3.8
- GitHub Check: swift (macos-latest)
- GitHub Check: swift (macos-13)
🔇 Additional comments (14)
sherpa-onnx/jni/offline-recognizer.cc (1)
335-338: LGTM! Improved validation consistency.The unconditional validation ensures all providers, including QNN, are validated before creating the recognizer. This is more consistent and safer than the previous provider-specific gating.
sherpa-onnx/csrc/offline-zipformer-ctc-model-config.h (1)
10-18: LGTM! QNN config integration looks good.The addition of QNN configuration support follows the same pattern used in other model configs (e.g., SenseVoice).
sherpa-onnx/csrc/CMakeLists.txt (1)
211-212: LGTM! Build configuration updated correctly.The new QNN model sources are properly registered under the
SHERPA_ONNX_ENABLE_QNNconditional block.sherpa-onnx/csrc/offline-recognizer-impl.cc (3)
50-50: LGTM! Header inclusion for QNN Zipformer CTC support.
105-110: LGTM! QNN provider logic extended correctly.The addition of Zipformer CTC support follows the existing pattern for SenseVoice, with proper fallback and error messaging.
361-367: LGTM! Template version matches non-template implementation.The template version correctly mirrors the QNN provider logic from the non-template Create function.
sherpa-onnx/csrc/offline-sense-voice-model-config.cc (1)
25-28: LGTM! Scoped QNN config registration prevents option collisions.Using a prefixed ParseOptions scope ensures QNN options for SenseVoice don't conflict with QNN options for other models (e.g., Zipformer CTC). This is the same pattern used in
offline-zipformer-ctc-model-config.cc.sherpa-onnx/csrc/offline-zipformer-ctc-model-config.cc (3)
11-11: LGTM! Required include for EndsWith utility.
18-21: LGTM! Scoped QNN config registration.The "zipformer-ctc" prefix ensures QNN options are namespaced, preventing collisions with other models' QNN configurations. This matches the pattern in
offline-sense-voice-model-config.cc.
31-33: LGTM! Appropriate validation delegation for QNN models.The logic correctly identifies QNN models by file extension (.so/.bin) and delegates validation to the QNN config. This allows different validation paths for ONNX vs QNN models.
sherpa-onnx/csrc/qnn-config.cc (1)
40-41: Runtime library loading validation is in place, but verify error messaging meets deployment needs.The removal of file existence checks is intentional and documented in the code. Runtime validation occurs via dlopen() calls in
qnn-backend.cc,qnn-model.cc, andqnn-model.cc, which include error checks and logging usingdlerror(). However, no dedicated test files for QNN library loading were found in the codebase, making test coverage unclear.Consider confirming that the
dlerror()messages logged bySHERPA_ONNX_LOGE()are sufficient for your deployment scenario, and verify whether integration tests elsewhere cover library loading failures.sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.h (1)
14-35: API and Pimpl structure look consistent with existing offline modelsThe class surface (constructors,
Run,VocabSize,SubsamplingFactor) and Pimpl usage are clean and align with how other offline model wrappers are structured. No functional concerns from the header side.sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.cc (1)
31-72: Overall QNN model wiring and concurrency handling look solid
- Context binary vs model-lib initialization follows the expected QNN pattern, including optional context creation and debug logging.
PostInit()/CheckModel()establishmax_num_frames_,feat_dim_,vocab_size_, andsubsampling_factor_once at construction.Run()does shape normalization (truncate/zero-pad) and guardsQnnModelcalls with a mutex, which is appropriate if the model object is shared across threads.Aside from the output-shape checks mentioned separately, the rest of the implementation reads correctly and matches how the recognizer expects to use it.
Also applies to: 82-112, 114-211, 213-235
sherpa-onnx/csrc/qnn/offline-recognizer-zipformer-ctc-qnn-impl.h (1)
31-84: Recognizer wiring and decoding flow are consistent with existing CTC implementationsThe new
OfflineRecognizerZipformerCtcQnnImplclosely follows the established pattern:
- Config and
SymbolTableinitialization mirror other offline recognizers.Init()enforcesgreedy_searchand derivesblank_idin the same way as the non-QNN CTC path, with the omnilingual special case.DecodeStream()performs model inference, CTC decoding, and then applies ITN and homophone replacement before storing the result.Functionally this looks correct and aligned with the rest of the codebase.
Also applies to: 90-120, 122-127
| auto out_shape = model_->TensorShape("log_probs"); | ||
| vocab_size_ = out_shape[2]; | ||
|
|
||
| subsampling_factor_ = max_num_frames_ / out_shape[1]; | ||
| if (config_.debug) { | ||
| SHERPA_ONNX_LOGE("max_num_frames: %d", max_num_frames_); | ||
| SHERPA_ONNX_LOGE("feat_dim: %d", feat_dim_); | ||
| SHERPA_ONNX_LOGE("vocab_size: %d", vocab_size_); | ||
| SHERPA_ONNX_LOGE("subsampling_factor: %d", subsampling_factor_); | ||
| } |
There was a problem hiding this comment.
Harden log_probs shape checks to avoid undefined behavior and bad subsampling factor
CheckModel() assumes log_probs is 3‑D and indexes out_shape[2] without verifying its rank, and also divides by out_shape[1] without checking for zero or consistency with max_num_frames_. A mis-exported or incompatible model could cause out-of-bounds access or a bogus subsampling_factor_.
Consider tightening this block:
- auto out_shape = model_->TensorShape("log_probs");
- vocab_size_ = out_shape[2];
-
- subsampling_factor_ = max_num_frames_ / out_shape[1];
+ auto out_shape = model_->TensorShape("log_probs");
+ if (out_shape.size() != 3) {
+ SHERPA_ONNX_LOGE(
+ "The output tensor 'log_probs' should be 3-d, actual '%d'",
+ static_cast<int32_t>(out_shape.size()));
+ SHERPA_ONNX_EXIT(-1);
+ }
+ if (out_shape[0] != 1) {
+ SHERPA_ONNX_LOGE(
+ "The log_probs.shape[0] should be 1, actual '%d'", out_shape[0]);
+ SHERPA_ONNX_EXIT(-1);
+ }
+
+ vocab_size_ = out_shape[2];
+
+ if (out_shape[1] <= 0 || max_num_frames_ % out_shape[1] != 0) {
+ SHERPA_ONNX_LOGE(
+ "Mismatched time dims: x.shape[1]=%d, log_probs.shape[1]=%d",
+ max_num_frames_, out_shape[1]);
+ SHERPA_ONNX_EXIT(-1);
+ }
+
+ subsampling_factor_ = max_num_frames_ / out_shape[1];This keeps failure modes explicit and consistent with the earlier input‑shape checks.
📝 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.
| auto out_shape = model_->TensorShape("log_probs"); | |
| vocab_size_ = out_shape[2]; | |
| subsampling_factor_ = max_num_frames_ / out_shape[1]; | |
| if (config_.debug) { | |
| SHERPA_ONNX_LOGE("max_num_frames: %d", max_num_frames_); | |
| SHERPA_ONNX_LOGE("feat_dim: %d", feat_dim_); | |
| SHERPA_ONNX_LOGE("vocab_size: %d", vocab_size_); | |
| SHERPA_ONNX_LOGE("subsampling_factor: %d", subsampling_factor_); | |
| } | |
| auto out_shape = model_->TensorShape("log_probs"); | |
| if (out_shape.size() != 3) { | |
| SHERPA_ONNX_LOGE( | |
| "The output tensor 'log_probs' should be 3-d, actual '%d'", | |
| static_cast<int32_t>(out_shape.size())); | |
| SHERPA_ONNX_EXIT(-1); | |
| } | |
| if (out_shape[0] != 1) { | |
| SHERPA_ONNX_LOGE( | |
| "The log_probs.shape[0] should be 1, actual '%d'", out_shape[0]); | |
| SHERPA_ONNX_EXIT(-1); | |
| } | |
| vocab_size_ = out_shape[2]; | |
| if (out_shape[1] <= 0 || max_num_frames_ % out_shape[1] != 0) { | |
| SHERPA_ONNX_LOGE( | |
| "Mismatched time dims: x.shape[1]=%d, log_probs.shape[1]=%d", | |
| max_num_frames_, out_shape[1]); | |
| SHERPA_ONNX_EXIT(-1); | |
| } | |
| subsampling_factor_ = max_num_frames_ / out_shape[1]; | |
| if (config_.debug) { | |
| SHERPA_ONNX_LOGE("max_num_frames: %d", max_num_frames_); | |
| SHERPA_ONNX_LOGE("feat_dim: %d", feat_dim_); | |
| SHERPA_ONNX_LOGE("vocab_size: %d", vocab_size_); | |
| SHERPA_ONNX_LOGE("subsampling_factor: %d", subsampling_factor_); | |
| } |
There was a problem hiding this comment.
Pull Request Overview
This PR extends the QNN (Qualcomm Neural Network) backend to support Zipformer CTC acoustic models for offline speech recognition, complementing the existing SenseVoice model support. The implementation follows established patterns from the SenseVoice QNN integration and removes unnecessary file existence checks that prevented using library search paths.
Key changes:
- Added complete QNN backend implementation for Zipformer CTC models with model initialization, context binary caching, and inference
- Refactored QNN configuration validation to allow dlopen() to resolve library paths instead of requiring explicit file paths
- Made the
Convertfunction non-static to enable code reuse across different recognizer implementations
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.h | Header defining the QNN-based Zipformer CTC model class with initialization and inference methods |
| sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.cc | Implementation of QNN Zipformer CTC model with context binary caching and model validation |
| sherpa-onnx/csrc/qnn/offline-recognizer-zipformer-ctc-qnn-impl.h | Recognizer implementation for Zipformer CTC models using QNN backend with CTC greedy search decoder |
| sherpa-onnx/csrc/offline-zipformer-ctc-model-config.h | Added QNN configuration field to Zipformer CTC model configuration |
| sherpa-onnx/csrc/offline-zipformer-ctc-model-config.cc | Integrated QNN config registration with model-specific prefix and validation for .so/.bin files |
| sherpa-onnx/csrc/offline-sense-voice-model-config.cc | Updated to use model-specific prefix for QNN config registration |
| sherpa-onnx/csrc/qnn-config.cc | Removed file existence checks for backend_lib and system_lib to allow dlopen() path resolution |
| sherpa-onnx/csrc/offline-recognizer-impl.cc | Updated factory methods to instantiate Zipformer CTC QNN recognizer and updated error messages |
| sherpa-onnx/csrc/offline-recognizer-ctc-impl.h | Changed Convert function from static to non-static for reuse in QNN implementations |
| sherpa-onnx/csrc/CMakeLists.txt | Added offline-zipformer-ctc-model-qnn.cc to QNN sources list |
| sherpa-onnx/jni/offline-recognizer.cc | Removed QNN-specific condition from config validation to allow QNN backend validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Decode a single stream. | ||
| // Some models do not support batch size > 1, e.g., WeNet CTC models. | ||
| void DecodeStream(OfflineStream *s) const { | ||
| int32_t feat_dim = s->FeatureDim(); |
There was a problem hiding this comment.
The variable feat_dim is assigned but never used in this function. Consider removing it if it's not needed for validation or debugging purposes.
| int32_t feat_dim = s->FeatureDim(); |
dosubot
Bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
sherpa-onnx/csrc/qnn/offline-recognizer-zipformer-ctc-qnn-impl.h (2)
56-65: Remove the confusing omnilingual model check.The condition
config_.model_config.omnilingual.model.empty()and associated comment (line 60) are irrelevant for a Zipformer CTC QNN-specific implementation. This appears to be copy-pasted from the generic CTC recognizer. Since this implementation is exclusively for Zipformer CTC models, simplify the logic to only verify that one of the blank symbols exists.Apply this diff to clarify the blank symbol validation:
- if (!symbol_table_.Contains("<blk>") && - !symbol_table_.Contains("<eps>") && - !symbol_table_.Contains("<blank>") && - config_.model_config.omnilingual.model.empty()) { - // for omnilingual asr, its blank id is 0 + if (!symbol_table_.Contains("<blk>") && + !symbol_table_.Contains("<eps>") && + !symbol_table_.Contains("<blank>")) { SHERPA_ONNX_LOGE( "We expect that tokens.txt contains " "the symbol <blk> or <eps> or <blank> and its ID.");
112-112: Use configured frame_shift_ms instead of hardcoded value.The
frame_shift_msis hardcoded to 10, which may produce incorrect timestamps if the feature configuration specifies a different frame shift. Useconfig_.feat_config.frame_shift_msto respect the user's configuration.Apply this diff:
- int32_t frame_shift_ms = 10; + int32_t frame_shift_ms = config_.feat_config.frame_shift_ms;
🧹 Nitpick comments (4)
android/SherpaOnnxSimulateStreamingAsr/app/src/main/java/com/k2fsa/sherpa/onnx/simulate/streaming/asr/SimulateStreamingAsr.kt (2)
128-131: Clarify QNN backend validation scope and model assumptionsThis guard enforces that at least one of
senseVoice.qnnConfig.backendLiborzipformerCtc.qnnConfig.backendLibis set whenprovider == "qnn". That’s good for catching misconfigured SenseVoice/Zipformer‑CTC models, but it also bakes in the assumption that those are the only QNN model families in use.If you ever introduce QNN support for other offline models (e.g., non‑CTC Zipformer or other types), this condition will cause a hard failure even when a different model’s
qnnConfigis correctly set. Consider either:
- Narrowing the check to “if we are actually using senseVoice/zipformerCtc here, then require backendLib”, or
- Centralizing QNN config validation in one place that knows which sub‑model is active.
For now this is fine if those are the only supported QNN models, but it’s worth verifying the assumption to avoid future surprises.
135-153: Confirm single‑model assumption and optionalcontextBinaryhandlingThe new copy logic prefers SenseVoice if
senseVoice.modelis non‑empty and only falls back tozipformerCtcif SenseVoice is empty. Two things to double‑check:
Mutual exclusivity of models
If bothsenseVoice.modelandzipformerCtc.modelcan ever be set simultaneously, only the SenseVoice assets will be copied and initialized, which may not match caller expectations. If they are intended to be mutually exclusive, a small assertion/log would make that contract explicit.Empty
contextBinarysemantics
IfqnnConfig.contextBinaryis allowed to be empty (e.g., no prebuilt context, runtime builds it),copyAssetToInternalStorage("")will return a directory path (filesDir) and log a misleading “ does not exist” message. It may be safer to guard with anisNotEmpty()check around the copy or explicitly document thatcontextBinarymust be non‑empty in QNN configs.These are mostly contract/robustness questions; behavior is fine if current configs always set exactly one model and a non‑empty context binary.
sherpa-onnx/csrc/offline-zipformer-ctc-model-config.cc (1)
11-22: QNN registration/validation path looks good; consider tighteningToString()formattingThe new
ParseOptionsscope withprefix = "zipformer-ctc"and the.so/.bingate intoqnn_config.Validate()integrate cleanly with existing config/validation.One small cosmetic improvement:
ToString()currently produces a trailing comma before the closing parenthesis whenqnn_configis present, e.g...., qnn_config=QnnConfig(...), ). You can avoid that with:- os << "model=\"" << model << "\""; - - if (!qnn_config.backend_lib.empty()) { - os << ", qnn_config=" << qnn_config.ToString() << ", "; - } - - os << ")"; + os << "model=\"" << model << "\""; + + if (!qnn_config.backend_lib.empty()) { + os << ", qnn_config=" << qnn_config.ToString(); + } + + os << ")";Also applies to: 31-33, 42-49
sherpa-onnx/kotlin-api/OfflineRecognizer.kt (1)
772-773: QNN presets for SenseVoice and Zipformer-CTC look consistent; double-check debug flags and consider dedup
- IDs
9011–9021for Zipformer-CTC QNN mirror the SenseVoice QNN presets: same directory naming scheme,libmodel.so+model.binlayout, and identicalQnnConfigwiring (backendLib,systemLib,contextBinary).- Enabling
debug = trueonly for the shorter-duration presets (9000–9002and9011–9013) is fine if you intentionally want extra logging there; otherwise you may want to aligndebugacross all QNN presets.If these blocks keep growing, it might be worth factoring out a small helper to construct these QNN
OfflineModelConfigpresets frommodelDirto avoid further copy‑paste.Also applies to: 789-790, 806-807, 938-1115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/apk-qnn-vad-asr-simulated-streaming.yaml(3 hunks).gitignore(1 hunks)android/SherpaOnnxSimulateStreamingAsr/app/src/main/java/com/k2fsa/sherpa/onnx/simulate/streaming/asr/SimulateStreamingAsr.kt(1 hunks)scripts/apk/generate-qnn-vad-asr-apk-script.py(1 hunks)sherpa-onnx/csrc/offline-zipformer-ctc-model-config.cc(2 hunks)sherpa-onnx/csrc/qnn/offline-recognizer-zipformer-ctc-qnn-impl.h(1 hunks)sherpa-onnx/csrc/qnn/qnn-model.cc(1 hunks)sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineZipformerCtcModelConfig.java(3 hunks)sherpa-onnx/jni/offline-recognizer.cc(2 hunks)sherpa-onnx/kotlin-api/OfflineRecognizer.kt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sherpa-onnx/jni/offline-recognizer.cc
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T04:23:50.237Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:23:50.237Z
Learning: The sherpa-onnx JNI library files are stored in Hugging Face repository at https://huggingface.co/csukuangfj/sherpa-onnx-libs under versioned directories like jni/1.12.7/, and the actual Windows JNI library filename is "sherpa-onnx-jni.dll" as defined in Core.java constants.
Applied to files:
.gitignore
📚 Learning: 2025-08-06T04:18:47.981Z
Learnt from: litongjava
Repo: k2-fsa/sherpa-onnx PR: 2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:18:47.981Z
Learning: In sherpa-onnx Java API, the native library names in Core.java (WIN_NATIVE_LIBRARY_NAME = "sherpa-onnx-jni.dll", UNIX_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.so", MACOS_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.dylib") are copied directly from the compiled binary filenames and should not be changed to match other libraries' naming conventions.
Applied to files:
.gitignore
🧬 Code graph analysis (4)
sherpa-onnx/csrc/qnn/qnn-model.cc (1)
sherpa-onnx/csrc/qnn/qnn-backend.cc (3)
ret(185-188)ret(190-194)ret(196-200)
sherpa-onnx/csrc/offline-zipformer-ctc-model-config.cc (2)
sherpa-onnx/csrc/qnn-config.cc (4)
Register(15-32)Register(15-15)ToString(55-64)ToString(55-55)sherpa-onnx/csrc/offline-ctc-fst-decoder-config.cc (1)
p(27-27)
sherpa-onnx/csrc/qnn/offline-recognizer-zipformer-ctc-qnn-impl.h (3)
sherpa-onnx/csrc/qnn/offline-zipformer-ctc-model-qnn.h (2)
sherpa_onnx(12-37)OfflineZipformerCtcModelQnn(14-35)sherpa-onnx/csrc/offline-recognizer-ctc-impl.h (5)
sherpa_onnx(24-318)Convert(26-316)Init(93-184)- `` (186-192)
DecodeStreams(194-264)sherpa-onnx/csrc/offline-recognizer-impl.h (1)
std(36-40)
sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineZipformerCtcModelConfig.java (2)
sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineSenseVoiceModelConfig.java (1)
Builder(38-67)sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/QnnConfig.java (1)
QnnConfig(5-56)
🔇 Additional comments (8)
sherpa-onnx/csrc/qnn/qnn-model.cc (1)
137-137: LGTM! Explicit cast improves type clarity.The explicit
static_cast<int32_t>(ret)ensures the QNN error code is properly formatted in the log message. This is a minor improvement that enhances type safety without any functional impact.scripts/apk/generate-qnn-vad-asr-apk-script.py (1)
214-228: Balancedpushd/popdin 9010 command block looks correctThe added
popdin thecmdfor modelidx=9010cleanly restores the directory stack and matches the pattern used for the other models. No further changes needed here..gitignore (1)
162-163: New ignore patterns for Zipformer-CTC QNN artifacts are consistentNames and placement match existing QNN-related model directories; no issues.
.github/workflows/apk-qnn-vad-asr-simulated-streaming.yaml (1)
7-7: Workflow trigger and 10-way sharding look correct; confirm generator script assumptionsSwitching the push branch to
zipformer-ctc-qnn-2, expanding the matrix to 10 shards (total: "10",index: "0".."9"), and using a distinct-android-qnnccache key are all consistent with the new QNN APK workload. Please just confirm thatscripts/apk/generate-qnn-vad-asr-apk-script.pydoes not assumetotal == 5or a smaller index range.Also applies to: 27-28, 50-50
sherpa-onnx/kotlin-api/OfflineRecognizer.kt (1)
35-38: Zipformer-CTC Kotlin config gains QNN support in a consistent wayAdding
qnnConfig: QnnConfig = QnnConfig()toOfflineZipformerCtcModelConfigmirrorsOfflineSenseVoiceModelConfigand keeps the Kotlin config surface in sync with Java/C++.sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineZipformerCtcModelConfig.java (1)
7-12: Java Zipformer-CTC config QNN wiring matches existing SenseVoice patternThe new
qnnConfigfield, getter, and builder plumbing are consistent withOfflineSenseVoiceModelConfigand keep the Java API aligned with the Kotlin/C++ layers. No issues from a construction or immutability standpoint.Also applies to: 22-24, 28-29, 39-42
sherpa-onnx/csrc/qnn/offline-recognizer-zipformer-ctc-qnn-impl.h (2)
25-29: LGTM: Forward declaration is correct.The forward declaration correctly references the
Convertfunction defined inoffline-recognizer-ctc-impl.h, aligning with the external linkage pattern for this shared utility.
101-119: Confirm whether the QNN model requires feature normalization before inference.The investigation reveals that the generic CTC implementation calls
model_->NormalizeFeatures()to perform mean-variance normalization before inference. The QNN implementation skips this step entirely—QnnModel::SetInputTensorData()only performs quantization conversion, not feature normalization. TheOfflineZipformerCtcModelQnnclass has noNormalizeFeaturesmethod at all.Verify whether:
- The QNN model was trained without normalization and does not require it, or
- Feature normalization should be added in
DecodeStream()before callingmodel_->Run()(similar to the generic CTC recognizer pattern)
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.