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

default option系定数をC/C++以外(主にC#)から取得する場合に煩雑になる #549

Closed
1 of 3 tasks
shigobu opened this issue Jul 26, 2023 · 8 comments · Fixed by #557
Closed
1 of 3 tasks

Comments

@shigobu
Copy link
Contributor

shigobu commented Jul 26, 2023

内容

Coreの新しいvvm-async-apiでは、voicevox_versionやdefault optionを取得する方法が、関数からDLL埋め込み定数に変更されました。C/C++で取得する場合は、通常の変数と同じように使えます。
しかし、C#で取得する場合、LoadLibraryGetProcAddressを使用する必要がありコード量が増えます。また、LoadLibraryGetProcAddressはWin32APIであり、クロスプラットフォーム対応する場合にはOSごとに処理を記述する必要があり、さらにコード量が増え複雑になります。 C#の新しいフレームワーク(.NET Core 3.0以降)では、LoadLibraryGetProcAddressクロスプラットフォーム対応版が標準で用意されているようでした。

関数として定義されている場合には、簡単なコードで使用できます。
以前のように、関数として定義されていると嬉しいです。

Pros 良くなる点

C#から使う場合に、簡単なコードでdefault optionを取得できる。

Cons 悪くなる点

構造体の初期値を取得するだけなのに、関数を呼ぶ必要がある。

実現方法

#503 で実装されているので、これを無かったことにする??

VOICEVOXのバージョン

2023/07/26現在のmainブランチ

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux

その他

#503 (comment)
での発言にあるように、定数のドキュメントがヘッダーファイルに反映されない問題があります。これも、関数に戻すことで解消されるかと思います。

また、Windowsで使用するためのdllinport定義が自動で作成されない問題もあります。
これも、関数に戻すことで解消されると思います。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 26, 2023

わかりやすい説明&提案ありがとうございます!

ちょっと調べてみた感じ、PythonのctypesやJavaのJNI?やNode.jsも定数の取得が困難そうでした。定数を返す関数を自分でcppで書いてもらえればいけるかもしれない、くらい。
RubyのFFIはできそう。Rustもできそう。

定数から関数にする場合のメリット・デメリットを列挙してみました。

  • メリット
    • 動的ライブラリから定数が取得できない言語でも使えるようになる
    • 定数externはcbindgenのサポートが手厚くないことで起こる課題を回避できる
  • デメリット
    • 間違えてdeleteできないようにする工夫やドキュメントの再整備が必要

定数を扱うのが難しい言語には別口でラッパーを提供できると嬉しいけどまだできてないので、まあ関数が良いんだろうなーという気持ちです。

一方で、Rust内で定数として取り回してexternまで持っていくのは結構考えて努力したこともあり、ちょっと名残惜しくもあります。
といってもまあ無駄になるわけではなく、定数を返す関数を定義する感じなのかなぁと想像しています。

@qryxip さんや @PickledChair さんの意見も聞いてみたいです。

(もし戻すとなったら僕の知識不足が原因なので、申し訳ないです。。。)

@PickledChair
Copy link
Member

@Hiroshiba

ちょっと調べてみた感じ、PythonのctypesやJavaのJNI?やNode.jsも定数の取得が困難そうでした。定数を返す関数を自分でcppで書いてもらえればいけるかもしれない、くらい。
RubyのFFIはできそう。Rustもできそう。

この辺りあまり詳しくないのですが、このように言語間で FFI 機能の差異がありそうというのは盲点でした……。もしかしたらどの言語でも定数の取得自体は可能なのかもしれませんが、やりやすさに差異があるかもしれません(前に Haskell の FFI を利用した実装を読む機会があったのですが、C 側で定数を返すだけの関数を定義しているのを見たことがありました。実際のところ Haskell で定数の取得が難しいのかどうかはわかっていませんが……)。その点では、関数呼び出しで値を取得する API はどの言語でも簡単に扱えそうだと思いました。

よく使われる言語それぞれのその辺りの事情をなるべく網羅的に調べる、というのはオーバーワークだと思ったので、関数で値を返す実装に戻すのが現実的なのかな、と感じました。

また、 @shigobu さんの指摘に

LoadLibraryとGetProcAddressはWin32APIであり、クロスプラットフォーム対応する場合にはOSごとに処理を記述する必要があり、さらにコード量が増え複雑になります。

とあるように、言語だけでなくプラットフォーム間の差異も関わってきそうです。関数で返す実装にすることで簡潔な実装を実現できるのであれば、そうするのが良さそうだと思いました。

@qryxip
Copy link
Member

qryxip commented Jul 26, 2023

関数に戻すのに賛成します。

この件については私の経験が不足していたと思っています。Stringとconst char*の相互変換まで提供しておきながら、変数へのアクセスができないというのは想像ができませんでした。

具体的な方法としては

といってもまあ無駄になるわけではなく、定数を返す関数を定義する感じなのかなぁと想像しています。

というよりは、 #503 を丸ごと取り消す勢いでconst-defaultごと剥がすのがいいと思っています(revertはきついと思うので上から剥がす形になると思います)。const-defaultはC APIのためだけに入れていたので、後腐れない方がよいかと。

voicevox_versionvoicevox_get_version(void)の両方を用意し、後者はFFI専用だと案内するという手もあるとは思うのですが、メリットはあまり無いと思います。


ちなみに定数に対応している言語についてですが、ctypesからはoptionsもversionも普通に読めました。Haskellも試してはいませんができるように見えました(Ptr aの形になるようです)。ただnode-ffiも @windymelt さんが使っているJNAも無理そうですし、C#の[DllImport]が無理なことには変わらないと思います。


あと本題からはずれるかもしれませんが、C#のような言語だとラッパーとしては"default option"は使わずにオプショナル引数にするのがすっきりするんじゃないかなと思っています。Python APIは今こうなっています。

async def synthesis(
self,
audio_query: AudioQuery,
style_id: int,
enable_interrogative_upspeak: bool = True,
) -> bytes:

@qryxip
Copy link
Member

qryxip commented Jul 26, 2023

定数を扱うのが難しい言語には別口でラッパーを提供できると嬉しいけどまだできてないので

個人的には結構手を広げたいんですよね。考えているのは次のものです。

もしこれらが全部できるなら、C APIについてはスキー場にある"EXPERT ONLY"の立て札のような感じで「Cに馴染みが無いならNodeとかPythonから使いなさい」みたいな案内にできるんじゃないかなーと思っています。

@Hiroshiba
Copy link
Member

お二人ともありがとうございます!! 関数に戻すのが丸いのかなと思いました!!

方針としては @qryxip さんの仰るとおり、 #503 をまるごと取り除く勢いで戻していくのがおそらくコードとメンテナンス上一番いいのかなと思います。
定数版を残しておくのもありだと思いますが、まあ関数一本化するのが綺麗かなと!
(ctypesは普通に読めるのもなるほどです)


各言語へのラッパーは面白いなと思いつつ、やっぱり0.15のアップデートが一区切りしてからかなと思います。
マイナー?バージョンアップデートとして追加でいろんな言語へのラッパーをリリースしていくのありかもですかね!
そういえばそういうラッパを作っていくissueがなかったので、もしよかったら作っちゃいますか・・・!

@sevenc-nanashi
Copy link
Member

Mobile用に作るならJava(Kotlin)とSwiftかなと思います。(構造的に)

@shigobu
Copy link
Contributor Author

shigobu commented Jul 27, 2023

C#ラッパーについては、私が協力できます。
すでに、私のリポジトリで以前のAPIで作成していて、テストも出来ています。

@qryxip
Copy link
Member

qryxip commented Jul 27, 2023

#550 を作ってみました(Swiftも入れました)。

@shigobu ありがとうございます。そのときがいつになるかはわかりませんが、助力頂けるのなら大変助かります。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants