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

Streaming decoder for compatible engine #875

Merged
merged 6 commits into from
Nov 26, 2024

Conversation

Yosshi999
Copy link
Contributor

内容

ストリーミング処理をC APIとして実装

testcaseとして一括変換・ストリーム変換の二つの生成結果を追加し、元の生成音声と比較して十分近いことを確かめた

関連 Issue

close #866

@Yosshi999 Yosshi999 marked this pull request as ready for review November 20, 2024 15:57

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated no suggestions.

Files not reviewed (1)
  • crates/test_util/compatible_engine.h: Language not supported
Comments skipped due to low confidence (2)

crates/voicevox_core_c_api/tests/e2e/testcases/compatible_engine.rs:87

  • [nitpick] The variable name 'wave2' is ambiguous. It should be renamed to 'intermediate_wave'.
let wave2 = {

crates/voicevox_core_c_api/tests/e2e/testcases/compatible_engine.rs:116

  • [nitpick] The variable name 'wave3' is ambiguous. It should be renamed to 'chunked_wave'.
let wave3 = {
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

ほぼLGTMです!!!
音声ファイルの確認もありがとうございます!!!

すみません、ちょっと将来の互換性も考えてAPI のシグネチャをいくつか提案させていただきました! 🙇

たぶん現状margin_widthを一切使わないことがありそうですが、コンパイルエラーとか出そうでしょうか・・・・・・?

crates/voicevox_core_c_api/src/compatible_engine.rs Outdated Show resolved Hide resolved
crates/voicevox_core_c_api/src/compatible_engine.rs Outdated Show resolved Hide resolved
crates/voicevox_core_c_api/src/compatible_engine.rs Outdated Show resolved Hide resolved
Comment on lines 386 to 385
let margin_width = margin_width as usize;
let feature_dim = feature_dim as usize;
const MARGIN_WIDTH: usize = 14;
const FEATURE_SIZE: usize = 80;
Copy link
Member

@Hiroshiba Hiroshiba Nov 25, 2024

Choose a reason for hiding this comment

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

こことその下に書かれてるアサート処理はこの関数に独自に追加された処理ですが、個人的にはそのままでもだけしてもどちらでも良さそうに思っています。
@qryxip さんにお任せできれば!

Copy link
Member

@qryxip qryxip Nov 25, 2024

Choose a reason for hiding this comment

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

私がsuggestしたやつですね。他の関数も後でこれで統一しようかなと。

ちなみにassert!じゃなくif … { panic!(…); }としたのは、私の理解では出力サイズはONNXモデル次第だからですね。Rust API側での保証はしてなかったはず?

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!!

ありがとうございました!!!

@qryxip
Copy link
Member

qryxip commented Nov 25, 2024

LGTM、なのですがCIがちょうど色々ぶっ壊れてますね。
今日の9:00(日本時間)までmacos-12が動かないのと、rustdocが動かないのと、ビルド自体も壊れてるのがあるという…

@qryxip
Copy link
Member

qryxip commented Nov 26, 2024

上記の三点は直ったのですが、別の理由でうっかりmainブランチのCIを壊してしまったので #877 を先にマージしたく…

Copy link
Member

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

LGTM!

@qryxip qryxip merged commit a5745c2 into VOICEVOX:main Nov 26, 2024
29 checks passed
qryxip added a commit to qryxip/voicevox_core that referenced this pull request Nov 30, 2024
qryxip added a commit that referenced this pull request Dec 1, 2024
#875 にて `extern "C" fn generate_full_intermediate`の中で定義された二つ
の定数について、以下の変更を加える。

`MARGIN_WIDTH`: Rust APIのものを持って来るようにすることで重複を避ける
`FEATURE_SIZE`: 出力サイズのチェック時に失敗したとき、`ncols`が
                `FEATURE_SIZE`と異なるならパニックメッセージを別のものに
                する

Refs: #875
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.

ストリーミング処理のC API実装
4 participants