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

[project-s] ハミング用APIを追加 #1008

Merged
merged 20 commits into from
Jan 22, 2024

Conversation

y-chan
Copy link
Member

@y-chan y-chan commented Jan 14, 2024

内容

題の通りです。

関連 Issue

ref VOICEVOX/voicevox_core#712

その他

ドキュメントなどは適当に作ったので、少し修正した方がいいかもしれません。

@y-chan y-chan requested a review from a team as a code owner January 14, 2024 09:11
@y-chan y-chan requested review from Hiroshiba and removed request for a team January 14, 2024 09:11
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.

まだちゃんと処理コード追えてないのですが、とりあえずコメントしてみました!
最初の二つ以外はAPIに関わらないので、コメントだけ足しといてあと回しでも全然良いかなと思います。
(そのための別ブランチですし)

voicevox_engine/metas/Metas.py Show resolved Hide resolved
"""

key: int | None = Field(title="音階")
length: int = Field(title="音符の長さ")
Copy link
Member

Choose a reason for hiding this comment

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

(細かいけど)説明は「フレーム長」とかのが秒と勘違いしなくて良いかも?
まあintなのでわかるのですが…。あ、JSだとどちらもnumberなのでちょっとだけわかりやすい……かも…?

いっそframe_lengthにした方が良いかも。

Copy link
Member Author

Choose a reason for hiding this comment

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

一旦説明を「フレーム長」に変える方向で対処しようかなと...!
Phonemeモデルについても、lengthプロパティを持つので、もし変えるならプロパティ名は統一した方がいいかもですね...

Copy link
Member

@Hiroshiba Hiroshiba Jan 14, 2024

Choose a reason for hiding this comment

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

考えてたんですが、FrameAudioQueryの方はlengthでも良いけど、Phonemeの方はframeレベルなのか自明ではないので、FramePhonemeにするかframe_lengthにしたほうが良い気がしました!
たぶんFramePhonemeにしつつframe_lengthにもするのが良さそう。

あとAPI構成はここだけですね!
エディタ側の変更も少なくなるし、とりあえずここだけ決めて変えておきたい気持ちがちょっとあります。

Copy link
Member Author

Choose a reason for hiding this comment

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

FramePhonemeにしつつframe_lengthにもする

これが良さそうに思いました、これで行きたいと思います...!

voicevox_engine/tts_pipeline/tts_engine.py Outdated Show resolved Hide resolved
voicevox_engine/tts_pipeline/tts_engine.py Outdated Show resolved Hide resolved
@@ -374,6 +426,143 @@ def synthesize_wave(
wave = raw_wave_to_output_wave(query, raw_wave, sr_raw_wave)
return wave

def get_sing_phoneme_and_f0_and_volume(
Copy link
Member

Choose a reason for hiding this comment

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

getは意図と反してるかも。書くなら…create…?

あとf0とphonemeと〜ってのをやめて、「歌い方」みたいなドメイン用語作っても良いかも。
create_歌い方 みたいな(英語考えてないですが。。)
あるいは「フレーム特徴量」みたいな用語作ってcreate_sing_frame_featureとか。

(まあAPIに露出しないので、fixmeコメントだけ書いて後回しでも。)

Copy link
Member Author

Choose a reason for hiding this comment

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

とりあえずgetcreateに置換して、FIXMEコメントにしてしまおうかなと...!

voicevox_engine/tts_pipeline/tts_engine.py Show resolved Hide resolved
Copy link

github-actions bot commented Jan 14, 2024

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 526 336 coverage-36%
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/cancellable_engine.py 94 72 coverage-23%
voicevox_engine/core_adapter.py 81 34 coverage-58%
voicevox_engine/core_initializer.py 59 30 coverage-49%
voicevox_engine/core_wrapper.py 257 183 coverage-29%
voicevox_engine/dev/core/init.py 2 0 coverage-100%
voicevox_engine/dev/core/mock.py 36 8 coverage-78%
voicevox_engine/dev/tts_engine/init.py 2 0 coverage-100%
voicevox_engine/dev/tts_engine/mock.py 28 0 coverage-100%
voicevox_engine/engine_manifest/EngineManifest.py 35 0 coverage-100%
voicevox_engine/engine_manifest/EngineManifestLoader.py 12 0 coverage-100%
voicevox_engine/engine_manifest/init.py 3 0 coverage-100%
voicevox_engine/library_manager.py 92 5 coverage-95%
voicevox_engine/metas/Metas.py 36 0 coverage-100%
voicevox_engine/metas/MetasStore.py 18 6 coverage-67%
voicevox_engine/metas/init.py 2 0 coverage-100%
voicevox_engine/model.py 180 9 coverage-95%
voicevox_engine/morphing.py 71 46 coverage-35%
voicevox_engine/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/preset/Preset.py 13 0 coverage-100%
voicevox_engine/preset/PresetError.py 2 0 coverage-100%
voicevox_engine/preset/PresetManager.py 80 2 coverage-98%
voicevox_engine/preset/init.py 4 0 coverage-100%
voicevox_engine/setting/Setting.py 11 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 17 0 coverage-100%
voicevox_engine/setting/init.py 3 0 coverage-100%
voicevox_engine/tts_pipeline/acoustic_feature_extractor.py 34 0 coverage-100%
voicevox_engine/tts_pipeline/kana_converter.py 88 1 coverage-99%
voicevox_engine/tts_pipeline/mora_list.py 7 0 coverage-100%
voicevox_engine/tts_pipeline/text_analyzer.py 146 6 coverage-96%
voicevox_engine/tts_pipeline/tts_engine.py 266 93 coverage-65%
voicevox_engine/user_dict.py 145 12 coverage-92%
voicevox_engine/utility/init.py 5 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/core_version_utility.py 8 1 coverage-88%
voicevox_engine/utility/mutex_utility.py 13 0 coverage-100%
voicevox_engine/utility/path_utility.py 35 9 coverage-74%
voicevox_engine/utility/run_utility.py 10 7 coverage-30%
TOTAL 2464 860 coverage-65%

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.

こちらのAPIに関わってくる部分以外はLGTMです!

気になったとこはissueの方にコメントで列挙しておいてみました。あとで潰していければ!

LGTMですが、子音がないモーラが来たときにバグりそうな気がしました!
こんな感じでCoreのモック書いちゃえば、意外とすぐテスト(兼チェック用のコード)書けるかもです。

あ、コメントがいっぱい書いてあって読みやすかったです!
すごい細かいですが、2行のコメントにしてるところはコミッターによるおしゃれの差が出ちゃうので、1行にしちゃったほうが良い気がしました。

run.py Show resolved Hide resolved
@@ -704,6 +706,77 @@ def _synthesis_morphing(
background=BackgroundTask(delete_file, f.name),
)

@app.post(
"/sing_frame_audio_query",
Copy link
Member

Choose a reason for hiding this comment

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

判断メモです。
ここをsingにすべきかsongにすべきかですごく迷うのですが、これは「歌うためのクエリ」であり、「歌のクエリ」ではないので、singが合っているのかなと思いました。

もし仮に名詞にするならsongではなくsong_snyhtesisとかかなと。長いのでsingで良さそう。

vowel_length = note_durations[i]
phoneme_durations.append(vowel_length)

phoneme_durations_array = np.array(phoneme_durations, dtype=np.int64)
Copy link
Member

@Hiroshiba Hiroshiba Jan 14, 2024

Choose a reason for hiding this comment

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

後回しで良いのですが、この関数の引数のnote_durationsはNDArrayな一方、phoneme_durationsはlistで、逆にphoneme_durations_arrayはNDArrayとなかなかややこしいことになっていそうです。

以前のエディタ辞書機能追加時に後回しにしたタスクはたしか全部は片付いてないんですよね・・・・・・。
後回しタスクは絶対やるという気持ちでいきましょう・・・!!

Comment on lines +265 to +267
# もし、次のノートの子音長が負になる場合、現在のノートの半分にする
if next_consonant_length < 0:
next_consonant_length = consonant_lengths[i + 1] = note_duration // 2
Copy link
Member

@Hiroshiba Hiroshiba Jan 14, 2024

Choose a reason for hiding this comment

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

ここのワークアラウンド、普通に考えるとノート長の半分よりも1フレームにするとかのが良い気がしました。
もし最低1フレーム保証にするだけなら、コアの方に書くべきかも?

Copy link
Member Author

Choose a reason for hiding this comment

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

普通に考えれば1にするのが正しそうですが、子音長を1にすると、滑舌が怪しくなることがあるので、できれば避けたいかもです。
とはいえ、前のノートが長い場合、子音長もつられて長くなりそうなので、少し検討した方がいいなとは思いますね...

とはいえ負になるのはまずモデルが悪いので、コア側で調整するのが正しいそうですね...

Copy link
Member

@Hiroshiba Hiroshiba Jan 14, 2024

Choose a reason for hiding this comment

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

普通に考えれば1にするのが正しそうですが、子音長を1にすると、滑舌が怪しくなることがあるので、できれば避けたいかもです。

気持ちはわかるのですが、それはそれで1だと推論したものがそのままになるので、意図と実装がずれてる雰囲気を感じます。
触ってないのでわからないのですが、子音長が1だった場合は2にする、とかが良い気がしてます。

とりあえずマージを目指すのが良さそうな気がするので、非負になる処理をコアに足すことをFIXMEコメントにしてとりあえず後回しにするのはどうでしょう。

Suggested change
# もし、次のノートの子音長が負になる場合、現在のノートの半分にする
if next_consonant_length < 0:
next_consonant_length = consonant_lengths[i + 1] = note_duration // 2
# もし、次のノートの子音長が負になる場合、現在のノートの半分にする
# FIXME: 非負にする処理をコア側に足す
if next_consonant_length < 0:
next_consonant_length = consonant_lengths[i + 1] = note_duration // 2

Comment on lines +243 to +246
def calc_phoneme_lengths(
consonant_lengths: NDArray[np.int64],
note_durations: NDArray[np.int64],
) -> NDArray[np.int64]:
Copy link
Member

Choose a reason for hiding this comment

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

あれ、これって子音長とノート長だけから、子音・母音の全音素の長さを返す感じですよね。
母音のみのノートだった場合どうなるんでしょう・・・?

Copy link
Member Author

@y-chan y-chan Jan 14, 2024

Choose a reason for hiding this comment

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

母音のみのノートは子音長が0になるようなマスクがモデル内部でかかっているので、ノートの頭から母音が始まるだけの処理になります。
暗黙的な処理ではあるので、どこかで説明は必要そうですね....
あと、子音長が0と誤って予測された場合のことを考えていないので、それも含めて改善してからマージすべきな気はします。

少し調整します。

Comment on lines +505 to +509
# 予測した子音長を元に、すべての音素長を計算する
phoneme_lengths = calc_phoneme_lengths(consonant_lengths, note_lengths_array)

# 時間スケールを変更する(音素 → フレーム)
frame_phonemes = np.repeat(phonemes_array, phoneme_lengths)
Copy link
Member

Choose a reason for hiding this comment

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

phoneme_lengthsは必ずノート数×2だけど、phonemes_arrayは音素数になってて数が合ってない気がする・・・?

y-chan and others added 2 commits January 14, 2024 22:01
@Hiroshiba
Copy link
Member

バグっている箇所もあるんですが、とりあえずAPIをブランチから生やしたいのでマージさせていただきます!!

@Hiroshiba Hiroshiba merged commit 7bc1b21 into VOICEVOX:project-s Jan 22, 2024
1 of 3 checks passed
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.

2 participants