feat: add SetOption/GetOption CXX wrapper - #3309
Conversation
dosubot
Bot
added
the
size:L
📝 WalkthroughWalkthroughThe changes extend sherpa-onnx's C and C++ APIs to support per-stream runtime options and final-chunk signaling for Paraformer streaming. New functions enable getting/setting options on both online and offline streams with underlying storage via unordered maps. Paraformer decoding logic is enhanced to handle final chunks through modified readiness checks, adjusted chunk sizing, and CIF tail-flush operations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant OnlineStream
participant Impl
participant Paraformer
Client->>OnlineStream: SetParaformerFinalChunk(true)
OnlineStream->>Impl: SetParaformerFinalChunk(true)
Impl->>Impl: paraformer_is_final_ = true
Client->>OnlineStream: DecodeStream()
OnlineStream->>Paraformer: IsReady()?
Paraformer->>OnlineStream: Check IsParaformerFinalChunk()
OnlineStream-->>Paraformer: true (final chunk mode)
Paraformer-->>OnlineStream: true (ready despite < chunk_size)
OnlineStream->>Paraformer: DecodeStream(final_chunk=true)
Paraformer->>Paraformer: Adjust chunk_size for remaining frames
Paraformer->>Paraformer: CIF processing with tail-flush check
alt Final chunk && integrate >= threshold
Paraformer->>Paraformer: Flush residual token
Paraformer->>Paraformer: Reset integrate & hidden state
end
Paraformer->>Paraformer: Skip special tokens (0,1,2)
Paraformer-->>OnlineStream: Decoded tokens
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility and robustness of stream handling in the Highlights
Changelog
Activity
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 introduces SetOption/GetOption methods for OnlineStream and OfflineStream in the C++ wrapper, along with the underlying C-API and implementation changes. It also adds logic to handle final chunks in the streaming Paraformer model.
The changes are generally good, but I have identified a few areas for improvement:
- There is a critical issue where
std::stoiandstd::stofare used without exception handling, which could lead to crashes if an option has an invalid format. - There is some implementation inconsistency where a specific
paraformer_is_final_flag is introduced alongside the genericoptions_map, while the documentation suggests using the generic mechanism. - The option handling logic (
GetOptionInt,GetOptionFloat, etc.) is duplicated betweenOnlineStream::ImplandOfflineStream::Impl. This duplicated utility code should be moved to a common file to improve reusability and maintainability, aligning with repository rules.
| int32_t GetOptionInt(const std::string &key, int32_t default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stoi(it->second); | ||
| } | ||
| return default_value; | ||
| } | ||
|
|
||
| float GetOptionFloat(const std::string &key, float default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stof(it->second); | ||
| } | ||
| return default_value; | ||
| } |
There was a problem hiding this comment.
The std::stoi and std::stof functions will throw an exception (e.g., std::invalid_argument or std::out_of_range) if the string value from the options map cannot be converted to a number. This unhandled exception will cause the program to crash.
Please add try-catch blocks to handle potential conversion errors gracefully and return the default value.
int32_t GetOptionInt(const std::string &key, int32_t default_value) const {
auto it = options_.find(key);
if (it != options_.end()) {
try {
return std::stoi(it->second);
} catch (const std::exception &) {
// You may want to log a warning here
return default_value;
}
}
return default_value;
}
float GetOptionFloat(const std::string &key, float default_value) const {
auto it = options_.find(key);
if (it != options_.end()) {
try {
return std::stof(it->second);
} catch (const std::exception &) {
// You may want to log a warning here
return default_value;
}
}
return default_value;
}| int32_t GetOptionInt(const std::string &key, int32_t default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stoi(it->second); | ||
| } | ||
| return default_value; | ||
| } | ||
|
|
||
| float GetOptionFloat(const std::string &key, float default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stof(it->second); | ||
| } | ||
| return default_value; | ||
| } |
There was a problem hiding this comment.
The std::stoi and std::stof functions will throw an exception (e.g., std::invalid_argument or std::out_of_range) if the string value from the options map cannot be converted to a number. This unhandled exception will cause the program to crash.
Please add try-catch blocks to handle potential conversion errors gracefully and return the default value.
int32_t GetOptionInt(const std::string &key, int32_t default_value) const {
auto it = options_.find(key);
if (it != options_.end()) {
try {
return std::stoi(it->second);
} catch (const std::exception &) {
// You may want to log a warning here
return default_value;
}
}
return default_value;
}
float GetOptionFloat(const std::string &key, float default_value) const {
auto it = options_.find(key);
if (it != options_.end()) {
try {
return std::stof(it->second);
} catch (const std::exception &) {
// You may want to log a warning here
return default_value;
}
}
return default_value;
}| bool paraformer_is_final_ = false; | ||
| std::unordered_map<std::string, std::string> options_; |
There was a problem hiding this comment.
The paraformer_is_final_ flag seems redundant now that a generic options_ map is available. This is also inconsistent with the documentation for SetOption which uses "is_final" as an example.
To improve consistency and avoid redundancy, I suggest removing paraformer_is_final_ and implementing SetParaformerFinalChunk() and IsParaformerFinalChunk() using the options_ map.
For example:
// In OnlineStream::Impl
void SetParaformerFinalChunk(bool is_final) {
options_["is_final"] = is_final ? "true" : "false";
}
bool IsParaformerFinalChunk() const {
auto it = options_.find("is_final");
if (it != options_.end()) {
return it->second == "true";
}
return false;
}
This change would make the implementation consistent with the documentation and unify the two mechanisms for setting stream options.
| int32_t GetOptionInt(const std::string &key, int32_t default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stoi(it->second); | ||
| } | ||
| return default_value; | ||
| } | ||
|
|
||
| float GetOptionFloat(const std::string &key, float default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stof(it->second); | ||
| } | ||
| return default_value; | ||
| } |
There was a problem hiding this comment.
The option handling logic, specifically GetOptionInt and GetOptionFloat methods, is duplicated between OfflineStream::Impl and OnlineStream::Impl. To adhere to repository guidelines and improve code reusability and maintainability, these utility functions should be extracted into a common utility file (e.g., options-utils.h and options-utils.cc) and reused by both implementations.
References
- Move duplicated utility functions, such as
Trim, to a common utility file (e.g.,text-utils.handtext-utils.cc) for reuse across the codebase.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sherpa-onnx/c-api/c-api.cc`:
- Around line 359-367: Add null-pointer checks and avoid returning pointers to
temporaries: in SherpaOnnxOnlineStreamSetOption and
SherpaOnnxOnlineStreamGetOption validate that stream is non-null and treat
key/value as empty strings when NULL (e.g., std::string k = key ? key : "";
std::string v = value ? value : "";), call stream->impl->SetOption(k, v) for
SetOption, and for GetOption call std::string out = stream->impl->GetOption(k)
and return a heap-allocated copy (e.g., strdup(out.c_str()) or the project's
string-copy helper) instead of returning out.c_str() from the temporary; apply
the same defensive null-check and return-copy pattern to
SherpaOnnxOfflineStreamSetOption and SherpaOnnxOfflineStreamGetOption.
- Around line 364-367: SherpaOnnxOnlineStreamGetOption currently returns
stream->impl->GetOption(key).c_str(), which can dangle when the stream is
destroyed or when SetOption mutates the underlying map; change it to return an
allocated copy (like strdup/malloc + memcpy) of the string so the C API owns
stable storage (matching patterns such as
SherpaOnnxGetOnlineStreamResultAsJson), and update the header comment for
SherpaOnnxOnlineStreamGetOption to document that the caller is responsible for
freeing the returned char* (or alternatively document lifetime semantics if you
choose to return an internal pointer instead). Ensure you reference
SherpaOnnxOnlineStreamGetOption, the impl::GetOption method, and SetOption when
making the change so the allocation and lifetime contract are consistent across
the C API.
In `@sherpa-onnx/c-api/c-api.h`:
- Around line 394-402: The doc comment for SherpaOnnxOnlineStreamSetOption
incorrectly uses "is_final" as an example (which Paraformer reads from a
dedicated flag, not this option map); update the example to a valid generic
per-stream option (e.g., "language" or "context" or another real option your
recognizer actually reads) or remove the specific example entirely so it does
not imply calling SherpaOnnxOnlineStreamSetOption will trigger final-chunk
behavior; keep references to SherpaOnnxOnlineStreamSetOption and
SherpaOnnxCreateOnlineStream so readers can locate the API.
In `@sherpa-onnx/c-api/cxx-api.h`:
- Around line 182-183: Change the API to avoid returning borrowed C strings and
to use std::string parameters: update SetOption(const char *key, const char
*value) to SetOption(const std::string &key, const std::string &value) and
change GetOption(const char *key) const to return std::string (not const char*)
so the caller owns the returned data; apply the same changes for the
OfflineStream variants (the other SetOption/GetOption declarations) and ensure
implementations copy/construct std::string from internal storage rather than
returning pointers into internal containers.
In `@sherpa-onnx/csrc/offline-stream.cc`:
- Around line 243-257: GetOptionInt and GetOptionFloat can throw from
std::stoi/std::stof and must not let exceptions escape into C callers; wrap the
calls to std::stoi and std::stof in try/catch blocks inside GetOptionInt and
GetOptionFloat, catch std::invalid_argument and std::out_of_range (optionally
std::exception as a fallback), and return default_value when parsing fails; keep
the same lookup via options_ and only attempt parsing if the key exists,
otherwise return default_value as before.
In `@sherpa-onnx/csrc/online-recognizer-paraformer-impl.h`:
- Around line 240-246: The current branch advances GetNumProcessedFrames() by
actual_chunk_size whenever s->IsParaformerFinalChunk(), which erroneously
removes the 1-frame overlap for final-chunk decodes that are actually full-size;
change the logic in online-recognizer-paraformer-impl.h so that
GetNumProcessedFrames() is increased by actual_chunk_size only when the final
chunk is truly short (actual_chunk_size < chunk_size_), otherwise continue to
advance by chunk_size_ - 1; update the branch around
s->IsParaformerFinalChunk(), actual_chunk_size, chunk_size_, and
GetNumProcessedFrames() to reflect this conditional behavior.
In `@sherpa-onnx/csrc/online-stream.cc`:
- Around line 159-172: GetOptionInt and GetOptionFloat currently call
std::stoi/std::stof directly and will throw exceptions for malformed or
out-of-range strings, so wrap the conversions in try/catch blocks that catch
std::invalid_argument and std::out_of_range (or std::exception) and return
default_value on error; specifically, in GetOptionInt(const std::string &key,
int32_t default_value) and GetOptionFloat(const std::string &key, float
default_value) check options_.find(key) as before, then attempt the
std::stoi/std::stof call inside a try block and return the parsed value on
success, but return default_value in the catch handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8737847-a1a3-4750-bb06-473bce0b62de
📒 Files selected for processing (11)
sherpa-onnx/c-api/c-api.ccsherpa-onnx/c-api/c-api.hsherpa-onnx/c-api/cxx-api.ccsherpa-onnx/c-api/cxx-api.hsherpa-onnx/c-api/sherpa-onnx-symbols-c.expsherpa-onnx/csrc/offline-stream.ccsherpa-onnx/csrc/offline-stream.hsherpa-onnx/csrc/online-recognizer-paraformer-impl.hsherpa-onnx/csrc/online-stream.ccsherpa-onnx/csrc/online-stream.hsherpa-onnx/python/csrc/online-stream.cc
| void SherpaOnnxOnlineStreamSetOption(const SherpaOnnxOnlineStream *stream, | ||
| const char *key, const char *value) { | ||
| stream->impl->SetOption(key, value); | ||
| } | ||
|
|
||
| const char *SherpaOnnxOnlineStreamGetOption( | ||
| const SherpaOnnxOnlineStream *stream, const char *key) { | ||
| return stream->impl->GetOption(key).c_str(); | ||
| } |
There was a problem hiding this comment.
Missing null-pointer checks for string parameters.
The SetOption and GetOption functions pass key and value directly to std::string constructors. If a C caller passes NULL, this is undefined behavior and will likely crash.
Consider adding null checks consistent with other functions in this file:
🛡️ Proposed fix for null safety
void SherpaOnnxOnlineStreamSetOption(const SherpaOnnxOnlineStream *stream,
const char *key, const char *value) {
+ if (!stream || !key || !value) {
+ return;
+ }
stream->impl->SetOption(key, value);
}
const char *SherpaOnnxOnlineStreamGetOption(
const SherpaOnnxOnlineStream *stream, const char *key) {
+ if (!stream || !key) {
+ return "";
+ }
return stream->impl->GetOption(key).c_str();
}Apply the same pattern to SherpaOnnxOfflineStreamSetOption and SherpaOnnxOfflineStreamGetOption.
Also applies to: 671-679
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/c-api/c-api.cc` around lines 359 - 367, Add null-pointer checks
and avoid returning pointers to temporaries: in SherpaOnnxOnlineStreamSetOption
and SherpaOnnxOnlineStreamGetOption validate that stream is non-null and treat
key/value as empty strings when NULL (e.g., std::string k = key ? key : "";
std::string v = value ? value : "";), call stream->impl->SetOption(k, v) for
SetOption, and for GetOption call std::string out = stream->impl->GetOption(k)
and return a heap-allocated copy (e.g., strdup(out.c_str()) or the project's
string-copy helper) instead of returning out.c_str() from the temporary; apply
the same defensive null-check and return-copy pattern to
SherpaOnnxOfflineStreamSetOption and SherpaOnnxOfflineStreamGetOption.
| const char *SherpaOnnxOnlineStreamGetOption( | ||
| const SherpaOnnxOnlineStream *stream, const char *key) { | ||
| return stream->impl->GetOption(key).c_str(); | ||
| } |
There was a problem hiding this comment.
Returning pointer to internal storage may cause dangling pointer issues.
The GetOption functions return .c_str() on a reference to internal string storage. While this works because the underlying GetOption returns a reference to either a map element or a static empty string, the pointer becomes invalid if:
- The stream is destroyed
- The option is modified via
SetOptionwith the same key
This differs from other C API patterns in this file (e.g., SherpaOnnxGetOnlineStreamResultAsJson) which allocate copies. Consider documenting the lifetime semantics in the header, or allocating a copy for consistency with the rest of the API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/c-api/c-api.cc` around lines 364 - 367,
SherpaOnnxOnlineStreamGetOption currently returns
stream->impl->GetOption(key).c_str(), which can dangle when the stream is
destroyed or when SetOption mutates the underlying map; change it to return an
allocated copy (like strdup/malloc + memcpy) of the string so the C API owns
stable storage (matching patterns such as
SherpaOnnxGetOnlineStreamResultAsJson), and update the header comment for
SherpaOnnxOnlineStreamGetOption to document that the caller is responsible for
freeing the returned char* (or alternatively document lifetime semantics if you
choose to return an internal pointer instead). Ensure you reference
SherpaOnnxOnlineStreamGetOption, the impl::GetOption method, and SetOption when
making the change so the allocation and lifetime contract are consistent across
the C API.
| /// Set a key-value option on an online stream. | ||
| /// This provides a generic mechanism for passing per-stream runtime parameters | ||
| /// to the recognizer (e.g., "is_final" for streaming Paraformer). | ||
| /// | ||
| /// @param stream A pointer returned by SherpaOnnxCreateOnlineStream() | ||
| /// @param key The option name (e.g., "is_final") | ||
| /// @param value The option value (e.g., "true") | ||
| SHERPA_ONNX_API void SherpaOnnxOnlineStreamSetOption( | ||
| const SherpaOnnxOnlineStream *stream, const char *key, const char *value); |
There was a problem hiding this comment.
Don’t use "is_final" as the example option here.
The Paraformer path reads the dedicated final-chunk flag, not the generic option map, so this example suggests an API call that will not trigger the behavior described above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/c-api/c-api.h` around lines 394 - 402, The doc comment for
SherpaOnnxOnlineStreamSetOption incorrectly uses "is_final" as an example (which
Paraformer reads from a dedicated flag, not this option map); update the example
to a valid generic per-stream option (e.g., "language" or "context" or another
real option your recognizer actually reads) or remove the specific example
entirely so it does not imply calling SherpaOnnxOnlineStreamSetOption will
trigger final-chunk behavior; keep references to SherpaOnnxOnlineStreamSetOption
and SherpaOnnxCreateOnlineStream so readers can locate the API.
| void SetOption(const char *key, const char *value) const; | ||
| const char *GetOption(const char *key) const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare text-returning APIs in the C++ wrapper.
rg -n 'const char \*GetOption|std::string ' sherpa-onnx/c-api/cxx-api.hRepository: k2-fsa/sherpa-onnx
Length of output: 5348
🏁 Script executed:
#!/bin/bash
# Find the implementation file for cxx-api.h
fd -t f 'cxx-api\.(cc|cpp)' sherpa-onnx/
# Also check what GetOption and SetOption do
grep -n "GetOption\|SetOption" sherpa-onnx/c-api/cxx-api.h | head -20Repository: k2-fsa/sherpa-onnx
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Let's check the implementation to understand the lifetime concern
# First find the implementation file
find sherpa-onnx -name "*.cc" -o -name "*.cpp" | xargs grep -l "GetOption" 2>/dev/null | head -5Repository: k2-fsa/sherpa-onnx
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Check what the C API returns (the underlying C API that is being wrapped)
grep -n "GetOption\|SetOption" sherpa-onnx/c-api/c-api.h 2>/dev/null | head -20Repository: k2-fsa/sherpa-onnx
Length of output: 309
🏁 Script executed:
#!/bin/bash
# Check the implementation of GetOption and SetOption in cxx-api.cc
grep -n -A 5 "GetOption\|SetOption" sherpa-onnx/c-api/cxx-api.cc | head -60Repository: k2-fsa/sherpa-onnx
Length of output: 902
🏁 Script executed:
#!/bin/bash
# Check the C API implementation to see what it returns
grep -n -A 5 "SherpaOnnxOnlineStreamGetOption\|SherpaOnnxOfflineStreamGetOption" sherpa-onnx/c-api/c-api.cc | head -40Repository: k2-fsa/sherpa-onnx
Length of output: 504
🏁 Script executed:
#!/bin/bash
# Check what stream->impl->GetOption returns in online-stream.cc and offline-stream.cc
grep -n "GetOption" sherpa-onnx/csrc/online-stream.cc | head -5
grep -n "GetOption" sherpa-onnx/csrc/offline-stream.cc | head -5Repository: k2-fsa/sherpa-onnx
Length of output: 750
🏁 Script executed:
#!/bin/bash
# Get the actual implementation
grep -n -A 3 'GetOption.*{' sherpa-onnx/csrc/online-stream.ccRepository: k2-fsa/sherpa-onnx
Length of output: 756
Return owning std::string from GetOption() — this is a lifetime safety issue.
GetOption() currently returns a borrowed const char * pointing to internal map storage that can be invalidated when SetOption() is called (reallocating the options map), when the stream is destroyed, or under concurrent access. This exposes a use-after-free risk to C++ callers. Return std::string to own the data safely.
Also, SetOption() should take const std::string & parameters for consistency with the rest of the API (e.g., CreateStream(const std::string &hotwords)).
Proposed fix
- void SetOption(const char *key, const char *value) const;
- const char *GetOption(const char *key) const;
+ void SetOption(const std::string &key, const std::string &value) const;
+ std::string GetOption(const std::string &key) const;Also applies to: 382–383 (OfflineStream)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/c-api/cxx-api.h` around lines 182 - 183, Change the API to avoid
returning borrowed C strings and to use std::string parameters: update
SetOption(const char *key, const char *value) to SetOption(const std::string
&key, const std::string &value) and change GetOption(const char *key) const to
return std::string (not const char*) so the caller owns the returned data; apply
the same changes for the OfflineStream variants (the other SetOption/GetOption
declarations) and ensure implementations copy/construct std::string from
internal storage rather than returning pointers into internal containers.
| int32_t GetOptionInt(const std::string &key, int32_t default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stoi(it->second); | ||
| } | ||
| return default_value; | ||
| } | ||
|
|
||
| float GetOptionFloat(const std::string &key, float default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stof(it->second); | ||
| } | ||
| return default_value; | ||
| } |
There was a problem hiding this comment.
Unhandled exceptions from std::stoi/std::stof may propagate to C callers.
GetOptionInt and GetOptionFloat use std::stoi and std::stof, which throw std::invalid_argument or std::out_of_range if the stored string is not a valid number. Since these are called through the C API, uncaught exceptions could cause crashes.
Consider catching exceptions and returning the default value:
🛡️ Proposed fix for exception safety
int32_t GetOptionInt(const std::string &key, int32_t default_value) const {
auto it = options_.find(key);
if (it != options_.end()) {
- return std::stoi(it->second);
+ try {
+ return std::stoi(it->second);
+ } catch (const std::exception &) {
+ return default_value;
+ }
}
return default_value;
}
float GetOptionFloat(const std::string &key, float default_value) const {
auto it = options_.find(key);
if (it != options_.end()) {
- return std::stof(it->second);
+ try {
+ return std::stof(it->second);
+ } catch (const std::exception &) {
+ return default_value;
+ }
}
return default_value;
}📝 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.
| int32_t GetOptionInt(const std::string &key, int32_t default_value) const { | |
| auto it = options_.find(key); | |
| if (it != options_.end()) { | |
| return std::stoi(it->second); | |
| } | |
| return default_value; | |
| } | |
| float GetOptionFloat(const std::string &key, float default_value) const { | |
| auto it = options_.find(key); | |
| if (it != options_.end()) { | |
| return std::stof(it->second); | |
| } | |
| return default_value; | |
| } | |
| int32_t GetOptionInt(const std::string &key, int32_t default_value) const { | |
| auto it = options_.find(key); | |
| if (it != options_.end()) { | |
| try { | |
| return std::stoi(it->second); | |
| } catch (const std::exception &) { | |
| return default_value; | |
| } | |
| } | |
| return default_value; | |
| } | |
| float GetOptionFloat(const std::string &key, float default_value) const { | |
| auto it = options_.find(key); | |
| if (it != options_.end()) { | |
| try { | |
| return std::stof(it->second); | |
| } catch (const std::exception &) { | |
| return default_value; | |
| } | |
| } | |
| return default_value; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/csrc/offline-stream.cc` around lines 243 - 257, GetOptionInt and
GetOptionFloat can throw from std::stoi/std::stof and must not let exceptions
escape into C callers; wrap the calls to std::stoi and std::stof in try/catch
blocks inside GetOptionInt and GetOptionFloat, catch std::invalid_argument and
std::out_of_range (optionally std::exception as a fallback), and return
default_value when parsing fails; keep the same lookup via options_ and only
attempt parsing if the key exists, otherwise return default_value as before.
| // For non-final chunks the original code uses chunk_size_ - 1 to create | ||
| // 1-frame overlap. For the final short chunk we consume all frames. | ||
| if (s->IsParaformerFinalChunk()) { | ||
| s->GetNumProcessedFrames() += actual_chunk_size; | ||
| } else { | ||
| s->GetNumProcessedFrames() += chunk_size_ - 1; | ||
| } |
There was a problem hiding this comment.
Keep the 1-frame overlap until the actual short final chunk.
This branch now advances by chunk_size_ for every final-chunk decode, even when a full chunk is still available. That drops the normal overlap on the remaining full chunks in the drain loop and can change the final recognition result.
💡 Suggested fix
- if (s->IsParaformerFinalChunk()) {
+ if (s->IsParaformerFinalChunk() && actual_chunk_size < chunk_size_) {
s->GetNumProcessedFrames() += actual_chunk_size;
} else {
s->GetNumProcessedFrames() += chunk_size_ - 1;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/csrc/online-recognizer-paraformer-impl.h` around lines 240 - 246,
The current branch advances GetNumProcessedFrames() by actual_chunk_size
whenever s->IsParaformerFinalChunk(), which erroneously removes the 1-frame
overlap for final-chunk decodes that are actually full-size; change the logic in
online-recognizer-paraformer-impl.h so that GetNumProcessedFrames() is increased
by actual_chunk_size only when the final chunk is truly short (actual_chunk_size
< chunk_size_), otherwise continue to advance by chunk_size_ - 1; update the
branch around s->IsParaformerFinalChunk(), actual_chunk_size, chunk_size_, and
GetNumProcessedFrames() to reflect this conditional behavior.
| int32_t GetOptionInt(const std::string &key, int32_t default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stoi(it->second); | ||
| } | ||
| return default_value; | ||
| } | ||
|
|
||
| float GetOptionFloat(const std::string &key, float default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stof(it->second); | ||
| } | ||
| return default_value; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does C++ std::stoi/std::stof throw exceptions on malformed or out-of-range input, and which exceptions are thrown?
💡 Result:
Yes.
- Malformed input (i.e., no conversion could be performed, such as the string not starting with a valid number): both
std::stoiandstd::stofthrowstd::invalid_argument. [1][2] - Out-of-range input (the parsed value can’t be represented in the function’s return type, or the underlying C conversion reports range error by setting
errnotoERANGE): both throwstd::out_of_range. Forstoithis is viastrtol/strtoll; forstofviastrtof/strtod/strtold. [1][2]
Note: if there is a valid numeric prefix (e.g. "123abc"), conversion succeeds; you can use the optional pos out-parameter to detect trailing junk. [1][2]
Sources: [1] [2]
🏁 Script executed:
head -n 180 sherpa-onnx/csrc/online-stream.cc | tail -n 30Repository: k2-fsa/sherpa-onnx
Length of output: 872
Wrap type conversions in exception handlers to honor default_value.
SetOption() accepts arbitrary strings, but std::stoi / std::stof throw std::invalid_argument on malformed input and std::out_of_range on values that cannot be represented in the target type. These exceptions propagate uncaught, rendering the default_value parameter useless and causing unexpected crashes.
Suggested fix
int32_t GetOptionInt(const std::string &key, int32_t default_value) const {
auto it = options_.find(key);
if (it != options_.end()) {
+ try {
return std::stoi(it->second);
+ } catch (...) {
+ return default_value;
+ }
}
return default_value;
}
float GetOptionFloat(const std::string &key, float default_value) const {
auto it = options_.find(key);
if (it != options_.end()) {
+ try {
return std::stof(it->second);
+ } catch (...) {
+ return default_value;
+ }
}
return default_value;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/csrc/online-stream.cc` around lines 159 - 172, GetOptionInt and
GetOptionFloat currently call std::stoi/std::stof directly and will throw
exceptions for malformed or out-of-range strings, so wrap the conversions in
try/catch blocks that catch std::invalid_argument and std::out_of_range (or
std::exception) and return default_value on error; specifically, in
GetOptionInt(const std::string &key, int32_t default_value) and
GetOptionFloat(const std::string &key, float default_value) check
options_.find(key) as before, then attempt the std::stoi/std::stof call inside a
try block and return the parsed value on success, but return default_value in
the catch handler.
There was a problem hiding this comment.
Pull request overview
This PR adds a generic per-stream key-value option mechanism (SetOption/GetOption) to both OnlineStream and OfflineStream, exposed through the C++ core, C API, CXX wrapper, and Python bindings. It also adds streaming Paraformer "final chunk" support, enabling short chunk acceptance and CIF tail token flushing for the last audio segment.
Changes:
- Added
SetOption/GetOption/HasOption/GetOptionInt/GetOptionFloatmethods toOnlineStreamandOfflineStreamat all API layers (core C++, C API, CXX wrapper) - Added
SetParaformerFinalChunk/IsParaformerFinalChunkfor streaming Paraformer final chunk handling, including short chunk padding, CIF tail flush, and SOS/EOS token filtering - Exported new C API symbols for macOS
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
sherpa-onnx/csrc/online-stream.h |
Declares new option and paraformer final chunk methods |
sherpa-onnx/csrc/online-stream.cc |
Implements option storage and paraformer final chunk in Impl |
sherpa-onnx/csrc/offline-stream.h |
Declares option methods for OfflineStream |
sherpa-onnx/csrc/offline-stream.cc |
Implements option storage in OfflineStream::Impl |
sherpa-onnx/csrc/online-recognizer-paraformer-impl.h |
Adds final chunk logic: short chunk acceptance, padding, CIF tail flush, SOS/EOS filtering |
sherpa-onnx/c-api/c-api.h |
Declares C API functions for SetFinalChunk, SetOption, GetOption |
sherpa-onnx/c-api/c-api.cc |
Implements C API wrappers |
sherpa-onnx/c-api/cxx-api.h |
Declares SetOption/GetOption on CXX wrapper classes |
sherpa-onnx/c-api/cxx-api.cc |
Implements CXX wrapper forwarding to C API |
sherpa-onnx/c-api/sherpa-onnx-symbols-c.exp |
Exports new symbols for macOS |
sherpa-onnx/python/csrc/online-stream.cc |
Adds Python binding for set_paraformer_final_chunk |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int32_t GetOptionInt(const std::string &key, int32_t default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stoi(it->second); | ||
| } | ||
| return default_value; | ||
| } | ||
|
|
||
| float GetOptionFloat(const std::string &key, float default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stof(it->second); | ||
| } | ||
| return default_value; | ||
| } |
| void SetOption(const char *key, const char *value) const; | ||
| const char *GetOption(const char *key) const; | ||
|
|
| void SetParaformerFinalChunk(bool is_final) { | ||
| paraformer_is_final_ = is_final; | ||
| } | ||
|
|
||
| bool IsParaformerFinalChunk() const { | ||
| return paraformer_is_final_; | ||
| } | ||
|
|
||
| void SetOption(const std::string &key, const std::string &value) { | ||
| options_[key] = value; | ||
| } | ||
|
|
||
| bool HasOption(const std::string &key) const { | ||
| return options_.count(key) != 0; | ||
| } | ||
|
|
||
| const std::string &GetOption(const std::string &key) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return it->second; | ||
| } | ||
| static const std::string kEmpty; | ||
| return kEmpty; | ||
| } | ||
|
|
||
| int32_t GetOptionInt(const std::string &key, int32_t default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stoi(it->second); | ||
| } | ||
| return default_value; | ||
| } | ||
|
|
||
| float GetOptionFloat(const std::string &key, float default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stof(it->second); | ||
| } | ||
| return default_value; | ||
| } |
| int32_t GetOptionInt(const std::string &key, int32_t default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stoi(it->second); | ||
| } | ||
| return default_value; | ||
| } | ||
|
|
||
| float GetOptionFloat(const std::string &key, float default_value) const { | ||
| auto it = options_.find(key); | ||
| if (it != options_.end()) { | ||
| return std::stof(it->second); | ||
| } | ||
| return default_value; | ||
| } |
fc0be49 to
753da15
Compare
dosubot
Bot
added
size:M
|
Please first fix the comments in the PR for C API. After the PR for C API is merged, please update this PR to include only code for C++ API. |
Add SetOption/GetOption methods to the C++ wrapper (cxx-api) for both OnlineStream and OfflineStream, calling the C API functions. Depends on: feat/setoption-c-api Ref: k2-fsa#3101 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
753da15 to
e270553
Compare
|
@csukuangfj Rebased onto latest master (with #3308 merged) and added HasOption CXX wrapper for both OnlineStream and OfflineStream. Could you please review when you have a chance? Thanks! |
csukuangfj
left a comment
csukuangfj
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
Summary
Ref #3101 (Part 1c) — depends on #3308
Add
SetOption/GetOptionmethods to the C++ wrapper (cxx-api) for bothOnlineStreamandOfflineStream.Files Changed (2 files, +22 lines)
cxx-api.hcxx-api.cc🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes