Repository navigation
Refactor JNI to remove casting. - #3103
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe PR replaces untyped JNI Array/Object[] returns with strongly typed Java/Kotlin objects (e.g., WaveData, AudioEvent, various Result classes), updating JNI C++ code, Java wrappers, Kotlin bindings, and example usages to consume the new typed return values. Changes
Sequence Diagram(s)sequenceDiagram
participant KotlinClient as Kotlin Client
participant KotlinAPI as Kotlin API (e.g., WaveReader)
participant JavaAPI as Java wrapper/native bridge
participant Native as JNI/C++ implementation
KotlinClient ->> KotlinAPI: readWaveFromFile(filename)
KotlinAPI ->> JavaAPI: external native readWaveFromFile(filename)
JavaAPI ->> Native: JNI call into C++
Native ->> Native: read file, build samples float[] and sampleRate
Native ->> JavaAPI: construct WaveData(float[], int) jobject and return
JavaAPI ->> KotlinAPI: return WaveData
KotlinAPI ->> KotlinClient: return WaveData (samples, sampleRate)
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 refactors the Java Native Interface (JNI) layer and its corresponding Java/Kotlin APIs to improve type safety and code clarity. By introducing dedicated data classes for JNI return types and modifying the JNI C++ implementations to directly construct and return these specific Java/Kotlin objects, the need for explicit and potentially unsafe casting of generic 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
|
dosubot
Bot
added
the
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sherpa-onnx/jni/audio-tagging.cc`:
- Around line 136-168: The FindClass local reference audioEventCls is never
released, causing a JNI local reference leak; after creating the jobjectArray
and before returning from the function that uses audioEventCls (the block that
calls env->FindClass("com/k2fsa/sherpa/onnx/AudioEvent"), env->GetMethodID,
NewObjectArray, and populates it using audioEventCls), call
env->DeleteLocalRef(audioEventCls) once (after the loop and before return) to
free the class local reference; ensure this is done even on error paths where
audioEventCls was obtained (i.e., delete it before any early return if
applicable).
In `@sherpa-onnx/jni/keyword-spotter.cc`:
- Around line 195-215: Add null checks after each JNI lookup in
keyword-spotter.cc: verify env->FindClass("java/lang/String") returned non-null
before using it to create j_tokens, verify
env->FindClass("com/k2fsa/sherpa/onnx/KeywordSpotterResult") returned non-null
and verify env->GetMethodID(result_cls, "<init>",
"(Ljava/lang/String;[Ljava/lang/String;[F)V") returned non-null before calling
the constructor; on any nullptr, clean up any local refs (e.g., delete
j_tokens/j_timestamps/local jstrings if created), optionally throw/clear a Java
exception via env->ThrowNew or return an error/null result, and avoid further
JNI calls that would crash (use the symbols string_cls, j_tokens, j_timestamps,
result_cls, ctor, env->FindClass, env->GetMethodID to locate the checks).
🧹 Nitpick comments (7)
sherpa-onnx/jni/voice-activity-detector.cc (1)
225-231: Consider adding null check afterNewObject.The implementation has good error handling for class/constructor lookup and array allocation, but
env->NewObject()can also returnnullptron failure (e.g., if the constructor throws an exception or memory allocation fails). Currently, the function would returnnullptrimplicitly, which may be acceptable, but adding an explicit check with logging would improve consistency with the other error paths.♻️ Suggested improvement
jobject speechSegment = env->NewObject(cls, ctor, static_cast<jint>(front.start), samples_arr); + if (!speechSegment) { + SHERPA_ONNX_LOGE("Failed to create SpeechSegment object"); + } + env->DeleteLocalRef(samples_arr); env->DeleteLocalRef(cls); return speechSegment;sherpa-onnx/jni/online-recognizer.cc (1)
394-399: Optimize: MoveFindClassoutside the loop and delete string local refs inside the loop.
FindClass("java/lang/String")is called on every iteration, which is wasteful. Additionally, thejstringobjects created byNewStringUTFinside the loop are local references that can accumulate and exhaust the local reference table for large token arrays.♻️ Suggested refactor
+ jclass stringClass = env->FindClass("java/lang/String"); jobjectArray tokens = env->NewObjectArray( - result.tokens.size(), env->FindClass("java/lang/String"), nullptr); + result.tokens.size(), stringClass, nullptr); for (size_t i = 0; i < result.tokens.size(); ++i) { - env->SetObjectArrayElement(tokens, i, - env->NewStringUTF(result.tokens[i].c_str())); + jstring tokenStr = env->NewStringUTF(result.tokens[i].c_str()); + env->SetObjectArrayElement(tokens, i, tokenStr); + env->DeleteLocalRef(tokenStr); } + env->DeleteLocalRef(stringClass);sherpa-onnx/kotlin-api/AudioTagging.kt (1)
57-60: Remove unnecessary@Suppress("UNCHECKED_CAST")annotation.Since the native method now returns
Array<AudioEvent>directly and there's no casting in the wrapper, this suppression is no longer needed.♻️ Suggested fix
- `@Suppress`("UNCHECKED_CAST") fun compute(stream: OfflineStream, topK: Int = -1): Array<AudioEvent> { return compute(ptr, stream.ptr, topK) }sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/WaveReader.java (1)
10-13: Consider null-checking the native result.If
readWaveFromFilereturnsnull(e.g., due to file read failure), subsequent calls togetSampleRate()orgetSamples()will throw aNullPointerException. While the comment says the program exits on wrong format, there may be other failure modes.🔧 Suggested defensive check
public WaveReader(String filename) { LibraryLoader.maybeLoad(); - this.data = readWaveFromFile(filename); + this.data = readWaveFromFile(filename); + if (this.data == null) { + throw new IllegalArgumentException("Failed to read wave file: " + filename); + } }sherpa-onnx/jni/offline-recognizer.cc (1)
516-521: Consider adding null checks for class/constructor lookup.Unlike
audio-tagging.ccwhich checks for null afterFindClassandGetMethodID, this code doesn't validate the results. While consistent with theOnlineRecognizerimplementation, adding null checks would improve robustness against class loading failures.🔧 Suggested defensive checks
// 2. Find the Java class and constructor jclass cls = env->FindClass("com/k2fsa/sherpa/onnx/OfflineRecognizerResult"); + if (cls == nullptr) { + SHERPA_ONNX_LOGE("Failed to find class com/k2fsa/sherpa/onnx/OfflineRecognizerResult"); + return nullptr; + } jmethodID ctor = env->GetMethodID(cls, "<init>", "(Ljava/lang/String;[Ljava/lang/String;[FLjava/lang/" "String;Ljava/lang/String;Ljava/lang/String;[F)V"); + if (ctor == nullptr) { + SHERPA_ONNX_LOGE("Failed to get OfflineRecognizerResult constructor"); + env->DeleteLocalRef(cls); + return nullptr; + }sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/WaveData.java (1)
15-17: Note:getSamples()exposes the internal array.Returning the internal array directly allows callers to modify the contents, breaking immutability. This is likely intentional for performance in audio processing scenarios, but if immutability is desired, consider returning a defensive copy.
sherpa-onnx/jni/keyword-spotter.cc (1)
221-225: Consider cleaning up additional local references.
string_clsandresult_clsare local references that could be released after use. While JNI automatically cleans up local references when the native method returns, explicit cleanup is a good practice for consistency and to avoid hitting the local reference table limit in loops or long-running functions.♻️ Suggested cleanup
env->DeleteLocalRef(j_keyword); env->DeleteLocalRef(j_tokens); env->DeleteLocalRef(j_timestamps); + env->DeleteLocalRef(string_cls); + env->DeleteLocalRef(result_cls); return result_obj;
| // Find the AudioEvent class | ||
| jclass audioEventCls = env->FindClass("com/k2fsa/sherpa/onnx/AudioEvent"); | ||
| if (audioEventCls == nullptr) { | ||
| SHERPA_ONNX_LOGE("Failed to find class com/k2fsa/sherpa/onnx/AudioEvent"); | ||
| return nullptr; | ||
| } | ||
|
|
||
| // Get the constructor: AudioEvent(String name, int index, float prob) | ||
| jmethodID ctor = | ||
| env->GetMethodID(audioEventCls, "<init>", "(Ljava/lang/String;IF)V"); | ||
| if (ctor == nullptr) { | ||
| SHERPA_ONNX_LOGE("Failed to get AudioEvent constructor"); | ||
| return nullptr; | ||
| } | ||
|
|
||
| // Create a jobjectArray of AudioEvent | ||
| jobjectArray obj_arr = | ||
| env->NewObjectArray(events.size(), audioEventCls, nullptr); | ||
|
|
||
| for (size_t i = 0; i < events.size(); ++i) { | ||
| const auto &e = events[i]; | ||
|
|
||
| jstring name = env->NewStringUTF(e.name.c_str()); | ||
| jobject event_obj = | ||
| env->NewObject(audioEventCls, ctor, name, e.index, e.prob); | ||
|
|
||
| env->SetObjectArrayElement(obj_arr, i, event_obj); | ||
|
|
||
| env->DeleteLocalRef(name); | ||
| env->DeleteLocalRef(event_obj); | ||
| } | ||
|
|
||
| return obj_arr; |
There was a problem hiding this comment.
Missing DeleteLocalRef for audioEventCls.
The audioEventCls local reference should be deleted before returning to avoid a local reference leak. This is consistent with the pattern in online-recognizer.cc (from relevant snippets) which deletes cls at the end.
🔧 Suggested fix
env->DeleteLocalRef(name);
env->DeleteLocalRef(event_obj);
}
+ env->DeleteLocalRef(audioEventCls);
+
return obj_arr;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Find the AudioEvent class | |
| jclass audioEventCls = env->FindClass("com/k2fsa/sherpa/onnx/AudioEvent"); | |
| if (audioEventCls == nullptr) { | |
| SHERPA_ONNX_LOGE("Failed to find class com/k2fsa/sherpa/onnx/AudioEvent"); | |
| return nullptr; | |
| } | |
| // Get the constructor: AudioEvent(String name, int index, float prob) | |
| jmethodID ctor = | |
| env->GetMethodID(audioEventCls, "<init>", "(Ljava/lang/String;IF)V"); | |
| if (ctor == nullptr) { | |
| SHERPA_ONNX_LOGE("Failed to get AudioEvent constructor"); | |
| return nullptr; | |
| } | |
| // Create a jobjectArray of AudioEvent | |
| jobjectArray obj_arr = | |
| env->NewObjectArray(events.size(), audioEventCls, nullptr); | |
| for (size_t i = 0; i < events.size(); ++i) { | |
| const auto &e = events[i]; | |
| jstring name = env->NewStringUTF(e.name.c_str()); | |
| jobject event_obj = | |
| env->NewObject(audioEventCls, ctor, name, e.index, e.prob); | |
| env->SetObjectArrayElement(obj_arr, i, event_obj); | |
| env->DeleteLocalRef(name); | |
| env->DeleteLocalRef(event_obj); | |
| } | |
| return obj_arr; | |
| // Find the AudioEvent class | |
| jclass audioEventCls = env->FindClass("com/k2fsa/sherpa/onnx/AudioEvent"); | |
| if (audioEventCls == nullptr) { | |
| SHERPA_ONNX_LOGE("Failed to find class com/k2fsa/sherpa/onnx/AudioEvent"); | |
| return nullptr; | |
| } | |
| // Get the constructor: AudioEvent(String name, int index, float prob) | |
| jmethodID ctor = | |
| env->GetMethodID(audioEventCls, "<init>", "(Ljava/lang/String;IF)V"); | |
| if (ctor == nullptr) { | |
| SHERPA_ONNX_LOGE("Failed to get AudioEvent constructor"); | |
| return nullptr; | |
| } | |
| // Create a jobjectArray of AudioEvent | |
| jobjectArray obj_arr = | |
| env->NewObjectArray(events.size(), audioEventCls, nullptr); | |
| for (size_t i = 0; i < events.size(); ++i) { | |
| const auto &e = events[i]; | |
| jstring name = env->NewStringUTF(e.name.c_str()); | |
| jobject event_obj = | |
| env->NewObject(audioEventCls, ctor, name, e.index, e.prob); | |
| env->SetObjectArrayElement(obj_arr, i, event_obj); | |
| env->DeleteLocalRef(name); | |
| env->DeleteLocalRef(event_obj); | |
| } | |
| env->DeleteLocalRef(audioEventCls); | |
| return obj_arr; |
🤖 Prompt for AI Agents
In `@sherpa-onnx/jni/audio-tagging.cc` around lines 136 - 168, The FindClass local
reference audioEventCls is never released, causing a JNI local reference leak;
after creating the jobjectArray and before returning from the function that uses
audioEventCls (the block that calls
env->FindClass("com/k2fsa/sherpa/onnx/AudioEvent"), env->GetMethodID,
NewObjectArray, and populates it using audioEventCls), call
env->DeleteLocalRef(audioEventCls) once (after the loop and before return) to
free the class local reference; ensure this is done even on error paths where
audioEventCls was obtained (i.e., delete it before any early return if
applicable).
![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 is a great refactoring that improves type safety across the JNI boundary for both Java and Kotlin APIs. By returning strongly-typed objects instead of generic Object[] or Array<Any>, the code becomes cleaner, safer, and easier to maintain, eliminating the need for unsafe casting on the client side. The changes are consistently applied across example files, API definitions, and the native JNI implementation. I've found a few potential memory leaks in the JNI layer and a minor inconsistency that should be addressed.
| env->DeleteLocalRef(j_keyword); | ||
| env->DeleteLocalRef(j_tokens); | ||
| env->DeleteLocalRef(j_timestamps); |
There was a problem hiding this comment.
There's a potential local reference leak here. The jclass objects result_cls and string_cls are not released before returning. You should call DeleteLocalRef on them to prevent memory issues.
env->DeleteLocalRef(j_keyword);
env->DeleteLocalRef(j_tokens);
env->DeleteLocalRef(j_timestamps);
env->DeleteLocalRef(result_cls);
env->DeleteLocalRef(string_cls);| env->DeleteLocalRef(jemotion); | ||
| env->DeleteLocalRef(jevent); | ||
| env->DeleteLocalRef(jdurations); | ||
| env->DeleteLocalRef(cls); |
There was a problem hiding this comment.
There's a potential local reference leak. The jclass object cls is not released before returning. You should call DeleteLocalRef on it to prevent memory issues. I see you are deleting it in the next line, but it is better to group all DeleteLocalRef calls together.
env->DeleteLocalRef(jevent);
env->DeleteLocalRef(jdurations);| for (size_t i = 0; i < result.tokens.size(); ++i) { | ||
| env->SetObjectArrayElement(tokens, i, | ||
| env->NewStringUTF(result.tokens[i].c_str())); | ||
| } |
There was a problem hiding this comment.
This loop creates a new jstring local reference in each iteration without explicitly deleting it. This can lead to a local reference table overflow and memory leaks. It's safer to explicitly create, use, and delete the reference within the loop.
for (size_t i = 0; i < result.tokens.size(); ++i) {
jstring token_str = env->NewStringUTF(result.tokens[i].c_str());
env->SetObjectArrayElement(tokens, i, token_str);
env->DeleteLocalRef(token_str);
}| env->SetObjectArrayElement(obj_arr, 0, samples_arr); | ||
| env->SetObjectArrayElement(obj_arr, 1, NewInteger(env, sampling_rate)); | ||
| // Clean up local refs | ||
| env->DeleteLocalRef(samples_arr); |
There was a problem hiding this comment.
The jclass local reference cls is not being released before the function returns. This can cause a memory leak. You should call env->DeleteLocalRef(cls);.
env->DeleteLocalRef(samples_arr);
env->DeleteLocalRef(cls);| public class WaveData { | ||
| private final float[] samples; | ||
| private final int sample_rate; | ||
|
|
||
| public WaveData(float[] samples, int sample_rate) { | ||
| this.samples = samples; | ||
| this.sample_rate = sample_rate; | ||
| } | ||
|
|
||
| public float[] getSamples() { | ||
| return samples; | ||
| } | ||
|
|
||
| public int getSampleRate() { | ||
| return sample_rate; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (this == obj) return true; | ||
| if (obj == null || getClass() != obj.getClass()) return false; | ||
| WaveData other = (WaveData) obj; | ||
| return sample_rate == other.sample_rate && Arrays.equals(samples, other.samples); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| int result = Arrays.hashCode(samples); | ||
| result = 31 * result + sample_rate; | ||
| return result; | ||
| } | ||
| } |
There was a problem hiding this comment.
The field sample_rate should be renamed to sampleRate to follow Java's camelCase naming convention. This will also make it consistent with the Kotlin WaveData class.
public class WaveData {
private final float[] samples;
private final int sampleRate;
public WaveData(float[] samples, int sampleRate) {
this.samples = samples;
this.sampleRate = sampleRate;
}
public float[] getSamples() {
return samples;
}
public int getSampleRate() {
return sampleRate;
}
@Override
public boolean equals(Object obj) {
if (this == obj) return true;
if (obj == null || getClass() != obj.getClass()) return false;
WaveData other = (WaveData) obj;
return sampleRate == other.sampleRate && Arrays.equals(samples, other.samples);
}
@Override
public int hashCode() {
int result = Arrays.hashCode(samples);
result = 31 * result + sampleRate;
return result;
}
}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@sherpa-onnx/jni/wave-reader.cc`:
- Around line 30-51: The WaveData construction path doesn't guard or clean up
the samples_arr allocation and doesn't check NewObject for null; ensure you
check the result of env->NewFloatArray (samples_arr) for nullptr and return
after logging while calling env->DeleteLocalRef(samples_arr) if allocated, and
also check env->NewObject (obj) for nullptr and on failure delete both
samples_arr and cls before returning; in short, add null checks for
NewFloatArray and NewObject, and call env->DeleteLocalRef for samples_arr and
cls on every early return (and after success) so no local refs leak (refer to
samples_arr, env->NewFloatArray, cls, ctor, env->NewObject, and
env->DeleteLocalRef).
🧹 Nitpick comments (4)
sherpa-onnx/jni/audio-tagging.cc (1)
136-152: Handle error paths to avoid local-ref leaks and null deref.If
GetMethodIDfails (Line 145) orNewObjectArrayfails (Line 151),clsisn’t released and the code can proceed with a null array. Add cleanup and a null check before the loop.♻️ Proposed fix
jclass cls = env->FindClass("com/k2fsa/sherpa/onnx/AudioEvent"); if (cls == nullptr) { SHERPA_ONNX_LOGE("Failed to find class com/k2fsa/sherpa/onnx/AudioEvent"); return nullptr; } // Get the constructor: AudioEvent(String name, int index, float prob) jmethodID ctor = env->GetMethodID(cls, "<init>", "(Ljava/lang/String;IF)V"); if (ctor == nullptr) { SHERPA_ONNX_LOGE("Failed to get AudioEvent constructor"); + env->DeleteLocalRef(cls); return nullptr; } // Create a jobjectArray of AudioEvent jobjectArray obj_arr = env->NewObjectArray(events.size(), cls, nullptr); + if (obj_arr == nullptr) { + env->DeleteLocalRef(cls); + return nullptr; + }sherpa-onnx/jni/keyword-spotter.cc (2)
192-199: Local reference leak on early return.
j_keywordis created at line 192 before thestring_clsnull check. IfFindClassfails, the function returnsnullptrwithout callingDeleteLocalRef(j_keyword), leaking the reference.Consider reordering to perform the class lookup first, or adding cleanup before the early return.
♻️ Suggested reordering
- jstring j_keyword = env->NewStringUTF(result.keyword.c_str()); - // Convert tokens (std::vector<std::string> -> String[]) jclass string_cls = env->FindClass("java/lang/String"); if (string_cls == nullptr) { SHERPA_ONNX_LOGE("Failed to find class java/lang/String"); return nullptr; } + + jstring j_keyword = env->NewStringUTF(result.keyword.c_str());
219-231: Resource cleanup missing in error paths.When
result_clsorctorlookup fails,j_keyword,j_tokens, andj_timestampsare leaked. While these error conditions are rare and references are eventually freed on method return, adding cleanup would make the code more robust.♻️ Suggested cleanup on error paths
if (result_cls == nullptr) { SHERPA_ONNX_LOGE( "Failed to find class com/k2fsa/sherpa/onnx/KeywordSpotterResult"); + env->DeleteLocalRef(j_keyword); + env->DeleteLocalRef(j_tokens); + env->DeleteLocalRef(j_timestamps); + env->DeleteLocalRef(string_cls); return nullptr; } jmethodID ctor = env->GetMethodID( result_cls, "<init>", "(Ljava/lang/String;[Ljava/lang/String;[F)V"); if (ctor == nullptr) { SHERPA_ONNX_LOGE("Failed to get KeywordSpotterResult constructor"); + env->DeleteLocalRef(j_keyword); + env->DeleteLocalRef(j_tokens); + env->DeleteLocalRef(j_timestamps); + env->DeleteLocalRef(string_cls); + env->DeleteLocalRef(result_cls); return nullptr; }sherpa-onnx/jni/offline-recognizer.cc (1)
509-515: Add pointer validation guard for getResult.Line 513 dereferences
streamPtrwithout a guard. Consider wrapping this method inSafeJNIand usingValidatePointer(as done in decode paths) to avoid native crashes on invalid pointers.💡 Possible refactor
JNIEXPORT jobject JNICALL Java_com_k2fsa_sherpa_onnx_OfflineRecognizer_getResult(JNIEnv *env, jobject /*obj*/, jlong streamPtr) { - auto stream = reinterpret_cast<sherpa_onnx::OfflineStream *>(streamPtr); - sherpa_onnx::OfflineRecognitionResult result = stream->GetResult(); + jobject jresult = nullptr; + SafeJNI(env, "OfflineRecognizer_getResult", [&] { + if (!ValidatePointer(env, streamPtr, "OfflineRecognizer_getResult", + "OfflineStream pointer is null.")) { + return; + } + auto stream = reinterpret_cast<sherpa_onnx::OfflineStream *>(streamPtr); + sherpa_onnx::OfflineRecognitionResult result = stream->GetResult(); ... - return jresult; // returned object is safe + }); + return jresult; // returned object is safe }
| // Find WaveData class | ||
| jclass cls = env->FindClass("com/k2fsa/sherpa/onnx/WaveData"); | ||
| if (cls == nullptr) { | ||
| SHERPA_ONNX_LOGE("Failed to find class com/k2fsa/sherpa/onnx/WaveData"); | ||
| return nullptr; | ||
| } | ||
|
|
||
| // Get constructor: WaveData(float[] samples, int sampleRate) | ||
| jmethodID ctor = env->GetMethodID(cls, "<init>", "([FI)V"); | ||
| if (ctor == nullptr) { | ||
| SHERPA_ONNX_LOGE("Failed to get WaveData constructor"); | ||
|
|
||
| env->DeleteLocalRef(cls); | ||
| return nullptr; | ||
| } | ||
|
|
||
| // Create WaveData object | ||
| jobject obj = env->NewObject(cls, ctor, samples_arr, sampling_rate); | ||
|
|
||
| env->SetObjectArrayElement(obj_arr, 0, samples_arr); | ||
| env->SetObjectArrayElement(obj_arr, 1, NewInteger(env, sampling_rate)); | ||
| // Clean up local refs | ||
| env->DeleteLocalRef(samples_arr); | ||
| env->DeleteLocalRef(cls); |
There was a problem hiding this comment.
Harden allocation/cleanup in WaveData construction.
samples_arr is created before the class/ctor lookups; on failures it isn’t released, and there’s no null guard for NewFloatArray. Add a null check and clean up samples_arr on early returns (and after NewObject failure).
🔧 Proposed fix
jfloatArray samples_arr = env->NewFloatArray(samples.size());
+ if (samples_arr == nullptr) {
+ SHERPA_ONNX_LOGE("Failed to allocate samples array");
+ return nullptr;
+ }
env->SetFloatArrayRegion(samples_arr, 0, samples.size(), samples.data());
jclass cls = env->FindClass("com/k2fsa/sherpa/onnx/WaveData");
if (cls == nullptr) {
SHERPA_ONNX_LOGE("Failed to find class com/k2fsa/sherpa/onnx/WaveData");
+ env->DeleteLocalRef(samples_arr);
return nullptr;
}
jmethodID ctor = env->GetMethodID(cls, "<init>", "([FI)V");
if (ctor == nullptr) {
SHERPA_ONNX_LOGE("Failed to get WaveData constructor");
+ env->DeleteLocalRef(samples_arr);
env->DeleteLocalRef(cls);
return nullptr;
}
jobject obj = env->NewObject(cls, ctor, samples_arr, sampling_rate);
+ if (obj == nullptr) {
+ env->DeleteLocalRef(samples_arr);
+ env->DeleteLocalRef(cls);
+ return nullptr;
+ }
env->DeleteLocalRef(samples_arr);
env->DeleteLocalRef(cls);🤖 Prompt for AI Agents
In `@sherpa-onnx/jni/wave-reader.cc` around lines 30 - 51, The WaveData
construction path doesn't guard or clean up the samples_arr allocation and
doesn't check NewObject for null; ensure you check the result of
env->NewFloatArray (samples_arr) for nullptr and return after logging while
calling env->DeleteLocalRef(samples_arr) if allocated, and also check
env->NewObject (obj) for nullptr and on failure delete both samples_arr and cls
before returning; in short, add null checks for NewFloatArray and NewObject, and
call env->DeleteLocalRef for samples_arr and cls on every early return (and
after success) so no local refs leak (refer to samples_arr, env->NewFloatArray,
cls, ctor, env->NewObject, and env->DeleteLocalRef).
Summary by CodeRabbit
API Improvements
New Features
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.