Adopt StreamId in SctpDataChannelControllerInterface

Bug: webrtc:11547
Change-Id: Iea2d706228b5a533eb7fae84613462165d7c9b54
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298300
Auto-Submit: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39618}

pc/BUILD.gn

@@ -517,7 +517,6 @@ rtc_source_set("sctp_utils") {
   deps = [
     "../api:libjingle_peerconnection_api",
     "../api:priority",
-    "../api:sequence_checker",
     "../api/transport:datagram_transport_interface",
     "../media:media_channel",
     "../media:rtc_data_sctp_transport_internal",
@@ -527,7 +526,6 @@ rtc_source_set("sctp_utils") {
     "../rtc_base:copy_on_write_buffer",
     "../rtc_base:logging",
     "../rtc_base:ssl",
-    "../rtc_base/system:no_unique_address",
   ]
   absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ]
 }

pc/data_channel_controller.cc

@@ -33,7 +33,7 @@ bool DataChannelController::HasUsedDataChannels() const {
   return has_used_data_channels_;
 }
 
-bool DataChannelController::SendData(int sid,
+bool DataChannelController::SendData(StreamId sid,
                                      const SendDataParams& params,
                                      const rtc::CopyOnWriteBuffer& payload,
                                      cricket::SendDataResult* result) {
@@ -43,21 +43,21 @@ bool DataChannelController::SendData(int sid,
   return false;
 }
 
-void DataChannelController::AddSctpDataStream(int sid) {
+void DataChannelController::AddSctpDataStream(StreamId sid) {
   if (data_channel_transport()) {
     network_thread()->BlockingCall([this, sid] {
       if (data_channel_transport()) {
-        data_channel_transport()->OpenChannel(sid);
+        data_channel_transport()->OpenChannel(sid.stream_id_int());
       }
     });
   }
 }
 
-void DataChannelController::RemoveSctpDataStream(int sid) {
+void DataChannelController::RemoveSctpDataStream(StreamId sid) {
   if (data_channel_transport()) {
     network_thread()->BlockingCall([this, sid] {
       if (data_channel_transport()) {
-        data_channel_transport()->CloseChannel(sid);
+        data_channel_transport()->CloseChannel(sid.stream_id_int());
       }
     });
   }
@@ -375,7 +375,7 @@ void DataChannelController::set_data_channel_transport(
 }
 
 bool DataChannelController::DataChannelSendData(
-    int sid,
+    StreamId sid,
     const SendDataParams& params,
     const rtc::CopyOnWriteBuffer& payload,
     cricket::SendDataResult* result) {
@@ -386,7 +386,8 @@ bool DataChannelController::DataChannelSendData(
   RTC_DCHECK(data_channel_transport());
 
   RTCError error = network_thread()->BlockingCall([this, sid, params, payload] {
-    return data_channel_transport()->SendData(sid, params, payload);
+    return data_channel_transport()->SendData(sid.stream_id_int(), params,
+                                              payload);
   });
 
   if (error.ok()) {

pc/data_channel_controller.h

@@ -47,12 +47,12 @@ class DataChannelController : public SctpDataChannelControllerInterface,
 
   // Implements
   // SctpDataChannelProviderInterface.
-  bool SendData(int sid,
+  bool SendData(StreamId sid,
                 const SendDataParams& params,
                 const rtc::CopyOnWriteBuffer& payload,
                 cricket::SendDataResult* result) override;
-  void AddSctpDataStream(int sid) override;
+  void AddSctpDataStream(StreamId sid) override;
-  void RemoveSctpDataStream(int sid) override;
+  void RemoveSctpDataStream(StreamId sid) override;
   bool ReadyToSendData() const override;
   void OnChannelStateChanged(SctpDataChannel* channel,
                              DataChannelInterface::DataState state) override;
@@ -124,7 +124,7 @@ class DataChannelController : public SctpDataChannelControllerInterface,
       RTC_RUN_ON(signaling_thread());
 
   // Called from SendData when data_channel_transport() is true.
-  bool DataChannelSendData(int sid,
+  bool DataChannelSendData(StreamId sid,
                            const SendDataParams& params,
                            const rtc::CopyOnWriteBuffer& payload,
                            cricket::SendDataResult* result);

pc/data_channel_unittest.cc

@@ -134,12 +134,10 @@ TEST_F(SctpDataChannelTest, ConnectedToTransportOnCreated) {
 
   EXPECT_TRUE(controller_->IsConnected(dc.get()));
   // The sid is not set yet, so it should not have added the streams.
-  EXPECT_FALSE(controller_->IsSendStreamAdded(dc->id()));
+  EXPECT_FALSE(controller_->IsStreamAdded(dc->sid()));
-  EXPECT_FALSE(controller_->IsRecvStreamAdded(dc->id()));
 
   dc->SetSctpSid(StreamId(0));
-  EXPECT_TRUE(controller_->IsSendStreamAdded(dc->id()));
+  EXPECT_TRUE(controller_->IsStreamAdded(dc->sid()));
-  EXPECT_TRUE(controller_->IsRecvStreamAdded(dc->id()));
 }
 
 // Tests the state of the data channel.

pc/sctp_data_channel.cc

@@ -211,7 +211,7 @@ SctpDataChannel::SctpDataChannel(
   // Try to connect to the transport in case the transport channel already
   // exists.
   if (id_.HasValue()) {
-    controller_->AddSctpDataStream(id_.stream_id_int());
+    controller_->AddSctpDataStream(id_);
   }
 }
 
@@ -370,7 +370,7 @@ void SctpDataChannel::SetSctpSid(const StreamId& sid) {
   RTC_DCHECK_EQ(state_, kConnecting);
 
   id_ = sid;
-  controller_->AddSctpDataStream(sid.stream_id_int());
+  controller_->AddSctpDataStream(sid);
 }
 
 void SctpDataChannel::OnClosingProcedureStartedRemotely() {
@@ -407,7 +407,7 @@ void SctpDataChannel::OnTransportChannelCreated() {
   // The sid may have been unassigned when controller_->ConnectDataChannel was
   // done. So always add the streams even if connected_to_transport_ is true.
   if (id_.HasValue()) {
-    controller_->AddSctpDataStream(id_.stream_id_int());
+    controller_->AddSctpDataStream(id_);
   }
 }
 
@@ -584,7 +584,7 @@ void SctpDataChannel::UpdateState() {
           // afterwards.
           if (!started_closing_procedure_ && controller_ && id_.HasValue()) {
             started_closing_procedure_ = true;
-            controller_->RemoveSctpDataStream(id_.stream_id_int());
+            controller_->RemoveSctpDataStream(id_);
           }
         }
       } else {
@@ -671,8 +671,8 @@ bool SctpDataChannel::SendDataMessage(const DataBuffer& buffer,
       buffer.binary ? DataMessageType::kBinary : DataMessageType::kText;
 
   cricket::SendDataResult send_result = cricket::SDR_SUCCESS;
-  bool success = controller_->SendData(id_.stream_id_int(), send_params,
+  bool success =
-                                       buffer.data, &send_result);
+      controller_->SendData(id_, send_params, buffer.data, &send_result);
 
   if (success) {
     ++messages_sent_;
@@ -749,8 +749,7 @@ bool SctpDataChannel::SendControlMessage(const rtc::CopyOnWriteBuffer& buffer) {
   send_params.type = DataMessageType::kControl;
 
   cricket::SendDataResult send_result = cricket::SDR_SUCCESS;
-  bool retval = controller_->SendData(id_.stream_id_int(), send_params, buffer,
+  bool retval = controller_->SendData(id_, send_params, buffer, &send_result);
-                                      &send_result);
   if (retval) {
     RTC_DLOG(LS_VERBOSE) << "Sent CONTROL message on channel "
                          << id_.stream_id_int();

pc/sctp_data_channel.h

@@ -39,15 +39,15 @@ class SctpDataChannel;
 class SctpDataChannelControllerInterface {
  public:
   // Sends the data to the transport.
-  virtual bool SendData(int sid,
+  virtual bool SendData(StreamId sid,
                         const SendDataParams& params,
                         const rtc::CopyOnWriteBuffer& payload,
                         cricket::SendDataResult* result) = 0;
   // Adds the data channel SID to the transport for SCTP.
-  virtual void AddSctpDataStream(int sid) = 0;
+  virtual void AddSctpDataStream(StreamId sid) = 0;
   // Begins the closing procedure by sending an outgoing stream reset. Still
   // need to wait for callbacks to tell when this completes.
-  virtual void RemoveSctpDataStream(int sid) = 0;
+  virtual void RemoveSctpDataStream(StreamId sid) = 0;
   // Returns true if the transport channel is ready to send data.
   virtual bool ReadyToSendData() const = 0;
   // Notifies the controller of state changes.

pc/sctp_utils.cc

@@ -16,7 +16,6 @@
 
 #include "absl/types/optional.h"
 #include "api/priority.h"
-#include "media/sctp/sctp_transport_internal.h"
 #include "rtc_base/byte_buffer.h"
 #include "rtc_base/copy_on_write_buffer.h"
 #include "rtc_base/logging.h"
@@ -47,53 +46,6 @@ enum DataChannelPriority {
   DCO_PRIORITY_HIGH = 1024,
 };
 
-StreamId::StreamId() : id_(absl::nullopt) {
-  thread_checker_.Detach();
-}
-
-StreamId::StreamId(int id)
-    : id_(id >= cricket::kMinSctpSid && id <= cricket::kSpecMaxSctpSid
-              ? absl::optional<uint16_t>(static_cast<uint16_t>(id))
-              : absl::nullopt) {
-  thread_checker_.Detach();
-}
-
-StreamId::StreamId(const StreamId& sid) : id_(sid.id_) {}
-
-bool StreamId::HasValue() const {
-  RTC_DCHECK_RUN_ON(&thread_checker_);
-  return id_.has_value();
-}
-
-int StreamId::stream_id_int() const {
-  RTC_DCHECK_RUN_ON(&thread_checker_);
-  return id_.has_value() ? static_cast<int>(id_.value().value()) : -1;
-}
-
-void StreamId::reset() {
-  RTC_DCHECK_RUN_ON(&thread_checker_);
-  id_ = absl::nullopt;
-}
-
-StreamId& StreamId::operator=(const StreamId& sid) {
-  RTC_DCHECK_RUN_ON(&thread_checker_);
-  RTC_DCHECK_RUN_ON(&sid.thread_checker_);
-  id_ = sid.id_;
-  return *this;
-}
-
-bool StreamId::operator==(const StreamId& sid) const {
-  RTC_DCHECK_RUN_ON(&thread_checker_);
-  RTC_DCHECK_RUN_ON(&sid.thread_checker_);
-  return id_ == sid.id_;
-}
-
-bool StreamId::operator<(const StreamId& sid) const {
-  RTC_DCHECK_RUN_ON(&thread_checker_);
-  RTC_DCHECK_RUN_ON(&sid.thread_checker_);
-  return id_ < sid.id_;
-}
-
 bool IsOpenMessage(const rtc::CopyOnWriteBuffer& payload) {
   // Format defined at
   // https://www.rfc-editor.org/rfc/rfc8832#section-5.1

pc/sctp_utils.h

@@ -14,13 +14,12 @@
 #include <string>
 
 #include "api/data_channel_interface.h"
-#include "api/sequence_checker.h"
 #include "api/transport/data_channel_transport_interface.h"
 #include "media/base/media_channel.h"
+#include "media/sctp/sctp_transport_internal.h"
 #include "net/dcsctp/public/types.h"
 #include "rtc_base/copy_on_write_buffer.h"
 #include "rtc_base/ssl_stream_adapter.h"  // For SSLRole
-#include "rtc_base/system/no_unique_address.h"
 
 namespace rtc {
 class CopyOnWriteBuffer;
@@ -36,9 +35,13 @@ struct DataChannelInit;
 // this class or the internal dcsctp::StreamID type.
 class StreamId {
  public:
-  StreamId();
+  StreamId() = default;
-  explicit StreamId(int id);
+  explicit StreamId(int id)
-  explicit StreamId(const StreamId& sid);
+      : id_(id >= cricket::kMinSctpSid && id <= cricket::kSpecMaxSctpSid
+                ? absl::optional<uint16_t>(static_cast<uint16_t>(id))
+                : absl::nullopt) {}
+  StreamId(const StreamId& sid) = default;
+  StreamId& operator=(const StreamId& sid) = default;
 
   // Returns `true` if a valid stream id is contained, in the range of
   // kMinSctpSid - kSpecMaxSctpSid ([0..0xffff]). Note that this
@@ -46,22 +49,23 @@ class StreamId {
   // the limit that is internally used by `SctpSidAllocator`. Sid values may
   // be assigned to `StreamId` outside of `SctpSidAllocator` and have a higher
   // id value than supplied by `SctpSidAllocator`, yet is still valid.
-  bool HasValue() const;
+  bool HasValue() const { return id_.has_value(); }
 
   // Provided for compatibility with existing code that hasn't been updated
   // to use `StreamId` directly. New code should not use 'int' for the stream
   // id but rather `StreamId` directly.
-  int stream_id_int() const;
+  int stream_id_int() const {
-  void reset();
+    return id_.has_value() ? static_cast<int>(id_.value().value()) : -1;
+  }
 
-  StreamId& operator=(const StreamId& sid);
+  void reset() { id_ = absl::nullopt; }
-  bool operator==(const StreamId& sid) const;
+
-  bool operator<(const StreamId& sid) const;
+  bool operator==(const StreamId& sid) const { return id_ == sid.id_; }
+  bool operator<(const StreamId& sid) const { return id_ < sid.id_; }
   bool operator!=(const StreamId& sid) const { return !(operator==(sid)); }
 
  private:
-  RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker thread_checker_;
+  absl::optional<dcsctp::StreamID> id_;
-  absl::optional<dcsctp::StreamID> id_ RTC_GUARDED_BY(thread_checker_);
 };
 
 // Read the message type and return true if it's an OPEN message.

pc/test/fake_data_channel_controller.h

@@ -44,7 +44,7 @@ class FakeDataChannelController
     return channel;
   }
 
-  bool SendData(int sid,
+  bool SendData(webrtc::StreamId sid,
                 const webrtc::SendDataParams& params,
                 const rtc::CopyOnWriteBuffer& payload,
                 cricket::SendDataResult* result) override {
@@ -60,28 +60,26 @@ class FakeDataChannelController
       return false;
     }
 
-    last_sid_ = sid;
+    last_sid_ = sid.stream_id_int();
     last_send_data_params_ = params;
     return true;
   }
 
-  void AddSctpDataStream(int sid) override {
+  void AddSctpDataStream(webrtc::StreamId sid) override {
-    RTC_CHECK(sid >= 0);
+    RTC_CHECK(sid.HasValue());
     if (!transport_available_) {
       return;
     }
-    send_ssrcs_.insert(sid);
+    known_stream_ids_.insert(sid);
-    recv_ssrcs_.insert(sid);
   }
 
-  void RemoveSctpDataStream(int sid) override {
+  void RemoveSctpDataStream(webrtc::StreamId sid) override {
-    RTC_CHECK(sid >= 0);
+    RTC_CHECK(sid.HasValue());
-    send_ssrcs_.erase(sid);
+    known_stream_ids_.erase(sid);
-    recv_ssrcs_.erase(sid);
     // Unlike the real SCTP transport, act like the closing procedure finished
     // instantly, doing the same snapshot thing as below.
     auto it = absl::c_find_if(connected_channels_,
-                              [&](const auto* c) { return c->id() == sid; });
+                              [&](const auto* c) { return c->sid() == sid; });
     // This path mimics the DCC's OnChannelClosed handler since the FDCC
     // (this class) doesn't have a transport that would do that.
     if (it != connected_channels_.end())
@@ -146,12 +144,8 @@ class FakeDataChannelController
     return connected_channels_.find(data_channel) != connected_channels_.end();
   }
 
-  bool IsSendStreamAdded(uint32_t stream) const {
+  bool IsStreamAdded(webrtc::StreamId id) const {
-    return send_ssrcs_.find(stream) != send_ssrcs_.end();
+    return known_stream_ids_.find(id) != known_stream_ids_.end();
-  }
-
-  bool IsRecvStreamAdded(uint32_t stream) const {
-    return recv_ssrcs_.find(stream) != recv_ssrcs_.end();
   }
 
   int channels_opened() const { return channels_opened_; }
@@ -167,8 +161,7 @@ class FakeDataChannelController
   int channels_closed_ = 0;
   int channels_opened_ = 0;
   std::set<webrtc::SctpDataChannel*> connected_channels_;
-  std::set<uint32_t> send_ssrcs_;
+  std::set<webrtc::StreamId> known_stream_ids_;
-  std::set<uint32_t> recv_ssrcs_;
   rtc::WeakPtrFactory<FakeDataChannelController> weak_factory_{this};
 };
 #endif  // PC_TEST_FAKE_DATA_CHANNEL_CONTROLLER_H_