Begin to add qnn C API - #2766
Conversation
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 initiates the integration of Qualcomm Neural Processing SDK (QNN) into the sherpa-onnx project. The changes lay the groundwork for enabling hardware-accelerated inference on Android devices utilizing QNN. It introduces core components for managing the QNN backend and loading/executing QNN models, along with essential utility functions for QNN operations. 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
|
There was a problem hiding this comment.
Code Review
This pull request begins to add support for the QNN C API, which is a significant feature. The overall structure is in place, but the current implementation has several critical issues that must be addressed. These include compilation errors (e.g., missing semicolons), logic errors (e.g., incorrect CMake conditions), and serious bugs related to memory management and data structure access (e.g., incorrect lambda captures, file handling without error checking, and widespread confusion between v1 and v2 of QNN tensor structs). These issues will prevent the code from compiling or cause crashes and memory leaks at runtime. Please address the feedback to ensure the new functionality is robust and correct.
| return false; | ||
| } | ||
|
|
||
| auto free_graphs_info = [graphs_info, graphs_count] { |
There was a problem hiding this comment.
The lambda free_graphs_info captures graphs_info and graphs_count by value. However, graphs_info is modified by CopyMetadataToGraphsInfo (it's passed as a reference to a pointer). Capturing by value means the lambda will hold the initial value of graphs_info (which is nullptr), and any memory allocated and assigned to it will be leaked on error paths. You should capture graphs_info and graphs_count by reference to ensure the allocated memory is correctly freed.
auto free_graphs_info = [&graphs_info, &graphs_count] {| ) | ||
| endif() | ||
|
|
||
| if(SHERPA_ONNX_ENABLE_RKNN) |
There was a problem hiding this comment.
| } | ||
|
|
||
| void PrintTensor(Qnn_TensorV2_t t) { | ||
| std::ostringstream os os << " id: " << t.id << "\n"; |
| SHERPA_ONNX_LOGE("Unsupported mem type: %s", | ||
| TensorMemTypeToString(dst.v2.memType).c_str()); | ||
| } else { | ||
| dst.v1.clientBuf.data = nullptr; |
| static void CopyTensorInfoV1(const Qnn_Tensor_t &src, Qnn_Tensor_t &dst) { | ||
| dst.version = src.version; | ||
| dst.v1.id = src.v1.id; | ||
| if (src.v1.name) { | ||
| dst.v1.name = strdup(src.v1.name); | ||
| } else { | ||
| dst.v1.name = strdup(""); | ||
| } | ||
|
|
||
| dst.v1.type = src.v1.type; | ||
| dst.v1.dataFormat = src.v1.dataFormat; | ||
| dst.v1.dataType = src.v1.dataType; | ||
|
|
||
| CopyQuantizeParams(src.v1.quantizeParams, dst.v1.quantizeParams); | ||
|
|
||
| dst.v1.rank = src.v1.rank; | ||
|
|
||
| CopyDimensions(src.v1.dimensions, src.v1.rank, &dst.v1.dimensions); | ||
|
|
||
| dst.v1.memType = src.v1.memType; | ||
| if (dst.v1.memType != QNN_TENSORMEMTYPE_RAW) { | ||
| SHERPA_ONNX_LOGE("Unsupported mem type: %s", | ||
| TensorMemTypeToString(dst.v1.memType).c_str()); | ||
| } else { | ||
| dst.v1.clientBuf.data = nullptr; | ||
| dst.v1.clientBuf.dataSize = | ||
| GetSizeInBytes(dst.v1.dimensions, dst.v1.rank, dst.v1.dataType); | ||
| } | ||
|
|
||
| CopyDimensions(src.v2.isDynamicDimensions, src.v2.rank, | ||
| &dst.v2.isDynamicDimensions); | ||
|
|
||
| dst.v2.sparseParams.type = src.v2.sparseParams.type; | ||
| dst.v2.sparseParams.hybridCoo.numSpecifiedElements = | ||
| src.v2.sparseParams.hybridCoo.numSpecifiedElements; | ||
| dst.v2.sparseParams.hybridCoo.numSparseDimensions = | ||
| src.v2.sparseParams.hybridCoo.numSparseDimensions; | ||
| dst.v2.isProduced = src.v2.isProduced; | ||
| } |
There was a problem hiding this comment.
CopyTensorInfoV1 is supposed to handle QNN_TENSOR_VERSION_1 tensors, but it incorrectly accesses fields from the v2 union member (e.g., src.v2.isDynamicDimensions, src.v2.sparseParams). This will lead to memory corruption and undefined behavior since Qnn_TensorV1_t does not have these fields. These lines should be removed from this function.
static void CopyTensorInfoV1(const Qnn_Tensor_t &src, Qnn_Tensor_t &dst) {
dst.version = src.version;
dst.v1.id = src.v1.id;
if (src.v1.name) {
dst.v1.name = strdup(src.v1.name);
} else {
dst.v1.name = strdup("");
}
dst.v1.type = src.v1.type;
dst.v1.dataFormat = src.v1.dataFormat;
dst.v1.dataType = src.v1.dataType;
CopyQuantizeParams(src.v1.quantizeParams, dst.v1.quantizeParams);
dst.v1.rank = src.v1.rank;
CopyDimensions(src.v1.dimensions, src.v1.rank, &dst.v1.dimensions);
dst.v1.memType = src.v1.memType;
if (dst.v1.memType != QNN_TENSORMEMTYPE_RAW) {
SHERPA_ONNX_LOGE("Unsupported mem type: %s",
TensorMemTypeToString(dst.v1.memType).c_str());
} else {
dst.v1.clientBuf.data = nullptr;
dst.v1.clientBuf.dataSize =
GetSizeInBytes(dst.v1.dimensions, dst.v1.rank, dst.v1.dataType);
}
}| if (!ok) { | ||
| SHERPA_ONNX_LOGE("Failed to init qnn interface from '%s'", | ||
| backend_lib.c_str()); | ||
| return; |
There was a problem hiding this comment.
The constructor for QnnBackend::Impl returns if InitQnnInterface fails. This leaves the object in a partially initialized and unusable state. Instead of returning, you should call SHERPA_ONNX_EXIT(-1) to be consistent with error handling in other parts of the codebase, or throw an exception.
| if (!ok) { | |
| SHERPA_ONNX_LOGE("Failed to init qnn interface from '%s'", | |
| backend_lib.c_str()); | |
| return; | |
| if (!ok) { | |
| SHERPA_ONNX_LOGE("Failed to init qnn interface from '%s'", | |
| backend_lib.c_str()); | |
| SHERPA_ONNX_EXIT(-1); | |
| } |
| SHERPA_ONNX_LOGE("tensor '%s' expects %d bytes, but you provide %d bytes", | ||
| name.c_str(), | ||
| static_cast<int32_t>(t->v1.clientBuf.dataSize), | ||
| static_cast<int32_t>(n * sizeof(uint32_t))); |
| if [ -z $SHERPA_ONNX_ANDRIOD_PLATFORM ]; then | ||
| SHERPA_ONNX_ANDRIOD_PLATFORM=android-21 | ||
| fi |
There was a problem hiding this comment.
There's a typo in the environment variable name SHERPA_ONNX_ANDRIOD_PLATFORM. It should be SHERPA_ONNX_ANDROID_PLATFORM. This typo is also present where the variable is used on line 166.
| if [ -z $SHERPA_ONNX_ANDRIOD_PLATFORM ]; then | |
| SHERPA_ONNX_ANDRIOD_PLATFORM=android-21 | |
| fi | |
| if [ -z $SHERPA_ONNX_ANDROID_PLATFORM ]; then | |
| SHERPA_ONNX_ANDROID_PLATFORM=android-21 | |
| fi |
| // | ||
| // Copyright (c) 2025 Xiaomi Corporation | ||
|
|
||
| #include " sherpa-onnx/csrc/qnn/qnn-model.h" |
| // Copyright (c) 2025 Xiaomi Corporation | ||
| #ifndef SHERPA_ONNX_CSRC_QNN_QNN_BACKEND_H_ | ||
| #define SHERPA_ONNX_CSRC_QNN_QNN_BACKEND_H_ | ||
| #pragma once |
|
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 optional Qualcomm QNN backend: new CMake/build flags and Android platform variable, QNN SDK include handling, new QnnBackend and QnnModel implementations, QNN utilities and macros, and wiring to dynamically load and run QNN graphs with tensor management. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QnnBackend
participant BackendImpl as Backend.Impl
participant QnnModel
participant ModelImpl as Model.Impl
participant QNNSDK as QNN SDK
rect rgba(135,206,250,0.12)
Client->>QnnBackend: QnnBackend(backend_lib)
QnnBackend->>BackendImpl: construct
BackendImpl->>QNNSDK: dlopen / dlsym (getProviders)
QNNSDK-->>BackendImpl: provider_list
BackendImpl->>QNNSDK: init log / create backend / create device / create context
BackendImpl-->>QnnBackend: initialized
end
rect rgba(144,238,144,0.12)
Client->>QnnModel: QnnModel(model_so | binary_ctx, backend)
QnnModel->>ModelImpl: construct
ModelImpl->>QNNSDK: dlopen(model_so) or load binary/system ctx
ModelImpl->>QNNSDK: compose_graphs / finalize_graphs
ModelImpl->>ModelImpl: allocate contiguous buffer & wire tensors
ModelImpl-->>QnnModel: initialized
end
rect rgba(255,228,181,0.12)
Client->>QnnModel: SetInputTensorData(name,data)
QnnModel->>ModelImpl: validate/convert & copy to buffer
Client->>QnnModel: Run()
QnnModel->>ModelImpl: prepare & graphExecute()
ModelImpl->>QNNSDK: execute graph
QNNSDK-->>ModelImpl: outputs ready
Client->>QnnModel: GetOutputTensorData(name)
QnnModel->>ModelImpl: extract & convert to floats
ModelImpl-->>Client: float vector
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested labels
Poem
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
build-android-arm64-v8a.sh (1)
137-139: Correct theSHERPA_ONNX_ANDROID_PLATFORMenv var.The variable name is still misspelled as
SHERPA_ONNX_ANDRIOD_PLATFORM, so an exportedSHERPA_ONNX_ANDROID_PLATFORMis silently ignored and the override never reaches CMake. Please fix the spelling here and in the-DANDROID_PLATFORMassignment so users can select a different API level.-if [ -z $SHERPA_ONNX_ANDRIOD_PLATFORM ]; then - SHERPA_ONNX_ANDRIOD_PLATFORM=android-21 +if [ -z $SHERPA_ONNX_ANDROID_PLATFORM ]; then + SHERPA_ONNX_ANDROID_PLATFORM=android-21 fi … - -DANDROID_PLATFORM=$SHERPA_ONNX_ANDRIOD_PLATFORM .. + -DANDROID_PLATFORM=$SHERPA_ONNX_ANDROID_PLATFORM ..sherpa-onnx/csrc/qnn/qnn-backend.cc (1)
24-34: Constructor must abort on interface init failure.Returning from the ctor keeps
impl_alive with an uninitializedqnn_interface_. Public callers then dereference null function pointers (e.g.,BackendHandle(),InitContext()) and crash. Please terminate (e.g.,SHERPA_ONNX_EXIT(-1)) or throw here so a half-constructed backend never escapes.bool ok = InitQnnInterface(backend_lib); if (!ok) { SHERPA_ONNX_LOGE("Failed to init qnn interface from '%s'", backend_lib.c_str()); - return; + SHERPA_ONNX_EXIT(-1); }sherpa-onnx/csrc/qnn/utils.h (1)
17-29: FixReadFilesafety bugs.
fopencan returnnullptr, sofseekdereferences a null pointer today. Additionally, passingn(bytes) as the element count tofreadoverreads bysizeof(T)forsizeof(T) > 1, corrupting memory. Please add the null check and useans.size()for the element count.std::vector<T> ReadFile(const std::string &filename) { FILE *fp = fopen(filename.c_str(), "rb"); + if (!fp) { + SHERPA_ONNX_LOGE("Failed to open '%s'", filename.c_str()); + return {}; + } fseek(fp, 0, SEEK_END); int32_t n = ftell(fp); fseek(fp, 0, SEEK_SET); std::vector<T> ans(n / sizeof(T)); - fread(ans.data(), sizeof(T), n, fp); + fread(ans.data(), sizeof(T), ans.size(), fp); fclose(fp);sherpa-onnx/csrc/qnn/qnn-model.cc (5)
5-6: Remove the leading space in the include.The header path currently has a leading space, so this won’t compile.
-#include " sherpa-onnx/csrc/qnn/qnn-model.h" +#include "sherpa-onnx/csrc/qnn/qnn-model.h"
31-35: Restore the missing semicolon and abort on symbol failure.The
SHERPA_ONNX_LOGEcall is missing its terminating semicolon, breaking the build. Also, dropping out of the constructor leavesImplinvalid; match the project pattern by exiting here.if (!ok) { SHERPA_ONNX_LOGE("Failed to get model symbols from '%s'", - model_so.c_str()) - return; + model_so.c_str()); + SHERPA_ONNX_EXIT(-1); }
47-51: CheckLoadSystemLiband abort on failure.Ignoring the boolean result allows the ctor to proceed with null
qnn_system_interface_, leading to crashes later. Please enforce success (exit/throw) after the call.- LoadSystemLib(binary_context_file, system_lib); + if (!LoadSystemLib(binary_context_file, system_lib)) { + SHERPA_ONNX_LOGE("Failed to load system lib '%s'", + system_lib.c_str()); + SHERPA_ONNX_EXIT(-1); + }
263-284: Handle both tensor versions.The tensor helpers blindly dereference
t->v1, so V2 tensors read garbage (union mismatch) and produce wrong shapes/sizes. Please branch ont->versionbefore touching union members.auto t = name2tensor_.at(name); - - shape = {t->v1.dimensions, t->v1.dimensions + t->v1.rank}; + if (t->version == QNN_TENSOR_VERSION_1) { + shape = {t->v1.dimensions, t->v1.dimensions + t->v1.rank}; + } else if (t->version == QNN_TENSOR_VERSION_2) { + shape = {t->v2.dimensions, t->v2.dimensions + t->v2.rank}; + } else { + SHERPA_ONNX_LOGE("Unknown tensor version for '%s'", name.c_str()); + }Same adjustment needed in
TensorSizeInBytes,SetInputTensorData, andGetOutputTensorDatabefore accessing union fields.
347-352: Fix the log message width typo.The diagnostic mixes
sizeof(uint32_t)into anint32_tcontext; this was previously flagged.static_cast<int32_t>(t->v1.clientBuf.dataSize), - static_cast<int32_t>(n * sizeof(uint32_t))); + static_cast<int32_t>(n * sizeof(int32_t)));sherpa-onnx/csrc/qnn/utils.cc (3)
264-302: Stop readingv2members in the V1 copy path.
CopyTensorInfoV1treats the union as V2, touchingsrc.v2.*. That’s undefined behaviour and corrupts the destination. Please remove thev2accesses from the V1 branch.- CopyDimensions(src.v2.isDynamicDimensions, src.v2.rank, - &dst.v2.isDynamicDimensions); - - dst.v2.sparseParams.type = src.v2.sparseParams.type; - dst.v2.sparseParams.hybridCoo.numSpecifiedElements = - src.v2.sparseParams.hybridCoo.numSpecifiedElements; - dst.v2.sparseParams.hybridCoo.numSparseDimensions = - src.v2.sparseParams.hybridCoo.numSparseDimensions; - dst.v2.isProduced = src.v2.isProduced;
323-331: Fix the V2 client buffer initialization.For V2 tensors the code writes to
dst.v1.clientBuf.data, leavingdst.v2.clientBuf.datauninitialized. Initialize the correct union member.if (dst.v2.memType != QNN_TENSORMEMTYPE_RAW) { SHERPA_ONNX_LOGE("Unsupported mem type: %s", TensorMemTypeToString(dst.v2.memType).c_str()); } else { - dst.v1.clientBuf.data = nullptr; + dst.v2.clientBuf.data = nullptr; dst.v2.clientBuf.dataSize = GetSizeInBytes(dst.v2.dimensions, dst.v2.rank, dst.v2.dataType); }
373-407: Resolve thestd::ostringstreamtypo.
std::ostringstream os osis invalid C++ and stops the build. Please split into two statements.- std::ostringstream os os << " id: " << t.id << "\n"; + std::ostringstream os; + os << " id: " << t.id << "\n";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
build-android-arm64-v8a.sh(3 hunks)sherpa-onnx/csrc/CMakeLists.txt(2 hunks)sherpa-onnx/csrc/qnn/macros.h(1 hunks)sherpa-onnx/csrc/qnn/qnn-backend.cc(1 hunks)sherpa-onnx/csrc/qnn/qnn-backend.h(1 hunks)sherpa-onnx/csrc/qnn/qnn-model.cc(1 hunks)sherpa-onnx/csrc/qnn/qnn-model.h(1 hunks)sherpa-onnx/csrc/qnn/utils.cc(1 hunks)sherpa-onnx/csrc/qnn/utils.h(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:
build-android-arm64-v8a.shsherpa-onnx/csrc/CMakeLists.txtsherpa-onnx/csrc/qnn/qnn-backend.ccsherpa-onnx/csrc/qnn/qnn-backend.hsherpa-onnx/csrc/qnn/qnn-model.cc
📚 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:
build-android-arm64-v8a.shsherpa-onnx/csrc/CMakeLists.txtsherpa-onnx/csrc/qnn/qnn-backend.hsherpa-onnx/csrc/qnn/qnn-model.cc
🧬 Code graph analysis (7)
sherpa-onnx/csrc/qnn/qnn-model.h (2)
sherpa-onnx/csrc/qnn/qnn-backend.h (2)
sherpa_onnx(13-34)QnnBackend(15-32)sherpa-onnx/csrc/qnn/qnn-model.cc (46)
QnnModel(595-595)QnnModel(597-598)QnnModel(600-604)model_so(420-431)model_so(420-420)binary_context_file(53-202)binary_context_file(53-54)SaveBinaryContext(606-608)SaveBinaryContext(606-606)filename(206-253)filename(206-206)InputTensorNames(610-612)InputTensorNames(610-610)OutputTensorNames(614-616)OutputTensorNames(614-614)TensorShape(618-620)TensorShape(618-618)name(263-276)name(263-263)name(278-284)name(278-278)name(286-288)name(286-286)name(290-329)name(290-290)name(331-359)name(331-332)name(361-394)name(361-361)TensorSizeInBytes(622-624)TensorSizeInBytes(622-622)HasTensor(626-628)HasTensor(626-626)SetInputTensorData(630-633)SetInputTensorData(630-631)SetInputTensorData(635-638)SetInputTensorData(635-636)p(544-564)n(530-542)GetOutputTensorData(640-643)GetOutputTensorData(640-641)Run(645-645)Run(645-645)Impl(23-42)Impl(44-51)Impl(204-204)
sherpa-onnx/csrc/qnn/qnn-backend.cc (3)
sherpa-onnx/csrc/qnn/qnn-model.cc (5)
Impl(23-42)Impl(44-51)Impl(204-204)symbol(433-453)p(544-564)sherpa-onnx/csrc/qnn/utils.cc (2)
LogCallback(344-371)LogCallback(344-345)sherpa-onnx/csrc/qnn/qnn-backend.h (1)
QnnBackend(15-32)
sherpa-onnx/csrc/qnn/qnn-backend.h (2)
sherpa-onnx/csrc/qnn/qnn-model.h (1)
sherpa_onnx(13-52)sherpa-onnx/csrc/qnn/qnn-backend.cc (22)
QnnBackend(213-213)QnnBackend(215-216)backend_lib(85-172)backend_lib(85-85)InitContext(218-218)InitContext(218-218)InitContext(220-222)InitContext(220-220)LogHandle(224-224)LogHandle(224-224)BackendHandle(226-228)BackendHandle(226-226)DeviceHandle(230-232)DeviceHandle(230-230)ContextHandle(234-236)ContextHandle(234-234)QnnInterface(238-240)QnnInterface(238-238)LogLevel(242-242)LogLevel(242-242)Impl(24-35)Impl(37-57)
sherpa-onnx/csrc/qnn/utils.h (1)
sherpa-onnx/csrc/qnn/utils.cc (20)
PrintTensor(373-407)PrintTensor(373-373)FillData(96-119)FillData(96-96)FillData(121-124)FillData(121-121)GetData(126-135)GetData(126-126)FreeTensor(150-158)FreeTensor(150-150)CopyTensorInfo(334-342)CopyTensorInfo(334-334)QuantizationEncodingToString(41-53)QuantizationEncodingToString(41-41)TensorDataTypeToString(55-81)TensorDataTypeToString(55-55)LogCallback(344-371)LogCallback(344-345)CopyMetadataToGraphsInfo(501-535)CopyMetadataToGraphsInfo(501-503)
sherpa-onnx/csrc/qnn/qnn-model.cc (3)
sherpa-onnx/csrc/qnn/qnn-backend.cc (7)
ret(174-177)ret(179-183)ret(185-189)Impl(24-35)Impl(37-57)t(70-70)t(70-70)sherpa-onnx/csrc/qnn/utils.cc (20)
CopyMetadataToGraphsInfo(501-535)CopyMetadataToGraphsInfo(501-503)FreeTensor(150-158)FreeTensor(150-150)TensorDataTypeToString(55-81)TensorDataTypeToString(55-55)QuantizationEncodingToString(41-53)QuantizationEncodingToString(41-41)FillData(96-119)FillData(96-96)FillData(121-124)FillData(121-121)GetData(126-135)GetData(126-126)LogCallback(344-371)LogCallback(344-345)CopyTensorInfo(334-342)CopyTensorInfo(334-334)PrintTensor(373-407)PrintTensor(373-373)sherpa-onnx/csrc/qnn/qnn-model.h (1)
QnnModel(19-50)
sherpa-onnx/csrc/qnn/utils.cc (1)
sherpa-onnx/csrc/qnn/qnn-model.cc (2)
n(530-542)p(544-564)
sherpa-onnx/csrc/qnn/macros.h (1)
sherpa-onnx/csrc/qnn/qnn-backend.cc (3)
ret(174-177)ret(179-183)ret(185-189)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
sherpa-onnx/csrc/qnn/qnn-model.cc (5)
23-42: Constructor must hard-fail on initialization errorsExiting the constructor with
returnleaves a partially built object in scope; all subsequent method calls then dereference null handles and crash. Please hard-fail (e.g.,SHERPA_ONNX_EXIT(-1)or throw) immediately whenInitModel/InitSymbolsfail so the process never observes an invalidImpl.Apply this diff:
- if (!ok) { - SHERPA_ONNX_LOGE("Failed to load '%s'", model_so.c_str()); - return; - } + if (!ok) { + SHERPA_ONNX_LOGE("Failed to load '%s'", model_so.c_str()); + SHERPA_ONNX_EXIT(-1); + } ... - if (!ok) { - SHERPA_ONNX_LOGE("Failed to get model symbols from '%s'", - model_so.c_str()); - return; - } + if (!ok) { + SHERPA_ONNX_LOGE("Failed to get model symbols from '%s'", + model_so.c_str()); + SHERPA_ONNX_EXIT(-1); + }
44-51: Check LoadSystemLib result before proceedingWhen
LoadSystemLibfails the constructor continues, soAllocateBuffer/SetupPointersrun with empty tensors andimpl_reports success while nothing is initialized. Bail out immediately whenLoadSystemLibreturnsfalse.Apply this diff:
- LoadSystemLib(binary_context_file, system_lib); + if (!LoadSystemLib(binary_context_file, system_lib)) { + SHERPA_ONNX_EXIT(-1); + }
186-191: Return failure when graph retrieval fails
graphRetrievefailure still returnstrue, so callers attempt to run with a nullgraph_handle_. This is the same fatal bug reported earlier—please returnfalseso initialization aborts.Apply this diff:
- free_graphs_info(); - SHERPA_ONNX_LOGE("Unable to retrieve graph handle for graph %d", 0); - return true; + free_graphs_info(); + SHERPA_ONNX_LOGE("Unable to retrieve graph handle for graph %d", 0); + return false;
263-393: Handle both tensor versionsEvery accessor here assumes
Qnn_Tensor_tis version 1 (v1.*fields, byte counts, quant params). Passing a V2 tensor (the default in many modern QNN binaries) yields garbage pointers, incorrect shapes, and crashes. Please branch ont->versionfor all reads/writes (shape, size, data type, quant parameters) and uset->v2.*when appropriate.Illustrative fix for
TensorShape/TensorSizeInBytes:- auto t = name2tensor_.at(name); - - shape = {t->v1.dimensions, t->v1.dimensions + t->v1.rank}; + auto t = name2tensor_.at(name); + if (t->version == QNN_TENSOR_VERSION_1) { + shape.assign(t->v1.dimensions, t->v1.dimensions + t->v1.rank); + } else if (t->version == QNN_TENSOR_VERSION_2) { + shape.assign(t->v2.dimensions, t->v2.dimensions + t->v2.rank); + } else { + SHERPA_ONNX_LOGE("Unknown tensor version: %d", t->version); + }Apply the same version-aware handling in
TensorSizeInBytes, bothSetInputTensorDataoverloads, andGetOutputTensorData(validating type/encoding and accessingclientBuf) so mixed-version graphs work correctly.
347-351: Fix the logged size unitThe validation compares against
n * sizeof(int32_t)but the error message reportssizeof(uint32_t), which makes debugging confusing.Apply this diff:
- static_cast<int32_t>(n * sizeof(uint32_t))); + static_cast<int32_t>(n * sizeof(int32_t)));sherpa-onnx/csrc/qnn/utils.cc (4)
21-39: Add a default return in TensorTypeToStringFalling through the switch produces undefined behaviour. Return a fallback string (as the other helpers do) so all callers get a valid value.
Apply this diff:
std::string TensorTypeToString(Qnn_TensorType_t t) { switch (t) { ... SHERPA_ONNX_TO_STRING(QNN_TENSOR_TYPE_UNDEFINED); + default: + return "Unknown"; } }
264-302: Stop reading V2 fields in CopyTensorInfoV1This function is invoked for version 1 tensors. Accessing
src.v2.*corrupts memory and drags garbage into the copy. Remove allv2accesses and keep the copy strictly within the v1 members.Apply this diff:
- CopyDimensions(src.v2.isDynamicDimensions, src.v2.rank, - &dst.v2.isDynamicDimensions); - - dst.v2.sparseParams.type = src.v2.sparseParams.type; - dst.v2.sparseParams.hybridCoo.numSpecifiedElements = - src.v2.sparseParams.hybridCoo.numSpecifiedElements; - dst.v2.sparseParams.hybridCoo.numSparseDimensions = - src.v2.sparseParams.hybridCoo.numSparseDimensions; - dst.v2.isProduced = src.v2.isProduced;(If you need that info, copy it only in the V2 branch.)
304-332: Write client buffer info into the correct union memberInside the V2 branch you still assign
dst.v1.clientBuf.data, leavingdst.v2.clientBufuninitialized. Use the V2 member instead.Apply this diff:
- dst.v1.clientBuf.data = nullptr; - dst.v2.clientBuf.dataSize = + dst.v2.clientBuf.data = nullptr; + dst.v2.clientBuf.dataSize = GetSizeInBytes(dst.v2.dimensions, dst.v2.rank, dst.v2.dataType);
502-517: Fail on unsupported binary versionsThe function still logs “Unsupported…” but returns
true, so callers assume success and dereferencegraphs_info[0]. Returnfalseto propagate the failure.Apply this diff:
case QNN_SYSTEM_CONTEXT_BINARY_INFO_VERSION_1: { SHERPA_ONNX_LOGE("Unsupported binary context version: %d", binary_info->version); - return true; + return false; } case QNN_SYSTEM_CONTEXT_BINARY_INFO_VERSION_2: { SHERPA_ONNX_LOGE("Unsupported binary context version: %d", binary_info->version); - return true; + return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sherpa-onnx/csrc/CMakeLists.txt(2 hunks)sherpa-onnx/csrc/qnn/qnn-model.cc(1 hunks)sherpa-onnx/csrc/qnn/utils.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/qnn-model.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/qnn/qnn-model.ccsherpa-onnx/csrc/CMakeLists.txt
🧬 Code graph analysis (2)
sherpa-onnx/csrc/qnn/qnn-model.cc (3)
sherpa-onnx/csrc/qnn/qnn-backend.cc (7)
ret(174-177)ret(179-183)ret(185-189)Impl(24-35)Impl(37-57)t(70-70)t(70-70)sherpa-onnx/csrc/qnn/utils.cc (20)
CopyMetadataToGraphsInfo(502-536)CopyMetadataToGraphsInfo(502-504)FreeTensor(150-158)FreeTensor(150-150)TensorDataTypeToString(55-81)TensorDataTypeToString(55-55)QuantizationEncodingToString(41-53)QuantizationEncodingToString(41-41)FillData(96-119)FillData(96-96)FillData(121-124)FillData(121-121)GetData(126-135)GetData(126-126)LogCallback(344-371)LogCallback(344-345)CopyTensorInfo(334-342)CopyTensorInfo(334-334)PrintTensor(373-408)PrintTensor(373-373)sherpa-onnx/csrc/qnn/qnn-model.h (1)
QnnModel(19-50)
sherpa-onnx/csrc/qnn/utils.cc (1)
sherpa-onnx/csrc/qnn/qnn-model.cc (2)
n(530-542)p(544-564)
🔇 Additional comments (1)
sherpa-onnx/csrc/CMakeLists.txt (1)
203-209: ✅ QNN source files block looks correct.The guard condition is now properly set to
SHERPA_ONNX_ENABLE_QNN, and the file paths are correct. This aligns with the established pattern for conditional backends like RKNN and Ascend NPU.
| for (uint32_t i = 0; i < graph.num_input_tensors; ++i) { | ||
| SHERPA_ONNX_LOGE("input %d", (int)i); | ||
| auto p = TensorPtr(new Qnn_Tensor_t(QNN_TENSOR_INIT), &FreeTensor); | ||
|
|
||
| CopyTensorInfo(graph.input_tensors[i], *p); | ||
| PrintTensor(p->v2); | ||
|
|
||
| std::string name = p->v1.name; | ||
| name2tensor_[name] = p.get(); | ||
| input_tensor_names_.push_back(std::move(name)); | ||
|
|
||
| input_tensors_.push_back(std::move(p)); | ||
| } | ||
| } | ||
|
|
||
| void InitOutputTensors(GraphInfo graph) { | ||
| output_tensors_.reserve(graph.num_output_tensors); | ||
| output_tensor_names_.reserve(graph.num_output_tensors); | ||
| for (uint32_t i = 0; i < graph.num_output_tensors; ++i) { | ||
| auto p = TensorPtr(new Qnn_Tensor_t(QNN_TENSOR_INIT), &FreeTensor); | ||
|
|
||
| CopyTensorInfo(graph.output_tensors[i], *p); | ||
|
|
||
| SHERPA_ONNX_LOGE("output %d", (int)i); | ||
| PrintTensor(p->v2); | ||
|
|
||
| std::string name = p->v1.name; | ||
| name2tensor_[name] = p.get(); | ||
| output_tensor_names_.push_back(std::move(name)); | ||
|
|
||
| output_tensors_.push_back(std::move(p)); | ||
| } | ||
| } | ||
|
|
||
| void AllocateBuffer() { | ||
| uint32_t n = 0; | ||
| for (const auto &p : name2tensor_) { | ||
| n += p.second->v1.clientBuf.dataSize; | ||
| } | ||
|
|
||
| if (debug_) { | ||
| SHERPA_ONNX_LOGE("Allocate %d bytes, or %.3f MB", static_cast<int32_t>(n), | ||
| static_cast<float>(n) / 1024 / 1024); | ||
| } | ||
|
|
||
| buffer_.resize(n); | ||
| } | ||
|
|
||
| void SetupPointers() { | ||
| uint8_t *p = buffer_.data(); | ||
| uint32_t n = 0; | ||
| for (auto &t : input_tensors_) { | ||
| t->v1.clientBuf.data = p; | ||
| p += t->v1.clientBuf.dataSize; | ||
| } | ||
|
|
||
| for (auto &t : output_tensors_) { | ||
| t->v1.clientBuf.data = p; | ||
| p += t->v1.clientBuf.dataSize; | ||
| } |
There was a problem hiding this comment.
Do not assume V1 layout when initializing tensors
InitInputTensors, InitOutputTensors, and SetupPointers all pull names, data pointers, and buffer sizes from v1. For V2 tensors those union members are undefined, so names become empty and client buffers are never wired. Please switch on p->version (and on t->version inside SetupPointers) before touching v1.* or v2.*.
Example update for the name extraction and buffer wiring:
- std::string name = p->v1.name;
+ std::string name =
+ (p->version == QNN_TENSOR_VERSION_1) ? p->v1.name : p->v2.name;
...
- t->v1.clientBuf.data = p;
- p += t->v1.clientBuf.dataSize;
+ if (t->version == QNN_TENSOR_VERSION_1) {
+ t->v1.clientBuf.data = p;
+ p += t->v1.clientBuf.dataSize;
+ } else if (t->version == QNN_TENSOR_VERSION_2) {
+ t->v2.clientBuf.data = p;
+ p += t->v2.clientBuf.dataSize;
+ }Apply the same pattern for outputs and for all places that currently dereference v1 unconditionally.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/qnn/qnn-model.cc around lines 496 to 555, the code assumes
Qnn_Tensor_t::v1 layout for extracting names and buffer sizes; for V2 tensors
those union fields are undefined which yields empty names and uninitialized
buffers. Fix InitInputTensors and InitOutputTensors by checking p->version and
reading name and clientBuf.dataSize from the corresponding union (v1 or v2)
based on that version before inserting into name2tensor_ and name vectors;
likewise update SetupPointers to switch on t->version when assigning
t->vX.clientBuf.data and advancing the pointer. Apply the same pattern
consistently for all places that currently dereference v1 unconditionally so
both V1 and V2 tensors are handled correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
sherpa-onnx/csrc/qnn/qnn-model.cc (8)
148-166: Critical: Lambda captures must be by reference to prevent memory leak.The lambda captures
graphs_infoandgraphs_countby value at line 148, butgraphs_infois stillnullptrat that point. WhenCopyMetadataToGraphsInfo(line 143) allocates memory and assigns it tographs_info, the lambda still holds the capturednullptr. This causes a memory leak when the lambda executes on error paths.Apply this diff to capture by reference:
- auto free_graphs_info = [&graphs_info, &graphs_count] { + auto free_graphs_info = [&graphs_info, &graphs_count] { for (uint32_t i = 0; i < graphs_count; ++i) {
118-146: Critical: System context handle leaks on error paths.After
systemContextCreatesucceeds at line 119, thesys_ctx_handlemust be freed on every exit path. Currently, early returns at lines 134 and 145 leak the handle. Only the success path at line 172 frees it.Use a scope guard to ensure cleanup:
QnnSystemContext_Handle_t sys_ctx_handle = nullptr; if (qnn_system_interface_.systemContextCreate(&sys_ctx_handle) != QNN_SUCCESS) { SHERPA_ONNX_LOGE("Could not create system handle."); return false; } + // Ensure sys_ctx_handle is freed on all paths + auto sys_ctx_guard = [&] { + if (sys_ctx_handle) { + qnn_system_interface_.systemContextFree(sys_ctx_handle); + sys_ctx_handle = nullptr; + } + }; + auto cleanup = std::shared_ptr<void>(nullptr, [&](void*) { sys_ctx_guard(); }); const QnnSystemContext_BinaryInfo_t *binary_info = nullptr;Then remove the explicit free at line 172 since the guard handles it.
264-277: Critical: Check tensor version before accessing union members.Line 274 directly accesses
t->v1.dimensionsandt->v1.rankwithout checkingt->version. For V2 tensors, thev1union members are undefined. This issue affects multiple methods throughout the file.Apply this pattern to check the version:
std::vector<int32_t> TensorShape(const std::string &name) const { std::vector<int32_t> shape; if (!HasTensor(name)) { SHERPA_ONNX_LOGE("No such tensor '%s'", name.c_str()); return shape; } auto t = name2tensor_.at(name); - - shape = {t->v1.dimensions, t->v1.dimensions + t->v1.rank}; + if (t->version == QNN_TENSOR_VERSION_1) { + shape.assign(t->v1.dimensions, t->v1.dimensions + t->v1.rank); + } else if (t->version == QNN_TENSOR_VERSION_2) { + shape.assign(t->v2.dimensions, t->v2.dimensions + t->v2.rank); + } else { + SHERPA_ONNX_LOGE("Unknown tensor version: %d", t->version); + } return shape; }
279-285: Critical: Check tensor version before accessing clientBuf.Line 284 accesses
t->v1.clientBuf.dataSizewithout checking the tensor version. Apply the same version-check pattern as recommended forTensorShape.int32_t TensorSizeInBytes(const std::string &name) const { if (!HasTensor(name)) { return 0; } - - return name2tensor_.at(name)->v1.clientBuf.dataSize; + auto t = name2tensor_.at(name); + if (t->version == QNN_TENSOR_VERSION_1) { + return t->v1.clientBuf.dataSize; + } else if (t->version == QNN_TENSOR_VERSION_2) { + return t->v2.clientBuf.dataSize; + } + SHERPA_ONNX_LOGE("Unknown tensor version: %d", t->version); + return 0; }
291-360: Critical: SetInputTensorData methods assume V1 tensors.Both
SetInputTensorDataoverloads (float at lines 298-316, int32 at lines 340-354) access V1 union members (v1.dataType,v1.quantizeParams,v1.clientBuf) without checking the tensor version. Additionally,FillData(lines 326, 356) also assumes V1 layout per the utils.cc implementation.Update both methods to check
t->versionand access the appropriate union member (v1orv2) before accessing dataType, quantizeParams, and clientBuf fields. Also ensureFillDatain utils.cc handles both V1 and V2 tensors.
362-395: Critical: GetOutputTensorData assumes V1 tensors.Lines 369-389 access V1 union members without checking the tensor version. Line 392 calls
GetData, which also assumes V1 layout. This prevents correct output retrieval for V2 tensors.Apply the same version-checking pattern as recommended for input tensors, and ensure
GetDatain utils.cc handles both V1 and V2 tensors.
502-538: Critical: Tensor name extraction assumes V1 layout.Lines 513 and 532 extract tensor names from
p->v1.namewithout checkingp->version. This is inconsistent with line 511 and 530 which print V2 info. When V2 tensors are encountered, the V1 name field is undefined, resulting in empty or garbage names in thename2tensor_map and breaking the entire tensor lookup API.Extract names based on version:
- std::string name = p->v1.name; + std::string name = (p->version == QNN_TENSOR_VERSION_1) ? p->v1.name : p->v2.name; name2tensor_[name] = p.get(); input_tensor_names_.push_back(std::move(name));Apply the same fix at line 532 for output tensors.
540-574: Critical: Buffer allocation and pointer setup assume V1 tensors.
AllocateBuffer(line 543) andSetupPointers(lines 558-559, 563-564) accessv1.clientBufwithout checking tensor versions. This results in incorrect buffer sizes and data pointer assignments for V2 tensors, causing memory corruption during inference.Update both methods to check tensor version:
void AllocateBuffer() { uint32_t n = 0; for (const auto &p : name2tensor_) { - n += p.second->v1.clientBuf.dataSize; + auto t = p.second; + n += (t->version == QNN_TENSOR_VERSION_1) ? t->v1.clientBuf.dataSize : t->v2.clientBuf.dataSize; } // ... } void SetupPointers() { uint8_t *p = buffer_.data(); for (auto &t : input_tensors_) { - t->v1.clientBuf.data = p; - p += t->v1.clientBuf.dataSize; + if (t->version == QNN_TENSOR_VERSION_1) { + t->v1.clientBuf.data = p; + p += t->v1.clientBuf.dataSize; + } else if (t->version == QNN_TENSOR_VERSION_2) { + t->v2.clientBuf.data = p; + p += t->v2.clientBuf.dataSize; + } } // Apply same pattern for output_tensors_ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CMakeLists.txt(3 hunks)sherpa-onnx/csrc/qnn/qnn-backend.cc(1 hunks)sherpa-onnx/csrc/qnn/qnn-backend.h(1 hunks)sherpa-onnx/csrc/qnn/qnn-model.cc(1 hunks)sherpa-onnx/csrc/qnn/qnn-model.h(1 hunks)sherpa-onnx/csrc/qnn/utils.cc(1 hunks)sherpa-onnx/csrc/qnn/utils.h(1 hunks)
🧰 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/qnn/qnn-backend.hCMakeLists.txtsherpa-onnx/csrc/qnn/qnn-backend.ccsherpa-onnx/csrc/qnn/qnn-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/qnn/qnn-backend.hCMakeLists.txtsherpa-onnx/csrc/qnn/qnn-backend.ccsherpa-onnx/csrc/qnn/qnn-model.cc
🧬 Code graph analysis (6)
sherpa-onnx/csrc/qnn/qnn-backend.h (2)
sherpa-onnx/csrc/qnn/qnn-model.h (1)
sherpa_onnx(13-53)sherpa-onnx/csrc/qnn/qnn-backend.cc (24)
QnnBackend(218-218)QnnBackend(220-221)backend_lib(89-176)backend_lib(89-89)InitContext(223-223)InitContext(223-223)InitContext(225-227)InitContext(225-225)LogHandle(229-229)LogHandle(229-229)BackendHandle(231-233)BackendHandle(231-231)DeviceHandle(235-237)DeviceHandle(235-235)ContextHandle(239-241)ContextHandle(239-239)QnnInterface(243-245)QnnInterface(243-243)LogLevel(247-247)LogLevel(247-247)IsInitialized(249-249)IsInitialized(249-249)Impl(24-37)Impl(39-59)
sherpa-onnx/csrc/qnn/qnn-backend.cc (3)
sherpa-onnx/csrc/qnn/qnn-model.cc (8)
is_initialized_(420-420)Impl(23-41)Impl(43-52)Impl(205-205)symbol(443-463)p(554-574)IsInitialized(658-658)IsInitialized(658-658)sherpa-onnx/csrc/qnn/utils.cc (2)
LogCallback(345-372)LogCallback(345-346)sherpa-onnx/csrc/qnn/qnn-backend.h (1)
QnnBackend(14-32)
sherpa-onnx/csrc/qnn/utils.h (1)
sherpa-onnx/csrc/qnn/utils.cc (20)
PrintTensor(374-409)PrintTensor(374-374)FillData(97-120)FillData(97-97)FillData(122-125)FillData(122-122)GetData(127-136)GetData(127-127)FreeTensor(151-159)FreeTensor(151-151)CopyTensorInfo(335-343)CopyTensorInfo(335-335)QuantizationEncodingToString(42-54)QuantizationEncodingToString(42-42)TensorDataTypeToString(56-82)TensorDataTypeToString(56-56)LogCallback(345-372)LogCallback(345-346)CopyMetadataToGraphsInfo(503-537)CopyMetadataToGraphsInfo(503-505)
sherpa-onnx/csrc/qnn/qnn-model.h (2)
sherpa-onnx/csrc/qnn/qnn-backend.h (2)
sherpa_onnx(12-34)QnnBackend(14-32)sherpa-onnx/csrc/qnn/qnn-model.cc (48)
QnnModel(606-606)QnnModel(608-609)QnnModel(611-615)model_so(430-441)model_so(430-430)binary_context_file(54-203)binary_context_file(54-55)SaveBinaryContext(617-619)SaveBinaryContext(617-617)filename(207-254)filename(207-207)InputTensorNames(621-623)InputTensorNames(621-621)OutputTensorNames(625-627)OutputTensorNames(625-625)TensorShape(629-631)TensorShape(629-629)name(264-277)name(264-264)name(279-285)name(279-279)name(287-289)name(287-287)name(291-330)name(291-291)name(332-360)name(332-333)name(362-395)name(362-362)TensorSizeInBytes(633-635)TensorSizeInBytes(633-633)HasTensor(637-639)HasTensor(637-637)SetInputTensorData(641-644)SetInputTensorData(641-642)SetInputTensorData(646-649)SetInputTensorData(646-647)p(554-574)n(540-552)GetOutputTensorData(651-654)GetOutputTensorData(651-652)Run(656-656)Run(656-656)IsInitialized(658-658)IsInitialized(658-658)Impl(23-41)Impl(43-52)Impl(205-205)
sherpa-onnx/csrc/qnn/qnn-model.cc (3)
sherpa-onnx/csrc/qnn/qnn-backend.cc (10)
ret(178-181)ret(183-187)ret(189-193)Impl(24-37)Impl(39-59)t(72-72)t(72-72)is_initialized_(86-86)IsInitialized(249-249)IsInitialized(249-249)sherpa-onnx/csrc/qnn/utils.cc (20)
CopyMetadataToGraphsInfo(503-537)CopyMetadataToGraphsInfo(503-505)FreeTensor(151-159)FreeTensor(151-151)TensorDataTypeToString(56-82)TensorDataTypeToString(56-56)QuantizationEncodingToString(42-54)QuantizationEncodingToString(42-42)FillData(97-120)FillData(97-97)FillData(122-125)FillData(122-122)GetData(127-136)GetData(127-127)LogCallback(345-372)LogCallback(345-346)CopyTensorInfo(335-343)CopyTensorInfo(335-335)PrintTensor(374-409)PrintTensor(374-374)sherpa-onnx/csrc/qnn/qnn-model.h (1)
QnnModel(19-51)
sherpa-onnx/csrc/qnn/utils.cc (1)
sherpa-onnx/csrc/qnn/qnn-model.cc (2)
n(540-552)p(554-574)
🔇 Additional comments (3)
sherpa-onnx/csrc/qnn/qnn-model.h (1)
1-55: Header structure looks good.The PIMPL design is correctly implemented, and the API surface is clean. The QnnBackend raw pointer parameter requires that callers ensure the backend outlives the QnnModel instance, which is a reasonable design choice for this use case.
sherpa-onnx/csrc/qnn/qnn-model.cc (2)
606-659: PIMPL delegation methods are correctly implemented.All public
QnnModelmethods properly delegate to theimpl_pointer. This is the standard PIMPL pattern with no issues.
23-52: Remove the verification requirement—there is no client code in this repository.The verification script found that QnnModel is only defined in
qnn-model.handqnn-model.ccwith no usage elsewhere in the codebase. Since there are no instantiations or calls to QnnModel methods in any other repository files, and no test files use it, the concern about verifying that "client code checks IsInitialized()" cannot be addressed within this repository.If QnnModel is a public library API meant for external consumers, document that external callers must check
IsInitialized()before calling methods. If it's currently unused internal code, its error handling pattern is acceptable but doesn't require in-repo verification.Likely an incorrect or invalid review comment.
| static bool CopyGraphsInfo(const QnnSystemContext_GraphInfo_t *graphs_input, | ||
| uint32_t num_graphs, GraphInfo **&graphs_info) { | ||
| SHERPA_ONNX_LOGE("version: %d", (int)graphs_input[0].version); | ||
|
|
||
| // remember to free graphs_info | ||
| graphs_info = | ||
| reinterpret_cast<GraphInfo **>(calloc(num_graphs, sizeof(GraphInfo *))); | ||
|
|
||
| GraphInfo *graph_info_arr = |
There was a problem hiding this comment.
Guard against empty graph arrays before logging
When num_graphs is 0, this dereferences graphs_input[0], which is out of bounds and results in undefined behaviour. We should short-circuit before touching the array and simply report success with an empty list instead.
- SHERPA_ONNX_LOGE("version: %d", (int)graphs_input[0].version);
-
- // remember to free graphs_info
- graphs_info =
- reinterpret_cast<GraphInfo **>(calloc(num_graphs, sizeof(GraphInfo *)));
+ if (num_graphs == 0) {
+ graphs_info = nullptr;
+ return true;
+ }
+
+ SHERPA_ONNX_LOGE("version: %d",
+ static_cast<int32_t>(graphs_input[0].version));
+
+ // remember to free graphs_info
+ graphs_info = reinterpret_cast<GraphInfo **>(
+ calloc(num_graphs, sizeof(GraphInfo *)));🤖 Prompt for AI Agents
In sherpa-onnx/csrc/qnn/utils.cc around lines 454 to 462, guard against
num_graphs == 0 before accessing graphs_input[0]; if num_graphs is 0, set
graphs_info to nullptr (or an empty allocation consistent with callers) and
return true immediately, avoiding the SHERPA_ONNX_LOGE("version: %d", ...)
dereference; add the short-circuit check as the first statement so empty graph
arrays are handled safely and callers still receive an empty list.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
sherpa-onnx/csrc/qnn/qnn-backend.cc (1)
24-37: Fail fast when the QNN interface cannot be initializedReturning from the constructor leaves
backend_handle_,device_handle_, andlog_handle_null while the public API still exposes them. A call toInitContext()after this early return will drivecontextCreatewith null handles and hitSHERPA_ONNX_QNN_CHECK. Please abort (e.g.,SHERPA_ONNX_EXIT(-1)or throw) instead of returning so the object can never escape in a broken state.bool ok = InitQnnInterface(backend_lib); if (!ok) { SHERPA_ONNX_LOGE("Failed to init qnn interface from '%s'", backend_lib.c_str()); - return; + SHERPA_ONNX_EXIT(-1); }sherpa-onnx/csrc/qnn/qnn-model.cc (4)
269-282: Fix TensorShape to copy dimensions correctly
shape = {t->v1.dimensions, t->v1.dimensions + t->v1.rank};builds a two-element vector of pointer values—none of the tensor dimensions survive. It also ignores version‑2 tensors entirely. Please branch ont->versionand useassign(begin, end)so you copy the actual extents.- auto t = name2tensor_.at(name); - - shape = {t->v1.dimensions, t->v1.dimensions + t->v1.rank}; + auto t = name2tensor_.at(name); + if (t->version == QNN_TENSOR_VERSION_1) { + shape.assign(t->v1.dimensions, t->v1.dimensions + t->v1.rank); + } else if (t->version == QNN_TENSOR_VERSION_2) { + shape.assign(t->v2.dimensions, t->v2.dimensions + t->v2.rank); + } else { + SHERPA_ONNX_LOGE("Unknown tensor version: %d", + static_cast<int32_t>(t->version)); + }
296-365: SetInputTensorData must branch on tensor versionAll of the type/size checks and buffer writes dereference
t->v1.*. For V2 tensors these fields are garbage, so every check fails and the write path hits the wrong buffer. Please inspectt->version, use the matching union member (v1vsv2) for datatype/quantization validation, and pass the correct buffer pointer intoFillData/Copyso both tensor versions work. Apply the same fix to theint32_toverload.- auto t = name2tensor_.at(name); - if (t->v1.dataType != QNN_DATATYPE_UFIXED_POINT_16) { + auto t = name2tensor_.at(name); + const Qnn_TensorV1_t *tv1 = + t->version == QNN_TENSOR_VERSION_1 ? &t->v1 : nullptr; + const Qnn_TensorV2_t *tv2 = + t->version == QNN_TENSOR_VERSION_2 ? &t->v2 : nullptr; + auto data_type = + tv1 ? tv1->dataType : (tv2 ? tv2->dataType : QNN_DATATYPE_UNDEFINED); + if (data_type != QNN_DATATYPE_UFIXED_POINT_16) { … - if (t->v1.quantizeParams.quantizationEncoding != + const auto &quant = tv1 ? tv1->quantizeParams : tv2->quantizeParams; + if (quant.quantizationEncoding != QNN_QUANTIZATION_ENCODING_SCALE_OFFSET) { … - if (n * sizeof(uint16_t) != t->v1.clientBuf.dataSize) { + uint32_t data_size = + tv1 ? tv1->clientBuf.dataSize : tv2->clientBuf.dataSize; + if (n * sizeof(uint16_t) != data_size) { … - FillData(t, p, n); + FillData(t, p, n);(Also update FillData to honor
t->version.)
507-543: Respect tensor version when ingesting graph metadata
CopyTensorInfopreserves the source version. For V2 tensors,p->v1.*is unset: usingp->v1.name,p->v1.clientBuf, and loggingp->v2unconditionally is undefined behaviour. Please branch onp->versionhere (and in the corresponding output loop) to select the right union member for names/data size, only callPrintTensoron V2 structs, and record both versions correctly so downstream lookups (SetInputTensorData,TensorSizeInBytes, etc.) can do their own version switch.- CopyTensorInfo(graph.input_tensors[i], *p); - PrintTensor(p->v2); - - std::string name = p->v1.name; + CopyTensorInfo(graph.input_tensors[i], *p); + + std::string name; + if (p->version == QNN_TENSOR_VERSION_1) { + name = p->v1.name ? p->v1.name : ""; + } else if (p->version == QNN_TENSOR_VERSION_2) { + PrintTensor(p->v2); + name = p->v2.name ? p->v2.name : ""; + } else { + SHERPA_ONNX_LOGE("Unsupported tensor version %d", + static_cast<int32_t>(p->version)); + continue; + }
545-579: AllocateBuffer/SetupPointers ignore tensor versionSumming
p.second->v1.clientBuf.dataSizeand writing intot->v1.clientBuf.databreaks every V2 tensor (sizes stay zero and pointers never set). Please switch ont->versionin both loops and driveclientBufon the correct union member.- for (const auto &p : name2tensor_) { - n += p.second->v1.clientBuf.dataSize; + for (const auto &p : name2tensor_) { + if (p.second->version == QNN_TENSOR_VERSION_1) { + n += p.second->v1.clientBuf.dataSize; + } else if (p.second->version == QNN_TENSOR_VERSION_2) { + n += p.second->v2.clientBuf.dataSize; + } } … - for (auto &t : input_tensors_) { - t->v1.clientBuf.data = p; - p += t->v1.clientBuf.dataSize; + for (auto &t : input_tensors_) { + if (t->version == QNN_TENSOR_VERSION_1) { + t->v1.clientBuf.data = p; + p += t->v1.clientBuf.dataSize; + } else if (t->version == QNN_TENSOR_VERSION_2) { + t->v2.clientBuf.data = p; + p += t->v2.clientBuf.dataSize; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sherpa-onnx/csrc/qnn/qnn-backend.cc(1 hunks)sherpa-onnx/csrc/qnn/qnn-model.cc(1 hunks)sherpa-onnx/csrc/qnn/utils.cc(1 hunks)sherpa-onnx/csrc/qnn/utils.h(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/qnn-model.ccsherpa-onnx/csrc/qnn/qnn-backend.cc
📚 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/qnn/qnn-model.ccsherpa-onnx/csrc/qnn/qnn-backend.cc
🧬 Code graph analysis (4)
sherpa-onnx/csrc/qnn/qnn-model.cc (3)
sherpa-onnx/csrc/qnn/qnn-backend.cc (11)
ret(178-181)ret(183-187)ret(189-193)Impl(24-37)Impl(24-24)Impl(39-59)t(72-72)t(72-72)is_initialized_(86-86)IsInitialized(249-249)IsInitialized(249-249)sherpa-onnx/csrc/qnn/utils.cc (20)
CopyMetadataToGraphsInfo(512-546)CopyMetadataToGraphsInfo(512-514)FreeTensor(151-159)FreeTensor(151-151)TensorDataTypeToString(56-82)TensorDataTypeToString(56-56)QuantizationEncodingToString(42-54)QuantizationEncodingToString(42-42)FillData(97-120)FillData(97-97)FillData(122-125)FillData(122-122)GetData(127-136)GetData(127-127)LogCallback(347-374)LogCallback(347-348)CopyTensorInfo(337-345)CopyTensorInfo(337-337)PrintTensor(376-411)PrintTensor(376-376)sherpa-onnx/csrc/qnn/qnn-model.h (1)
QnnModel(19-51)
sherpa-onnx/csrc/qnn/utils.h (1)
sherpa-onnx/csrc/qnn/utils.cc (20)
PrintTensor(376-411)PrintTensor(376-376)FillData(97-120)FillData(97-97)FillData(122-125)FillData(122-122)GetData(127-136)GetData(127-127)FreeTensor(151-159)FreeTensor(151-151)CopyTensorInfo(337-345)CopyTensorInfo(337-337)QuantizationEncodingToString(42-54)QuantizationEncodingToString(42-42)TensorDataTypeToString(56-82)TensorDataTypeToString(56-56)LogCallback(347-374)LogCallback(347-348)CopyMetadataToGraphsInfo(512-546)CopyMetadataToGraphsInfo(512-514)
sherpa-onnx/csrc/qnn/qnn-backend.cc (3)
sherpa-onnx/csrc/qnn/qnn-model.cc (8)
is_initialized_(425-425)Impl(24-42)Impl(44-53)Impl(210-210)symbol(448-468)p(559-579)IsInitialized(663-663)IsInitialized(663-663)sherpa-onnx/csrc/qnn/utils.cc (2)
LogCallback(347-374)LogCallback(347-348)sherpa-onnx/csrc/qnn/qnn-backend.h (1)
QnnBackend(14-32)
sherpa-onnx/csrc/qnn/utils.cc (1)
sherpa-onnx/csrc/qnn/qnn-model.cc (2)
n(545-557)p(559-579)
| // quantized = float / scale - offset; | ||
| void FillData(Qnn_Tensor_t *t, const float *data, int32_t n) { | ||
| float scale = t->v1.quantizeParams.scaleOffsetEncoding.scale; | ||
| int32_t offset = t->v1.quantizeParams.scaleOffsetEncoding.offset; | ||
|
|
||
| size_t bit_width = 16; | ||
| double true_bit_width_max = pow(2, bit_width) - 1; | ||
| double encoding_min = offset * scale; | ||
| double encoding_max = (true_bit_width_max + offset) * scale; | ||
| double encoding_range = encoding_max - encoding_min; | ||
|
|
||
| uint16_t *out = reinterpret_cast<uint16_t *>(t->v1.clientBuf.data); | ||
|
|
||
| for (size_t i = 0; i < n; ++i) { | ||
| int32_t quantized_value = | ||
| round(true_bit_width_max * (data[i] - encoding_min) / encoding_range); | ||
|
|
||
| if (quantized_value < 0) { | ||
| quantized_value = 0; | ||
| } else if (quantized_value > static_cast<int32_t>(true_bit_width_max)) { | ||
| quantized_value = static_cast<int32_t>(true_bit_width_max); | ||
| } | ||
| out[i] = static_cast<uint16_t>(quantized_value); | ||
| } | ||
| } | ||
|
|
||
| void FillData(Qnn_Tensor_t *t, const int32_t *data, int32_t n) { | ||
| int32_t *out = reinterpret_cast<int32_t *>(t->v1.clientBuf.data); | ||
| std::copy(data, data + n, out); | ||
| } | ||
|
|
||
| void GetData(const Qnn_Tensor_t *t, float *data, int32_t n) { | ||
| double scale = t->v1.quantizeParams.scaleOffsetEncoding.scale; | ||
| double offset = t->v1.quantizeParams.scaleOffsetEncoding.offset; | ||
|
|
||
| const uint16_t *p = reinterpret_cast<const uint16_t *>(t->v1.clientBuf.data); | ||
| for (int32_t i = 0; i < n; ++i) { | ||
| double quantizedValue = static_cast<double>(p[i]); | ||
| data[i] = (quantizedValue + offset) * scale; | ||
| } | ||
| } |
There was a problem hiding this comment.
Make FillData/GetData honor tensor version
These helpers unconditionally touch t->v1.*, but CopyTensorInfo can hand you V2 tensors (the default for QNN v2 graphs). That means scale/offset and clientBuf come from the wrong union member and you quantize into uninitialized memory. Please check t->version and branch to the appropriate v1/v2 fields (or reject unsupported versions) in both FillData overloads and GetData.
- float scale = t->v1.quantizeParams.scaleOffsetEncoding.scale;
- int32_t offset = t->v1.quantizeParams.scaleOffsetEncoding.offset;
+ float scale = 0.0f;
+ int32_t offset = 0;
+ void *client_buf = nullptr;
+ if (t->version == QNN_TENSOR_VERSION_1) {
+ scale = t->v1.quantizeParams.scaleOffsetEncoding.scale;
+ offset = t->v1.quantizeParams.scaleOffsetEncoding.offset;
+ client_buf = t->v1.clientBuf.data;
+ } else if (t->version == QNN_TENSOR_VERSION_2) {
+ scale = t->v2.quantizeParams.scaleOffsetEncoding.scale;
+ offset = t->v2.quantizeParams.scaleOffsetEncoding.offset;
+ client_buf = t->v2.clientBuf.data;
+ } else {
+ SHERPA_ONNX_LOGE("Unsupported tensor version: %d",
+ static_cast<int32_t>(t->version));
+ return;
+ }
…
- uint16_t *out = reinterpret_cast<uint16_t *>(t->v1.clientBuf.data);
+ uint16_t *out = reinterpret_cast<uint16_t *>(client_buf);(Apply the same branching in the int32 overload and GetData.)
Committable suggestion skipped: line range outside the PR's diff.
| fseek(fp, 0, SEEK_END); | ||
| int32_t n = ftell(fp); | ||
| fseek(fp, 0, SEEK_SET); | ||
|
|
||
| std::vector<T> ans(n / sizeof(T)); | ||
| fread(ans.data(), sizeof(T), ans.size(), fp); | ||
| fclose(fp); | ||
|
|
||
| return ans; |
There was a problem hiding this comment.
Handle file size and short read errors in ReadFile
fseek, ftell, and fread results are unchecked and the file length is truncated into int32_t. Any seek/tell failure or short read leaves ans partially/incorrectly populated, and files >2 GB will overflow n. Please promote the size calculation to long/size_t, bail out on seek/tell failure, and verify that fread actually pulled ans.size() elements before returning the buffer.
- fseek(fp, 0, SEEK_END);
- int32_t n = ftell(fp);
- fseek(fp, 0, SEEK_SET);
-
- std::vector<T> ans(n / sizeof(T));
- fread(ans.data(), sizeof(T), ans.size(), fp);
+ if (fseek(fp, 0, SEEK_END) != 0) {
+ SHERPA_ONNX_LOGE("Failed to seek '%s'", filename.c_str());
+ fclose(fp);
+ return {};
+ }
+ long size = ftell(fp);
+ if (size < 0) {
+ SHERPA_ONNX_LOGE("Failed to tell size of '%s'", filename.c_str());
+ fclose(fp);
+ return {};
+ }
+ if (fseek(fp, 0, SEEK_SET) != 0) {
+ SHERPA_ONNX_LOGE("Failed to rewind '%s'", filename.c_str());
+ fclose(fp);
+ return {};
+ }
+
+ size_t count = static_cast<size_t>(size) / sizeof(T);
+ std::vector<T> ans(count);
+ size_t read = fread(ans.data(), sizeof(T), ans.size(), fp);
+ if (read != ans.size()) {
+ SHERPA_ONNX_LOGE(
+ "Short read from '%s' (expected %zu elements, got %zu)",
+ filename.c_str(), ans.size(), read);
+ ans.resize(read);
+ }🤖 Prompt for AI Agents
In sherpa-onnx/csrc/qnn/utils.h around lines 25 to 33, the code uses
fseek/ftell/fread without checking return values and truncates file length into
int32_t; update to use long/size_t for file size, check fseek and ftell return
values and bail out on failure, compute the element count from a size_t file
length (guard against overflow for very large files), call fread and verify it
returns ans.size() elements, and on any error ensure the file is closed and
return/propagate an error (or empty vector) instead of returning a partially
filled buffer.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
sherpa-onnx/csrc/qnn/utils.cc:1
- fread() return value is not checked. If the read fails, the function will return a partially filled vector without indication of the error.
// sherpa-onnx/csrc/qnn/utils.h
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,546 @@ | |||
| // sherpa-onnx/csrc/qnn/utils.h | |||
There was a problem hiding this comment.
The comment references 'utils.h' but this is the 'utils.cc' file.
| // sherpa-onnx/csrc/qnn/utils.h | |
| // sherpa-onnx/csrc/qnn/utils.cc |
| @@ -0,0 +1,665 @@ | |||
| // sherpa-onnx/csrc/qnn/qnn-model.h | |||
There was a problem hiding this comment.
The comment references 'qnn-model.h' but this is the 'qnn-model.cc' file.
| // sherpa-onnx/csrc/qnn/qnn-model.h | |
| // sherpa-onnx/csrc/qnn/qnn-model.cc |
| // | ||
| // Copyright (c) 2025 Xiaomi Corporation | ||
|
|
||
| #include "sherpa-onnx/csrc/qnn/qnn-model.h" |
There was a problem hiding this comment.
Extra space before the include path. Should be '#include "sherpa-onnx/csrc/qnn/qnn-model.h"' without the leading space.
Summary by CodeRabbit