Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow modify model config at decode time for ASR #1124

Conversation

iprovalo
Copy link
Contributor

Allow modify model config at decode time for ASR.

#1116

Also, exposes the language which was recognized by whisper to the calling client in the SherpaOnnxOfflineRecognizerResult.

Allow modify model config at decode time for ASR.

k2-fsa#1116
@@ -491,7 +496,9 @@ SHERPA_ONNX_API void DecodeMultipleOfflineStreams(
SHERPA_ONNX_API typedef struct SherpaOnnxOfflineRecognizerResult {
const char *text;

// Pointer to continuous memory which holds timestamps
const char *lang;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the newly added member, please add it as the last field.

That is, move line 499 to line 529

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Addressing PR comments
@@ -308,8 +308,27 @@ struct SherpaOnnxOfflineStream {
: impl(std::move(p)) {}
};

sherpa_onnx::OfflineRecognizerConfig convertConfig(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sherpa_onnx::OfflineRecognizerConfig convertConfig(
static sherpa_onnx::OfflineRecognizerConfig convertConfig(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


return recognizer;
void SetSherpaOnnxOfflineRecognizerConfig(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void SetSherpaOnnxOfflineRecognizerConfig(
void SherpaOnnxOfflineRecognizerSetConfig(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

char *c_lang = new char[lang.size() + 1];
std::copy(lang.begin(), lang.end(), c_lang);
c_lang[lang.size()] = '\0';
r->lang = c_lang;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to use

delete[] r->lang;

to avoid memory leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator

@csukuangfj csukuangfj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Left some minor comments. Otherwise, it looks good to me.

Addressing PR comments.
@iprovalo
Copy link
Contributor Author

@csukuangfj I addressed the comments. Thank you for the suggestions!

@csukuangfj
Copy link
Collaborator

Thank you! We will merge it once the CI gets passed.

(Don't worry about the failed CI tests.)

@csukuangfj csukuangfj merged commit de04b3b into k2-fsa:master Jul 13, 2024
174 of 185 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants