data_channels 項目に header 項目を追加 #104

torikizi · 2024-10-29T01:46:05Z

data_channels 項目に header 項目を追加しました。
dc.header は string から json にパースして設定しています。

This pull request introduces a new Header property to the DataChannel class and updates related files to support this addition. The most important changes include updates to the Sora.Config, DataChannel class, and associated methods in both C# and C++ code, as well as the protocol buffer definition.

Enhancements to DataChannel:

Sora/Sora.cs: Added Header property to the DataChannel class.
Sora/Sora.cs: Updated Connect method to set the Header property if it is not null.

Protocol buffer updates:

proto/sora_conf_internal.proto: Added header field to the DataChannel message.

C++ code updates:

src/sora.cpp: Updated DoConnect method to parse and set the Header property from JSON.

Documentation updates:

CHANGES.md: Documented the addition of the Header property to DataChannels.

miosakuma

対応ありがとうございます。

torikizi · 2024-12-26T02:51:55Z

src/sora.cpp

@@ -419,6 +419,13 @@ void Sora::DoConnect(const sora_conf::internal::ConnectConfig& cc,
      if (dc.has_compress()) {
        d.compress = dc.compress;
      }


他は項目を追加に関しての設定なので、一番確認していただきたいのはこの下の追加部分になります。

melpon · 2024-12-26T11:38:22Z

src/sora.cpp

@@ -419,6 +419,13 @@ void Sora::DoConnect(const sora_conf::internal::ConnectConfig& cc,
      if (dc.has_compress()) {
        d.compress = dc.compress;
      }
+      if (dc.has_header()) {
+        boost::json::value parsed_value = boost::json::parse(dc.header);
+        if (parsed_value.is_array()) {


配列しか許可しないなら、最初から header を配列として定義して下さい。

配列として定義し直しました。

melpon · 2024-12-27T10:16:09Z

Sora/Sora.cs

@@ -62,6 +62,7 @@ public class DataChannel
        public int? MaxRetransmits;
        public string? Protocol;
        public bool? Compress;
+        public string? Header;


こっちも配列になってないですね

melpon · 2024-12-27T10:17:54Z

Sora/Sora.cs

@@ -453,6 +454,10 @@ public void Connect(Config config)
            {
                c.SetCompress(m.Compress.Value);
            }
+            if (m.Header != null)
+            {
+                c.SetHeader(m.Header);


これコンパイル通ります…？

melpon · 2024-12-27T10:22:20Z

src/sora.cpp

+      if (dc.has_header()) {
+        const auto& header_content = dc.header.content;
+        d.header->insert(d.header->end(), header_content.begin(),
+                         header_content.end());


proto の各要素の型は JSON 文字列にシリアライズされた string 型で、SoraSignalingConfig::DataChannel::header の各要素の型は boost::json::value なんだから、絶対に JSON 文字列をデシリアライズする処理が必要なはずです。

melpon · 2024-12-28T15:49:08Z

Sora/Sora.cs

+    public class Header
+    {
+        public List<string> content { get; set; } = new List<string>();
+    }


C# は proto と違って string 配列が nullable なので、Header クラスを作る必要は無いです。DataChannel クラスで直接 List<string>? Header すれば良いです。
proto 側で Header クラスを作ってるのは nullable な string 配列（optional repeated string header）が定義できないからです。

melpon · 2024-12-28T15:49:38Z

proto/sora_conf_internal.proto

@@ -2,6 +2,10 @@ syntax = "proto3";

 package sora_conf.internal;

+message Header {
+  repeated string content = 1;
+}


DataChannel の下に作らないなら Header という名前は汎用的すぎるので DataChannelHeader にした方が良いです

ForwardingFilter の Rules と同じように DataChannel の下に作るよう修正します。

melpon · 2024-12-28T15:51:36Z

src/sora.cpp

+        const auto& header_content = dc.header.content;
+        d.header->reserve(header_content.size());
+        for (const auto& json_str : header_content) {
+          auto parsed = boost::json::parse(json_str);


これだとパースエラー時に例外投げちゃうので、他の parse と合わせて error_code を渡すようにして下さい。

melpon · 2024-12-30T13:39:49Z

src/sora.cpp

@@ -419,6 +419,19 @@ void Sora::DoConnect(const sora_conf::internal::ConnectConfig& cc,
      if (dc.has_compress()) {
        d.compress = dc.compress;
      }
+      if (dc.has_header()) {
+        d.header = std::vector<boost::json::value>();
+        d.header->reserve(dc.header.content.size());


reserve するような最適化はいらないので削除して下さい

ご指摘のとおり削除します。

melpon · 2024-12-30T14:03:38Z

src/sora.cpp

+          boost::system::error_code ec;
+          auto parsed = boost::json::parse(json_str, ec);
+          if (ec) {
+            RTC_LOG(LS_WARNING) << "Invalid JSON in header: " << json_str;


他のやつと書き方のフォーマットが違いますね。

Suggested change

RTC_LOG(LS_WARNING) << "Invalid JSON in header: " << json_str;

RTC_LOG(LS_WARNING) << "Invalid JSON: header=" << json_str;

失礼しました。メッセージを修正します。

melpon · 2024-12-30T15:17:16Z

Sora/Sora.cs

+                var header = new SoraConf.Internal.DataChannel.Header();
+                header.content.AddRange(m.Header);


初期化と同時に代入の方がいい感じになりそう。

Suggested change

var header = new SoraConf.Internal.DataChannel.Header();

header.content.AddRange(m.Header);

var header = new SoraConf.Internal.DataChannel.Header() {

content = m.Header,

};

初期化と同時に代入するように修正します。
c9087e4 で対応しました。

melpon · 2024-12-30T17:00:17Z

Sora/Sora.cs

@@ -454,6 +454,10 @@ public void Connect(Config config)
            {
                c.SetCompress(m.Compress.Value);
            }
+            if (m.Header != null)
+            {
+                c.SetHeader(new SoraConf.Internal.DataChannel.Header { content = new List<string>(m.Header) });


これ content = m.Header とは書けないですか？

ここでまた List 作らなくても良いですね。修正します。

melpon

良さそう

torikizi · 2025-01-06T01:58:42Z

レビューありがとうございました。こちらマージします。

torikizi added 3 commits October 28, 2024 17:43

data_channels に header を追加する

484cbb5

CHANGES の更新

7a3a41e

parse を入れてヘッダーを取得するように修正

8bca104

torikizi requested review from melpon and miosakuma October 29, 2024 01:46

miosakuma approved these changes Dec 16, 2024

View reviewed changes

torikizi commented Dec 26, 2024

View reviewed changes

melpon reviewed Dec 26, 2024

View reviewed changes

header を配列として定義する

fda2904

melpon reviewed Dec 27, 2024

View reviewed changes

torikizi added 2 commits December 27, 2024 22:53

Merge branch 'develop' into feature/add-datachannels-header

457a557

設定方法を見直す

4db3bc5

melpon reviewed Dec 28, 2024

View reviewed changes

torikizi added 5 commits December 29, 2024 17:05

Header の定義は DataChannel の中

1a328cf

他と同様パースエラーの時にエラーを出す

4951e6f

Header クラスは不要

eb88c0d

設定を見直し

36bbab5

ちゃんと初期化する

b9e59d7

melpon reviewed Dec 30, 2024

View reviewed changes

設定方法の見直しと、不要な処理を削除

c9087e4

melpon reviewed Dec 30, 2024

View reviewed changes

List 作らなくて良い

d900cd2

melpon approved these changes Dec 31, 2024

View reviewed changes

torikizi merged commit 1778cbe into develop Jan 6, 2025
7 checks passed

torikizi deleted the feature/add-datachannels-header branch January 6, 2025 01:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data_channels 項目に header 項目を追加 #104

data_channels 項目に header 項目を追加 #104

torikizi commented Oct 29, 2024 •

edited

Loading

miosakuma left a comment

torikizi Dec 26, 2024

melpon Dec 26, 2024

torikizi Dec 27, 2024

melpon Dec 27, 2024

melpon Dec 27, 2024

melpon Dec 27, 2024

melpon Dec 28, 2024

melpon Dec 28, 2024

torikizi Dec 29, 2024

melpon Dec 28, 2024

melpon Dec 30, 2024

torikizi Dec 30, 2024

melpon Dec 30, 2024

torikizi Dec 30, 2024

melpon Dec 30, 2024

torikizi Dec 30, 2024 •

edited

Loading

melpon Dec 30, 2024

torikizi Dec 30, 2024 •

edited

Loading

melpon left a comment

torikizi commented Jan 6, 2025

	RTC_LOG(LS_WARNING) << "Invalid JSON in header: " << json_str;
	RTC_LOG(LS_WARNING) << "Invalid JSON: header=" << json_str;

		var header = new SoraConf.Internal.DataChannel.Header();
		header.content.AddRange(m.Header);

data_channels 項目に header 項目を追加 #104

data_channels 項目に header 項目を追加 #104

Conversation

torikizi commented Oct 29, 2024 • edited Loading

miosakuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torikizi Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torikizi Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

melpon left a comment

Choose a reason for hiding this comment

torikizi commented Jan 6, 2025

torikizi commented Oct 29, 2024 •

edited

Loading

torikizi Dec 30, 2024 •

edited

Loading

torikizi Dec 30, 2024 •

edited

Loading