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

ENH: speaker_infoのリソースをURLで返すようにする #1318

Merged
merged 92 commits into from
Jun 23, 2024

Conversation

sabonerune
Copy link
Contributor

@sabonerune sabonerune commented May 24, 2024

内容

/speaker_info /singer_infoの画像と音声ファイルをURLで返し、そのURLから取得できるように変更します。
評価・議論のため開発速度重視で雑に実装しています。
現時点ではマージするべきではない状態なのでDraftです。

関連 Issue

その他

エディタ側の対応PR

Copy link
Contributor Author

@sabonerune sabonerune left a comment

Choose a reason for hiding this comment

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

非常に雑なコードでテストも書いていません。
ここから問題点の洗い出しや設計方針の決定ができていければと思います。

build_util/file_map.py Outdated Show resolved Hide resolved
speaker_info/filemap.json Outdated Show resolved Hide resolved
voicevox_engine/app/routers/speaker.py Outdated Show resolved Hide resolved
@@ -78,6 +78,7 @@ def __init__(self, engine_speakers_path: Path) -> None:
**json.loads((folder / "metas.json").read_text(encoding="utf-8"))
)
for folder in engine_speakers_path.iterdir()
if folder.is_dir()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

speaker_infoにファイルがあるとエラーが発生する。

voicevox_engine/app/routers/speaker.py Outdated Show resolved Hide resolved
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.

プルリクエストありがとうございます!!

よりすっきりした設計できるなと感じました!!
ということで提案です・・・!(なんかどこか考慮漏れありそう)


たぶん、元のリソースファイル名にハッシュ値を足す形だと思うのですが、元のファイル名無視してハッシュ値だけのファイル名にすれば設計がかなり減る気がしました!
ルールが減るのと、ディレクトリ構造を意識しなくて済むのと、speakerUuidfilemap.jsonの有無が関係なくなるので便利になるんじゃないかなと。

こんな感じをイメージしてます。
↓元

resources/speaker_infos/speakerA/icon.png
resources/speaker_infos/speakerA/potrtait.png
...

↓filemap作成後(ビルド時)

resources/HASH_A1
resources/HASH_A2
resources/filemap.json
...

↓resources/filemap.jsonの中身

{
  "speaker_infos/speakerA/icon.png": "HASH_A1.png",
  "speaker_infos/speakerA/potrtait.png": "HASH_A2.png",
  ...
}

実装手順はこんな感じ?

  1. filemapUtilityみたいなクラスを作って、中でfilemap.jsonを読み込んでマッピングを持っておき、resourcePathを受け取ってハッシュ値を返す。ハッシュ化されてなかったらそのままのパスを返すようにする。

  2. filemapUtilityを使うとこ(/speaker_info)で、アイコンなどのリソースパスをfilemapUtilityに渡してハッシュ値を返してもらい、URL を作って返す。

  3. あとはdef staticAPIはresourcePath(ものによってはハッシュ値になってる)を受け取るようにし、ファイル名に従ったリソースファイルを返して終わり!たぶんなんならFastAPIのStaticFilesに置き換えられる。

  4. ビルド時にfilemap.jsonを作る

  5. (オプション)製品版はハッシュ値になったリソースファイルのみを使いたいので、ハッシュ値になってないファイルへのgetが来たらエラーを返しても良いかも。

ハッシュ値はただのファイル名になってるので、def staticspeakerUuidなどを気にしなくて良くなってます。
あとfilemap.jsonを用意しなくても動くので、OSSでfilemap.jsonを更新し続けるメンテナンスをしなくて大丈夫になります。
あとspeaker_info以外がリソースパスのハッシュ化を使う時もそのまま流用できるはず!

@@ -50,6 +50,9 @@ class SupportedFeatures(BaseModel):
manage_library: bool | None = Field(
title="音声ライブラリのインストール・アンインストール"
)
resource_url: bool | None = Field(
Copy link
Member

Choose a reason for hiding this comment

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

他が全部動詞始まりなので、return_resource_urlが良いかも。
まあでもこの値に関係なく古いエンジンでも新しいエンジンでもresource_url=Trueを引数に渡せば良い気もするので、もしかしたらいらないかも・・・?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FastAPIは大丈夫ですが他のフレームワークは大丈夫なのでしょうか?
それとも未知のクエリを受け取ったとき無視することが一般的なのでしょうか?

Copy link
Member

Choose a reason for hiding this comment

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

少なくともデフォルトで無視しないフレームワークは無いかもです。

気になってるのは、これを考慮することでエディタ側でのコードが複雑化しないかという点だけだったりします。
実装してみた感じ、どうでしたか・・・?(manifestへのアクセスが簡単だったらわりと簡単そう)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

そもそもソングAPIの有無をマニフェストのsingで確認しているので簡単ですね。

Copy link
Member

Choose a reason for hiding this comment

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

たしかに問題なさそうでした!

まあ、マニフェストにreturn_resource_url的なの追加しますかー・・・!

というかよく考えたらこのフラグがないとresource_urlに対応しているかわからないから、返ってきたものがURLかどうかを判定するコードを書かないとかもですね。エディタ以外のサードパーティとかだと。
となるといろいろ不便なので、マニフェストにフラグあるべきな気がしてきました!!

voicevox_engine/app/routers/speaker.py Outdated Show resolved Hide resolved
voicevox_engine/app/routers/speaker.py Outdated Show resolved Hide resolved
@sabonerune
Copy link
Contributor Author

@Hiroshiba

filemapUtilityみたいなクラスを作って、中でfilemap.jsonを読み込んでマッピングを持っておき、resourcePathを受け取ってハッシュ値を返す。ハッシュ化されてなかったらそのままのパスを返すようにする。

speaker_info以外がリソースパスのハッシュ化を使う時もそのまま流用できるはず!

filemapUtilityクラスのようなもので管理するのはいいと思いますがハッシュ化されてなかったらそのままのパスを返すようにするspeaker_info以外がリソースパスのハッシュ化を使うのは意図しないファイルを公開してしまったりキャッシュ周りの問題を起こすリスクがあるので少なくともこのPRでは考慮するべきではないと思いました。
また、ライブラリの管理機能が実装された場合ハッシュ生成が動的に変化することになるので結局他のリソースとは分離しなければならないと思います。
(そのためエンドポイントを/character_resources/にしました)

またspeakerUuidがあるのはエンジンのポートが衝突したときハッシュだけだと偶然衝突が起きる可能性があるのではと思ったからです。
今考えるとエンジンIDの方がシンプルでいい気がしますが…

@Hiroshiba
Copy link
Member

ilemapUtilityクラスのようなもので管理するのはいいと思いますがハッシュ化されてなかったらそのままのパスを返すようにするspeaker_info以外がリソースパスのハッシュ化を使うのは意図しないファイルを公開してしまったりキャッシュ周りの問題を起こすリスクがあるので少なくともこのPRでは考慮するべきではないと思いました。

ちゃんとわかってないかもなのですが、例えば製品版ではfile_map.jsonにあるものだけ公開する、とかにしておけば安全だと思います。
まあ製品版判定コードが必要になりますが、開発の快適さを捨てるのは結構リスク高いので結構避けたい気持ちがあります。
(まあもちろん、そういう実装が難しくなさそうだな〜という前提がありますが)

ハッシュだけだと偶然衝突

まあ少なくともSHA256にしておけばハッシュの衝突は考えなくて大丈夫です!

また、ライブラリの管理機能が実装された場合ハッシュ生成が動的に変化することになるので結局他のリソースとは分離しなければならないと思います。

これは完全に盲点でした・・・。だいぶ僕の中の前提が崩れました、ご指摘ありがとうございます!!!!!

ちょっと考え直してみたので書きます 🙏

@Hiroshiba
Copy link
Member

Hiroshiba commented May 29, 2024

リソースURL系APIは/resource/以下にまとめたいかもです!
FastAPIの都合上、routerを一括で扱えるので便利なんですよね。
将来的にはこの下に、例えばAPI /resource/library/とかつなげてく想定です。

file_map.jsonはやっぱりなくても動いてほしいかなぁと・・・。開発が楽なのは正義なので・・・。
考えたのですが、開発版ならresourcesディレクトリ以下全部が/resource/APIで見える形にして、製品版なら例えば_staticディレクトリが見える形にすれば安全かなとか思いました。
ファイル名がハッシュになったものが_staticディレクトリに配置されていく感じです。

あとファイルパスをハッシュ値.拡張子にするのはどっちでも良いかなと思い始めました!
ファイル数が30キャラ×7ずつくらいあるので、キャラごとにディレクトリ分けるのはやぶさかではない感じです。
あ、file_map.jsonの中身はキャラ名で区切らず{"元のパス": "移動先のパス"}のがデータ構造も引数も減ってらくだと思います!
・・・json.loadが重くない限り。

キャラクターリソースのAPIは/resource/character/{speakerUuid}/{hash}.{拡張子}でも良いかもですが、/character/はなくても良いかもな〜とちょっと思ってます。
今これしかないので・・・。

一旦こんなところで。。。 🙇

@sabonerune
Copy link
Contributor Author

sabonerune commented May 29, 2024

@Hiroshiba
色々考えていたのですがリソースのURLについては少なくとも現状は深く考える必要はないことに気づきました。
というのもspeaker_infoに関しては直接URLにアクセスすることを想定しないためURLを変更してしまっても破壊的変更にならないからです。ハッシュの衝突もなのでSHA256辺りにすれば気にしなくてもよさそうということなので/resourceでまとめてよさそうですね。
エンジンのポート衝突を考えると/resourceの前か後ろにエンジンIDを入れた方がよさそう。

とりあえず開発時にはfile_map.jsonは不要であることは必須にしようと思います。


ところでリソース系APIについてドキュメントの欠如やキャッシュ周り扱いにくさからポータルページ等が使用するリソースやスクリプト等を配置するくらいしか思い浮かばないのですが他にどのような用途を考えていますか?

`filemap.json`の構造変更
ハッシュアルゴリズムをsha256へ
リソースの管理をクラスで行う
@sabonerune
Copy link
Contributor Author

sabonerune commented May 30, 2024

@Hiroshiba

クラスで管理するように変更しました。
また、filemap.jsonがなくても開発時は起動時にファイルのハッシュを生成することで対応するようにしました。
(現状はとりあえず常に開発中であることにしています)
filemap.jsonの保存場所はspeaker_infoと同じディレクトリです。
(resourcesへ移動するPRが入ることを見越して)

@sevenc-nanashi
Copy link
Member

製品版検知、これが使えそう?:https://pyinstaller.org/en/stable/runtime-information.html

@sabonerune
Copy link
Contributor Author

@sevenc-nanashi

def _is_development() -> bool:
"""
動作環境が開発版であるか否かを返す。
Nuitka/Pyinstallerでコンパイルされていない場合は開発環境とする。
"""
# nuitkaビルドをした際はグローバルに__compiled__が含まれる
if "__compiled__" in globals():
return False
# pyinstallerでビルドをした際はsys.frozenが設定される
elif getattr(sys, "frozen", False):
return False
return True

説明不足ですみません、これが使えることは気づいていますがマージまで時間がかかることは目に見えているのでコンフリクト解消の手間を減らすためにとりあえず固定値にしたという感じです。
(path_utilityの内部関数なので適切に切り出す必要がある)

@Hiroshiba
Copy link
Member

ところでリソース系APIについてドキュメントの欠如やキャッシュ周り扱いにくさからポータルページ等が使用するリソースやスクリプト等を配置するくらいしか思い浮かばないのですが他にどのような用途を考えていますか?

意図がちょっとわかってないのですが、今後いくらでも増える可能性があるだろうな〜とは思います。サンプル音楽の配布とか。
どういう拡張があるか現時点では思いつかないかもですが、増えうるとは感じるので、とりあえず静的ファイルは1つのAPIにまとめときたかった感じです。

@Hiroshiba
Copy link
Member

@sabonerune

エンジンのポート衝突を考えると/resourceの前か後ろにエンジンIDを入れた方がよさそう。

requestからURL取ってると思ってて、そこにport番号も付く感じだと思うので大丈夫な気がします。
それでもなお間違っちゃう可能性の考慮であれば、まあわからなくもないですが、他のAPIはそれを考慮してないので大丈夫な気がしました!

engine_manifest.json Outdated Show resolved Hide resolved
build_util/file_map.py Outdated Show resolved Hide resolved

def main() -> None:
parser = ArgumentParser()
parser.add_argument("--save", type=Path)
Copy link
Member

@Hiroshiba Hiroshiba Jun 2, 2024

Choose a reason for hiding this comment

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

意味的にはoutput_pathかな
ファイルパスなら_file、ディレクトリパスなら_dirにしとくとわかりやすいかもです

def main() -> None:
parser = ArgumentParser()
parser.add_argument("--save", type=Path)
parser.add_argument("--target", default=SPEAKER_INFO_DIR, type=Path)
Copy link
Member

Choose a reason for hiding this comment

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

こちらもtarget_dirにしとくとちょっとわかりやすいかもです
まあrun.pyと統一感があるかな、くらいですが。

def mapping(target: Path) -> dict[str, str]:
return {
to_posix_str_path(filepath): filehash
for filepath, filehash in walk_character_files(target, ("wav", "png"))
Copy link
Member

@Hiroshiba Hiroshiba Jun 2, 2024

Choose a reason for hiding this comment

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

("wav", "png")も与えられるようにしておくと使い回すときに便利&コマンド実行時に対象がわかって便利かも。
まあ過剰かもなので、とりあえず定数としてファイルの一番上に定義する形でも良さそう。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

一応--target_suffixで変更可能にしておきました。

build_util/file_map.py Outdated Show resolved Hide resolved
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.

すみません、まだだったかもなのですがドキュメントにコメントしました!!

docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@sabonerune sabonerune left a comment

Choose a reason for hiding this comment

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

指摘部分の修正完了しました。

docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
docs/ResourceManagerの仕様について.md Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 21, 2024

再レビューOKになったら右上のReviewersのre-review requestなどで合図をいただけると!

@sabonerune sabonerune requested a review from Hiroshiba June 22, 2024 02:32
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です!!!

細かい部分だけコメントさせていただきました!!

docs/ファイルURLとfilemap.md Outdated Show resolved Hide resolved
test/unit/resource_manager/test_resource_manager.py Outdated Show resolved Hide resolved
test/unit/resource_manager/test_resource_manager.py Outdated Show resolved Hide resolved
voicevox_engine/app/routers/speaker.py Outdated Show resolved Hide resolved
voicevox_engine/resource_manager.py Outdated Show resolved Hide resolved
@sabonerune sabonerune requested a review from Hiroshiba June 23, 2024 01:56
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!!!

こちらで少し変えさせていただきます!!


@tarepan
実装周りの共有です!

APIドキュメントには乗らないGET /_resources/APIで、キャラごとの画像・音声ファイルを返せる形になります。
/speaker_infoなどで引数でurlを指定するとURLが返ります。

キャッシュをなくすために、URLはリソースファイルのハッシュ値を含めています。
ハッシュはResourceManagerにて、パスとハッシュを対応付ける形で管理しています。
その対応マップをfilemapと呼んでいます。
filemapは事前にgenerate_filemap.pyで作成できます。

/_resources/APIは現状speakerrouterで行っています。
より良い設計が思いつかず、一旦この形の実装で落ち着いた感じです 🙇

test/e2e/test_characters.py Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit 3b90dcf into VOICEVOX:master Jun 23, 2024
4 checks passed
@Hiroshiba
Copy link
Member

プレビュー版のビルドを開始しました!

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.

SpeakerInfoの画像や音声のデータをURLで返すようにする?
3 participants