search

VOICEVOX / voicevox_core

無料で使える中品質なテキスト読み上げソフトウェア、VOICEVOXのコア
https://voicevox.hiroshiba.jp/
MIT License
872 stars 116 forks source link

add: C APIの`#[repr(Rust)]`なものへのアクセスをすべて安全にする #849

Closed qryxip closed 1 month ago

qryxip commented 1 month ago

内容

C APIにおいて、Rust APIのオブジェクトそのものの代わりにトークンのような1-bit長の構造体ユーザーに渡すようにすることで、次のことを実現する。

  1. "delete"時に対象オブジェクトに対するアクセスがあった場合、アクセスが終わるまで待つ
  2. 次のユーザー操作に対するセーフティネットを張り、パニックするようにする
    1. "delete"後に他の通常のメソッド関数の利用を試みる
    2. "delete"後に"delete"を試みる
    3. そもそもオブジェクトとして変なダングリングポインタが渡される

ドキュメントとしてのsafety requirementsもあわせて緩和する。

このPRは #836 の解決 ではなく、ドキュメントにも手を加えていない。というのもVoicevoxVoiceModelFileには次のゲッターメソッドがあり、これらをカバーするのは現状のAPIの形だと不可能だからである。

関連 Issue

Resolves #836.

その他

qryxip commented 1 month ago

元々やりたかったこととしては #832 みたいな設計をC APIでもやることでした。しかしそのためにはVoicevoxVoiceModelFileに生えている二つのゲッターメソッドが邪魔であることが今回わかりました。(このPRはこのPRでマージするとして、その後)どうにかするための対応としては次のように考えています。

  1. 二つのゲッターメソッドをゲッターではなくする

    voicevox_voice_model_file_idはuint8_t (*output)[16]のような引数に吐き出すようにし、voicevox_voice_model_file_get_metas_jsonの方はcreate_metas_jsonに改名してvoicevox_json_freeが必要なようにする。

  2. 単なるゲッターではなくbracket patternにする

    関数オブジェクトをどう表現するかがすごい面倒そう…

  3. char*自体の代わりにVoicevoxStringRefみたいなオブジェクトを返すようにし、VoicevoxStringRefは"delete"を必要とする

    VoicevoxStringRef *metas = voicevox_voice_model_file_get_metas_json(model);

char *metas_cstr = voicevox_string_ref_get_data(metas);
do_something(metas_cstr);

// `char*`を使い終えたことをVOICEVOX CORE C APIに伝える
voicevox_string_ref_delete(metas);

    bracket patternほどではないけど使うのが面倒そう… あとVoicevoxStringRefもまた同じように管理するの？という気もしてくる。

  4. [追記] voicevox_voice_model_file_deleteと〃_closeの二種類を作る

    Closableや__exit__のようなクローズ処理に対応する"close"と、本当のデストラクトに対応する"delete"の二種類を作る。"close"の方は他言語バインディング作成専用という感じで。

  5. 諦める

    このPRで対応を打ち切り、「Synthesizer::load_voice_model中にVoiceModelをデストラクトしたらどうなるのか」の問題は各バインディング製作者に任せる。

個人的には1.か4.なのですが、VoicevoxCoreSharpを作っている @yamachu さんにも意見を伺いたいなーと。

yamachu commented 1 month ago

今回の変更ですが、言語バインディングライブラリを作る者として扱いやすいC APIになるなと感じました、ありがとうございます。

https://github.com/VOICEVOX/voicevox_core/pull/849#issuecomment-2395172908 こちらの VoicevoxVoiceModelFile の扱いですが、私も 1 or 4 、良さそう具合で言えば 1 > 4 かなと考えています。 正直な所、自分の不勉強で申し訳ないのですが 2 がどんな感じになるのかがイメージできてないからという理由もあります。 3 は扱いが面倒だなという一言に尽きるかな…と…

1 は他の API との一貫性が取れているように感じるので、良いのかなとは思いました。

Hiroshiba commented 1 month ago

自分もちゃんとわかってない気もしますが、１が良さそうに思いました！ っていうかまあ、これでstring周りの処理が全部共通になるって感じですかね？ 確か単純にポイントが得られるパターンと、deleteが必要なパターンがあった記憶。

qryxip commented 1 month ago

464d6f7 (#849): CApiObject::BodyをRustApiObjectに改名しました。

850 をマージできたので、#836 をやらないとですね…

qryxip commented 1 month ago

あとはドキュメントを更新すれば #836 をresolveできるはず。

qryxip commented 1 month ago
qryxip commented 1 month ago

大丈夫です！