[Rust]OpenJtalkのbindingsを実装した #162
Conversation
qwerty2501
changed the title
qwerty2501
force-pushed
the
feature/add_openjtalk
branch
2 times, most recently
from
81ef845
to
1ef25be
Compare
|
windowsだとlinkエラーになるなあ |
|
良いですね！！！ windowsで動かない件は一旦保留にしてマージしても良いかもと感じました。 ちょっとさすがにコード量が多いので参考にお伺いしたいのですが、openjtalk部分だけ別リポジトリに分けるのはかなり手間だったりしますか 👀 |
|
@Hiroshiba CI部分も含んでいるので手間は少しかかります |
|
なるほどです！ いろいろ考えたのですが、まずglue code（
（変更行数が2000行ありますが･･･！） |
説明がちょっと難しいのですがこのglue codeはC++で書く必要があります。
これなのですが、本来薄いラッパーにしたいのはopenjtalk-sys crateではなく、openjtalk crateになります。 変更行数が多くて申し訳ありませんが、openjtalk-sysにあるgeneratedディレクトリ配下のものについては自動出力されたものなので無視していただいて問題ないと思います。それ以外についてはそこまで行数はないのでレビューしやすいかと思います |
|
レビューが停滞しちゃってすみません、一旦判断だけコメントします！ なるほどです、openjtalkを外に出すのは現状だと大変ということなんですね！ openjtalk-sysのあるべき場所としては、余力があればopenjtalkリポジトリに、更に気合があれば新しいリポジトリにあると良いのかなという所感です。 |
There was a problem hiding this comment.
LGTM！！！
しっかり見てないのでwrapper部分とかもしかしたら実装足りないかもですが、まああとで足りないとこを足すのが良いのかなと思いました！
@PickledChair さんも見て頂けると心強いです！！
|
@Hiroshiba 念の為TODOで将来的にopenjtalk (openjtalk-sysではなく) を薄いwrapperにする旨のTODOを記載しておきました |
| @@ -0,0 +1,36 @@ | |||
| fn main() { | |||
There was a problem hiding this comment.
openjtalk-sys と openjtalk クレートをビルドする分にはこの build.rs の記述でビルドが通るのですが、C++ コードを用いている関係で、最終的に共有ライブラリをビルドする時にはリンクのために -lstdc++ 等のコンパイラフラグが必要になります。
この build.rs に直接記述するのが普通の方法だと思うのですが、自分が実装したときは面倒に思い、 https://github.com/dtolnay/link-cplusplus を見つけたので使いました。これは lib.rs に
extern crate link_cplusplus;と記述するだけで、このクレートの build.rs に記載されている C++ 標準ライブラリのリンクのためのコンパイラフラグが追加されるようになる、というものです。これを用いても良いと思いますし、これなしで自前でコンパイラフラグを書くのも良いと思います。
| cmake_minimum_required(VERSION 3.16) | ||
| project(OpenjtalkSys) | ||
|
|
||
| include(FetchContent) |
There was a problem hiding this comment.
#121 (comment) で FetchContent の機能について触れられていたので、自分も気になっていたのですが使い方がわかっておりませんでした。実例を見ることができて勉強になります！
ただ、試しにビルドされた openjtalk クレートを voicevox_core 側から使ってみようとすると、ビルド時にリンクエラーが発生しました：
$ cargo build
... 省略 ...
error: linking with `cc` failed: exit status: 1
... 中略 ...
= note: Undefined symbols for architecture x86_64:
"_Mecab_initialize", referenced from:
(anonymous namespace)::OpenJTalk::OpenJTalk() in libopenjtalk_sys-65082b9991f752de.rlib(openjtalk.cpp.o)
"_NJD_initialize", referenced from:
(anonymous namespace)::OpenJTalk::OpenJTalk() in libopenjtalk_sys-65082b9991f752de.rlib(openjtalk.cpp.o)
"_JPCommon_initialize", referenced from:
(anonymous namespace)::OpenJTalk::OpenJTalk() in libopenjtalk_sys-65082b9991f752de.rlib(openjtalk.cpp.o)
"_Mecab_clear", referenced from:
(anonymous namespace)::OpenJTalk::clear() in libopenjtalk_sys-65082b9991f752de.rlib(openjtalk.cpp.o)
"_NJD_clear", referenced from:
(anonymous namespace)::OpenJTalk::clear() in libopenjtalk_sys-65082b9991f752de.rlib(openjtalk.cpp.o)
"_JPCommon_clear", referenced from:
(anonymous namespace)::OpenJTalk::clear() in libopenjtalk_sys-65082b9991f752de.rlib(openjtalk.cpp.o)
"_Mecab_load", referenced from:
(anonymous namespace)::OpenJTalk::load(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in libopenjtalk_sys-65082b9991f752de.rlib(openjtalk.cpp.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
open_jtalk 内で定義されていると思われる幾つかのシンボルが未定義になっていました。どういうことかと思って、コンパイルされた libOpenjtalkSys.a 内のシンボルを調べてみたところ、上記のエラーで出ていたシンボルは以下のように確かに未定義になっていました：
$ nm libOpenjtalkSys.a
libOpenjtalkSys.a(openjtalk.cpp.o):
0000000000000a5c s GCC_except_table0
0000000000000a6c s GCC_except_table2
0000000000000aa8 s GCC_except_table26
0000000000000a7c s GCC_except_table5
0000000000000a98 s GCC_except_table6
U _JPCommon_clear
U _JPCommon_get_label_feature
U _JPCommon_get_label_size
U _JPCommon_initialize
U _JPCommon_make_label
U _JPCommon_refresh
U _Mecab_analysis
U _Mecab_clear
U _Mecab_get_feature
U _Mecab_get_size
U _Mecab_initialize
U _Mecab_load
U _Mecab_refresh
U _NJD_clear
U _NJD_initialize
U _NJD_refresh
0000000000000560 T _OpenJTalk_clear
0000000000000000 T _OpenJTalk_create
00000000000005c0 T _OpenJTalk_delete
00000000000000a0 T _OpenJTalk_extract_fullcontext
00000000000003f0 T _OpenJTalk_load
... 以下省略 ...
自分の以前の実装 https://github.com/PickledChair/voicevox-tts-rs では問題がなかったので、そちらがなぜ大丈夫だったのか調べたところ、libOpenjtalkSys.a にあたる静的ライブラリの他に、open_jtalk 自体の静的ライブラリ libopenjtalk.a が生成物として存在しており、そちらに該当のシンボルが定義されていました。
FetchContent の機能に詳しくないので、解決方法がわかりません……。もし FetchContent に元の open_jtalk のコードを静的ライブラリにする機能がない場合は、git submodule を使う方法を使わざるを得ないのかもしれない、と考えました。
There was a problem hiding this comment.
これについては直接参照できるようになったので解決できました
crates/openjtalk/src/lib.rs
Outdated
| let labels_ptr = OpenJTalk_extract_fullcontext( | ||
| self.ptr, | ||
| text.as_ptr(), | ||
| (&mut extract_size) as *mut usize, | ||
| ); | ||
| for ptr in std::slice::from_raw_parts(labels_ptr, extract_size) { | ||
| let c_str = CString::from_raw(*ptr); | ||
| result.push(c_str.to_str().unwrap().to_string()); |
There was a problem hiding this comment.
元のコードを書いた自分が悪い（後で気づきました……）のですが、この部分は以下の２点において良くないやり方になってしまっています：
labels_ptrで参照されている配列は C++ のコード側でmallocによって動的確保されたものなので、メモリリークが発生してしまっているCString::from_rawメソッドで C++ 側でメモリ確保した文字列を扱おうとしているが、 https://doc.rust-lang.org/std/ffi/struct.CString.html#method.from_raw によるとこのメソッドは、Rust 側で確保された文字列データの所有権を取り戻すために使うことを想定したもの（引数となるポインタは Rust 側で確保されたメモリ領域を参照するポインタであってほしい）であり、Rust の外部で確保された文字列データのポインタを引数にとった上での動作は未定義動作扱いのようである
これらを考え合わせると、代わりに
void OpenJTalk_free_labels(char **labels_ptr, int labels_size)のようなメモリ解放用の関数を C++ コード側に用意（labelsの配列内の各文字列ポインタについて文字列データを free した後、labels自体の配列データを free するような関数）- Rust 側で取得した
labels_ptrから各文字列を取得するにはCString::from_rawではなくCStr::from_ptrを用いて、そこからto_ownedメソッドで文字列データをコピーする - 1. で定義した関数を用いて
labels_ptrが参照しているヒープ領域を解放
という方法をとるのが最も素直な実装方法かと思います。メモリコピーの回数が増えてしまっているので、もしもっとスマートな方法が思いついた場合はご提案ください……！
There was a problem hiding this comment.
wrapperから書き直しになったので解消されると思います
qwerty2501
force-pushed
the
feature/add_openjtalk
branch
9 times, most recently
from
f73f7bd
to
56af26f
Compare
別のリポジトリですが誰がやりますか？
これは openjtalkのヘッダ内でC標準ライブラリのFILE構造体を使ってるのですが、出力する際にこの型が何かわからなくなるので stdio.hも出力対象に含めているためですね。(stdio.hは各プラットフォーム依存のheaderをincludeしているためbindgenで出力する際にそれらすべてが出力されます。一応出力内容をある程度制御できるのですが必要なものだけとなるとかなり手間がかかる上、どのみち最終的な成果物からは使っていないものは取り除かれるのでこのようになっています) ただ流石に量が多いので、すこし時間がかかりますがリポジトリ分けますか？ |
|
@Hiroshiba とりあえずopen_jtalkのほうにPR出してみました |
あー、なるほどです！！ テストコード（？）、こんな感じでした。 |
qwerty2501
force-pushed
the
feature/add_openjtalk
branch
from
3ea7af7
to
726b630
Compare
qwerty2501
force-pushed
the
feature/add_openjtalk
branch
from
726b630
to
b2d0802
Compare
…_bindings_x86_64-unknown-linux-gnu Automated generate bindings for x86_64-unknown-linux-gnu
…_bindings_x86_64-apple-darwin Automated generate bindings for x86_64-apple-darwin
…_bindings_i686-pc-windows-msvc Automated generate bindings for i686-pc-windows-msvc
…_bindings_aarch64-apple-darwin Automated generate bindings for aarch64-apple-darwin
…_bindings_x86_64-pc-windows-msvc Automated generate bindings for x86_64-pc-windows-msvc
qwerty2501
force-pushed
the
feature/add_openjtalk
branch
from
5295acc
to
234c2a4
Compare
|
@Hiroshiba include順を変えたらwrapper.hppから stdio.h を消すことには成功したのですが、恐らくopenjtalk内部のheaderのどこかで stdio.hをincludeしてるので結局生成されるコード量削減にはつながらなかったです 前のコメント でさすがにコード量が多いのでリポジトリ分けるか質問していたのですがどうしましょうか？ |
おお、なるほどです！
すみません、お答えしてませんでした！ 方法はおまかせできればと思っています。 |
では分ける方向ですかね。onnxruntimeのようにwrapperも分けたリポジトリ先で管理するということでいいですかね？ |
こちらの処理に該当するコードですよね。 |
|
@Hiroshiba すみませんそこではないです。 |
|
あ！勘違いしました、onnxruntime-rsにおけるonnxruntime-sys crateとonnxruntime crateに該当するとこを別リポジトリに、↑のコードはVOICEVOX coreに、ということですね！！一番良いと思います･･･！！ |
|
リポジトリ作成したのでcloseします |
|
あ、ちなみにPickledChair さんからのコメントは反映されている感じでしょうか 👀 |
|
概ね大丈夫だと思います。 |
内容
OpenJtalkのbindingsとwrapperを実装しました
関連 Issue
refs #128
その他
@PickledChair さんの実装を元に作ってみました
cmakeは外部ライブラリをそのまま出力できるような機能がなく、もとの実装のようにC++で薄いwrapperを実装しました
外部コマンドを使えばうまくできるかもしれませんが、OpenJtalkのリポジトリ側でそのCMakeLists.txtを元にrustのbindingsを生成したほうが良いかもしれません。
bindコードは更新を検知するとCI上で自動生成し、自動でPRされるようになっています