Skip to content

Conversation

@zztkm
Copy link
Contributor

@zztkm zztkm commented Jan 30, 2025

  • [ADD] サイマルキャストの映像のエンコーディングパラメーター scaleResolutionDownTo を追加する

This pull request includes changes to add a new encoding parameter, scaleResolutionDownTo, for simulcast video in the WebRTC implementation. The changes involve updates to both the documentation and the codebase to support this new parameter.

Documentation Updates:

  • CHANGES.md: Added an entry for the new encoding parameter scaleResolutionDownTo.

Codebase Updates:

  • Sora/PeerChannel.swift: Added support for logging and setting the scaleResolutionDownTo parameter in the RTCRtpSender extension.
  • Sora/Signaling.swift:
    • SignalingOffer struct: Added the scaleResolutionDownTo property.
    • SignalingOffer struct: Updated the toParameters method to include the scaleResolutionDownTo parameter.
    • SignalingOffer.Configuration: Added a private struct ScaleResolutionDownTo for JSON decoding and updated the Codable extension to handle this new parameter. [1] [2] [3]

@zztkm zztkm self-assigned this Feb 4, 2025
@zztkm zztkm requested a review from torikizi February 4, 2025 08:03
@zztkm zztkm changed the title [WIP] scaleResolutionDownTo 追加 scaleResolutionDownTo 追加 Feb 4, 2025
Copy link
Contributor

@torikizi torikizi left a comment

Choose a reason for hiding this comment

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

修正ありがとうございます。一点だけコメントしました。

/// 映像解像度を送信前に下げる度合
public let scaleResolutionDownBy: Double?

/// エンコーディングを制限する最大の寸法
Copy link
Contributor

Choose a reason for hiding this comment

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

/// エンコーディングを制限する最大のサイズ
とかの方が良いかもしれません。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

そうします!

Copy link
Contributor

@torikizi torikizi left a comment

Choose a reason for hiding this comment

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

修正ありがとうございます。LGTM です。

@zztkm zztkm merged commit aeb0647 into develop Feb 4, 2025
4 checks passed
@zztkm zztkm deleted the feature/simulcast-encodings-scale-resolution-down-to branch February 4, 2025 10:12
@zztkm
Copy link
Contributor Author

zztkm commented Jul 10, 2025

!sonnet レビューして

@github-actions
Copy link

github-actions bot commented Jul 10, 2025

Claude finished @zztkm's task —— View jobclaude/pr-228-20250710_034745 • [Create PR ➔](https://github.com/shiguredo/sora-ios-sdk/compare/develop...claude/pr-228-20250710_034745?quick_pull=1&title=PR%20%23228%3A%20Changes%20from%20Claude&body=This%20PR%20addresses%20pr%20%23228%0A%0AGenerated%20with%20%5BClaude%20Code%5D(https%3A%2F%2Fclaude.ai%2Fcode)


コードレビュー完了

コードレビュー結果

優先度: 高

  1. コメント不整合 (Sora/Signaling.swift:384-385)
    • 現在のコメント: /// エンコーディングを制限する最大のサイズ
    • 問題: scaleResolutionDownTo の実際の機能を正確に表現していない
    • 修正提案: /// エンコーディングの最大解像度制限 または /// エンコーディング時の最大解像度

優先度: 中

  1. プライベート構造体のネーミング (Sora/Signaling.swift:998-1002)

    • ScaleResolutionDownTo 構造体がプライベートだが、パブリックな同名プロパティと紛らわしい
    • 修正提案: _ScaleResolutionDownToDecodableScaleResolutionDownTo など、より明確な名前
  2. 型安全性の向上 (Sora/PeerChannel.swift:1390-1391)

    • scaleResolutionDownTo の設定で force unwrap を使用
    • 修正提案: ガード文や if let を使用してより安全に処理

優先度: 低

  1. ログメッセージの一貫性 (Sora/PeerChannel.swift:1387-1389)
    • 他のログメッセージと形式が微妙に異なる
    • 修正提案: 他のパラメーターと同じ形式に統一

実装品質

良い点:

  • WebRTC の RTCResolutionRestriction を適切に使用
  • JSON デコード処理が適切に実装されている
  • 既存のコードパターンに従っている
  • エラーハンドリングが適切

改善の余地:

  • コメントの正確性
  • 型安全性の向上
  • ネーミングの明確性

変更点とリリースノートの整合性

確認済み: CHANGES.md の記載内容と実装が一致している

総合評価

実装は全体的に適切だが、上記の指摘事項の修正により、コードの品質をさらに向上できる。特に優先度: 高の項目は修正を推奨する。


Job run

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