Add C++ runtime for Whisper with Ascend NPU - #3009
Conversation
dosubot
Bot
added
the
size:XL
📝 WalkthroughWalkthroughAdds Ascend NPU Whisper support: new Ascend-specific OfflineWhisperModel implementation and header, Whisper model/config utilities and token mappings, feature-normalization helpers, a templated Whisper recognizer, Ascend utility fixes (move/Release, MB calc), CMake registration, and a minor Zipformer zero-init change. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Recognizer as OfflineRecognizerWhisperTplImpl
participant Model as OfflineWhisperModelAscend
participant Ascend as Ascend NPU Runtime
App->>Recognizer: CreateStream()
Recognizer->>App: stream (includes feature_dim)
App->>Recognizer: Push features / Request decode
Recognizer->>Recognizer: NormalizeWhisperFeatures(features)
Recognizer->>Model: Run(normalized_features)
Model->>Ascend: Upload inputs / Execute encoder
Ascend-->>Model: encoder outputs
Model->>Ascend: Prepare decoder inputs / Execute decoder steps (KV cache updates)
Ascend-->>Model: logits / KV states
Model-->>Recognizer: OfflineWhisperDecoderResult (tokens, lang)
Recognizer-->>App: OfflineRecognitionResult (text, tokens, lang)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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
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 extends the 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 a C++ runtime for Whisper models on Ascend NPU, which is a significant feature enhancement. The overall structure, including the use of a PIMPL pattern and a templated recognizer implementation, is well-designed for extensibility. However, as a work-in-progress, there are several critical issues and areas for improvement. I've identified a compilation bug in a move assignment operator, some unimplemented constructors and methods that would lead to runtime crashes, and incomplete logic in the decoding process. Additionally, I've suggested some refactoring to reduce code duplication and improve readability. Addressing these points will be crucial for finalizing this feature.
| Impl(Manager *mgr, const OfflineModelConfig &config) : config_(config) { | ||
| SHERPA_ONNX_LOGE("Not implemented"); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } |
There was a problem hiding this comment.
The constructor for loading models via an asset manager is not implemented and will cause the program to exit if called. This is a critical omission if this functionality is expected to be supported, as it is in other backends within the project. Please implement this constructor to handle model loading from a manager.
| other.p_ = nullptr; | ||
| } | ||
| AclDataBuffer &operator=(AclDataBuffer &&other) { | ||
| if (other == *this) { |
There was a problem hiding this comment.
The self-assignment check if (other == *this) is incorrect because operator== is not defined for AclDataBuffer. This will cause a compilation error. The check should compare the addresses of the objects.
| if (other == *this) { | |
| if (this == &other) { |
|
|
||
| NormalizeWhisperFeatures(f.data(), num_frames, feat_dim); | ||
|
|
||
| model_->Run(std::move(f)); |
There was a problem hiding this comment.
The DecodeStream function is incomplete. It calls model_->Run(), but the returned token IDs are discarded, and no recognition result is set on the stream. The implementation should process the tokens, convert them to text, and set the result on the stream s using s->SetResult(). Without this, the recognizer will not produce any output.
| "small chunks."); | ||
| } | ||
|
|
||
| features.resize(num_frames_ * feat_dim_); |
There was a problem hiding this comment.
For clarity and to avoid any ambiguity with default initialization, it's better to explicitly specify the padding value when resizing the features vector. This is consistent with changes made in other parts of the codebase.
features.resize(num_frames_ * feat_dim_, 0.0f);| cross_kv_ptr_.reserve(n_text_layer_ * 2); | ||
| for (int32_t i = 0; i < n_text_layer_ * 2; ++i) { | ||
| auto p = std::make_unique<AclDevicePtr>(num_out_frames_ * n_text_state_ * | ||
| sizeof(float)); | ||
| cross_kv_ptr_.push_back(std::move(p)); | ||
| total += num_out_frames_ * n_text_state_ * sizeof(float); | ||
| } | ||
|
|
||
| self_kv_ptr_.reserve(n_text_layer_ * 2); | ||
| for (int32_t i = 0; i < n_text_layer_ * 2; ++i) { | ||
| auto p = std::make_unique<AclDevicePtr>(n_text_ctx_ * n_text_state_ * | ||
| sizeof(float)); | ||
| self_kv_ptr_.push_back(std::move(p)); | ||
| total += n_text_ctx_ * n_text_state_ * sizeof(float); | ||
| } | ||
|
|
||
| delta_kv_ptr_.reserve(n_text_layer_ * 2); | ||
| for (int32_t i = 0; i < n_text_layer_ * 2; ++i) { | ||
| auto p = std::make_unique<AclDevicePtr>(n_text_state_ * sizeof(float)); | ||
| delta_kv_ptr_.push_back(std::move(p)); | ||
| total += n_text_state_ * sizeof(float); | ||
| } |
There was a problem hiding this comment.
The loops for allocating cross_kv_ptr_, self_kv_ptr_, and delta_kv_ptr_ are very similar. This code can be refactored into a helper function to reduce duplication and improve maintainability.
For example, you could introduce a helper function like this:
void AllocateDevicePtrs(std::vector<std::unique_ptr<AclDevicePtr>>& vec, int32_t count, size_t size, int32_t& total) {
vec.reserve(count);
for (int32_t i = 0; i < count; ++i) {
auto p = std::make_unique<AclDevicePtr>(size);
vec.push_back(std::move(p));
total += size;
}
}| const std::vector<std::vector<int64_t>> &input_shapes = | ||
| decoder_model_->GetInputShapes(); | ||
| // tokens, self_kv, cross_kv, offset, mask | ||
| int32_t expected_num_inputs = 1 + 2 * n_text_layer_ + 2 * n_text_layer_ + 2; |
There was a problem hiding this comment.
The expression for expected_num_inputs can be simplified for better readability.
int32_t expected_num_inputs = 3 + 4 * n_text_layer_;| auto memory_info = | ||
| Ort::MemoryInfo::CreateCpu(OrtDeviceAllocator, OrtMemTypeDefault); |
There was a problem hiding this comment.
The memory_info variable is initialized but never used. It 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: 4
🤖 Fix all issues with AI agents
In @sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc:
- Around line 72-76: The template constructor Impl(Manager *mgr, const
OfflineModelConfig &config) currently aborts at runtime (SHERPA_ONNX_LOGE /
SHERPA_ONNX_EXIT) but is explicitly instantiated elsewhere, causing crashes on
Android/OHOS; fix by either implementing the Manager-based path (e.g., delegate
to the existing non-template Impl(const OfflineModelConfig&) initializer or
perform proper Manager-specific initialization and set config_ without exiting)
or, if Manager support is not intended, remove the explicit template
instantiations and the template constructor declaration (or replace with a
compile-time guard/static_assert that clearly indicates WIP). Ensure changes
reference the Impl(Manager *mgr, const OfflineModelConfig &config) template and
the explicit instantiations so the build no longer hits the runtime exit.
- Around line 85-97: The code always calls features.resize(num_frames_ *
feat_dim_) even when input is shorter, causing unintended padding; change the
logic to compute an explicit frames_to_use = std::min(num_frames, num_frames_)
and only call features.resize(num_frames_ * feat_dim_) when num_frames >
num_frames_ (i.e., truncation), leaving smaller inputs unpadded unless padding
is intentional; update downstream uses (e.g., the transpose call and any buffer
lengths) to use frames_to_use (or num_frames_ only when you actually truncated)
so the transpose uses the correct frame count and the warning remains for
truncation cases.
In @sherpa-onnx/csrc/ascend/utils.h:
- Around line 178-188: The move assignment operator
AclDataBuffer::operator=(AclDataBuffer &&other) incorrectly attempts to compare
objects with `if (other == *this)`; replace this with an address comparison `if
(this == &other)` to detect self-assignment, leaving the rest of the method
(calling Release(), moving p_, and nulling other.p_) unchanged.
In @sherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.h:
- Around line 75-86: DecodeStream currently creates an unused memory_info, drops
model_->Run() output, and never writes recognition results to the OfflineStream;
remove the dead memory_info, capture the returned std::vector<int32_t> tokens =
model_->Run(std::move(f)), convert those token IDs to text using symbol_table_
(e.g., look up each id and join into a string, handling any special tokens), and
store the resulting text back on the stream (call the stream's result setter
such as OfflineStream::SetResult or similar). Keep
NormalizeWhisperFeatures(f.data(), num_frames, feat_dim) and ensure
FeatureDim(), GetFrames(), symbol_table_, and model_->Run are used as described.
🧹 Nitpick comments (5)
sherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.h (1)
65-66: Consider making debug log conditional.Using
SHERPA_ONNX_LOGEfor non-error messages like "use greedy_search" may clutter logs in production. Consider guarding this with a debug flag similar to other parts of the codebase (e.g.,if (config_.model_config.debug)).sherpa-onnx/csrc/math.cc (1)
106-117: Disabled Eigen implementation has incorrect syntax.The
#if 0block has a syntax error at line 110:Map<ArrayXXf, Eigen::RowMajor> feats(features, num_frames, feat_dim);
Eigen::RowMajoris not a valid alignment parameter forMap. For row-major mapping, you need:using RowMajorArrayXXf = Eigen::Array<float, Eigen::Dynamic, Eigen::Dynamic, Eigen::RowMajor>; Eigen::Map<RowMajorArrayXXf> feats(features, num_frames, feat_dim);If you plan to enable this optimization later, the current code would not compile.
♻️ Suggested fix for the Eigen block
#if 0 - using Eigen::ArrayXXf; using Eigen::Map; + using RowMajorArrayXXf = Eigen::Array<float, Eigen::Dynamic, Eigen::Dynamic, Eigen::RowMajor>; - Map<ArrayXXf, Eigen::RowMajor> feats(features, num_frames, feat_dim); + Map<RowMajorArrayXXf> feats(features, num_frames, feat_dim); feats = feats.max(1e-10f).log10();sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.h (1)
24-28: Consider clarifying the features parameter documentation.The comment says features shape is
(1, feat_dim, num_frames)but the parameter is a flatstd::vector<float>. Consider clarifying whether the vector should be in row-major or column-major order, and what the expected size is (feat_dim * num_frames).sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (2)
34-39: Unused function:CreateCausalMask.This function is defined but never called anywhere in this file. Consider removing dead code or adding a TODO if it's needed for the decoder implementation.
102-107: Debug logging in production path and incomplete decoder.
Lines 102-105:
SHERPA_ONNX_LOGEcalls for "features size" and "run encoder done" should be guarded withconfig_.debugto avoid log noise in production.Line 107:
Runalways returns an empty vector since the decoder is not implemented. This is WIP, but callers will receive no tokens.♻️ Guard debug logs
- SHERPA_ONNX_LOGE("features size: %d. %dx%d", (int)features.size(), - (int)features.size() / feat_dim_, feat_dim_); + if (config_.debug) { + SHERPA_ONNX_LOGE("features size: %d. %dx%d", (int)features.size(), + (int)features.size() / feat_dim_, feat_dim_); + } RunEncoder(std::move(features)); - SHERPA_ONNX_LOGE("run encoder done!"); + if (config_.debug) { + SHERPA_ONNX_LOGE("run encoder done!"); + } + // TODO(fangjun): Implement decoder to produce token IDs return {};
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
sherpa-onnx/csrc/CMakeLists.txtsherpa-onnx/csrc/ascend/offline-whisper-model-ascend.ccsherpa-onnx/csrc/ascend/offline-whisper-model-ascend.hsherpa-onnx/csrc/ascend/offline-zipformer-ctc-model-ascend.ccsherpa-onnx/csrc/ascend/utils.ccsherpa-onnx/csrc/ascend/utils.hsherpa-onnx/csrc/math.ccsherpa-onnx/csrc/math.hsherpa-onnx/csrc/offline-recognizer-impl.ccsherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.hsherpa-onnx/csrc/offline-whisper-model-config.ccsherpa-onnx/csrc/offline-whisper-model-config.hsherpa-onnx/csrc/offline-whisper-model.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:
sherpa-onnx/csrc/offline-whisper-model.cc
📚 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/offline-whisper-model.cc
🧬 Code graph analysis (9)
sherpa-onnx/csrc/math.h (1)
sherpa-onnx/csrc/math.cc (2)
NormalizeWhisperFeatures(101-143)NormalizeWhisperFeatures(101-102)
sherpa-onnx/csrc/ascend/utils.cc (1)
sherpa-onnx/csrc/ascend/utils.h (1)
AclDataBuffer(166-197)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.h (1)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (16)
OfflineWhisperModelAscend(303-305)OfflineWhisperModelAscend(308-310)OfflineWhisperModelAscend(312-312)OfflineWhisperModelAscend(324-325)OfflineWhisperModelAscend(329-330)Run(314-317)Run(314-315)features(78-108)features(78-78)features(113-138)features(113-113)FeatureDim(319-321)FeatureDim(319-319)Impl(63-70)Impl(63-63)Impl(73-76)
sherpa-onnx/csrc/offline-whisper-model-config.h (1)
sherpa-onnx/csrc/offline-whisper-model-config.cc (6)
ToString(80-91)ToString(80-80)ToString(93-115)ToString(93-93)ParseWhisperModelType(117-133)ParseWhisperModelType(117-117)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (2)
sherpa-onnx/csrc/offline-whisper-model-config.cc (6)
ParseWhisperModelType(117-133)ParseWhisperModelType(117-117)ToString(80-91)ToString(80-80)ToString(93-115)ToString(93-93)sherpa-onnx/csrc/math.cc (2)
Transpose(62-76)Transpose(62-62)
sherpa-onnx/csrc/math.cc (2)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (4)
features(78-108)features(78-78)features(113-138)features(113-113)sherpa-onnx/csrc/offline-stream.cc (1)
features(198-198)
sherpa-onnx/csrc/ascend/utils.h (1)
sherpa-onnx/csrc/ascend/utils.cc (4)
AclDataBuffer(344-351)AclDataBuffer(353-353)Release(355-361)Release(355-355)
sherpa-onnx/csrc/offline-whisper-model.cc (1)
sherpa-onnx/csrc/math.cc (2)
NormalizeWhisperFeatures(101-143)NormalizeWhisperFeatures(101-102)
sherpa-onnx/csrc/ascend/offline-zipformer-ctc-model-ascend.cc (1)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (5)
features(78-108)features(78-78)features(113-138)features(113-113)feat_dim_(110-110)
🔇 Additional comments (12)
sherpa-onnx/csrc/ascend/utils.h (2)
174-177: LGTM! Move constructor correctly transfers ownership.The move constructor properly transfers ownership of the underlying
aclDataBuffer*and nullifies the source pointer.
190-190: Good addition of the Release() method.The explicit
Release()method provides clear separation between resource cleanup and object destruction, which is essential for proper move semantics.sherpa-onnx/csrc/ascend/utils.cc (1)
353-361: LGTM! Destructor and Release() implementation are correct.The destructor properly delegates to
Release(), and theRelease()implementation:
- Guards against null pointers
- Performs proper error checking
- Ensures idempotency by setting
p_tonullptrThis pattern enables safe move semantics and allows
Release()to be called multiple times without issues.sherpa-onnx/csrc/math.h (1)
164-165: LGTM! Clean utility function declaration.The new
NormalizeWhisperFeaturesfunction provides a centralized implementation for Whisper feature normalization, promoting code reuse across the codebase.sherpa-onnx/csrc/ascend/offline-zipformer-ctc-model-ascend.cc (1)
67-67: Good clarity improvement with explicit zero-initialization.Making the zero-initialization explicit in the
resizecall improves code readability, even thoughstd::vector<float>already defaults new elements to 0.0.sherpa-onnx/csrc/offline-whisper-model-config.cc (2)
93-115: LGTM! Comprehensive enum-to-string conversion.The
ToStringfunction covers all Whisper model types with a defensive fallback for unexpected values.
117-133: LGTM! Appropriate error handling for unknown model types.The function correctly exits on unknown model types, and the unreachable return statement (line 132) is a common pattern to satisfy compiler requirements after a no-return call.
sherpa-onnx/csrc/CMakeLists.txt (1)
232-232: LGTM! Proper addition of Ascend Whisper model source.The new source file is correctly added to the build configuration within the Ascend NPU conditional block, consistent with other Ascend backend sources.
sherpa-onnx/csrc/offline-whisper-model.cc (1)
27-27: Excellent refactoring to centralize feature normalization!Delegating to the
NormalizeWhisperFeaturesutility function eliminates code duplication and improves maintainability while preserving the public API.Also applies to: 452-455
sherpa-onnx/csrc/offline-whisper-model-config.h (1)
58-73: LGTM!The new
WhisperModelTypeenum and utility functions are cleanly added. The enum class provides type safety, and the comment clarifies the intended use across multiple backends (ascend/rknn/qnn/axera).sherpa-onnx/csrc/offline-recognizer-impl.cc (2)
159-165: LGTM!The Whisper model integration for Ascend NPU follows the established pattern used for SenseVoice, Paraformer, and Zipformer CTC models. The error message is appropriately updated to reflect the new capability.
496-503: LGTM!The Manager-based
Createoverload is consistently updated to match the non-Manager overload, maintaining symmetry across both API paths.
| int32_t num_frames = features.size() / feat_dim_; | ||
| if (num_frames > num_frames_) { | ||
| SHERPA_ONNX_LOGE( | ||
| "Number of input frames %d is too large. Truncate it to %d frames.", | ||
| num_frames, num_frames_); | ||
|
|
||
| SHERPA_ONNX_LOGE( | ||
| "Recognition result may be truncated/incomplete. Please select a " | ||
| "model accepting longer audios or use VAD to cut your audio into " | ||
| "small chunks."); | ||
| } | ||
|
|
||
| features.resize(num_frames_ * feat_dim_); |
There was a problem hiding this comment.
Frame truncation logic has issues.
-
The warning is logged when
num_frames > num_frames_but the code at line 97 resizes regardless of the condition, potentially padding smaller inputs with zeros unnecessarily. -
The resize should only happen for truncation, not padding:
🔧 Suggested fix
int32_t num_frames = features.size() / feat_dim_;
if (num_frames > num_frames_) {
SHERPA_ONNX_LOGE(
"Number of input frames %d is too large. Truncate it to %d frames.",
num_frames, num_frames_);
SHERPA_ONNX_LOGE(
"Recognition result may be truncated/incomplete. Please select a "
"model accepting longer audios or use VAD to cut your audio into "
"small chunks.");
+ features.resize(num_frames_ * feat_dim_);
+ num_frames = num_frames_;
}
- features.resize(num_frames_ * feat_dim_);
-
// (num_frames_, feat_dim_) -> (feat_dim_, num_frames_)
- features = Transpose(features.data(), num_frames_, feat_dim_);
+ features = Transpose(features.data(), num_frames, feat_dim_);Note: If padding to num_frames_ is intentional for the model, the transpose call should still use num_frames_ not the original num_frames.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc around lines 85 -
97, The code always calls features.resize(num_frames_ * feat_dim_) even when
input is shorter, causing unintended padding; change the logic to compute an
explicit frames_to_use = std::min(num_frames, num_frames_) and only call
features.resize(num_frames_ * feat_dim_) when num_frames > num_frames_ (i.e.,
truncation), leaving smaller inputs unpadded unless padding is intentional;
update downstream uses (e.g., the transpose call and any buffer lengths) to use
frames_to_use (or num_frames_ only when you actually truncated) so the transpose
uses the correct frame count and the warning remains for truncation cases.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sherpa-onnx/csrc/ascend/utils.cc (1)
254-313: Make output “size in MB” consistent with input (avoid integer truncation).Inputs now print MB as floating-point (Line 269), but outputs still use integer division (Line 294), which truncates and may confuse debugging.
Proposed patch
- os << " size out MB: " << size_out_bytes / 1024 / 1024 << "\n"; + os << " size out MB: " << size_out_bytes / 1024. / 1024 << "\n";
🤖 Fix all issues with AI agents
In @sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc:
- Around line 238-241: In DetectLanguage, the return value from aclrtMemcpy
(assigned to ret) is not being checked; update the code after the aclrtMemcpy
call that copies from logits_ptr_ to logits_cpu_.data() to validate ret using
SHERPA_ONNX_ASCEND_CHECK(ret) (consistent with other memcpy calls in this file),
ensuring the memcpy arguments remain the same and the check triggers the
existing error handling on failure.
In @sherpa-onnx/csrc/ascend/utils.h:
- Around line 180-197: The move-assignment currently tests self-move by
comparing the wrapped pointer (other == *this) which checks pointer equality,
not object identity; change the self-move guard in
AclDataBuffer::operator=(AclDataBuffer&&) to check object identity (if (&other
== this) return *this), keep Release() and pointer transfer logic, and mark both
the move constructor AclDataBuffer(AclDataBuffer&&) and move-assignment
AclDataBuffer& operator=(AclDataBuffer&&) as noexcept to reflect that they
cannot throw.
In @sherpa-onnx/csrc/math.cc:
- Around line 145-153: The function MaxElementIndex currently calls Eigen::Map
and vec.maxCoeff without handling n==0; add a precondition check or assert to
prevent undefined behavior: in MaxElementIndex(const float *v, int32_t n)
validate that n > 0 (e.g., assert(n > 0) or throw/return an error) before
mapping and calling vec.maxCoeff, and/or update the function comment to document
the requirement that n must be > 0; also consider adding a defensive check at
the call site (offline-whisper-model-ascend.cc:230) where logits_cpu_.size() is
passed to ensure the size is non-zero before calling MaxElementIndex.
In @sherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.h:
- Around line 75-89: In DecodeStream, remove the unused local memory_info and
add validation before computing num_frames: ensure feat_dim (from
s->FeatureDim()) is > 0 and that f.size() is divisible by feat_dim (e.g.,
f.size() % feat_dim == 0); if the checks fail, handle the error early (return or
set an error result on the OfflineStream) to avoid division by zero or silent
truncation, then proceed to call NormalizeWhisperFeatures(f.data(), num_frames,
feat_dim), model_->Run(std::move(f)), Convert(...), and s->SetResult(...) as
before.
- Around line 41-52: The Whisper implementation must override CreateStream(const
std::string &hotwords) to avoid invoking the base-class exit path; add a method
with signature std::unique_ptr<OfflineStream> CreateStream(const std::string
&hotwords) const override in OfflineRecognizerWhisperTplImpl that simply
constructs a WhisperTag with tag.dim = model_->FeatureDim() and returns
std::make_unique<OfflineStream>(tag), ignoring the hotwords string (no logging
or exit). This mirrors the existing CreateStream() implementation and prevents
process termination when users call the hotwords overload.
🧹 Nitpick comments (12)
sherpa-onnx/csrc/math.h (1)
164-167: Consider adding documentation for the new functions.The declarations are clear, but adding brief documentation comments would help users understand:
- What NormalizeWhisperFeatures does (Whisper-specific log-mel normalization)
- What MaxElementIndex returns (index of max element)
- Expected constraints (e.g., non-empty inputs)
sherpa-onnx/csrc/math.cc (1)
106-117: Clarify the intent of the disabled Eigen implementation.The
#if 0block preserves an alternative Eigen-based implementation. If this is intentional (e.g., for future optimization or reference), consider adding a comment explaining why both versions exist. If it's no longer needed, removing it would reduce maintenance burden.sherpa-onnx/csrc/ascend/utils.cc (1)
353-361: Destructor calls SHERPA_ONNX_ASCEND_CHECK via Release(): confirm it can’t throw.Centralizing cleanup in
Release()is good, but destructors must not throw; ifSHERPA_ONNX_ASCEND_CHECKcan throw, this becomes UB during stack unwinding. If it alwaysexit()s, it’s “safe” but still abrupt—worth confirming/documenting.sherpa-onnx/csrc/ascend/utils.h (1)
63-69:AclDevicePtr::Get<T>()is OK, but consider naming to reduce ambiguity.Having both
void *Get() constandtemplate <typename T> T *Get() constworks, butGetAs<T>()(or similar) tends to be clearer at call sites and avoids accidentalvoid*usage + cast patterns.sherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.h (1)
61-73:Init()log message looks like an error path.
SHERPA_ONNX_LOGE("use greedy_search");on the supported path is confusing/noisy; consider INFO/DEBUG or remove.sherpa-onnx/csrc/offline-whisper-model-config.cc (3)
95-115:IsMultilingual()looks fine; consider making “unsupported” unreachable at compile time if enum expands.Right now, a new
WhisperModelTypevalue added later will fall through to the runtime error. If you want compile-time pressure, consider adding adefault:with astatic_assertpattern (or ensure exhaustive handling is enforced by warnings in your toolchain).
141-157:ParseWhisperModelType()exits; consider returning an error instead for library usage.
SHERPA_ONNX_EXIT(-1)is consistent with some CLI-style code, but if this is used in library contexts, returningstd::optional<WhisperModelType>(or a status) may be friendlier.
159-253: Avoid 4 separate “sources of truth” for language mappings (drift risk).You now have:
kLangToTokenkTokenToLangkLanguageTokenIdskLanguageCodesAny mismatch will be painful to debug. Prefer generating the reverse map + the two vectors from a single static table (e.g.,
std::array<pair<string_view,int32_t>>), or at least add a one-time runtime sanity check (sizes match; every entry round-trips).sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (3)
297-307: Hardcoded device ID may limit multi-device scenarios.The device ID is hardcoded to
0. Consider making this configurable viaOfflineModelConfigif multi-NPU deployments are anticipated.
442-466: Pointer aliasing and offset calculation relies onsizeof(float) == sizeof(int32_t).The code uses a single
offsetcounter for bothfloat*andint32_t*pointer arithmetic. This works because both types are 4 bytes on most platforms, but it's fragile and unclear. Consider using separate offset trackers or byte-based addressing for clarity and safety.// Current: offset is used for both float* and int32_t* increments float *start = ptr_->Get<float>(); int32_t *start_int32 = ptr_->Get<int32_t>(); // same memory location int32_t offset = 0; features_ptr_ = start + offset; // float* + offset offset += feat_dim_ * num_frames_; // offset now in "float units" token_ptr_ = start_int32 + offset; // int32_t* + offset (reinterprets as int32 units)♻️ Suggested approach
Use byte-based offset calculation to make the intent explicit:
char *base = reinterpret_cast<char*>(ptr_->Get<void>()); size_t byte_offset = 0; features_ptr_ = reinterpret_cast<float*>(base + byte_offset); byte_offset += feat_dim_ * num_frames_ * sizeof(float); token_ptr_ = reinterpret_cast<int32_t*>(base + byte_offset); byte_offset += sizeof(int32_t); // ... etc
284-295: Consider adding bounds check for defensive coding.While the caller currently ensures
offset < n_text_ctx_ / 2, a bounds check here would prevent potential out-of-bounds writes if the calling logic changes in the future.♻️ Optional defensive check
void UpdateSelfKvCache() { int32_t offset = token_offset_mask_cpu_[1]; + if (offset >= n_text_ctx_) { + SHERPA_ONNX_LOGE("UpdateSelfKvCache: offset %d >= n_text_ctx_ %d", offset, n_text_ctx_); + return; + } for (int32_t i = 0; i < n_text_layer_ * 2; ++i) {sherpa-onnx/csrc/offline-whisper-model-config.h (1)
87-99: Token constant structs are declared but not used.These structs define token constants that match the hardcoded values in
InitSotSequence()(lines 369-399 in the .cc file). Consider using these structs inInitSotSequence()to centralize the token definitions and avoid duplication.♻️ Example usage in InitSotSequence
void InitSotSequence() { if (IsMultilingual(model_type_)) { WhisperModelMultilingualTokens tokens; sot_sequence_ = {tokens.sot, 50259, tokens.transcribe, tokens.no_timestamps}; eot_ = tokens.eot; translate_ = tokens.translate; } else { WhisperModelEnglishTokens tokens; sot_sequence_ = {tokens.sot, tokens.no_timestamps}; eot_ = tokens.eot; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.ccsherpa-onnx/csrc/ascend/offline-whisper-model-ascend.hsherpa-onnx/csrc/ascend/utils.ccsherpa-onnx/csrc/ascend/utils.hsherpa-onnx/csrc/math.ccsherpa-onnx/csrc/math.hsherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.hsherpa-onnx/csrc/offline-whisper-decoder.hsherpa-onnx/csrc/offline-whisper-model-config.ccsherpa-onnx/csrc/offline-whisper-model-config.h
💤 Files with no reviewable changes (1)
- sherpa-onnx/csrc/offline-whisper-decoder.h
🧰 Additional context used
🧬 Code graph analysis (6)
sherpa-onnx/csrc/ascend/utils.cc (1)
sherpa-onnx/csrc/ascend/utils.h (1)
AclDataBuffer(172-203)
sherpa-onnx/csrc/ascend/utils.h (3)
sherpa-onnx/csrc/ascend/utils.cc (6)
Get(128-128)Get(128-128)AclDataBuffer(344-351)AclDataBuffer(353-353)Release(355-361)Release(355-355)sherpa-onnx/csrc/axcl/utils.h (1)
Get(45-45)sherpa-onnx/csrc/axcl/utils.cc (2)
Release(24-37)Release(24-24)
sherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.h (4)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.h (1)
sherpa_onnx(13-36)sherpa-onnx/csrc/offline-whisper-model.cc (2)
tokens(106-128)tokens(108-110)sherpa-onnx/csrc/offline-recognizer-impl.h (1)
std(36-40)sherpa-onnx/csrc/math.cc (2)
NormalizeWhisperFeatures(101-143)NormalizeWhisperFeatures(101-102)
sherpa-onnx/csrc/offline-whisper-model-config.h (1)
sherpa-onnx/csrc/offline-whisper-model-config.cc (16)
ToString(82-93)ToString(82-82)ToString(117-139)ToString(117-117)IsMultilingual(95-115)IsMultilingual(95-95)ParseWhisperModelType(141-157)ParseWhisperModelType(141-141)GetWhisperLanguageTokenId(159-190)GetWhisperLanguageTokenId(159-159)GetWhisperLanguageCode(192-222)GetWhisperLanguageCode(192-192)GetAllWhisperLanguageTokenIds(224-238)GetAllWhisperLanguageTokenIds(224-224)GetAllWhisperLanguageCodes(240-253)GetAllWhisperLanguageCodes(240-240)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (2)
sherpa-onnx/csrc/offline-whisper-model-config.cc (14)
ParseWhisperModelType(141-157)ParseWhisperModelType(141-141)IsMultilingual(95-115)IsMultilingual(95-95)GetWhisperLanguageTokenId(159-190)GetWhisperLanguageTokenId(159-159)GetWhisperLanguageCode(192-222)GetWhisperLanguageCode(192-192)GetAllWhisperLanguageTokenIds(224-238)GetAllWhisperLanguageTokenIds(224-224)ToString(82-93)ToString(82-82)ToString(117-139)ToString(117-117)sherpa-onnx/csrc/math.cc (4)
Transpose(62-76)Transpose(62-62)MaxElementIndex(145-153)MaxElementIndex(145-145)
sherpa-onnx/csrc/math.h (1)
sherpa-onnx/csrc/math.cc (4)
NormalizeWhisperFeatures(101-143)NormalizeWhisperFeatures(101-102)MaxElementIndex(145-153)MaxElementIndex(145-145)
⏰ 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). (17)
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: swift (macos-15-intel)
- GitHub Check: rknn shared ON
- GitHub Check: rknn shared OFF
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: ubuntu-24.04 3.11
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: ubuntu-24.04 3.12
- GitHub Check: ubuntu-24.04 3.9
- GitHub Check: ubuntu-24.04 3.8
🔇 Additional comments (10)
sherpa-onnx/csrc/math.cc (1)
119-142: LGTM: The scalar implementation is correct.The normalization logic properly implements the Whisper feature preprocessing:
- Clamps values to prevent log of zero/negative
- Computes log10 transformation
- Applies max-relative threshold (-8 dB)
- Scales to [0, 1] range approximately
The initialization of
max_v = -1e20is appropriate as a sentinel value for max computation.sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.h (1)
15-34: The template constructor is properly defined and explicitly instantiated — no issue here.The template constructor is defined in the
.ccfile (lines 265–268) with explicit instantiations forAAssetManagerandNativeResourceManager(lines 282–289). This avoids the ODR risk you mentioned.
OfflineWhisperDecoderResultis available through the include chain: the header includesoffline-model-config.h, which includesoffline-whisper-model-config.hwhere the result type is defined.sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (5)
32-37: LGTM!The
UpdateCausalMaskhelper function correctly fills the mask array with 0s up tooffsetand 1s fromoffsettocapacity, which aligns with the masking semantics (masked=1, unmasked=0).
39-57: LGTM!The parsing logic correctly validates the input format (expects
{model_type}-mel) and delegates toParseWhisperModelTypefor the actual model type parsing. Error handling is consistent with the project's patterns.
170-191: LGTM!The decoding loop correctly implements autoregressive token generation: sets the current token, runs the decoder to predict the next token, updates the causal mask, and increments the offset. The termination conditions (offset limit and EOT token) are properly handled.
333-366: LGTM!The decoder buffer initialization correctly sets up input and output buffers matching the expected model input/output structure documented at line 539.
660-690: LGTM!The public API implementation correctly forwards to the
Implclass following the PIMPL pattern, with appropriate template instantiations for Android and OHOS platforms.sherpa-onnx/csrc/offline-whisper-model-config.h (3)
8-8: LGTM!Added
<vector>include is required for the newstd::vectorreturn types in the declared functions.
59-63: LGTM!The
OfflineWhisperDecoderResultstruct provides a clean data structure for decoder output containing token IDs and detected language.
65-85: LGTM!The
WhisperModelTypeenum covers all official Whisper model variants, and the utility function declarations properly expose model type conversion, language token lookup, and multilingual detection capabilities.
| AclDataBuffer(AclDataBuffer &&other) { | ||
| p_ = other.p_; | ||
| other.p_ = nullptr; | ||
| } | ||
| AclDataBuffer &operator=(AclDataBuffer &&other) { | ||
| if (other == *this) { | ||
| return *this; | ||
| } | ||
|
|
||
| Release(); | ||
|
|
||
| p_ = other.p_; | ||
| other.p_ = nullptr; | ||
| return *this; | ||
| } | ||
|
|
||
| void Release(); | ||
|
|
There was a problem hiding this comment.
Fix self-move guard in AclDataBuffer::operator=, and consider noexcept for move ops.
if (other == *this) compares underlying aclDataBuffer* via implicit conversion, not object identity; it can incorrectly treat two distinct wrappers as “self” if they (erroneously) hold the same pointer.
Proposed patch
- AclDataBuffer(AclDataBuffer &&other) {
+ AclDataBuffer(AclDataBuffer &&other) noexcept {
p_ = other.p_;
other.p_ = nullptr;
}
- AclDataBuffer &operator=(AclDataBuffer &&other) {
- if (other == *this) {
+ AclDataBuffer &operator=(AclDataBuffer &&other) noexcept {
+ if (this == &other) {
return *this;
}🤖 Prompt for AI Agents
In @sherpa-onnx/csrc/ascend/utils.h around lines 180 - 197, The move-assignment
currently tests self-move by comparing the wrapped pointer (other == *this)
which checks pointer equality, not object identity; change the self-move guard
in AclDataBuffer::operator=(AclDataBuffer&&) to check object identity (if
(&other == this) return *this), keep Release() and pointer transfer logic, and
mark both the move constructor AclDataBuffer(AclDataBuffer&&) and
move-assignment AclDataBuffer& operator=(AclDataBuffer&&) as noexcept to reflect
that they cannot throw.
| int32_t MaxElementIndex(const float *v, int32_t n) { | ||
| // Map raw pointer to an Eigen vector (no copy) | ||
| Eigen::Map<const Eigen::VectorXf> vec(v, n); | ||
|
|
||
| Eigen::Index maxIndex; | ||
| vec.maxCoeff(&maxIndex); | ||
|
|
||
| return static_cast<int32_t>(maxIndex); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -B3 -A1 'MaxElementIndex\s*\(' --type=cppRepository: k2-fsa/sherpa-onnx
Length of output: 1074
🏁 Script executed:
# Get more context around the MaxElementIndex call
sed -n '210,240p' sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc
# Also search for logits_cpu_ initialization
rg -n 'logits_cpu_' sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc -A 2 -B 2
# Search more broadly for any other calls we might have missed
rg -n 'MaxElementIndex' --type=cpp --type=hRepository: k2-fsa/sherpa-onnx
Length of output: 2422
Add a defensive check or document the precondition for n > 0.
Calling maxCoeff on an empty Eigen vector (when n = 0) results in undefined behavior. The sole call site at offline-whisper-model-ascend.cc:230 passes logits_cpu_.size() directly without validating that it's non-empty. While the Whisper vocabulary size should always be positive in practice, add either a defensive check in MaxElementIndex (e.g., assert(n > 0)) or explicitly document that n must be greater than zero.
🤖 Prompt for AI Agents
In @sherpa-onnx/csrc/math.cc around lines 145 - 153, The function
MaxElementIndex currently calls Eigen::Map and vec.maxCoeff without handling
n==0; add a precondition check or assert to prevent undefined behavior: in
MaxElementIndex(const float *v, int32_t n) validate that n > 0 (e.g., assert(n >
0) or throw/return an error) before mapping and calling vec.maxCoeff, and/or
update the function comment to document the requirement that n must be > 0; also
consider adding a defensive check at the call site
(offline-whisper-model-ascend.cc:230) where logits_cpu_.size() is passed to
ensure the size is non-zero before calling MaxElementIndex.
| std::unique_ptr<OfflineStream> CreateStream() const override { | ||
| WhisperTag tag; | ||
| tag.dim = model_->FeatureDim(); | ||
| return std::make_unique<OfflineStream>(tag); | ||
| } | ||
|
|
||
| void DecodeStreams(OfflineStream **ss, int32_t n) const override { | ||
| // batch decoding is not implemented yet | ||
| for (int32_t i = 0; i != n; ++i) { | ||
| DecodeStream(ss[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the OfflineRecognizerImpl class definition
find . -type f -name "*.h" | xargs grep -l "class OfflineRecognizerImpl"Repository: k2-fsa/sherpa-onnx
Length of output: 146
🏁 Script executed:
# Also search for CreateStream method overloads
rg "CreateStream\(" --type h -A 2 | head -100Repository: k2-fsa/sherpa-onnx
Length of output: 7040
🏁 Script executed:
# Look at the Whisper implementation file more carefully
cat -n sherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.h | head -60Repository: k2-fsa/sherpa-onnx
Length of output: 2389
🏁 Script executed:
# View the OfflineRecognizerImpl class definition with the CreateStream methods
cat -n sherpa-onnx/csrc/offline-recognizer-impl.h | grep -A 5 "CreateStream"Repository: k2-fsa/sherpa-onnx
Length of output: 544
🏁 Script executed:
# Get more context around the CreateStream(hotwords) method
sed -n '1,100p' sherpa-onnx/csrc/offline-recognizer-impl.h | tail -50Repository: k2-fsa/sherpa-onnx
Length of output: 1609
🏁 Script executed:
# Search for calls to CreateStream with a string parameter
rg "CreateStream\s*\(\s*['\"]" --type h --type cppRepository: k2-fsa/sherpa-onnx
Length of output: 234
🏁 Script executed:
# Check the public OfflineRecognizer API wrapper to see if it exposes CreateStream(hotwords)
cat -n sherpa-onnx/csrc/offline-recognizer.h | head -80Repository: k2-fsa/sherpa-onnx
Length of output: 3117
🏁 Script executed:
# Get more of the OfflineRecognizer public API
cat -n sherpa-onnx/csrc/offline-recognizer.h | sed -n '79,150p'Repository: k2-fsa/sherpa-onnx
Length of output: 2195
Override CreateStream(const std::string &hotwords) in Whisper implementation to prevent process termination.
The public API OfflineRecognizer::CreateStream(const std::string &hotwords) is exposed to users. Without an override in OfflineRecognizerWhisperTplImpl, calls to this method will invoke the base class implementation, which logs "Only transducer models support contextual biasing." and calls exit(-1), crashing the entire process. Even empty hotwords strings will trigger this.
🤖 Prompt for AI Agents
In @sherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.h around lines 41 - 52,
The Whisper implementation must override CreateStream(const std::string
&hotwords) to avoid invoking the base-class exit path; add a method with
signature std::unique_ptr<OfflineStream> CreateStream(const std::string
&hotwords) const override in OfflineRecognizerWhisperTplImpl that simply
constructs a WhisperTag with tag.dim = model_->FeatureDim() and returns
std::make_unique<OfflineStream>(tag), ignoring the hotwords string (no logging
or exit). This mirrors the existing CreateStream() implementation and prevents
process termination when users call the hotwords overload.
| void DecodeStream(OfflineStream *s) const { | ||
| auto memory_info = | ||
| Ort::MemoryInfo::CreateCpu(OrtDeviceAllocator, OrtMemTypeDefault); | ||
|
|
||
| int32_t feat_dim = s->FeatureDim(); | ||
| std::vector<float> f = s->GetFrames(); | ||
| int32_t num_frames = f.size() / feat_dim; | ||
|
|
||
| NormalizeWhisperFeatures(f.data(), num_frames, feat_dim); | ||
|
|
||
| auto r = model_->Run(std::move(f)); | ||
| auto res = Convert(r, symbol_table_); | ||
|
|
||
| s->SetResult(res); | ||
| } |
There was a problem hiding this comment.
Remove dead code and guard feature dimensions before division.
memory_infois unused.num_frames = f.size() / feat_dimshould guardfeat_dim > 0(and ideallyf.size() % feat_dim == 0) to avoid UB / silent truncation.
Proposed patch
void DecodeStream(OfflineStream *s) const {
- auto memory_info =
- Ort::MemoryInfo::CreateCpu(OrtDeviceAllocator, OrtMemTypeDefault);
-
int32_t feat_dim = s->FeatureDim();
+ if (feat_dim <= 0) {
+ SHERPA_ONNX_LOGE("Invalid feature dim: %d", feat_dim);
+ SHERPA_ONNX_EXIT(-1);
+ }
std::vector<float> f = s->GetFrames();
+ if (f.size() % feat_dim != 0) {
+ SHERPA_ONNX_LOGE("Invalid feature vector size: %zu for dim %d", f.size(),
+ feat_dim);
+ SHERPA_ONNX_EXIT(-1);
+ }
int32_t num_frames = f.size() / feat_dim;🤖 Prompt for AI Agents
In @sherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.h around lines 75 - 89,
In DecodeStream, remove the unused local memory_info and add validation before
computing num_frames: ensure feat_dim (from s->FeatureDim()) is > 0 and that
f.size() is divisible by feat_dim (e.g., f.size() % feat_dim == 0); if the
checks fail, handle the error early (return or set an error result on the
OfflineStream) to avoid division by zero or silent truncation, then proceed to
call NormalizeWhisperFeatures(f.data(), num_frames, feat_dim),
model_->Run(std::move(f)), Convert(...), and s->SetResult(...) as before.
csukuangfj
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc:
- Around line 241-243: The aclrtMemcpy call that copies device logits to
logits_cpu_ assigns the result to ret but omits the standard error check; add
the same error handling used elsewhere by passing ret into
SHERPA_ONNX_ASCEND_CHECK(...) immediately after the aclrtMemcpy that uses
logits_cpu_.data(), logits_cpu_.size() * sizeof(float), logits_ptr_, and
ACL_MEMCPY_DEVICE_TO_HOST so failures are caught consistently with other memcpy
calls.
🧹 Nitpick comments (4)
sherpa-onnx/csrc/ascend/utils.h (1)
180-194: Move semantics are correct; consider addingnoexceptfor STL optimization.The move constructor and move assignment properly transfer ownership and null the source. However, adding
noexceptwould enable move optimizations in STL containers (e.g.,std::vectorreallocation).Optional: Add noexcept specifiers
- AclDataBuffer(AclDataBuffer &&other) { + AclDataBuffer(AclDataBuffer &&other) noexcept { p_ = other.p_; other.p_ = nullptr; } - AclDataBuffer &operator=(AclDataBuffer &&other) { + AclDataBuffer &operator=(AclDataBuffer &&other) noexcept { if (this == &other) { return *this; } Release(); p_ = other.p_; other.p_ = nullptr; return *this; }sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (3)
174-180: Signed/unsigned comparison warning.Comparing
int32_t iwithsot_sequence.size()(which returnssize_t) may trigger compiler warnings. Consider usingsize_tfor the loop variable or casting.Optional: Use size_t for loop index
- for (int32_t i = 0; i < sot_sequence.size(); ++i) { + for (size_t i = 0; i < sot_sequence.size(); ++i) {
300-310: Device ID is hardcoded to 0.Consider making the device ID configurable via
OfflineModelConfigto support multi-device deployments in the future.
445-469: Mixed pointer arithmetic relies on sizeof(float) == sizeof(int32_t).The code reinterprets the same memory block as both
float*andint32_t*, using a singleoffsetvariable for pointer arithmetic on both types. This works because both types are 4 bytes, but it's fragile and potentially confusing.Consider using byte offsets with explicit casting, or adding a static assertion to document and enforce the assumption:
Optional: Add static assertion to document assumption
static_assert(sizeof(float) == sizeof(int32_t), "Preallocate() offset arithmetic assumes float and int32_t have equal size");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.ccsherpa-onnx/csrc/ascend/utils.hsherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.h
🚧 Files skipped from review as they are similar to previous changes (1)
- sherpa-onnx/csrc/offline-recognizer-whisper-tpl-impl.h
🧰 Additional context used
🧬 Code graph analysis (2)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (2)
sherpa-onnx/csrc/offline-whisper-model-config.cc (14)
ParseWhisperModelType(141-157)ParseWhisperModelType(141-141)IsMultilingual(95-115)IsMultilingual(95-95)GetWhisperLanguageTokenId(159-190)GetWhisperLanguageTokenId(159-159)GetWhisperLanguageCode(192-222)GetWhisperLanguageCode(192-192)GetAllWhisperLanguageTokenIds(224-238)GetAllWhisperLanguageTokenIds(224-224)ToString(82-93)ToString(82-82)ToString(117-139)ToString(117-117)sherpa-onnx/csrc/math.cc (4)
Transpose(62-76)Transpose(62-62)MaxElementIndex(145-153)MaxElementIndex(145-145)
sherpa-onnx/csrc/ascend/utils.h (3)
sherpa-onnx/csrc/ascend/utils.cc (6)
Get(128-128)Get(128-128)AclDataBuffer(344-351)AclDataBuffer(353-353)Release(355-361)Release(355-355)sherpa-onnx/csrc/axcl/utils.h (1)
Get(45-45)sherpa-onnx/csrc/axcl/utils.cc (2)
Release(24-37)Release(24-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). (17)
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: ubuntu-24.04 3.9
- GitHub Check: ubuntu-24.04 3.8
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: ubuntu-24.04 3.11
- GitHub Check: ubuntu-24.04 3.12
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: rknn shared ON
- GitHub Check: rknn shared OFF
- GitHub Check: swift (macos-latest)
- GitHub Check: swift (macos-15-intel)
🔇 Additional comments (8)
sherpa-onnx/csrc/ascend/utils.h (2)
65-68: LGTM! Type-safe accessor provides convenient typed access.The templated
Get<T>()complements the existingvoid* Get()and will be useful for avoiding repeated casts at call sites.
196-196: LGTM!The
Release()declaration aligns with the implementation inutils.ccwhich properly destroys the buffer and nullifies the pointer.sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (6)
32-37: LGTM!The mask update logic correctly sets unmasked (0) for positions before offset and masked (1) for positions from offset onwards.
39-57: LGTM!The parsing logic correctly validates the expected format and delegates to
ParseWhisperModelTypefor the model type extraction.
501-536: LGTM!The encoder post-initialization properly extracts dimensions from model shapes and validates the batch size constraint.
538-578: LGTM!The decoder post-initialization correctly validates input count and dimension consistency with the encoder.
614-661: LGTM!Member variables are properly initialized with sensible defaults. The raw pointer members (lines 639-647) are correctly documented as pointing into preallocated device memory, clarifying that they don't need separate cleanup.
663-691: LGTM!Clean PIMPL pattern implementation with proper forwarding. The explicit template instantiations for Android and OHOS platforms are correctly guarded.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (3)
87-197: LGTM: Main inference logic is well-structured.The
Runmethod correctly handles Whisper's encoder-decoder inference flow, including multilingual models, language detection, and SOT sequence processing. Thread safety is ensured via mutex.Optional: Document frame rate assumption.
Line 112 assumes a frame rate of 100 Hz (10ms frames) when estimating token count. Consider adding a comment to clarify this assumption:
// Assume at most 6 tokens per second; frame rate is 100 Hz (10ms frames) int32_t num_possible_tokens = num_frames / 100.0 * 6;
418-501: Memory layout assumes sizeof(float) == sizeof(int32_t).The
Preallocatemethod uses a singleoffsetcounter for bothfloat*andint32_t*pointer arithmetic (lines 447-467). This implicitly assumessizeof(float) == sizeof(int32_t), which is true on all modern platforms but not guaranteed by the C++ standard.Consider adding a static assertion to make this assumption explicit:
static_assert(sizeof(float) == sizeof(int32_t), "Preallocate assumes float and int32_t have the same size");Alternatively, use separate offset counters or byte-based offset calculations for better clarity.
202-287: LGTM: Encoder/decoder execution is correctly implemented.The execution paths properly handle ACL model invocation, memory transfers, and KV cache updates for Whisper's autoregressive decoding.
Optional: Add runtime validation of model outputs.
Consider adding runtime checks to verify that encoder/decoder outputs match expected shapes, especially for
cross_kv_ptr_,delta_kv_ptr_, andlogits_ptr_. This would catch model mismatches early:// In RunEncoder, after aclmdlExecute: if (output_dataset.GetSize() != cross_kv_ptr_.size()) { SHERPA_ONNX_LOGE("Encoder output count mismatch"); SHERPA_ONNX_EXIT(-1); }While the shapes are validated during initialization, runtime checks add an extra safety layer.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.ccsherpa-onnx/csrc/math.cc
🧰 Additional context used
🧬 Code graph analysis (2)
sherpa-onnx/csrc/math.cc (2)
sherpa-onnx/csrc/offline-whisper-model.cc (2)
features(98-104)features(98-98)sherpa-onnx/csrc/offline-dolphin-model.cc (4)
features(52-61)features(52-53)features(67-78)features(67-68)
sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (2)
sherpa-onnx/csrc/offline-whisper-model-config.cc (14)
ParseWhisperModelType(141-157)ParseWhisperModelType(141-141)IsMultilingual(95-115)IsMultilingual(95-95)GetWhisperLanguageTokenId(159-190)GetWhisperLanguageTokenId(159-159)GetWhisperLanguageCode(192-222)GetWhisperLanguageCode(192-192)GetAllWhisperLanguageTokenIds(224-238)GetAllWhisperLanguageTokenIds(224-224)ToString(82-93)ToString(82-82)ToString(117-139)ToString(117-117)sherpa-onnx/csrc/math.cc (4)
Transpose(62-76)Transpose(62-62)MaxElementIndex(120-128)MaxElementIndex(120-120)
🔇 Additional comments (5)
sherpa-onnx/csrc/math.cc (2)
101-118: LGTM! Correct Whisper feature normalization.The implementation correctly follows the Whisper log-mel spectrogram normalization as documented in the PyTorch reference comments. The Eigen-based approach is efficient and the in-place modification is appropriate for this use case.
120-128: LGTM! Simple and correct implementation.The function correctly wraps Eigen's
maxCoeffto find the maximum element index. The cast fromEigen::Indextoint32_tis safe in practice since the input parameternis alreadyint32_t, ensuring the returned index will fit.sherpa-onnx/csrc/ascend/offline-whisper-model-ascend.cc (3)
34-57: LGTM: Helper functions are well-implemented.Both
UpdateCausalMaskandParseWhisperModelFromStringare straightforward, with appropriate error handling and validation.
302-614: LGTM: Initialization logic is thorough and well-validated.The initialization sequence properly:
- Sets up ACL device and context
- Loads encoder/decoder models from files or memory buffers
- Validates model shapes and dimensions
- Allocates and configures buffers
- Sets up SOT sequences based on model type
Debug logging throughout aids troubleshooting.
665-694: LGTM: Public interface cleanly implements the PIMPL pattern.The forwarding to
Implis straightforward, and the templated constructor with explicit instantiations for Android and OHOS platforms enables platform-specific asset loading.
There was a problem hiding this comment.
Pull request overview
This pull request adds C++ runtime support for Whisper speech recognition models on Ascend NPU hardware, enabling efficient offline transcription with language detection capabilities.
Key Changes:
- Templated Whisper recognizer implementation supporting different backend hardware
- Centralized Whisper feature normalization utilities
- Multilingual support with language token mappings and automatic language detection
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
offline-whisper-model.cc |
Refactors feature normalization to use centralized utility function |
offline-whisper-model-config.h |
Adds shared types (OfflineWhisperDecoderResult, WhisperModelType enum), language utilities, and token definitions |
offline-whisper-model-config.cc |
Implements model type parsing, language token mappings, and multilingual support utilities |
offline-whisper-decoder.h |
Removes OfflineWhisperDecoderResult struct (moved to config header) |
math.h / math.cc |
Adds NormalizeWhisperFeatures and MaxElementIndex utility functions |
offline-recognizer-whisper-tpl-impl.h |
New templated recognizer implementation for hardware-agnostic Whisper support |
offline-recognizer-impl.cc |
Integrates Ascend Whisper support into recognizer factory |
ascend/offline-whisper-model-ascend.h / .cc |
Complete Ascend NPU implementation with encoder/decoder execution and language detection |
ascend/utils.h / .cc |
Adds move semantics to AclDataBuffer and fixes floating-point division for size display |
ascend/offline-zipformer-ctc-model-ascend.cc |
Fixes uninitialized buffer by explicitly initializing resized vector elements to 0 |
CMakeLists.txt |
Adds offline-whisper-model-ascend.cc to build when Ascend NPU is enabled |
Comments suppressed due to low confidence (1)
sherpa-onnx/csrc/math.cc:143
- Dead code wrapped in preprocessor directive. The Eigen-based implementation is disabled with
#if 0and a manual loop implementation is used instead. If the Eigen version is not needed for future reference, this dead code should be removed. If it's being kept for comparison or debugging purposes, add a comment explaining why.
using Eigen::ArrayXXf;
using Eigen::Map;
Map<ArrayXXf, Eigen::RowMajor> feats(features, num_frames, feat_dim);
feats = feats.max(1e-10f).log10();
float max_v = feats.maxCoeff() - 8.0f;
feats = feats.max(max_v);
feats = (feats + 4.0f) / 4.0f;
}
int32_t MaxElementIndex(const float *v, int32_t n) {
// Map raw pointer to an Eigen vector (no copy)
Eigen::Map<const Eigen::VectorXf> vec(v, n);
Eigen::Index maxIndex;
vec.maxCoeff(&maxIndex);
return static_cast<int32_t>(maxIndex);
}
} // namespace sherpa_onnx
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return r; | ||
| } | ||
|
|
||
| private: |
There was a problem hiding this comment.
Duplicate private access specifier. There is already a private section starting at line 63, so this second private declaration at line 116 is redundant and should be removed.
| private: |
| symbol_table_.ApplyBase64Decode(); | ||
|
|
||
| if (config_.decoding_method == "greedy_search") { | ||
| SHERPA_ONNX_LOGE("use greedy_search"); |
There was a problem hiding this comment.
Using LOGE (error log level) for informational message. When greedy_search is correctly configured, the message "use greedy_search" is logged as an error. This should use a debug or info log level instead, or be removed entirely if it's not needed.
| SHERPA_ONNX_LOGE("use greedy_search"); | |
| SHERPA_ONNX_LOGI("use greedy_search"); |
| } | ||
|
|
||
| // assume at most 6 tokens per second | ||
| int32_t num_possible_tokens = num_frames / 100.0 * 6; |
There was a problem hiding this comment.
Mixed integer and floating-point division may cause confusion. The expression num_frames / 100.0 * 6 divides an int32_t by a double, then multiplies by an integer, before assigning to an int32_t. This should be clarified, either by using num_frames * 6 / 100 or by making the intent explicit with a cast and comment.
| int32_t num_possible_tokens = num_frames / 100.0 * 6; | |
| int32_t num_possible_tokens = num_frames * 6 / 100; |
|
|
||
| RunEncoder(std::move(features)); | ||
|
|
||
| // Note(fangjun): No need to intialize the self kv cache to 0 |
There was a problem hiding this comment.
Spelling error in comment. "intialize" should be "initialize".
| // Note(fangjun): No need to intialize the self kv cache to 0 | |
| // Note(fangjun): No need to initialize the self kv cache to 0 |
| struct WhisperModelMultilingualTokens { | ||
| int32_t sot = 50258; | ||
| int32_t eot = 50257; | ||
| int32_t transcribe = 50359; | ||
| int32_t translate = 50358; | ||
| int32_t no_timestamps = 50363; | ||
| }; | ||
|
|
||
| struct WhisperModelEnglishTokens { | ||
| int32_t sot = 50257; | ||
| int32_t eot = 50256; | ||
| int32_t no_timestamps = 50362; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Unused struct definitions. The WhisperModelMultilingualTokens and WhisperModelEnglishTokens structs are defined but not used anywhere in this PR. The comment indicates they are for ascend/rknn/qnn/axera, but the Ascend implementation in this PR doesn't use them. Consider removing these if they're not needed yet, or document their intended future use more clearly.
| struct WhisperModelMultilingualTokens { | |
| int32_t sot = 50258; | |
| int32_t eot = 50257; | |
| int32_t transcribe = 50359; | |
| int32_t translate = 50358; | |
| int32_t no_timestamps = 50363; | |
| }; | |
| struct WhisperModelEnglishTokens { | |
| int32_t sot = 50257; | |
| int32_t eot = 50256; | |
| int32_t no_timestamps = 50362; | |
| }; |
Screenshots
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.