-
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
voicevox_coreのクラス設計discussion #280
Comments
できるなら、一緒のほうがいいと思います。 一緒にした場合の懸念点として、どのようにエラーを通知するかが有ると思います。 VoicevoxResultCode voicevox_core_new(VoicevoxCoreInitializeOptions options, VoicevoxCore** voicevox_core); |
自分も同意です。 VoicevoxCore vvc = voicevox_core_new();
if(!voicevox_core_initialize(...))
... のようにすぐ下でinitializeが呼ばれると思うので、それならまとめてもいいんじゃないかと思いました。 (Cは詳しくないのでコードが間違ってて変なことを言ってるかも…) |
@shigobu @sevenc-nanashi そうですね。一緒にやるとするならそのような形になります。 |
結論から言うと、コンストラクタと一緒にしちゃって良いかなと思いました! 一応現状のinitializeがあった方が良い理由もいくつかあります。 まず、デストラクタを呼ばずにmodelを解放(GPUメモリを解放)できるのは需要が無くはないと思います。 あとは同じくデストラクタ呼ばずにCPU・GPU版を切り替えが可能なことです。 一方、initializeを呼ぶ必要があるのはバグや面倒さにつながると思います。 |
となるとRust側もくっつけて良さそうというかすでに new_with_initializeが存在しますね。 |
ところでPyO3実装あたりで特に何も議論せずに |
Sessionという単語はonnxruntime文脈のもので voicevox_coreのユビキタス言語としてはSessionは無いような気がします。 |
どういうクラスになるのかまだ見えていないというのもあるので、いったん |
確かにドメイン設計からやるとなるとかなり時間かかっちゃいそうなので今回はVoicevoxCoreクラス単一で提供して良さそうですね。 |
versionやmetasやOptions系はどうしましょうか? 選択肢としては今のところ次の4つがあると思います。
|
@qryxip この視点だと、metasは関数VoicevoxCoreのインスタンスメソッドか定数が良さそうかなと思いました。 ・・・本当は1つのモデルに1つのクラスインスタンスを対応させるのが良いのかもとちょっと思いました。 |
metas関数はメンバ関数にしたほうが良さそうですね。
これ今のうちに対応するのがベストかもですね。voicevox_core_newを以下のような感じにすると対応できそう? VoicevoxResultCode voicevox_core_new(
uint32_t speaker_id,
VoicevoxCoreInitializeOptions options,
VoicevoxCore** output_voicevox_core); その場合だとdecodeなどの関数はspeaker_idを受け取る必要はなくなりそうですね。 |
あ!とりあえずAPIとしての提供は実際に製品版がloadを使うようになってからでも良いかも・・・?
モデルとspeaker_idは1:1対応じゃないのでspeaker_idは必要そう・・・? いくつかの観点を思いついたので列挙してみます。
|
@Hiroshiba ちょっと思ったんですが、metas関数をメンバー関数にした場合、ユーザーはどうやってコンストラクタ関数に指定するspeaker_idを知ることができるんでしょうか? |
|
@qwerty2501 たぶん指定する必要がないはずです。そのspeaker_idはコンストラクタ内の何に使われる想定でしょうか 👀 |
@Hiroshiba 一つのモデルに一つのクラスインスタンスを対応させるにはspeaker_idを受け取る必要があると思ったんですが違うのでしょうか? |
ちょっと考えたのですが、確かにspeaker_id的なものをどこかで指定する必要があるなと思いました。 いったん整理すると、現状の実装だと
という感じですよね。これを、1モデルに1metas.jsonにしたいなと考えています。 となるとどうにかしてマッピングをしないといけないのですが・・・・・・どうしよう・・・。 |
@Hiroshiba このコメント で以下のコメントがあったので、インスタンスコンストラクト時にモデルを識別できる情報が必要だと考えてそれがcore側のspeaker_idであると思ってました。コンストラクト時にload_modelするべきか是非はあると思いますが私はload_modelもコンストラクト時に行うものだと思っていました。
またload_model時にmetasの内容が変わるとから読み込んだmodelに関連するmetas.jsonに内容が変わると考えてました。
私も1モデルに1metas.jsonになると考えてたのでこのあたりは認識合ってそうですね。
これは私と同じ悩みであると思うのですがいかがでしょうか? |
あとcoreのspeaker_id、整数型だけど他のIDと区別するためにもtype aliasでいいから独自型名で提供したほうが多少はマシになるかもですね。 C API側はtype aliasなので間違えて使われてもコンパイルエラーにさせることはできないですが、Rustサイドはnew typeパターンでコンパイルエラーにさせることはできそう |
ぶった切っちゃってすみません、理想と現状や、話者から声まで多段になってる構造がかなりややこしい気がしたので、いったんまずドメイン知識を整理してみました。
モデルは話者やスタイルと対応関係はないこと、本来話者ユニークになるべき話者ID(speaker_id)が声ユニークになっていることがややこしさの根幹かなと思いました。 これをもとに、一旦現状の相関図みたいなのを絵があると議論しやすそうなのですが・・・なにか便利なツールはないでしょうか・・・ |
mermaid 記法使えるのでこれで関係性を表すのはどうでしょう? classDiagram
Class01 <|-- AveryLongClass : Cool
Class03 *-- Class04
Class05 o-- Class06
Class07 .. Class08
Class09 --> C2 : Where am i?
Class09 --* C3
Class09 --|> Class07
Class07 : equals()
Class07 : Object[] elementData
Class01 : size()
Class01 : int chimp
Class01 : int gorilla
Class08 <--> C2: Cool label
erDiagram
CUSTOMER ||--o{ ORDER : places
ORDER ||--|{ LINE-ITEM : contains
CUSTOMER }|..|{ DELIVERY-ADDRESS : uses
|
サンプル図の2つ目の図が良い感じそうです。ER図、なるほど。
ここは認識のずれがありそうです。 |
@Hiroshiba あ、model IDと声 IDは別にありましたね。
これについて理解度の浅い人間から図を作ってしまうと間違いだらけのものができてしまいそれもとに議論してしまうと最終型の図もreviewがもれてしまい間違った形になってしまうリスクが高そうだなと感じました。現に私は理解していないわけですし。 |
なるほどです。ちょっと頑張って図を作ってみます。 |
図、まとめました!! パワポの下の方に提案の形もあります! |
@Hiroshiba ありがとうございます。よくわかりました。speakerが同じでもmodelが違う場合があるんですね パフォーマンスを考えると一つずつ読み込んだほうが良いですが、一方でユーザーにmodelを意識させてしまうという問題点が存在します。 またコンストラクト前にmodelを識別できる情報は必要なためやはり また最後のAPI提案についてですが質問があります。
|
たしかに仰るとおりで、提案APIではモデルを読み込んだ後の扱いを考えていませんでした・・・。
コンストラクタ前にmetas情報が必要というのは同意なのですが、どうやってall_metas情報を得るのかがわからないでいます。
互換性維持のためです。
なるほどです。Voice構造体、文字通り音声が入っていそうなんですよね・・・。VoiceTypeとかなら良いのですが・・・。 |
で詳細が更に議論されています。
|
モデルファイルのファイル構造についてはこちらで議論するのが良さそうでしょうか。
個人的にはzipからバイナリを読めるようなライブラリでまあまあ良い感じのがあるなら、zip方式でいいかなと思ってます。 |
まあzipでいいんじゃないですかね |
賛成です! manifest.json 他のファイルのファイル名・バージョンなどが入ってる |
manifest.jsonとmetas.jsonって分ける必要ありますか? |
作る側的には、意味の異なる情報は分けたほうが管理しやすいので嬉しいです。 ファイルパスに関しては拡張子が違うことを考慮してます。 |
これはスピーカーを新しく追加するときとかですか?
これはmanifestにファイルパスを持たせずにmanifest_versionをもとに読み込むファイルパスを変えるでも対応できそうですがどうしましょうか? |
新しくVVMを作る時とか、いっぱいあるVVMのマニフェストを刷新する時とか想像してました。
確かにそうですが、ファイル名をコーディングするとテストやメンテが少し大変だったりしそうです。
スタイルのバージョンはmetas.jsonにあるのでそっち使うことになると考えてます。 このあたりはどれもAPIには現れないことなので、そこまで議論するようなことではない気もしました…! |
運用のことを考えると分けたほうが良さそうですね |
ですです、基本的に同一VVM内のスタイルのバージョンが全部上がる感じです。 |
↑このタスクリストですが、#370 マージ後にすぐ |
なるほどです、良いと思います!! |
@qryxip |
|
あ、そういえばproject-vvm-async-apiだと3つとも既に消滅していますね。すみません忘れて下さい。 |
内容
#276 (comment) , #274 (comment) によりvoicevox_coreの C APIをオブジェクト指向に寄せたAPIにする必要が出てきて、そのための議論をしたいと考えています。
個人的には以下のようなクラスでC APIを考えてるのですがいかがでしょうか?
またpython APIについてもこのissueで議論したクラス設計を元にAPI設計を作りたいと考えてます。
今考えてるものとしてはversion,metas,supported_devices情報の取得はVoicevoxCoreクラスのstaticメンバ関数として実装しようと考えています。
voicevox_core_newとvoicevox_core_initializeは一緒にするべきかもしれませんがresultcodeの関係上分けようかと思ってます。
また現在のvoicevox_coreの関数のprefixは
voicevox
ですが、これもきちんとオブジェクト指向的な名前にするとvoicevox_core
にprefixがなると思い、 prefix をvoicevox_core
にしようと考えています。その他
@sevenc-nanashi @shigobu お二人はvoicevox_coreのwrapperを作成されているように見受けられますのでぜひご意見をいただきたいです。
The text was updated successfully, but these errors were encountered: