voicevox_coreのクラス設計discussion

qwerty2501 commented 2 years ago

内容

https://github.com/VOICEVOX/voicevox_core/issues/276#issuecomment-1245472220 , https://github.com/VOICEVOX/voicevox_core/pull/274#issuecomment-1246896362 によりvoicevox_coreの C APIをオブジェクト指向に寄せたAPIにする必要が出てきて、そのための議論をしたいと考えています。個人的には以下のようなクラスでC APIを考えてるのですがいかがでしょうか？またpython APIについてもこのissueで議論したクラス設計を元にAPI設計を作りたいと考えてます。今考えてるものとしてはversion,metas,supported_devices情報の取得はVoicevoxCoreクラスのstaticメンバ関数として実装しようと考えています。

struct VoicevoxCore;

VoicevoxCore* voicevox_core_new();

VoicevoxResultCode voicevox_core_initialize(VoicevoxCore* voicevox_core, VoicevoxCoreInitializeOptions options);

void voicevox_core_delete(VoicevoxCore* voicevox_core);

// staticメンバ関数のためVoicevoxCore のオブジェクトpointerは不要
char* voicevox_core_get_version();

// staticメンバ関数のためVoicevoxCore のオブジェクトpointerは不要
char* voicevox_core_get_metas_json();

// staticメンバ関数のためVoicevoxCore のオブジェクトpointerは不要
char* voicevox_core_get_supported_devices_json();

VoicevoxResultCode voicevox_core_load_model(VoicevoxCore* voicevox_core,uint32_t speaker_id);

bool voicevox_core_is_gpu_mode(const VoicevoxCore* voicevox_core);

//以下、同様にオブジェクト指向的にC APIを定義していく

voicevox_core_newとvoicevox_core_initializeは一緒にするべきかもしれませんがresultcodeの関係上分けようかと思ってます。また現在のvoicevox_coreの関数のprefixは voicevox ですが、これもきちんとオブジェクト指向的な名前にすると voicevox_core にprefixがなると思い、 prefix を voicevox_core にしようと考えています。

その他

@sevenc-nanashi @shigobu お二人はvoicevox_coreのwrapperを作成されているように見受けられますのでぜひご意見をいただきたいです。

shigobu commented 2 years ago

voicevox_core_newとvoicevox_core_initializeは一緒にするべきかもしれませんがresultcodeの関係上分けようかと思ってます。

できるなら、一緒のほうがいいと思います。別にすると、initializeの要・不要がわからない問題が残ると思います。例えば、voicevox_core_load_model関数を使うには、voicevox_core_new関数でオブジェクトを作成し、voicevox_core_initialize関数で初期化する必要がありますが、voicevox_core_initialize関数を呼ばなくてもvoicevox_core_load_model関数を呼ぶことができてしまいます。newとinitializeを一緒にすれば、このような間違いは発生しなくなります。 initializeの要・不要をドキュメントに書くのでもいいですけど、間違いが起きない(起きにくい)仕組みにできるならしたほうが良いと思いました。

一緒にした場合の懸念点として、どのようにエラーを通知するかが有ると思います。これに関しては他の関数と同様に、戻り値でVoicevoxResultCodeを返し、引数でVoicevoxCoreオブジェクトを返すのが統一感があって良いかなと思いました。例えばこんな感じ。

VoicevoxResultCode voicevox_core_new(VoicevoxCoreInitializeOptions options, VoicevoxCore** voicevox_core);

sevenc-nanashi commented 2 years ago

自分も同意です。また、実際に使うコードは

VoicevoxCore vvc = voicevox_core_new();
if(!voicevox_core_initialize(...))
  ...

のようにすぐ下でinitializeが呼ばれると思うので、それならまとめてもいいんじゃないかと思いました。

（Cは詳しくないのでコードが間違ってて変なことを言ってるかも…）

qwerty2501 commented 2 years ago

@shigobu @sevenc-nanashi そうですね。一緒にやるとするならそのような形になります。しかしinitializeは何度も呼び出される想定であるとのことでした。その場合 voicevox_core_reinitialize のようなメンバー関数をつくってそこで何度も呼び出せるような方式にすれば対応はできなくもないとは思いますが、そもそもinitializeを何度も呼び出される想定であることに違和感を覚えます。なぜinitializeを何度も呼び出される必要があるのか、またinitializeのどの部分の処理が何度も呼び出す必要があるのか明らかにする必要がありそうです。 @Hiroshiba initilalizeが何度も呼び出されることを期待する理由とは何でしょうか？またそれは本当に必要でしょうか？

Hiroshiba commented 2 years ago

結論から言うと、コンストラクタと一緒にしちゃって良いかなと思いました！ initializeがある理由は、以前のAPIだとVoicevoxCoreクラスがなかったという歴史的経緯が大きそうです。

一応現状のinitializeがあった方が良い理由もいくつかあります。それらも踏まえて考えを書きます。

まず、デストラクタを呼ばずにmodelを解放（GPUメモリを解放）できるのは需要が無くはないと思います。が、これはどちらかというと作るべきはunload_modelなので（しかも必須級ではない）、やっぱりinitializeはなくて良いかなと思いました！

あとは同じくデストラクタ呼ばずにCPU・GPU版を切り替えが可能なことです。これもどちらかというと需要があるのは切り替え用メソッドだと思いました（こちらも必須級ではない）。

一方、initializeを呼ぶ必要があるのはバグや面倒さにつながると思います。総合すると、initializeとコンストラクタはくっ付けて良いと思います。 unload_modelやGPU CPU切り替え機能がなくなりますが、別にクラスインスタンスごと作り直しても良いと思うし、追々追加くらいで良さそうな印象です。

qwerty2501 commented 2 years ago

となるとRust側もくっつけて良さそうというかすでに new_with_initializeが存在しますね。

qryxip commented 2 years ago

ところでPyO3実装あたりで特に何も議論せずにstruct Internalをstruct VoicevoxCoreに変更したと思うのですが、今ならまだリネームする選択肢があるかなとふと思いました。例えばVoicevoxCoreSessionとか。

qwerty2501 commented 2 years ago

Sessionという単語はonnxruntime文脈のもので voicevox_coreのユビキタス言語としてはSessionは無いような気がします。いやどうだろう・・・

Hiroshiba commented 2 years ago

どういうクラスになるのかまだ見えていないというのもあるので、いったんVoicevoxCoreのママでも良いのかなと思いました！

qwerty2501 commented 2 years ago

確かにドメイン設計からやるとなるとかなり時間かかっちゃいそうなので今回はVoicevoxCoreクラス単一で提供して良さそうですね。しかし将来的にはちゃんとしたドメイン設計を行いそれを(内部の改修も含めて)元にしたAPIの提供をしても良さそうですね。

qryxip commented 2 years ago

versionやmetasやOptions系はどうしましょうか?

選択肢としては今のところ次の4つがあると思います。

型"VoicevoxCore"のstatic method (@qwerty2501 さんの案)

voicevox_core_get_metas_json() // C

voicevox_core.VoicevoxCore.metas()  # Python

モジュール"voicevox_core"の関数

voicevox_get_metas_json()

voicevox_core.metas()

1.の定数版

voicevox_core_metas_json

voicevox_core.VoicevoxCore.METAS

2.の定数版
```
voicevox_metas_json
```
```
voicevox_core.METAS
```

Hiroshiba commented 2 years ago

@qryxip あ！そういえば、将来的な観点になるのですが、onnxモデルを読み込めるようになったときもAPIがあまり変わらないようになっていると嬉しそうです。 metasとonnxが結びついているので、load_model時にmetasの内容が変わりそうです。

この視点だと、metasは関数VoicevoxCoreのインスタンスメソッドか定数が良さそうかなと思いました。

･･･本当は１つのモデルに１つのクラスインスタンスを対応させるのが良いのかもとちょっと思いました。

qwerty2501 commented 2 years ago

metas関数はメンバ関数にしたほうが良さそうですね。

･･･本当は１つのモデルに１つのクラスインスタンスを対応させるのが良いのかもとちょっと思いました。

これ今のうちに対応するのがベストかもですね。voicevox_core_newを以下のような感じにすると対応できそう？

VoicevoxResultCode voicevox_core_new(
    uint32_t speaker_id,
    VoicevoxCoreInitializeOptions options,
    VoicevoxCore** output_voicevox_core);

その場合だとdecodeなどの関数はspeaker_idを受け取る必要はなくなりそうですね。

Hiroshiba commented 2 years ago

あ！とりあえずAPIとしての提供は実際に製品版がloadを使うようになってからでも良いかも･･･？いや今からあっても良いかもですね！APIの設計確認もできそうですし。

その場合だとdecodeなどの関数はspeaker_idを受け取る必要はなくなりそう

モデルとspeaker_idは1:1対応じゃないのでspeaker_idは必要そう･･･？

いくつかの観点を思いついたので列挙してみます。

モデルはyukarin_s yukarin_sa decodeで１セットで、そのセットが複数ある
- ので、このセット（モデルセット？）を足したり消したりできると嬉しそう
1つのモデルセットは1つのmetasと対応
「用意されてるのを全部読み込む」方法が無いと不便そう

qwerty2501 commented 2 years ago

@Hiroshiba ちょっと思ったんですが、metas関数をメンバー関数にした場合、ユーザーはどうやってコンストラクタ関数に指定するspeaker_idを知ることができるんでしょうか？

qryxip commented 2 years ago

SupportedDevicesの方なんですがSupportedDevices::get_supported_devicesがORT依存のfallibleな関数なので、どうせならfallibleな関数としてそのまま_c_apiや_python_apiに持ってくるというのはどうでしょうか? VOICEVOX_RESULT_GET_SUPPORTED_DEVICES_ERRORというのもせっかくありますし。 (_c_api側はLazy<CString>の代わりにLazy<voicevox_core::Result<CString>>の形で保持)

Hiroshiba commented 2 years ago

@Hiroshiba ちょっと思ったんですが、metas関数をメンバー関数にした場合、ユーザーはどうやってコンストラクタ関数に指定するspeaker_idを知ることができるんでしょうか？

@qwerty2501 たぶん指定する必要がないはずです。そのspeaker_idはコンストラクタ内の何に使われる想定でしょうか 👀

qwerty2501 commented 2 years ago

@Hiroshiba 一つのモデルに一つのクラスインスタンスを対応させるにはspeaker_idを受け取る必要があると思ったんですが違うのでしょうか？

Hiroshiba commented 2 years ago

ちょっと考えたのですが、確かにspeaker_id的なものをどこかで指定する必要があるなと思いました。どちらかというと、マッピングするための情報が必要そうに感じました。

いったん整理すると、現状の実装だと

１モデルセットの中には複数のspeaker_id（＝style_id）がある
それとは別にcoreが受け取るspeaker_idがある
include_speaker_id_mapで、coreが受け取るspeaker_idをモデルが持っているspeaker_idにマッピングしている
metas.jsonで、coreが受け取るspeaker_idとその意味（話者名やスタイル名）を対応付ける

という感じですよね。これを、１モデルに１metas.jsonにしたいなと考えています。（僕が提案している形で、ここが認識ずれに起因してそう？）

となるとどうにかしてマッピングをしないといけないのですが･･････どうしよう･･･。

qwerty2501 commented 2 years ago

@Hiroshiba このコメントで以下のコメントがあったので、インスタンスコンストラクト時にモデルを識別できる情報が必要だと考えてそれがcore側のspeaker_idであると思ってました。コンストラクト時にload_modelするべきか是非はあると思いますが私はload_modelもコンストラクト時に行うものだと思っていました。 load_model は時間がかかるため関数を分けるべきという考えもありますが。

本当は１つのモデルに１つのクラスインスタンスを対応させるのが良いのかもとちょっと思いました。

またload_model時にmetasの内容が変わるとから読み込んだmodelに関連するmetas.jsonに内容が変わると考えてました。

という感じですよね。これを、１モデルに１metas.jsonにしたいなと考えています。（僕が提案している形で、ここが認識ずれに起因してそう？）

私も１モデルに１metas.jsonになると考えてたのでこのあたりは認識合ってそうですね。しかしその場合上で書いたとおりmetas関数はメンバー関数になるためコンストラクト時あるいは load_model 時に渡す modelを識別できる情報が必要なのに取得する手段がなくて困ってしまうなあという感じです。

となるとどうにかしてマッピングをしないといけないのですが･･････どうしよう･･･。

これは私と同じ悩みであると思うのですがいかがでしょうか？対応策としては全modelのmetas情報を含んだjsonを用意し、static class関数として get_all_metas_jsonといった感じでコンストラクト前にmodelを識別できる情報(speaker_id?) をユーザーが取得できるようにする必要があるかも？そのためメンバー関数としての voicevox_core_get_metas_json 関数と static class関数としての voicevox_core_get_all_metas_json 関数を用意する必要があるかも？

qwerty2501 commented 2 years ago

あとcoreのspeaker_id、整数型だけど他のIDと区別するためにもtype aliasでいいから独自型名で提供したほうが多少はマシになるかもですね。 C API側はtype aliasなので間違えて使われてもコンパイルエラーにさせることはできないですが、Rustサイドはnew typeパターンでコンパイルエラーにさせることはできそう

Hiroshiba commented 2 years ago

ぶった切っちゃってすみません、理想と現状や、話者から声まで多段になってる構造がかなりややこしい気がしたので、いったんまずドメイン知識を整理してみました。

話者：speaker
- ≒キャラクター。
声：voice
- 話者＋スタイルごとにユニーク。
スタイル：style
- 同じ話者での違う声質を表すもの。ノーマルとかささやきとか。
モデル：model
- 複数の声で音声合成できる。
- 現状だと声は0から始まる整数（とりあえず声IDと呼称）と対応する。
- 話者やスタイルと対応関係は一切ない
- 同じモデルに複数話者が入っていることもあるし、同一話者の別スタイルが別モデルに入っていることもある。
話者ID：speaker_id
- 話者ごとにユニークな値･･･なはずだけど、現状だと声ごとにユニークな値になっている。
- 現状だと0から始まる整数。
話者IDのマッピング：speaker_id_map
- 話者IDとモデル・声IDを対応付けるもの。
- 現状だと各モデルは0から始まる整数で管理されているので、話者ID→モデルID＋声IDになっている。
メタ情報：metas
- 話者＋スタイルを話者IDを対応付けるもの。
- プログラム内で使うことはない、実際のユーザー向けの情報。

モデルは話者やスタイルと対応関係はないこと、本来話者ユニークになるべき話者ID（speaker_id）が声ユニークになっていることがややこしさの根幹かなと思いました。

これをもとに、一旦現状の相関図みたいなのを絵があると議論しやすそうなのですが･･･なにか便利なツールはないでしょうか･･･

qwerty2501 commented 2 years ago

mermaid 記法使えるのでこれで関係性を表すのはどうでしょう？ modelを識別する整数(ID)のことを声IDとされているようですがここはシンプルにmodel IDとしたほうが伝わりやすいかもしれません。

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

Hiroshiba commented 2 years ago

サンプル図の２つ目の図が良い感じそうです。ER図、なるほど。 UML図に慣れている方いらっしゃったらおまかせしたいかもです。

modelを識別する整数(ID)のことを声IDとされているようですがここはシンプルにmodel IDとしたほうが伝わりやすいかもしれません。

ここは認識のずれがありそうです。ちょっと書き方ややこしかったかもなのですが、モデル内には複数の声があるので、そのモデル内の声を声IDと表記してみました。

qwerty2501 commented 2 years ago

@Hiroshiba あ、model IDと声 IDは別にありましたね。

UML図に慣れている方いらっしゃったらおまかせしたいかもです。

これについて理解度の浅い人間から図を作ってしまうと間違いだらけのものができてしまいそれもとに議論してしまうと最終型の図もreviewがもれてしまい間違った形になってしまうリスクが高そうだなと感じました。現に私は理解していないわけですし。なのでまずhihoさんからご自身が考えていることを図に落とし込む必要があるのではないでしょうか？ mermaidが書きにくいまたは書き方がわからないということであればpngでもなんでも良いので好きな形で書いていただき、その後mermaidか管理しやすい形で誰かが書くというのが安全そうに感じました

Hiroshiba commented 2 years ago

なるほどです。ちょっと頑張って図を作ってみます。

Hiroshiba commented 2 years ago

図、まとめました！！いろいろ試行錯誤が必要そうだったので、パワポで作りました。 https://docs.google.com/presentation/d/1ltsMERyNNcuVExJZunZRTI7WsqlP4cMSfFrmhXUwjG4/edit?usp=sharing

パワポの下の方に提案の形もあります！

qwerty2501 commented 2 years ago

@Hiroshiba ありがとうございます。よくわかりました。speakerが同じでもmodelが違う場合があるんですね 1model 1クラスインスタンスについてはユーザーが理解するのは難しいかもですね。しかしパフォーマンスから考えるとやったほうが良いかもしれない。

パフォーマンスを考えると一つずつ読み込んだほうが良いですが、一方でユーザーにmodelを意識させてしまうという問題点が存在します。 modelを意識させずに1modelずつ読み込む工夫はしたほうがよいかもしれません。

またコンストラクト前にmodelを識別できる情報は必要なためやはり voicevox_core_get_all_metas_json static 関数のようんばものは必要な気がするのですがいかがでしょうか？

また最後のAPI提案についてですが質問があります。

style_idについて本当はstyle uuidが良いとしているのにintとしているのはなぜですか？
VoiceOptionsは不要だと思います。OptionsはOptionalな引数を受け取るためのものですが、synthesis APIではspeaker_id,style_idともに必須であるためそのまま引数として受け取ったほうが良いと思います。もしくはVoiceを表現したいのであれば、単純に型名をVoiceとするか。

Hiroshiba commented 2 years ago

パフォーマンスを考えると一つずつ読み込んだほうが良いですが、一方でユーザーにmodelを意識させてしまうという問題点が存在します。

たしかに仰るとおりで、提案APIではモデルを読み込んだ後の扱いを考えていませんでした･･･｡

またコンストラクト前にmodelを識別できる情報は必要なためやはり voicevox_core_get_all_metas_json static 関数のようんばものは必要な気がするのですがいかがでしょうか？

コンストラクタ前にmetas情報が必要というのは同意なのですが、どうやってall_metas情報を得るのかがわからないでいます。 all_metasというと全モデルのmetas情報だと思うのですが、その「全モデル」をどうやって取得しているのかがわかっていません。カレントディレクトリにある全てのディレクトリ直下のmetas.jsonを読み込むとか･･･？

style_idについて本当はstyle uuidが良いとしているのにintとしているのはなぜですか？

互換性維持のためです。

VoiceOptionsは不要だと思います。

なるほどです。Voice構造体、文字通り音声が入っていそうなんですよね･･･。VoiceTypeとかなら良いのですが･･･。まあでもここは細かい箇所なので、とりあえず今議論しなくても良いかもと思いました。

qwerty2501 commented 2 years ago

たしかに仰るとおりで、提案APIではモデルを読み込んだ後の扱いを考えていませんでした･･･｡

どちらかというとモデルを読み込む前で、voicevox coreコンストラクタに影響が出ると思いますAPIとしてはこんな感じになるかと

VoicevoxResultCode vocievox_core_new(
uint32_t model_id, 
VoievoxInitializeOptions options,
VoicevoxCore** output_voicevox_core);

1model 1クラスインスタンスをやろうとした場合、ここでmodel_idが出てきてしまうとユーザーにmodel_idとは何か？という疑問が持たれるのではないかという懸念です。

コンストラクタ前にmetas情報が必要というのは同意なのですが、どうやってall_metas情報を得るのかがわからないでいます。 all_metasというと全モデルのmetas情報だと思うのですが、その「全モデル」をどうやって取得しているのかがわかっていません。

これについては私も正直わからないです。metasは恐らく埋め込み時に置き換わるので実際の形が想像しにくいことと、今回のmetasを分けることをhihoさんがどのように実現しようとしているか把握していないためです。ただ今のmetas.jsonは全てのmodel情報を含んでいるのでそれと同じmetas.jsonをどこかに提供してやれば行けるのかなとは思ってます。

カレントディレクトリにある全てのディレクトリ直下のmetas.jsonを読み込むとか･･･？

埋め込んであるjsonを読み込むのでカレントディレクトリは関係ないのではないですか？

Voice構造体、文字通り音声が入っていそうなんですよね･･･

この場合 声 というユビキタス言語に問題があるかも？

Hiroshiba commented 2 years ago

なるほどです。僕から一度しっかり提案の形を示してみます。

これについては私も正直わからないです。metasは恐らく埋め込み時に置き換わるので実際の形が想像しにくいことと、今回のmetasを分けることをhihoさんがどのように実現しようとしているか把握していないためです。

１モデルに１metasを対応付けるのはそんな難しくなくて、例えばmodel.onnxとmetas.jsonを無圧縮zipにして１ファイルにして.vvm（voicevox model）拡張子にし、load_vvm関数を用意してファイルパスを指定するだけで良いと思います。

model_idですが、引数にするのではなく、load_vvmの返り値でランダムなIDかModelクラスを返すのもありかなと思いました。

あと、loadしたモデルをユーザーに管理させるのではなく、VoicevoxCore側に状態を持たせるのが使いやすいかなと思っています。コア側がモデルリストを持てるようにし、load_vvmしたら勝手にそのリストに登録される感じです。こうすれば、ユーザー側はmodel_idなどを考える必要がなく、あとは話者IDとスタイルIDを指定するだけで音声合成できるはずです。ただこの場合でも、一度読み込んだモデルを外す方法が必要になりそうなので、どっちにしろ適当なIDかModelクラスをユーザーに返す必要はありそうだなと考えてます。

qwerty2501 commented 2 years ago

@Hiroshiba たしかにファイルパスから読み込むようにすればload前にmetasを取得する必要はありませんね。しかしその提案だとmodelとライブラリの分離ができていて、外部からmodelの読み込みを行うように見えるのですが、それって権利者側の許諾が必要ということで止まっていましたよね？ model分離について目途が立ったということでしょうか？それともmodel分離できるようになってから新APIをリリースする予定でしょうか？急がないのでその方が良いかもしれませんが

Hiroshiba commented 2 years ago

すみません、そのあたり僕自身ごっちゃになっていました！そもそもは「model分離できたときもAPIが変わらないようになっていてほしい」という感じだったんでした。 https://github.com/VOICEVOX/voicevox_core/issues/280#issuecomment-1249580910

となると、埋め込みからloadするAPI（≒メモリからloadするAPI？）とファイルからloadするAPIを用意して、load以降の処理は↑の方法で実装するというのが僕の提案の形になりそうです。

提案のAPI設計が完璧な設計である自信はない（特に内部的にどうモデルを持つかあたり）です･･･。

qwerty2501 commented 2 years ago

@Hiroshiba 埋め込みからloadするAPIがあるということは、load前にmodel_idが必要=load前にmetasを取得する必要があると思ったのですがどうでしょうか？

懸念として埋め込みからloadするAPIが提供されてると、ファイルからloadするAPIを提供してもユーザーの移行が進まず結局埋め込み続ける必要がありそうです(消すとユーザーから不満がでるため)。なのでファイルサイズが大きいまま配布しなければならない期間が相当長くなりそうだなと思いました。そのあたりは許容されるということで良いでしょうか？

また都度loadするようなAPI想定であるため以前のコメントで言われていた1model 1クラスインスタンスではなく、一つのVoicevoxCoreクラスインスタンスに複数のモデルを持つ方向に気持ちが傾いてる感じですか？コメント見る限り悩まれているようなのでまだどちらにするか決めかねている印象を受けました。

Hiroshiba commented 2 years ago

また都度loadするようなAPI想定であるため以前のコメントで言われていた1model 1クラスインスタンスではなく、一つのVoicevoxCoreクラスインスタンスに複数のモデルを持つ方向に気持ちが傾いてる感じですか？

僕が提案してその承諾をもらうのではなく、どういう形が良いか一緒に考えたいからぜひ意見がほしい感じです！

埋め込みからloadするAPIがあるということは、load前にmodel_idが必要=load前にmetasを取得する必要があると思ったのですがどうでしょうか？

initializeされたときに勝手に全部読み込むのを想定していました。･･･が、これだと必要な声の分だけload_modelできないですね･･･。 metasを読み込むのと、sessionを作るの（今のload_model）は関数を分けるとか･･･？

懸念として埋め込みからloadするAPIが提供されてると

このあたりはなんとでもなりそうだなと思いました！例えばモデル埋め込みがないコアと、埋め込まれてるコアと、モデルファイルをしばらくのあいだ全部配布するとか･･･！

qwerty2501 commented 2 years ago

@Hiroshiba

僕が提案してその承諾をもらうのではなく、どういう形が良いか一緒に考えたいからぜひ意見がほしい感じです！

一応補足しておくとhihoさんの承諾をもらいたいというよりかはまずhihoさんがどうしたいかオープンにしないとディベートのしようがないと考えてるためです。個人的な意見としては1model 1VoicevoxCoreインスタンスとするのではなく、1VoicevoxCoreインスタンスに複数modelを隠蔽する形で持った方が良いと思いました。例えばずんだもんでもスタイルが違うとmodelが違う可能性があるんですよね？その場合同じずんだもんであっても使用するVoicevoxCoreインスタンスを分けなくてはならないという事態になりこれはユーザーからするとかなり混乱しそうでした。Voicevoxはspeaker(キャラクター)に重きを置いてるのでユーザーとしてもデータはspeakerごとに分けたいモチベーションはあると思うのでそれができないのであればmodel隠蔽する方向にしたほうが良いと思いました。

initializeされたときに勝手に全部読み込むのを想定していました。･･･が、これだと必要な声の分だけload_modelできないですね･･･。

たしかにinitialize(constract)するときにすべて読めば隠蔽できますねしかし指摘の通り個別に読み込めないとパフォーマンス上の問題がありますね。実際engineでパフォーマンス上の問題があったからload_modelを生やしたわけですし。

metasを読み込むのと、sessionを作るの（今のload_model）は関数を分けるとか･･･？

これについては全ての埋め込みvvmファイルにあるmetas.jsonの内容をmergeした内容のall_metas.jsonを埋め込んで置き、 voicevox_core_get_all_metas_json static 関数でその内容をそのまま返す実装にすればよいと考えていましたがどうでしょうか？

そういえば提案されたmetas.jsonでにmappingIndexというフィールドがありましたがこれはmodel_idですか？

Hiroshiba commented 2 years ago

ユーザーとしてもデータはspeakerごとに分けたいモチベーションはあると思うのでそれができないのであればmodel隠蔽する方向にしたほうが良いと思いました。

補足ありがとうございます！おっしゃる通りなので、読み込んだ後はなるべくモデルを隠蔽して話者単位で操作可能にしたいなと思いました。

全ての埋め込みvvmファイルにあるmetas.jsonの内容をmergeした内容のall_metas.jsonを埋め込んで置き

埋め込む場合の処理はその形でも良いのですが、ファイル読み込みの経路と処理の流れが大きく異なるのは避けたいなと思ってました。が、埋め込みの処理はユーザーから見えないので割となんでも良いですね！埋め込みの場合はモデルごとにmetasとonnxを作らなくても良さそうに思いました。

そういえば提案されたmetas.jsonでにmappingIndexというフィールドがありましたがこれはmodel_idですか？

「そのモデルにとってその声がベクトル要素の何番目か」を指す値を意図していました。今のコアのコードだ、onnxruntimeのrunに与えているspeaker_idがこれです。 https://github.com/VOICEVOX/voicevox_core/blob/084bf63df6824922d398ae49668aa619f2517843/crates/voicevox_core/src/publish.rs#L358

qwerty2501 commented 2 years ago

埋め込む場合の処理はその形でも良いのですが、ファイル読み込みの経路と処理の流れが大きく異なるのは避けたいなと思ってました。

大きく異なるの指す意味によりますが、metaデータを読み込んでからloadするのとファイルからの読み込みのloadは別々にする必要があると思います。

が、埋め込みの処理はユーザーから見えないので割となんでも良いですね！埋め込みの場合はモデルごとにmetasとonnxを作らなくても良さそうに思いました。

ここについては埋め込んだ場合はbyteデータの参照、ファイル読み込みの場合はfileを読み取ったbyteデータを取得したあとは共通化できると思うので埋め込みの場合でもファイル配布と同様なファイル形式で埋め込んだほうが良いと思います。

「そのモデルにとってその声がベクトル要素の何番目か」を指す値を意図していました。今のコアのコードだ、onnxruntimeのrunに与えているspeaker_idがこれです。

model_indexとmodel_idは別物ですか？

Hiroshiba commented 2 years ago

ここについては埋め込んだ場合はbyteデータの参照、ファイル読み込みの場合はfileを読み取ったbyteデータを取得したあとは共通化できると思うので埋め込みの場合でもファイル配布と同様なファイル形式で埋め込んだほうが良いと思います。

おっしゃるとおりだと思いました！こういう流れが良さそうに思います。

埋め込み/ファイルからmetasを読み込む
voicevox_core_get_all_metas_jsonなどで話者IDやスタイルIDなどがわかる
話者IDやスタイルIDを指定してモデルを読みこむ
話者IDやスタイルIDを指定して音声合成する

この方式も１点問題があるので悩んでいます。モデルのunloadです。モデルのunloadもloadと同じく話者IDやスタイルIDを指定する形にすると、そのモデルが持つ別の声もunloadされることが直感的にわかりづらくなります。 unloadだけmodel隠蔽ができなそうで、どうしたものかなと･･･。いまのところ、ドキュメントでしっかり案内するか、unload_modelだけはmodel_idを引数に取る形にするかかなと想像しています。後者はload_modelが話者ID/スタイルIDを受け取る形と非対称っぽくなっちゃうので、どうしたものかと･･･。

model_indexとmodel_idは別物ですか？

どちらも同じでモデルを一意に指定可能にするという意味で、目的は同じだと思います。 model_idはポインタか文字列を想定していました。モデルのunloadを考えたとき、model_indexは順番がずれるのでバグを生みやすそうだなーと。

qwerty2501 commented 2 years ago

@Hiroshiba 確かにunloadについてはmodelを隠蔽するのは難しいですね。これについては以下の選択になってきそうですね

modelを隠蔽するのを諦めてloadもmodel_idを指定する形にする
メモリ消費量を犠牲にしてspeakerごとにmodelを独自に読み込む
割り切ってunloadを提供しない(これは厳しそう)

またmappingIndexについては名称をmodelIdにしたほうが良いように感じました。

Hiroshiba commented 2 years ago

2の方法は思いつきませんでした。最悪、GPUメモリ消費量が２〜３倍くらいに増えるので結構厳しそうではあります。

3も全然ありだと思います。 unloadはあまり使われなさそうだし、VoicevoxCoreインスタンスごと破棄すれば全モデルのunloadはできそうなので･･･。ただなんか、unload以外にも問題がある気がするんですよね･･･。

まあ、ユーザーを考えず、開発者として素直に考えるなら本来1が一番良いだろうなと思います。この場合はModelクラスを提供するのが扱いやすそうに思います。あとはそのModelインスタンスをまとめるクラスを提供して、話者ID/スタイルIDを指定したらsessionを作ったり音声合成したりunloadしたりできるようにするのが落とし所になるだろうなと思ってます。

3で行くか1で行くか、かなり悩んでいます。

またmappingIndexについては名称をmodelIdにしたほうが良いように感じました。

mappingIndexとmodelIdは別物です。似てるのはmodel_indexとmodel_idの方です。

model_index/model_idは全モデルの中からモデルを特定するためのもので、 mappingIndexは特定のモデルの中からどの声で音声合成するか特定するためのものです。

qryxip commented 2 years ago

1の場合ですが、モデルをまとめるModelSetみたいな ~~中身が多倍長bitsetの~~(モデル自体を持たないと駄目ですね) structだけを提供すればほどほどに隠蔽できるのでは、という気がします。

VoicevoxModelSet* voicevox_model_set_new(void)
void voicevox_model_set_delete(VoicevoxModelSet*)
size_t voicevox_model_set_count(const VoicevoxModelSet*) (モデル数を返す)
~~void voicevox_model_set_unify(VoicevoixModelSet*, const VoicevoxModelSet*)~~
bool voicevox_model_set_load(VoicevoxModelSet*, VoicevoxVoice) (モデルを新たに読んだらtrue)
size_t voicevox_model_set_unload(VoicevoxModelSet*, VoicevoxVoice) (unloadしたモデルの数を返す)

Hiroshiba commented 2 years ago

@qryxip 自分もそんな感じのを想像していました！あと、ファイルから読み込むことも考えると、ModelSetにModelを追加するAPIが必要だと思います･･･！

qwerty2501 commented 2 years ago

VoicevoxModelSetで扱う場合だとVoicevoxModelSet AとVoicevoxModelSet Bで持っているModelが重複してしまう可能性はありませんか？

Hiroshiba commented 2 years ago

いやーーー･･･たしかに･･･。Rustだと移譲でなんとかなりそうですが、C APIだと･･･。

ModelSetにModelを登録する形式からちょっと変えて、ModelSetにファイルやメモリからモデルを読み込み機能（metasやonnxをloadする機能）やらunloadやらを全部持たせて、ユーザーはModelを触ることができないようにするとか･･･？ただ、ModelSetとのやり取りで話者ID使ったりするとまた問題が出るので、モデル読み込み時にmodel_idを発行して、それをもとにModelSetとやり取りしてもらうとか･･･

qwerty2501 commented 2 years ago

いやーーー･･･たしかに･･･。Rustだと移譲でなんとかなりそうですが、C APIだと･･･。

ModelSetで所有してるModelのメモリが重複してしまってメモリ消費量が増大していることが問題なのでRustでもこの問題は基本的に解決できないですね。

ModelSetにModelを登録する形式からちょっと変えて、ModelSetにファイルやメモリからモデルを読み込み機能（metasやonnxをloadする機能）やらunloadやらを全部持たせて、ユーザーはModelを触ることができないようにするとか･･･？

ModelSetを操作するのはユーザーなので結局ユーザーがModel触ってるのと同じではそもそもModelSetについて存在意義がないように見えます。

ただ、ModelSetとのやり取りで話者ID使ったりするとまた問題が出るので、モデル読み込み時にmodel_idを発行して、それをもとにModelSetとやり取りしてもらうとか･･･

例えばVoice AとVoice Bのmodelをloadしてそのmodel_idを発行したとしてもユーザーとしてはVoice AとVoice Bが同じmodel_idになる可能性があることなど夢にも思わないと思います。なのでmodel_idを発行したとしても一緒に開放したくないVoiceのmodelをunloadしてしまう危険性はあります。

そういう意味でunloadを提供しないことでほんとうにmodelを隠蔽することができるのであれば、unloadを提供せず、modelを開放したければVoicevoxCoreをdeleteしてねというスタンスもありかなと思いました。

Hiroshiba commented 2 years ago

ModelSetで所有してるModelのメモリが重複してしまってメモリ消費量が増大していることが問題なのでRustでもこの問題は基本的に解決できないですね。

あ、問題に思っている部分が違っていました。仰っている問題点は、「ユーザーが全く同じモデルを２個作ったり、同じモデルを２つ以上のModelSetに登録してしまってメモリ消費量が増大する」ということでしょうか。だとしたら、まあユーザー側に気をつけてもらえばそこまで問題にならないような･･･？（まだ僕が勘違いしてそう）

僕が問題に思ったのは、同じModelインスタンスを２つのModelSetに登録したときの挙動がおかしくなりそうという点でした。２つのModelSetがModelの状態をそれぞれコントロールしようとしてややこしくなるだろうなと。けどよく考えるとModelに状態を持たせなければいいだけですね･･･！

ModelSetを操作するのはユーザーなので結局ユーザーがModel触ってるのと同じでは

まあ･･･そうですよね･･･。unload含め、Modelを扱うときの煩雑さを取っ払っているわけではないと思います。

ModelSetの利点は複数作れることだと考えています。モデルAとBでModelSet 1を作って、モデルCとDでModelSet 2を作る、みたいな。あと、辞書とかAudioQueryとかの機能をいっぱい持っているVoicevoxCoreから、モデル管理機能を分離させて責務を分けられるのも利点だと思います。

qwerty2501 commented 2 years ago

あ、問題に思っている部分が違っていました。仰っている問題点は、「ユーザーが全く同じモデルを２個作ったり、同じモデルを２つ以上のModelSetに登録してしまってメモリ消費量が増大する」ということでしょうか。だとしたら、まあユーザー側に気をつけてもらえばそこまで問題にならないような･･･？（まだ僕が勘違いしてそう）

それであってますが、loadがファイルからのみならそれでも良いかもですが、埋め込みの場合はVoiceの情報を元に読み込みますから違うVoiceのモデルが同じかどうかはユーザーとしては気をつけようがないと思うのですが

僕が問題に思ったのは、同じModelインスタンスを２つのModelSetに登録したときの挙動がおかしくなりそうという点でした。２つのModelSetがModelの状態をそれぞれコントロールしようとしてややこしくなるだろうなと。けどよく考えるとModelに状態を持たせなければいいだけですね･･･！

確かにその点も問題にはなってきそうですね

ModelSetの利点は複数作れることだと考えています。モデルAとBでModelSet 1を作って、モデルCとDでModelSet 2を作る、みたいな。

これの利点が正直わからないです。

あと、辞書とかAudioQueryとかの機能をいっぱい持っているVoicevoxCoreから、モデル管理機能を分離させて責務を分けられるのも利点だと思います。

個人的にはそもそもmodelをAPIとして見せるのはあまり良くないかなと思ってます。ちょっと考えたのですが、 unloadをVoice情報を元に機能提供できる気がしてきたのでやはりAPIとしてはmodelはAPIでは隠蔽する方向が良いと考えてます。

qryxip commented 2 years ago

ModelSetで所有してるModelのメモリが重複してしまってメモリ消費量が増大していることが問題なのでRustでもこの問題は基本的に解決できないですね。

参照カウントで包めばよいような気もします(ORTのSessionは!Syncなのでドキュメントに注意書きを添えて)。ただそれでもModelが持つ状態がやはり問題ですが、今Modelが持っている状態って何でしたっけ? ユーザーから認識できないものであるならmutexとかに包めば大丈夫なんじゃないかと。

(edit1: Session自体mutex必須でしたね) (edit2: !SyncならRefCellですね)

qryxip commented 2 years ago

個々のモデルの存在の隠蔽ですが、ファイルから読むことがある以上ある程度は意識せざるを得ないのではないかと思います。「個々のモデル」の存在はドキュメント上で示しつつ、かつそれでもうっかり必要なモデルをunloadしないように常に「モデルの集合」として扱うことを強制する、というのもありなんじゃないかと。

qryxip commented 2 years ago

今Modelが持っている状態って何でしたっけ?

これですが、「どのvoiceのために今loadされているのか」の情報が無いと駄目ですね。じゃないとある声に対するunloadで無条件でモデルがunloadされてしまい、ModelSetで扱うメリットが皆無になります。

VOICEVOX / voicevox_core

voicevox_coreのクラス設計discussion #280

内容

その他