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

[Rust]C++ Windows向けのexampleを修正します #208

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

shigobu
Copy link
Contributor

@shigobu shigobu commented Jul 26, 2022

内容

rust版coreに対応するための修正をしました。

initialize関数に引数を追加しました。
initialize関数の返り値を見てfalseの場合にメッセージを表示するようにしました。

voicevox_error_result_to_message関数の返り値をワイド文字に変換してからコンソール画面に表示する関数OutErrorMessageを作成しました。

関連 Issue

ref #128

その他

別でissueを出すべきだと思いますが、日本語を含むパスで動作しませんでした。

if (!initialize(false, 0, true)) {
std::wcout << L"coreの初期化に失敗しました" << std::endl;
return 0;
}

VoicevoxResultCode result = VoicevoxResultCode::VOICEVOX_RESULT_SUCCEED;

std::wcout << L"openjtalk辞書の読み込み" << std::endl;
result = voicevox_load_openjtalk_dict(GetOpenJTalkDict().c_str());
Copy link
Member

@PickledChair PickledChair Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

別でissueを出すべきだと思いますが、日本語を含むパスで動作しませんでした。

とのことでしたが、ここの GetOpenJTalkDict の戻り値の文字コードで問題が起こっていそうですね……。GetOpenJTalkDict の実装を見ると文字コードを Shift-JIS としているみたいですが、以前の C++ 版コアの voicevox_load_openjtalk_dict 関数は引数のパス文字列の文字コードが Shift-JIS だったのでしょうか?

また、現在の Rust 版の voicevox_load_openjtalk_dict の実装は以下なのですが、UTF-8 の文字列としてデコードできない入力ははじくようになっています(Rust の文字列の内部エンコーディングが環境によらず UTF-8 なので):

#[no_mangle]
pub extern "C" fn voicevox_load_openjtalk_dict(dict_path: *const c_char) -> VoicevoxResultCode {
let (_, result_code) = {
if let Ok(dict_path) = unsafe { CStr::from_ptr(dict_path) }.to_str() {
convert_result(lock_internal().voicevox_load_openjtalk_dict(dict_path))
} else {
(None, VoicevoxResultCode::VOICEVOX_RESULT_INVALID_UTF8_INPUT)
}
};
result_code
}

もし UTF-8 の文字列を渡した場合も試していた場合、それでも日本語を含むパスをうまく扱えなかった感じでしょうか?(もしそうであれば、引数の文字コードは UTF-8 のままとしたいですが、内部でOpenJTalk の API に渡す際は Windows 版だけ扱う文字コードを変更するように実装する必要がありそうですね……!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UTF-8でも試しましたが、文字化けします。
きれいに、UTF-8をShift-JISで見たときの化け方です。

C++版コアのときも、パスはShift-JISで渡していました。

rust側で文字コードの変換を行うということですね。
GetOpenJTalkDict の戻り値をUTF-8に変更します。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

文字化けしてるのRustで実装されてるcore側の実装バグっぽそうなのでなおしたらutf-8で行けそうなきがします

Copy link
Member

@PickledChair PickledChair Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

文字化けしてるのRustで実装されてるcore側の実装バグっぽそうなのでなおしたらutf-8で行けそうなきがします

そうですね! 個人的にはおそらく以下の open_jtalk-rs 内の open_jtalk_sys::Mecab_load を呼ぶ部分で Windows 版だけパス文字列を Shift-JIS に変換するのが良い気がしています(Rust コード内では一貫して UTF-8 で扱った方が都合が良いので)。

https://github.com/VOICEVOX/open_jtalk-rs/blob/4cd7ff11943f2c0b976f9c8e9593f47339a60444/crates/open_jtalk/src/mecab/mod.rs#L40

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OS別に分けるのではなくto_strをas_os_strにするだけでいいはずです

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメント化されていますが、WPATH_WIN32が定義されていたらMeCab::Utf8ToWideに渡して文字コードを変換するようです。なんでコメント化されているのでしょうか?

言われてみれば、確かに元々は文字コードを変換するようにしていたのをコメントアウトしたようですね……。

https://github.com/VOICEVOX/open_jtalk/blob/dc5c4fadcb0d281f5ca19a58c9df94292b1358fc/src/mecab/src/common.h#L147-L154

経緯は不明です……(コミットログを見ると、CreateFileA 関数を使うようにした変更は 2012 年、WPATH マクロをコメントアウトしたのは 2016 年のようでした。コミットは大雑把にまとめられており、有用な情報は得られませんでした。おそらく、元の MeCab のソースを OpenJTalk 開発時の都合で変更したものと思われます。ユーザーが Shift-JIS の文字列を入力した時に変換なしでそれを扱いたかった?)。

確かにそっちのほうが正攻法そうですね

そうですね……! OpenJTalk に渡す文字列を全て UTF-8 に統一できそうなのでスッキリしそうです。WPATH はファイルパスだけに関わるマクロだと思うので、これを変更したところで不具合は出ない、と思いたいですね……。

Copy link
Member

@PickledChair PickledChair Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あ、ただ、open_jtalk のコードをいじると、それに依存している pyopenjtalk にも影響が出ますね!

https://github.com/VOICEVOX/pyopenjtalk/blob/master/.gitmodules

pyopenjtalk は、今回の Mecab_load 関数に関わるところで言えば、たとえば以下のような箇所でシステムロケールの文字コードを要求しているようです(他の箇所にもある)。open_jtalk を直すなら、pyopenjtalk もそれに合わせて修正しなければなりませんね……。

https://github.com/VOICEVOX/pyopenjtalk/blob/50b0296a9e1b666e5a09a41ec9e9284a2a9b608f/pyopenjtalk/__init__.py#L103

これを考えると、今はとりあえず open_jtalk-rs 内で対応するのが無難のような気がしてきました……

Copy link
Member

@Hiroshiba Hiroshiba Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

記憶を呼び覚ましていました。
結論から言うと、ボイボのpyopenjtalkのことは無視してopenjtalk/mecabの中のコードを変えちゃうのが良いのかなと思いました!

理由は単純に、本家のpyopenjtalk(とopenjtalk)は全部UTF8で扱う実装になっているのに対して、VOICEVOXはpyopenjtalkだけがsystem localの文字コードを使うようになっているためです。
本家に合わせるのがいろいろ楽そうなので、pyopenjtalk側を合わせちゃうのが良いのかな~と。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2転しちゃってすみません 🙇‍♂️
が、たぶんですが、system localの文字コードを扱い始めるのはなかなかにしんどさを連れてくる可能性があるので、ゴリッと全部UTF8にしちゃいたいな~という気持ちです・・・!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRつくりました
VOICEVOX/open_jtalk#7

wide_to_multi_capi関数が不要になったので削除。
Copy link
Member

@PickledChair PickledChair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 手元の Windows 環境でも、README の指示に従って example を実行できることを確認しました。PR ありがとうございました……!

日本語が含まれるパスに example を配置すると動作しない問題が残っていますが、 https://github.com/VOICEVOX/voicevox_core/pull/208/files#r930838316 で指摘されたように open_jtalk-rs で対応した方が良い問題だと思うので、example の側の作業はこれで完結していると思います……! 解決しました!

@PickledChair
Copy link
Member

PickledChair commented Jul 28, 2022

approve 済みですが、VOICEVOX/open_jtalk#7 がマージされたので、一応確認のため、自前で新しいコアをビルドして、それを使って日本語を含むパスで example をビルド・実行してみました。正常に実行できることを確認できました!

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

修正ありがとうございます、とても助かります!!

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

Successfully merging this pull request may close these issues.

4 participants