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

simulcast を offer の内容で上書きする #93

Closed

Conversation

miosakuma
Copy link
Contributor

@miosakuma miosakuma commented Jan 16, 2023

auth webhook から simulcast を true に設定した場合に映像が 3 本とならない不具合がありました。

この PR にて offer の simulcast の内容で上書きを行うことで事象は改善しています。
バリエーションについてはこれから試験をしますが、対応の方法に問題ないかご確認いただけますでしょうか。

@miosakuma miosakuma requested a review from shino January 16, 2023 09:54
@shino
Copy link
Contributor

shino commented Jan 17, 2023

修正の方向性は正しいと思います 👍

で、この手の offer による上書きはいろんなところで発生するので、いまのコードベースの状態と
今回どうすべきかについて、ちょっと考えてみました。

話として2点あります。

  • SoraMediaOption (などの Option 系) の値の書き換えをやるかどうか
  • offer で上書きされる(可能性のある)値の取り扱い方法

まず SoraMediaOption の書き換えについて。

このクラスは、sdk のアプリとの界面で設定を受け渡すものです。
可能性として、アプリが参照を持ち続けているかもしれないですし、再利用するかもしれません。
そのときに sdk が値を書き換えていると、アプリ側コードからは思いもしない変更になってるかもしれません。

それとはまた別の話ですが、様々なプロパティはできる限り書き換えしない(kotlin でいうと var じゃなくて val をつかう)
ほうがデバッグ(特にマルチスレッド系)が楽になります。

でも、SoraMediaOption のフィールドは var じゃねーか! というのはありますが、これはフィールドが沢山あるので、
コンストラクタで全部もらうんじゃなくて apply{} で順次設定していくという設計になってるからだと思います。

以上を踏まえて、SoraMediaOption の中の値を sdk で書き換えるのはちょっと気になります。


次に offer で上書きされる値の取り扱い方法について。

たぶん、きれいな設計としては

  • SoraMediaOption と offer の中身を足したようなデータクラス(仮に SoraData とよぶ)を作る
  • offer をもらったタイミングで offer で上書きするものを全部上書きした SoraData を作ってしまう
  • そのオブジェクトを以降の処理で使いまわす (SoraSignalingChannelImpl 等の個別ロジックでは offer による
    上書きは気にしない)
    というものじゃないかと思います。

ところが、すでに data channel signaling とか各所でアドホックな(改築増築系の) offer 上書き処理がある

cf.

// コンストラクタ以外で dataChannelSignaling, ignoreDisconnectWebSocket を参照すべきではない
// 各種ロジックの判定には Sora のメッセージに含まれる値を参照する必要があるため、以下を利用するのが正しい
// - offerDataChannelSignaling
// - switchedIgnoreDisconnectWebSocket

ので、それと上の設計を混ぜるのは良くないです。

そのため、やるとしたら

  • 腰を据えて offer 上書き系をリファクタリングしてから simulcast オプションも同じ扱いにする (かなり面倒)
  • data channel signaling のように、connect で渡す値と offer でもらった値を別個に管理する (今回の変更としては楽)

のどっちかになるかなと思います。

@miosakuma
Copy link
Contributor Author

#129 で再作成したのでこちらはクローズします

@miosakuma miosakuma closed this Aug 2, 2024
@miosakuma miosakuma deleted the feature/update-simulcast-enabled-with-offer-message branch August 15, 2024 06:47
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