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

styleIdとsession.runに渡す数値が異なっているVVMでも音声合成できるようにする #551

Conversation

Hiroshiba
Copy link
Member

内容

の解決です。

manifest.jsonにマッピング情報を記載し、id_relationsに格納して値を取り回すようにしました。

InferenceCoreとStatusの役割がちょっとごちゃごちゃになっている印象を受けました。
VvmModelの情報をとりあえずStatusに持たせてみています。
ちょっとどういう設計にすればいいのかはパッと思いつけませんでした。

関連 Issue

fix #548

その他

crates/voicevox_core/src/voice_model.rs Outdated Show resolved Hide resolved
Comment on lines 235 to 239
pub fn predict_duration_session_run(
&self,
style_id: StyleId,
model_id: &VoiceModelId,
inputs: Vec<&mut dyn AnyArray>,
) -> Result<Vec<f32>> {
Copy link
Member Author

Choose a reason for hiding this comment

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

ここはスタイルIDではなくモデルIDを渡す設計の方が正しそうだったのでそうしました

VOICEVOX_RESULT_INVALID_MODEL_INDEX_ERROR => "無効なmodel_indexです\0",
VOICEVOX_RESULT_INVALID_MODEL_ID_ERROR => "無効なmodel_indexです\0",
Copy link
Member Author

@Hiroshiba Hiroshiba Jul 27, 2023

Choose a reason for hiding this comment

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

Model IndexではなくModel Idになったけど、このエラーメッセージが1回も使われてなかったのでスルーされてました

crates/voicevox_core/src/voice_model.rs Outdated Show resolved Hide resolved
@qryxip
Copy link
Member

qryxip commented Jul 28, 2023

今CIが落ちていますが、直しかたは次のようになると思います。

  • test-workflow / rust-integration-test

    crates/voicevox_core_c_api/tests/e2e/snapshots.tomlの、compatible_engine.metasの更新

  • test workflow / c-header

    cargo xtask update-c-header

crates/voicevox_core/src/voice_model.rs Show resolved Hide resolved
crates/voicevox_core/src/voice_model.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/manifest.rs Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member Author

mainに追従して、エラーメッセージがおかしかったのと、ドキュメント的にこっちのがちょっといいかなと思って修正しました e7896bb

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です!

#[derive(Deserialize, Getters, Clone)]
pub struct Manifest {
manifest_version: ManifestVersion,
metas_filename: String,
decode_filename: String,
predict_duration_filename: String,
predict_intonation_filename: String,
#[serde(default)]
style_id_to_model_inner_id: BTreeMap<StyleId, ModelInnerId>,
Copy link
Member

@qryxip qryxip Aug 2, 2023

Choose a reason for hiding this comment

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

今言うのもなんですが、"model_inner_id"という名前に欠点があるとすれば、声(voice)を指すことが少しだけわかりずらくなっているというのがあるかなと思いました。

"speaker_id"という表現はもうcompatible_engine以外で使っていないので、"true_speaker_id"とかにするというのもアリなんじゃないかと思いました。

Copy link
Member Author

Choose a reason for hiding this comment

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

確かに言われておっしゃる通りなかなか分かりにくくなっているなと思いました。

大事なのはmodel_innerの部分だと思っていて、例えば同じ声を作れるものが、別のモデルでは別のIDになっていることもなくはない感じです。
speakerは声なのか話者なのか一意に定まらないので、やるとしたらmodel_inner_voice_idあたりなのかな~と思いました。

とりあえずこの値はここでしか使われていないから、一旦このままでも通じるのかなと思いました!
ただ分かりにくいのはおっしゃる通りだと思うので、いつか変更する場合はたぶん賛成できます。

Copy link
Member

Choose a reason for hiding this comment

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

ああ言われてみればモデルごとに違ってもいいんですね。
他にあるとすれば...local_voice_idとか...?

Copy link
Member Author

Choose a reason for hiding this comment

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

確かにmodel含めなくてもいいかもですね! inner_voice_idとかもありかもです。

@Hiroshiba
Copy link
Member Author

レビュー本当にありがとうございます!!!
マージします!!

@Hiroshiba Hiroshiba merged commit e0d32a5 into VOICEVOX:main Aug 3, 2023
31 checks passed
@Hiroshiba Hiroshiba deleted the styleIdとsession.runに渡す数値が異なっているVVMでも音声合成できるようにする branch August 3, 2023 22:19
@qryxip qryxip mentioned this pull request Aug 9, 2023
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.

[vvm] styleIdとsession.runに渡す数値が異なっているVVMでも音声合成できるようにする
3 participants