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

c++ api for keyword spotter #642

Merged
merged 4 commits into from
Mar 11, 2024
Merged

c++ api for keyword spotter #642

merged 4 commits into from
Mar 11, 2024

Conversation

xinhecuican
Copy link
Contributor

No description provided.

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.

Thanks! Left some minor comments.

if (config->model_config.debug) {
SHERPA_ONNX_LOGE("%s\n", spotter_config.ToString().c_str());
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add

  if (!spotter_config.Validate()) {
    SHERPA_ONNX_LOGE("Errors in config!");
    return nullptr;
  }

SherpaOnnxKeywordSpotter *spotter, SherpaOnnxOnlineStream **streams,
int32_t n) {
std::vector<sherpa_onnx::OnlineStream*> ss(n);
for(int32_t i=0; i!=n; ++i) {
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
for(int32_t i=0; i!=n; ++i) {
for(int32_t i = 0; i != n; ++i) {

Please use clang-format to format your code.

You can install clang-format with

pip install clang-format


/// timestamps.size() == tokens.size()
/// timestamps[i] records the time in seconds when tokens[i] is decoded.
float* timestamps;
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
float* timestamps;
const float* timestamps;

* }
*/
const char* json;
}SherpaOnnxKeywordResult ;
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
}SherpaOnnxKeywordResult ;
} SherpaOnnxKeywordResult ;

/// @param spotter A pointer returned by CreateKeywordSpotter()
/// @return Return a pointer to an OnlineStream. The user has to invoke
/// DestroyOnlineStream() to free it to avoid memory leak.
SHERPA_ONNX_API SherpaOnnxOnlineStream* CreateKeywordStream(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you prefix every function name with SherpaOnnx?
That is, replace CreateKeywordStream with SherpaOnnxCreateKeywordStream.


I think it was an error to not use SherpaOnnx for online and offline ASR C APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after add this many line exceed 80 char, I tried the style is not uniform
and change api name means must modify code of other project

@csukuangfj
Copy link
Collaborator

Please use the following command to check the code style locally

cd sherpa-onnx
./scripts/check_style_cpplint.sh 2

@csukuangfj
Copy link
Collaborator

This PR has conflicts with #648 since both of them add C API for KWS.

@lovemefan What do you think about this PR?

@lovemefan
Copy link
Contributor

This PR has conflicts with #648 since both of them add C API for KWS.

@lovemefan What do you think about this PR?

ok, I'm going to copy the code of c api from this PR

@csukuangfj
Copy link
Collaborator

This PR has conflicts with #648 since both of them add C API for KWS.
@lovemefan What do you think about this PR?

ok, I'm going to copy the code of c api from this PR

Thanks! I am merging this PR. Please merge the latest master to your branch after we merge this PR.

@csukuangfj
Copy link
Collaborator

@xinhecuican Thank you for your first-time contribution!

@csukuangfj csukuangfj merged commit f43139e into k2-fsa:master Mar 11, 2024
188 of 194 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.

3 participants