OpenJTalkをグローバルに持たせないようにする #98
Conversation
core/src/engine.cpp
Outdated
| static std::shared_ptr<OpenJTalk> openjtalk; | ||
| static std::unique_ptr<SynthesisEngine> engine; | ||
|
|
||
| VoicevoxResultCode voicevox_initialize_openjtalk(const char *dict_path) { |
There was a problem hiding this comment.
この処理内容だと初期化してるのはopenjtalkではなくengineなので関数名変えたほうが良いと思います。
There was a problem hiding this comment.
確かに...!
まだプレビュービルドしか存在してないですし、APIの互換性を破棄しても大丈夫でしょう...
そもそもやりたいのはopenjtalkの辞書の読み込みなので、初期化というよりも辞書のロードである節を明確化する意味も込めてvoicevox_load_openjtalk_dictionaryとかにすると良いかもと思いました。どうでしょうか？
There was a problem hiding this comment.
確かにやってることはopenjtalkの辞書読み込みですが、ライブラリユーザー側からすると求めていることってopenjtalkの辞書読み込みではなくengineを使える状態にするではないでしょうか？ 🤔
また内部でopenjtalkとengineを生成しているのでその関数名では中の処理を適切に説明できていないと思います。加えて例えばengineで使っている機能が増えて初期化時にやりたいことが増えた場合関数名が限定的にされているため別にその初期化関数を定義しなければならなくなるでしょう。これは呼び出し側でやることが増えてしまうので避けたほうが良いように感じます。
私案ですが以下のような感じの関数が良いかなと思いました。ただこの案だとengine初期化時に必要な引数が増えた場合対応しづらいという問題はありますが。(関数定義が変わってdllを通しての呼び出しだと再コンパイルが必要になるため)
| VoicevoxResultCode voicevox_initialize_openjtalk(const char *dict_path) { | |
| VoicevoxResultCode voicevox_initialize_engine(const char *openjtalk_dict_path) { |
There was a problem hiding this comment.
ライブラリユーザー側からすると求めていることってopenjtalkの辞書読み込みではなくengineを使える状態にするではないでしょうか？🤔
確かに、現状はそうなっていますね。
将来的に、AquesTalkライク記法によるTTS関数などを実装すると、それを実行する際はあらかじめエンジンを初期化して辞書をロードしておくことは不要(今までのvoicevox_tts関数の実装みたく、エンジンがグローバルに存在しなければif文で分岐して初期化するようにすればいい)になるはずなので、voicevox_initialize_engineだと誤解を生んでしまうように思っています(AquesTalkライク記法が使える未来では、全ての場合においてSynthesisEngineの初期化+辞書のロード==engineを使える状態にするではないからです)。
以上のことを踏まえて、voicevox_load_openjtalk_dictionaryといった、辞書の読み込みであることを明確化した関数名にすべきと私は考えているのですが、改めてどうでしょうか...?
There was a problem hiding this comment.
なるほど辞書読み込まない場合もありえるんですね。
エンジンがグローバルに存在しなければif文で分岐して初期化するようにすればいい
非同期で voicevox_tts が呼ばれた場合二重に初期化されてしまう問題がありますね。きちんとしたSingletonを実装し、lockを適切に行えば解決はできそうですが。
関数名については異論はありません
There was a problem hiding this comment.
非同期で
voicevox_ttsが呼ばれた場合二重に初期化されてしまう問題があり
なるほど、確かにありえそうですが、これはまた別のPRに分けるべきですね。
(あまり詳しくないので、できればお任せしたいなと思います👀)
core/src/engine.cpp
Outdated
| VoicevoxResultCode voicevox_tts(const char *text, int64_t speaker_id, int *output_binary_size, uint8_t **output_wav) { | ||
| if (!openjtalk) { | ||
| if (!engine || !engine->is_openjtalk_dict_loaded()) { | ||
| return VOICEVOX_RESULT_NOT_INITIALIZE_OPEN_JTALK_ERR; |
There was a problem hiding this comment.
ここですがエラーをエンジンの未初期化エラーとOpenJTalkの辞書未読み込みエラーに分けたほうが良いのでは。
また VOICEVOX_RESULT_NOT_INITIALIZE_OPEN_JTALK_ERRについてですがエンジン初期化時にOpenJTalkも初期化されるので不要になってると思うので消したほうが良いように感じました。
There was a problem hiding this comment.
確かにそうですね...
特に何も考えず既存のコードを流用していましたが、変更してみます...!
There was a problem hiding this comment.
少し考えたのですが、最初からSynthesisEngineを初期化しておくのはどうでしょうか...?
static std::unique_ptr<SynthesisEngine> engine = std::make_unique<SynthesisEngine>();こうすれば、先ほど懸念があった非同期実行時等にSynthesisEngineが2重以上に初期化されることはないでしょうし、エンジンの未初期化エラーを用意する必要もなくなります(辞書が2重以上にロードされる可能性は否めませんが)。
(将来的に実装されるAquesTalkライク記法のTTS関数ではSynthesisEngineは勝手に初期化される仕様にしますが、それだとOpenJTalkを利用する際にエンジン未初期化エラーが出るのは変ですし、SynthesisEngineのみを初期化する関数はそもそも用意してないため、そのような解決策が不明なエラーが出るのはおかしいように思ったのでこのような提案をさせてもらいました)
There was a problem hiding this comment.
そのアプローチもありだとは思いますが、SynthesisEngine使ってない場合でもSynthesisEngineが初期化されてしまうのでその分計算リソースとheapリソースが消費されます。
現状voicevox engineだとcore側のSynthesisEngineは使ってないはずなのでその分余分にリソースを消費してしまいます。
また初めから初期化する場合はunique_ptrは不要で以下で良いと思います
static SynthesisEngine engine;There was a problem hiding this comment.
なるほど。確かに使わない際は無駄ではありますね。
個人的な考えとしては、公式のPython実装や任意のサードバーティ実装のエンジンを使うような場合を除いて、ほぼ確実にtts系関数を使うことになるので気にしないかなというのと、メインの機械学習モデルやら辞書やらの方が圧倒的にリソースを食うので、過度にリソースを気にすることはないかなと思っています。
@Hiroshiba どうでしょうか...
There was a problem hiding this comment.
そうですね。engine初期化済みにするのであればエンジン未初期化エラーは発生し得ないかなと思います。
そもそも私が書いたコードにするのであればengineが未初期化かどうか判定することもできないかと思いますので
There was a problem hiding this comment.
ちょっと正確に把握していないのですが、SynthesisEngineの初期化を遅延させるかさせないかは、ぶっちゃけどっちでも良い気がしました！
音声を生成するとき、１秒辺り24000*4B=94KBを必ず消費します。生成するとなるともっと消費します。
おそらくSynthesisEngineの初期化だけなら1KBもかからないので、どっちでも良いと思います。
強いて言うなら今は開発速度を早くしたいので、最初から初期化しちゃうほうが関数が少なく実装コストも少し小さいのかなと思いました！
There was a problem hiding this comment.
coreの読み込みが少し遅くなっちゃうんですが、そのあたりも問題ないですかね？
There was a problem hiding this comment.
遅くなる時間にもよると思います。1秒以内とかなら全然問題ないかと…！
d9e4bd9 to
ad745e1
Compare
ad745e1 to
4e6013a
Compare
|
リソース周りの問題は一旦そこまで気にしないということで、SynthesisEngineを最初から初期化するようにしました。 |
|
御二方からapprove頂いたので、セルフマージします...! |
内容
SynthesisEngine初期化時に、OpenJTalkも同時に初期化するようにし、辞書のロードは後から行える(辞書のロードをしなくてもSynthesisEngine内の関数にアクセスできる)ようにします。
現状のままでも辞書のロードをしなくともSynthesisEngine内の関数にアクセスできますが、OpenJTalk自体や、辞書の読み込み状況などの取り回しが若干面倒だったのと、TODOを解決する目的で、このような変更を加えました。
これによるC APIユーザー側から見た挙動の変化は特にありません。