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

refactor: Synthesizerの実装をInner<_, A: Async>の形にする #865

Merged

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Oct 30, 2024

内容

#831 で残した次のTODOのうち、Synthesizerについてだけ解決します。

// TODO: `VoiceModelFile`のように、次のような設計にする。
//
// ```
// pub(crate) mod blocking {
//     pub struct Synthesizer(Inner<SingleTasked>);
//     // …
// }
// pub(crate) mod nonblocking {
//     pub struct Synthesizer(Inner<BlockingThreadPool>);
//     // …
// }
// ```

#687 のようなことを行うのを円滑にする目的です。今後ソングとかストリーミングとかでSynthesizerに手を入れることが予定されているので、その前に早めにやった方がよいと思った次第です。

なんかパフォーマンス的にはもしかしたら破壊的になってるかもですが、もしそうでもパフォーマンスだけならということでcommit typeはrefactor:としてしまいました。

関連 Issue

Refs: #831, #687

その他

そこそこ大きく手を入れているので、今度こそスナップショットテストを作った方がよいかも?

@qryxip qryxip requested a review from Hiroshiba October 30, 2024 17:15
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.

たしかにスナップショット導入できると嬉しいかもではありますね!!

エンジンリポジトリの方では、作った音声を有効数字何桁か絶対値何桁かでroundしたバイナリをハッシュ化してスナップショットにしてます。
たぶんosを限定するか、osごとにスナップショット取れば完全一致する気がします。

けどまあ、このprくらいなら気合いで目でチェックも不可能ではなさそうなのと、あとは @Yosshi999 さんが作ってくださったpython用チェックコードを手元で動かしてみるとかでも良さそう!!

@qryxip qryxip changed the title refactor: Synthesizerの実装をInner<_, impl Async>に集約する refactor: Synthesizerの実装をInner<_, impl Async>の形する Oct 31, 2024
@qryxip qryxip changed the title refactor: Synthesizerの実装をInner<_, impl Async>の形する refactor: Synthesizerの実装をInner<_, impl Async>の形にする Oct 31, 2024
@qryxip qryxip changed the title refactor: Synthesizerの実装をInner<_, impl Async>の形にする refactor: Synthesizerの実装をInner<_, A: Async>の形にする Oct 31, 2024
@qryxip
Copy link
Member Author

qryxip commented Oct 31, 2024

けどまあ、このprくらいなら気合いで目でチェックも不可能ではなさそうなのと、あとは @Yosshi999 さんが作ってくださったpython用チェックコードを手元で動かしてみるとかでも良さそう!!

mainと比較しましたが、Yosshi999さんのスクリプトを使うまでもなくWAVが完全一致しました。
(というのも、処理自体は一切変わっていないはずなので)

diff {a,b}.wav; echo $?
0

@qryxip qryxip requested a review from Hiroshiba October 31, 2024 06:13
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です!

ちょっとSynthesizerがだいぶリッチになってきたというか、内部用の処理を持ち始めてるかもと感じました。
Synthesizer内でndarrayが現れないようにするときれいに分離できるかも。
(まあcompatible engine側との都合もあるかもですが)

@qryxip
Copy link
Member Author

qryxip commented Nov 1, 2024

割とありかなと思います。synthesizer.rsとは別ファイルの、engine/synthesis.rsとかでimpl Status<_>の延長(あるいはtrait StatusExt)を書くという形がよいかなと。実質的な「層」にはなるかもですが、Synthesizerから直接Statusが透けて見える分認知負荷は抑えられるはず?

@qryxip qryxip requested a review from Hiroshiba November 2, 2024 07:56
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 Author

qryxip commented Nov 2, 2024

c941504 (#865)を入れてしまいましたが、マージしちゃおうと思います!

@qryxip qryxip merged commit a8131d9 into VOICEVOX:main Nov 2, 2024
29 checks passed
@qryxip qryxip deleted the refactor-introduce-inner-type-for-synthesizer branch November 2, 2024 15:49
qryxip added a commit that referenced this pull request Dec 3, 2024
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.

2 participants