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

fix compat breaking: revive workaround padding in decode() #867

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Yosshi999
Copy link
Contributor

@Yosshi999 Yosshi999 commented Nov 2, 2024

内容

#854 でdecodeからworkaround用の無音paddingを消してしまい、compatible engineの挙動が変わってしまっていた

#866 (comment) に従い、
もともと

  • Synthesizer::decode ... 無音パディングを付加
  • Synthesizer::synthesis ... Synthesizer::decode を利用
  • extern "C" fn decode_forward ... Synthesizer::decode をそのまま利用 (→無音パディングあり)

だったものに対して #854

  • Synthesizer::generate_full_intermediate ... 無音パディングなしで中間物生成
  • Synthesizer::render_audio_segment ... マージンなしで与えられた音声特徴全体で音声生成
  • Synthesizer::decode ... Synthesizer::generate_full_intermediateSynthesizer::render_audio_segment を利用(→無音パディングなし)
  • Synthesizer::precompute_render ... 無音パディングを付加して Synthesizer::generate_full_intermediate
  • Synthesizer::render ... マージンを付けて&指定された区間で Synthesizer::render_audio_segment (+暗黙的にパディングが除去される)
  • Synthesizer::synthesis ... Synthesizer::precompute_renderSynthesizer::render を利用
  • extern "C" fn decode_forward ... Synthesizer::decode をそのまま利用(→無音パディングなし)

になっていたので、

  • Synthesizer::generate_full_intermediate ... 無音パディングを付加して中間物生成
  • Synthesizer::render_audio_segment ... マージンを付けて&指定された区間で音声生成
  • Synthesizer::decode ... Synthesizer::generate_full_intermediateSynthesizer::render_audio_segment を利用
  • Synthesizer::precompute_render ... Synthesizer::generate_full_intermediate を利用
  • Synthesizer::render ... Synthesizer::render_audio_segment を利用
  • Synthesizer::synthesis ... Synthesizer::precompute_renderSynthesizer::render を利用
  • extern "C" fn decode_forward ... Synthesizer::decode をそのまま利用
  • 将来実装する extern "C" fn generate_full_intermediate ... Synthesizer::generate_full_intermediate をそのまま利用
  • 将来実装する extern "C" fn render_audio_segment ... Synthesizer::render_audio_segment をそのまま利用

に変更した

関連 Issue

その他

@Yosshi999 Yosshi999 marked this pull request as ready for review November 3, 2024 07:28
@Yosshi999
Copy link
Contributor Author

#866 (comment)
こちらに方針を変更します

種類 関数 説明
Synthesizer generate_full_intermediate 無音パディングを付加して中間物生成、マージン込みのを返す
Synthesizer render_audio_segment マージンなしで与えられた音声特徴全体で音声生成
Synthesizer decode Synthesizer::generate_full_intermediate と Synthesizer::render_audio_segment を利用
Synthesizer precompute_render  Synthesizer::generate_full_intermediate
Synthesizer render マージンを付けて&指定された区間で Synthesizer::render_audio_segment 
Synthesizer synthesis Synthesizer::precompute_render と Synthesizer::render を利用
extern "C" decode_forward Synthesizer::decode をそのまま利用
extern "C" generate_full_intermediate Synthesizer::generate_full_intermediate をそのまま利用
extern "C" render_audio_segment Synthesizer::render_audio_segment をそのまま利用

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.

うおーーーありがとうございます!!!!
かなりすっきりさせられそうかもと感じました!!

start/endが範囲外ならエラーを返すようにすれば更にシンプルにできそうに感じました!
個人的には、範囲外を指定したときにclipされるのかpadされるのか自明じゃないので、エラー返すのが誤解しづらくて良いかもとちょっと思いました。

あとtrim_waveは要らないはず・・・?

crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
Comment on lines 161 to 169
/// 変換前に追加した安全マージンを生成音声から取り除く
fn trim_margin_from_wave(
wave_with_margin: &ndarray::Array1<f32>,
) -> Result<ndarray::ArrayView1<f32>> {
let wave = wave_with_margin.slice(ndarray::s![
MARGIN * 256..wave_with_margin.len() - MARGIN * 256
]);
Ok(wave)
}
Copy link
Member

@Hiroshiba Hiroshiba Nov 5, 2024

Choose a reason for hiding this comment

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

この関数が必要になることはない・・・はず・・・?
(必ずpaddingもmarginもない音声が出てくるので)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

render_audio_segmentでは単純にonnxを回すだけの関数であるため、crop_with_marginで取り出したマージン付きの音声特徴量を音声に変換した後、両端の(長さが256倍され、かつ一括変換の場合と結果が異なっている)マージンを削除する必要があります。

Copy link
Member

Choose a reason for hiding this comment

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

あれ、なるほどです! ちょっとどこか認識が違うことがわかりました!! 🙇

render_audio_segmentのonnxって、MARGIN*2+Nフレーム入力すると、N*256サンプル出力される・・・とい理解で合ってそうでしょうか 👀
であれば、crop_with_marginは、目的の音声のフレーム数+MARGIN*2だけクロップする感じだと思うので、出てきた音声の両端を削除する必要はない・・・・はず・・・・?

Copy link
Contributor Author

@Yosshi999 Yosshi999 Nov 10, 2024

Choose a reason for hiding this comment

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

onnxはMARGIN*2+Nに対し256*(MARGIN*2+N)が出てきます。元のonnxと同じ実装です

Copy link
Member

@Hiroshiba Hiroshiba Nov 10, 2024

Choose a reason for hiding this comment

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

!!!!! あっなるほどです!!! onnxの方はzero-paddingする感じになってるんですね!!
この前提が抜けていた提案をしまくっていました 🙇 🙇 🙇

これはちょっと話題が変わってしまうのですが、ちなみに元実装の方をpaddingしないようにって難しそうでしょうか・・・? 👀
というのもMARGIN*2が28なので、仮に0.5秒ごとに生成すると93.75*0.5≒46フレームだから、結構な計算量削減になりそうだなーーーと思い。
(再生を始められる、つまりRTFが1を下回るフレーム数をより下げることができるので、より再生開始を早められるなぁーーと・・・)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

marginについても出力の一貫性を保つためのものなので縮められません(多少は無理して減らせると思いますが..)
再生開始までの時間を早くするためにいじれる変数はおそらくバッファ長(何秒ごとに生成するか)しかないと思います。RTFから最適なバッファ長を計算する方程式を立てることができ、陽に解くことができるとおもいます

Copy link
Member

@Hiroshiba Hiroshiba Nov 10, 2024

Choose a reason for hiding this comment

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

あ、paddingはCONVが自動的挟んでるゼロパディングのことを指していました!!

ゼロパディングありで学習した場合でも、十分なMARGINが確保されていれば同じ値が出力されます!
例えばこちらの説明だと、padding有り無しで真ん中9つの値は変わってないのがわかると思います。
https://zero2one.jp/ai-word/padding/

影響を受けるのはゼロパディングを計算に含めた場所、つまり両端14フレームだけ・・・なはず。
Nフレーム入力すれば中央N-14*2フレームは同じが結果になります。
なので逆に両端14フレーム伸ばすことで、Nフレーム同じ結果が出力されるはず・・・!!!

decoderの再学習なしでも、ゼロパディングをなくして合成することは可能だという認識です・・・!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

つまりすべてのconvolutionのpaddingをSAMEからVALIDに変えるということですか?試したことはないですね..

Copy link
Member

Choose a reason for hiding this comment

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

ですです!! 結構計算量が削減できるのではと思ってます!!

あ、onnxruntimeだとVALIDが指定可能なんですね!!!
変換前のpytorchで全convのpadを0にするのを考えてたのですが、onnxruntime変換時や変換後でも良さそう。
pytorchでやるなら、hifiganのmodel.modules()を総なめしてConvの.pad=0にするとかなのかなと考えてました。

Copy link
Member

Choose a reason for hiding this comment

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

あ、とはいえ今マストでは無いかもです!
リリースするまでならあとで変えるとかでも良さそうですし、なんならこのままリリースも超問題ってわけではなさそう。
例えば先にエンジン作ってからという手もありだと思います!

crates/voicevox_core/src/synthesizer.rs Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Show resolved Hide resolved
Copy link
Member

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

LGTM!

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!!!

ありがとうございました!!!\

コード中のコメントの細かいお願いがいくつかあったのでプルリクエストを出してみました。
@Yosshi999 さん側でちょっと取り込んでいただければ!(もちろんプルリクエストの内容をさらに変更しても大丈夫です!)

それとなのですが、またpythonを使った音声の生成結果がmainブランチと全く同じものになっているか確認お願いしてもよろしいでしょうか 🙇
念のために確認しておくと、あとあと計算結果が合わないときに「コアは大丈夫」と自信を持って進めそうだなーと・・・!

Comment on lines +138 to +143
if range.start > audio.frame_length || range.end > audio.frame_length {
panic!(
"{range:?} is out of range for audio feature of length {frame_length}",
frame_length = audio.frame_length,
);
}
Copy link
Member

@Hiroshiba Hiroshiba Nov 15, 2024

Choose a reason for hiding this comment

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

(細かいですが)

start>=0も確認した方が良さそう
あと原理上end>lengthを確認したならstart>lengthは自明そう?(なので処理が簡単になりそう)

Copy link
Member

Choose a reason for hiding this comment

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

start>=0

#867 (comment)でも書きましたが、型的に無意味というかむしろそれを書いたらリンタが激怒すると思います。

あと原理上end>lengthを確認したならstart>lengthは自明そう?(なので処理が簡単になりそう)

(おそらく言語問わず) $\texttt{start} \leq \texttt{end}$ とは限らないと思います。

現状だと $\texttt{start} &gt; \texttt{end}$ も、#867 (comment)の表の通り、 $\texttt{start}, \texttt{end} &lt; \texttt{length}$ である限りは空の区間として受け取ることになるかなと。

ただ今考えたら、一貫性のために $\texttt{start} &gt; \texttt{end}$ もついでに弾いておくという判断もありかもしれません。どうしましょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

start > end はRangeインスタンスの生成時に落ちるものと思ってたんですが、ドキュメントを見た感じそうでもなさそうですかね?
バリデーションは積極的に通すみたいな方針にするというのをどこかで話した気がするのでこれもpanicさせましょうか

Copy link
Member

@qryxip qryxip Nov 15, 2024

Choose a reason for hiding this comment

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

そうですね。Range自体はただの二つの整数の組です。あと10..0みたいなのも、Pythonのsliceじゃなくrange的な使い方をする分には空のイテレータを返します。リンタには怒られますが。

バリデーションは積極的に通すみたいな方針にする

ですね。パニックにしちゃいましょう。パニックメッセージ的にはこう?

Suggested change
if range.start > audio.frame_length || range.end > audio.frame_length {
panic!(
"{range:?} is out of range for audio feature of length {frame_length}",
frame_length = audio.frame_length,
);
}
if range.start > range.end {
panic!("index starts at {} but ends at {}", range.start, range.end);
}
if range.end > audio.frame_length {
panic!(
"{range:?} is out of range for audio feature of length {frame_length}",
frame_length = audio.frame_length,
);
}

Python API側はIndexErrorじゃなくてValueErrorにすればよさそう。

Copy link
Member

Choose a reason for hiding this comment

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

あ、30分前にコミットされてましたね。これでよさそう。

Copy link
Member

Choose a reason for hiding this comment

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

今気付いたのですが、ONNX Runtimeに渡すのはマージン入れたやつでは?なので別に早期リターンは要らないのでは…?


あとこれ↓についてはいかがいたしましょう
#867 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ONNX Runtimeに渡すのはマージン入れたやつ

たしかに 無駄な処理を省けるという利点はあります

Python APIではstart > endの場合、IndexErrorというよりはValueError…?

pythonでstart > endで落ちる例が思いつきません

Copy link
Member

Choose a reason for hiding this comment

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

pythonでstart > endで落ちる例が思いつきません

むしろ普通のPythonではありえないからこそ、IndexErrorは避けた方がよいのではないかと思いました。

ValueErrorはJavaで言うIllegalArgumentExceptionだと理解しています。あとissubclass(IndexError, LookupError)なので、AudioFeatureのlengthに関わらずstart > endは不正ということで、ValueErrorでいいんじゃないかなーと思った次第です。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほど、ValueErrorにします

Copy link
Member

@qryxip qryxip Nov 17, 2024

Choose a reason for hiding this comment

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

  • start > endの場合を弾くようにしたので、最初ヒホさんが言ったようにrange.start > audio.frame_lengthは省けそう

これについてですがRustの[T]では

let a = [1, 2, 3];
let _ = a[5000..1000];

slice index starts at 5000 but ends at 1000というメッセージになる(i.e. range start index {i} out of range for slice of length {len}range end index {i} out of range for slice of length {len}よりも優先される)ので、それにならうという意味でもやった方がいいかなと思いました。

crates/voicevox_core/src/synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/synthesizer.rs Show resolved Hide resolved
crates/voicevox_core_python_api/src/lib.rs Show resolved Hide resolved
example/python/run.py Outdated Show resolved Hide resolved
@Yosshi999
Copy link
Contributor Author

streaming有り無しの比較結果

> python .\compare.py .\PR867-ce545a9.wav .\PR867-ce545a9-streaming.wav                                    
compare PR867-ce545a9.wav and PR867-ce545a9-streaming.wav
diff max: 2.6855618e-05
diff mean: 2.3077598e-08

mainブランチとの比較結果

> python .\compare.py .\PR867-ce545a9.wav .\main-7dd6738.wav           
compare PR867-ce545a9.wav and main-7dd6738.wav
diff max: 3.7331134e-05
diff mean: 1.7770306e-07

@Hiroshiba
Copy link
Member

Hiroshiba commented Nov 15, 2024

検証ありがとうございます!!!
誤差的に問題なさそう・・・!!!

現時点の変更も確認しました、良さそう!!

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.

3 participants