[WIP] offer メッセージの simulcast を利用する #129
Open
+8
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
miosakuma
changed the title
miosakuma
changed the title
zztkm
reviewed
Jul 22, 2024
| @@ -79,6 +79,7 @@ class PeerChannelImpl( | |||
| private val appContext: Context, | |||
| private val networkConfig: PeerNetworkConfig, | |||
| private val mediaOption: SoraMediaOption, | |||
| private val simulcastEnabled: Boolean = false, | |||
There was a problem hiding this comment.
この simulcastEnabled が offer message から入ってきた値を表すのであれば、offerSimulcastEnabled という命名の方が良さそうに思いましたが、どうでしょうか？
offer message であることを意識した命名は、他の箇所にもあったのでリンク貼っておきます。
There was a problem hiding this comment.
PeerChannel クラスは offer かどうかを意識する必要がないためこの名称にしています。
offer の値を渡すかどうかを判断するのは SoraMediaChannel クラスであるべきという考えです。
PeerChannel は以下リンクのように各所から必要な時に用途に応じて呼び出せるものだと考えています。
offerDataChannelSignaling は SoraMediaChannel 内の変数なので名称を分けるのが正しいと思います。
There was a problem hiding this comment.
ありがとうございます。変数の命名についての考え方は理解できました。
ちょっと上のコメントの補足をすると
僕が当初、「この simulcastEnabled が offer message から入ってきた値を表すのであれば」と書いたのは、この変数が offer message に含まれている simulcast の値以外が入ることを期待していないのではないか、と思ったからでした。
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
動作するところまで確認したので相談の Draft PR です。
対応内容
simulcast について connect でクライアントから払い出した値を認証ウェブフックで 上書きしても
クライアントに反映されない不具合を修正した。
ユーザーから指定された simulcast を利用していたので offer メッセージから取得した simulcast を利用するように修正した。
相談内容
offer で取得した simulcast の値を EncoderFactory まで引き回すように各種引数を追加したが、この対応で問題ないか。
現在ユーザが設定した接続オプション(MediaOption) をそのままコア部分まで渡しているがそこから抜本的に変えた方がよいか。
修正経緯として、はじめは #93 にて MediaOption の simulcastEnabled を offer の値で上書きしていましたが、
ユーザが設定した接続オプションを SDK 側で書き換えるのはよくない、という指摘を受け、MediaOption を更新しない書き方で対応しています。
#93 (comment)
現在の接続の流れ
関係ない項目は割愛していますが、以下のような内容です。
修正後の接続の流れ
4 の処理について PeerChannel, RTCComponentFactory のコンストラクタ引数に simulcastEnabled を追加しています。
そもそも MediaOption が最後まで連携されるのはおかしい、MediaOption + simulcastEnabled の値を持った別の構造体を用意すべき、という意見もあるかなと思い、対応内容がこれでよかったかコメントをもらえるとうれしいです。
This pull request primarily focuses on adding simulcast support to the
SoraMediaChannelandPeerChannelImplclasses in thesora-android-sdkproject. The changes include adding a newsimulcastEnabledparameter to several methods and classes, and replacing the use ofmediaOption.simulcastEnabledwith this new parameter.Changes related to
simulcastEnabled:sora-android-sdk/src/main/kotlin/jp/shiguredo/sora/sdk/channel/SoraMediaChannel.kt: AddedsimulcastEnabledas a parameter to theSoraMediaChannelconstructor.sora-android-sdk/src/main/kotlin/jp/shiguredo/sora/sdk/channel/rtc/PeerChannel.kt: AddedsimulcastEnabledas a parameter to thePeerChannelImplconstructor and replacedmediaOption.simulcastEnabledwithsimulcastEnabledin several places. [1] [2] [3] [4]sora-android-sdk/src/main/kotlin/jp/shiguredo/sora/sdk/channel/rtc/RTCComponentFactory.kt: AddedsimulcastEnabledas a parameter to theRTCComponentFactoryconstructor and replacedmediaOption.simulcastEnabledwithsimulcastEnabled. [1] [2]Changes related to
OfferMessage:sora-android-sdk/src/main/kotlin/jp/shiguredo/sora/sdk/channel/signaling/message/Catalog.kt: Addedsimulcastas a serialized field to theOfferMessagedata class.