-
Notifications
You must be signed in to change notification settings - Fork 118
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
Add windows cpp sample #112
Conversation
完成したので、レビューをお願いします。 |
いまVS2022が手元にないのでとりあえずVS2019 (v142)でビルドしてみましたが、core.dllの序数5が見つからないといわれて実行できませんでした。多分こっちのローカル環境がごちゃごちゃしてるせいなのでもうちょっと調べてみますが、@shigobuさんなにか心当たりありますか? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SHOULD] 実行までの手順をまとめたREADME(具体的にはこのPRの冒頭の内容)をexample/cpp/windows/のところに欲しいです。
[QUESTION] あとcore.libについてはどうしましょうか。releasesに含めるべき(cc. @Hiroshiba )か dllからlibを生成するバッチファイル を復活させるか...
動的なロードならlibファイルは要らないんでしたっけ?この選択肢はこういったVC++プロジェクトでもアリなんでしょうか
すみません。詳しいレビューはもう少し待ってください 🙇
.libに関しては #111 でRelease対象に含まれるようになったため問題ないと思います |
initialize関数の定義がRelease(0.12.0-preview.1)に上がっているものから変更になっています。 心当たりがあるといえばこのくらいです。 |
README追加しました。 |
この件解決しました。core.dllのビルドに使っていたonnxruntimeとsimple_ttsが読み込んでいたonnxruntimeのバージョンが一致していませんでした。 確認ですが、
|
あっています。
NuGetでダウンロードした |
私の環境では正常にダミー音声が再生されたので、そのままにしてたのですが、 |
お願いします 🙇 将来にわたってこのexampleのバージョンを揃える必要があるので出来ればこのNuGetなしで完結するのがいいんですが、その作業は製品版にDirectMLが同梱されてからでよさそうですかね |
…oicevox_core into add_windows_cpp_sample
OnnxRuntimeのバージョンを1.10.0に変更しました。 |
すみません。対応してもらって申し訳ないのですが、versionを揃えてもまだ序数のエラーが出てしまいました。 いまonnxruntime.dllのexportsを確認したところ、python configure.pyで拾ってくるものとNuGetが拾ってくるものが違っているようでした。もう少し調べてみます。 dumpbin /exports の結果
|
すみませんずっと勘違いしていてDirectML版のcore.dllを使っていました。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
日本語パスでも動作を確認しました!
いくつかコメント残したので対応お願いします。
int64_t speaker_id = 0; | ||
int output_binary_size = 0; | ||
uint8_t* output_wav = nullptr; | ||
result = voicevox_tts(wide_to_utf8_cppapi(speak_words).c_str(), speaker_id, &output_binary_size, &output_wav); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SHOULD] 現在のcore.dll実装には voicevox_tts
に長すぎる文字列を与えるとbuffer overflowでクラッシュするというバグが存在するので、入力する文字列のbyte数に制限を与えてほしいです。
VOICEVOX/voicevox_engine#384 ここの議論にあるように2560文字くらいにするのがよさそうです。
[MAY] voicevox_tts
は std::runtime
例外が飛びうる(あと多分bad_allocも)のでcatchしてほしいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これ思ったんですがどっちもcore実装の方を直すべきですかね...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これ思ったんですがどっちもcore実装の方を直すべきですかね...
そうだと思います! #93 の時点で自分も #93 (comment) のように質問したことがあったのですが、例外処理を core 内で完結させる作業は別 PR に分けたほうが良さそうという意見に落ち着き、それ以来これについて特に進捗はない、というステートにある感じな気がします。なのでサンプル側で catch する必要はなくて、今後 core 側での作業が必要そうです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
とりあえず、現状維持しときます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あ、もし余力があれば、サンプルコードがビルドできるかのテストCIをGithub Actionsで回しておくと良いかもとちょっと思いました!
たぶんcd example/cpp/windows/simple_tts
してmsbuild
をいい感じに実行すればOKかなと・・・!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
実装いただきありがとうございます
遅れましたが確認させていただきました
こちらの方で再生に関して問題がありましたが、それ以外に関しては問題ないと思います
再生の件についてはよくわかりませんでした…
とりあえず一旦Request changesにしますが、他の方で同様の事例がなさそうであればApproveします
# Conflicts: # .github/workflows/build.yml
build.ymlにjobを追加する形で良いでしょうか? 私のリポジトリでブランチ分けて試しに作ってみたところ、ビルドに成功しました。 |
良いと思います!! |
build.ymlに、サンプルコードをビルドするjobを追加しました。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
内容
Windowsで動くC++用のサンプルコードを追加します。
Microsoft Visual Studio Community 2022 で作成しています。
core.dllを暗黙的リンクするために、core.libが必要です。
自分のリポジトリでbuild.ymlを編集してlibファイルを作成するようにして使っています。後でPR出します。
出力ディレクトリにcore.dllとOpen JTalk辞書フォルダの配置が必要です。
lib\x64(or Win32)にcore.libを配置する必要があります。
出力ディレクトリとlibディレクトリはビルドすると生成されます。
OnnxRuntimeはNuGetで取得します。
関連 Issue
VOICEVOX/voicevox_project#1
その他