-
Notifications
You must be signed in to change notification settings - Fork 201
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
最適な話者ピッチ範囲を送れるように #1121
base: master
Are you sure you want to change the base?
最適な話者ピッチ範囲を送れるように #1121
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,10 @@ | ||
{ | ||
"supported_features": { "permitted_synthesis_morphing": "NOTHING" } | ||
} | ||
"supported_features": {}, | ||
"range": [ | ||
{ | ||
"style_id": 8, | ||
"low": 5.07, | ||
"high": 6.5 | ||
} | ||
] | ||
} | ||
Comment on lines
1
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. データを配置する場所についてこれはエンジンのmetas.jsonに書いてるけど、制作の都合上コアのmetas.jsonに書く方がやりやすいかもしれません。 englishAbout where to place the dataThis is written in the engine's metas.json, but it might be easier to handle if written in the core's metas.json, especially if we're calculating the pitch range from the dataset. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 同意です。ENGINE の |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,17 @@ | ||
{ | ||
"supported_features": { "permitted_synthesis_morphing": "SELF_ONLY" } | ||
} | ||
"supported_features": { | ||
"permitted_synthesis_morphing": "SELF_ONLY" | ||
}, | ||
"range": [ | ||
{ | ||
"style_id": 1, | ||
"low": 5.38, | ||
"high": 6.44 | ||
}, | ||
{ | ||
"style_id": 3, | ||
"low": 5.11, | ||
"high": 6.5 | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,17 @@ | ||
{} | ||
{ | ||
"supported_features": { | ||
"permitted_synthesis_morphing": "SELF_ONLY" | ||
}, | ||
"range": [ | ||
{ | ||
"style_id": 2, | ||
"low": 5.16, | ||
"high": 6.2 | ||
}, | ||
{ | ||
"style_id": 0, | ||
"low": 5.21, | ||
"high": 6.13 | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
{ | ||
"supported_features": { "permitted_synthesis_morphing": "ALL" } | ||
} | ||
"supported_features": {}, | ||
"range": [] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APIとデータ構造について
ピッチ範囲を取得できる
/optimal_pitch
APIを実装していますが、これはAPIを足すのではなく、スタイル関連情報がまとまっているところに足す形だとより扱いやすいかもしれません。例えば
SpeakerStyle
内などです。ただそれをするにはコードを読んでみた感じかなり改革が必要そうでした。でも良いAPI設計のためにやるべきだと感じています。
改革はプルリクエスト範囲から外れているので興味なければ苦痛かもしれません。もし興味あれば以下の議論に参加して頂ければ。
@y-chan さんと @tarepan さんに相談です。
たぶんピッチ範囲を格納するのは、StyleType等が保存されてる
SpeakerStyle
内が最適だと思うがどうでしょうか。ただ、もし仮に
SpeakerStyle
にピッチ範囲を含める場合は問題があります。SpeakerStyle
はCoreSpeaker
内にあるのですが、CoreSpeaker
はコアの読み込みから利用されているため、変更できなくなっています。そもそもAPIと無関係であるべきな
CoreSpeaker
モデルがここに定義されてることが変で、コア用のデータ構造はコア読み込みの部分だけで閉じているべきに感じました。なので
metas/Metas.py
内からCoreSpeaker
の定義をやめ、Speaker
直下に.name
や.speaker_uuid
や.supported_features
を定義するのとかどうでしょうか。たぶんAPIの返り値は一切変えずに
CoreSpeaker
定義を失くせると思っています。この健全化作業を @tarepan さんにお任せしても良いでしょうか🙇
その変更のあと、 @Patchethium さんがこのプルリクエスト内で今の
SpeakerStyle
内に.pitch_range
を追加するというのはどうでしょうか。english
About the API and data structure
We are implementing an
/optimal_pitch
API that can retrieve the pitch range, but instead of adding this as a separate API, it might be more manageable to add it to a place where style-related information is aggregated, such as insideSpeakerStyle
.However, from what I've seen reading the code, this might require quite a bit of reform. But I feel it's worth doing for good API design.
This reform might be outside the scope of a pull request, so it could be a pain if you're not interested. But if you are interested, I would appreciate your participation in the following discussion.
@y-chan and @tarepan
Perhaps the best place to store the pitch range would be inside
SpeakerStyle
, whereStyleType
, etc., are stored, but there are issues with that.SpeakerStyle
is insideCoreSpeaker
, butCoreSpeaker
is being used from the core load, making it unchangeable.It's odd that the
CoreSpeaker
model, which should be unrelated to the API, is defined here. I feel that the data structure for the core should be confined to just the core loading part.So, how about stopping the definition of
CoreSpeaker
insidemetas/Metas.py
and defining.name
,.speaker_uuid
,.supported_features
, etc., directly underSpeaker
?I believe we can remove the definition of
CoreSpeaker
without changing any API responses at all.Could we leave this rationalization task to @tarepan 🙇
After that change, how about @Patchethium adding
.pitch_range
to the currentSpeakerStyle
within this pull request?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍️
同意です。
意味論からすると
SpeakerStyle
内に配置するのが妥当と考えます。👍️
同意です。
現状は、
list[Speaker]
が VV API に露出する型であり、CoreSpeaker
が Core API を包む型となっています。Speaker
の定義時にCoreSpeaker
継承を用いた結果 VV API と Core API が相互依存化し、CoreAPI が不変だからピッチ情報追加 VV API 変更ができない、という状況に陥っています。👍️
同意です。
class Speaker(CoreSpeaker, EngineSpeaker):
による VV API - Core API 相互依存がこれで解けます。👍️
承りました。複数 steps / PRs で構成されるリファクタリングをおこないます。
影響範囲が複数ファイルにまたがるため、いま健全化すると既存 PRs とコンフリクト祭りします。他 PRs が review & merge され着手可能になった段階で、@Hiroshiba さんから @ tarepan へ改めてメンションをお願いします。着手可能メンションを受け次第、実装に着手します。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
私はコアに関する知識が一切ないのでなんとも言えないですが、スタイル情報を持っているコア側から取るのは一番自然なのは同意です。もし @tarepan さん)が実装してくればこれで一番良いと思います。
それでは一応ここをcloseにして、コア側の健全化作業を待つことにします。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
遅くなりました!! 🙇
@tarepan ありがとうございます!!
承知しました!と言いたいところなのですが、ちょっとどのタイミングならいけやすいかPR全部把握しているわけではないのでわからないというのが正直なところです。。 🙇
今ぱっとPRのタイトルを見て回った感じ、API Routerの変更とテストの追加が主だと感じました。
このタスクはWebAPI周り・コア周りのSpeakerデータ構造のタスクなので、意外と今衝突していないかもしれません。
@Patchethium さんもありがとうございます!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Patchethium
お待たせしました。#1202 がマージされ、エンジンのデータ構造が整理されました。
ピッチ範囲をエンジンの
metas.json
に配置するのであれば実装可能かと思います(コアのmetas.json
に置くかどうかはメンテナ判断かと思います)。