pc: Provide DtlsTransport to SctpTransport constr
This code looked a bit weird before this CL - probably because of old
refactorings.
In JsepTransport constructor, there is a DCHECK assuring that the RTP
DTLS transport is always present, so it can be passed directly to the
SctpTransport constructor, which avoids having the SetDtlsTransport
method in it.
Also, in the SctpTransport constructor, there was code that would set
the SCTP transport state to `kConnecting` if the DTLS transport was
present, but that was dead code, as it was always `nullptr` inside the
constructor before this CL. With this CL, it's always present, and the
SCTP Transport's state will initially always be `kConnecting` now. Which
is a step to deprecating the `kNew` state that doesn't exist in
https://w3c.github.io/webrtc-pc/#dom-rtcsctptransportstate.
One test case was modified, as it didn't test the reality. The test
created a SctpTransport, registered an observer, and added the DTLS
transport, and expected to receive a "statechange" from `kNew` (which is
not a state that exists in the spec) to `kConnecting`. If the test had
tested the opposite ordering - adding the DTLS transport first, and then
adding an observer, it wouldn't have experienced this. And since in
reality (with the implementation of JsepTransport before and
after this CL), it always adds the DTLS transport before any observer is
registered. So it wouldn't ever be fired, outside of tests.
Bug: webrtc:15897
Change-Id: I6ac24e0a331b686eb400fcf388ece50f2ad46a32
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/345420
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41987}
diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc
index faff4e8..a6c9ebc 100644
--- a/pc/jsep_transport.cc
+++ b/pc/jsep_transport.cc
@@ -96,7 +96,8 @@
: nullptr),
sctp_transport_(sctp_transport
? rtc::make_ref_counted<webrtc::SctpTransport>(
- std::move(sctp_transport))
+ std::move(sctp_transport),
+ rtp_dtls_transport_)
: nullptr),
rtcp_mux_active_callback_(std::move(rtcp_mux_active_callback)) {
TRACE_EVENT0("webrtc", "JsepTransport::JsepTransport");
@@ -118,10 +119,6 @@
RTC_DCHECK(!unencrypted_rtp_transport);
RTC_DCHECK(!sdes_transport);
}
-
- if (sctp_transport_) {
- sctp_transport_->SetDtlsTransport(rtp_dtls_transport_);
- }
}
JsepTransport::~JsepTransport() {
diff --git a/pc/sctp_transport.cc b/pc/sctp_transport.cc
index eb60930..6c5e66f 100644
--- a/pc/sctp_transport.cc
+++ b/pc/sctp_transport.cc
@@ -22,19 +22,27 @@
namespace webrtc {
SctpTransport::SctpTransport(
- std::unique_ptr<cricket::SctpTransportInternal> internal)
+ std::unique_ptr<cricket::SctpTransportInternal> internal,
+ rtc::scoped_refptr<DtlsTransport> dtls_transport)
: owner_thread_(rtc::Thread::Current()),
- info_(SctpTransportState::kNew),
- internal_sctp_transport_(std::move(internal)) {
+ info_(SctpTransportState::kConnecting,
+ dtls_transport,
+ /*max_message_size=*/absl::nullopt,
+ /*max_channels=*/absl::nullopt),
+ internal_sctp_transport_(std::move(internal)),
+ dtls_transport_(dtls_transport) {
RTC_DCHECK(internal_sctp_transport_.get());
+ RTC_DCHECK(dtls_transport_.get());
+
+ dtls_transport_->internal()->SubscribeDtlsTransportState(
+ [this](cricket::DtlsTransportInternal* transport,
+ DtlsTransportState state) {
+ OnDtlsStateChange(transport, state);
+ });
+
+ internal_sctp_transport_->SetDtlsTransport(dtls_transport->internal());
internal_sctp_transport_->SetOnConnectedCallback(
[this]() { OnAssociationChangeCommunicationUp(); });
-
- if (dtls_transport_) {
- UpdateInformation(SctpTransportState::kConnecting);
- } else {
- UpdateInformation(SctpTransportState::kNew);
- }
}
SctpTransport::~SctpTransport() {
@@ -134,31 +142,6 @@
UpdateInformation(SctpTransportState::kClosed);
}
-void SctpTransport::SetDtlsTransport(
- rtc::scoped_refptr<DtlsTransport> transport) {
- RTC_DCHECK_RUN_ON(owner_thread_);
- SctpTransportState next_state = info_.state();
- dtls_transport_ = transport;
- if (internal_sctp_transport_) {
- if (transport) {
- internal_sctp_transport_->SetDtlsTransport(transport->internal());
-
- transport->internal()->SubscribeDtlsTransportState(
- [this](cricket::DtlsTransportInternal* transport,
- DtlsTransportState state) {
- OnDtlsStateChange(transport, state);
- });
- if (info_.state() == SctpTransportState::kNew) {
- next_state = SctpTransportState::kConnecting;
- }
- } else {
- internal_sctp_transport_->SetDtlsTransport(nullptr);
- }
- }
-
- UpdateInformation(next_state);
-}
-
void SctpTransport::Start(int local_port,
int remote_port,
int max_message_size) {
diff --git a/pc/sctp_transport.h b/pc/sctp_transport.h
index 60434d8..5508843 100644
--- a/pc/sctp_transport.h
+++ b/pc/sctp_transport.h
@@ -35,8 +35,8 @@
class SctpTransport : public SctpTransportInterface,
public DataChannelTransportInterface {
public:
- explicit SctpTransport(
- std::unique_ptr<cricket::SctpTransportInternal> internal);
+ SctpTransport(std::unique_ptr<cricket::SctpTransportInternal> internal,
+ rtc::scoped_refptr<DtlsTransport> dtls_transport);
// SctpTransportInterface
rtc::scoped_refptr<DtlsTransportInterface> dtls_transport() const override;
@@ -58,7 +58,6 @@
// Internal functions
void Clear();
- void SetDtlsTransport(rtc::scoped_refptr<DtlsTransport>);
// Initialize the cricket::SctpTransport. This can be called from
// the signaling thread.
void Start(int local_port, int remote_port, int max_message_size);
diff --git a/pc/sctp_transport_unittest.cc b/pc/sctp_transport_unittest.cc
index 2849889..4eb83d3 100644
--- a/pc/sctp_transport_unittest.cc
+++ b/pc/sctp_transport_unittest.cc
@@ -117,19 +117,16 @@
SctpTransportObserverInterface* observer() { return &observer_; }
void CreateTransport() {
- auto cricket_sctp_transport =
- absl::WrapUnique(new FakeCricketSctpTransport());
- transport_ =
- rtc::make_ref_counted<SctpTransport>(std::move(cricket_sctp_transport));
- }
-
- void AddDtlsTransport() {
std::unique_ptr<cricket::DtlsTransportInternal> cricket_transport =
std::make_unique<FakeDtlsTransport>(
"audio", cricket::ICE_CANDIDATE_COMPONENT_RTP);
dtls_transport_ =
rtc::make_ref_counted<DtlsTransport>(std::move(cricket_transport));
- transport_->SetDtlsTransport(dtls_transport_);
+
+ auto cricket_sctp_transport =
+ absl::WrapUnique(new FakeCricketSctpTransport());
+ transport_ = rtc::make_ref_counted<SctpTransport>(
+ std::move(cricket_sctp_transport), dtls_transport_);
}
void CompleteSctpHandshake() {
@@ -152,13 +149,20 @@
TEST(SctpTransportSimpleTest, CreateClearDelete) {
rtc::AutoThread main_thread;
+ std::unique_ptr<cricket::DtlsTransportInternal> cricket_transport =
+ std::make_unique<FakeDtlsTransport>("audio",
+ cricket::ICE_CANDIDATE_COMPONENT_RTP);
+ rtc::scoped_refptr<DtlsTransport> dtls_transport =
+ rtc::make_ref_counted<DtlsTransport>(std::move(cricket_transport));
+
std::unique_ptr<cricket::SctpTransportInternal> fake_cricket_sctp_transport =
absl::WrapUnique(new FakeCricketSctpTransport());
rtc::scoped_refptr<SctpTransport> sctp_transport =
rtc::make_ref_counted<SctpTransport>(
- std::move(fake_cricket_sctp_transport));
+ std::move(fake_cricket_sctp_transport), dtls_transport);
ASSERT_TRUE(sctp_transport->internal());
- ASSERT_EQ(SctpTransportState::kNew, sctp_transport->Information().state());
+ ASSERT_EQ(SctpTransportState::kConnecting,
+ sctp_transport->Information().state());
sctp_transport->Clear();
ASSERT_FALSE(sctp_transport->internal());
ASSERT_EQ(SctpTransportState::kClosed, sctp_transport->Information().state());
@@ -167,18 +171,15 @@
TEST_F(SctpTransportTest, EventsObservedWhenConnecting) {
CreateTransport();
transport()->RegisterObserver(observer());
- AddDtlsTransport();
CompleteSctpHandshake();
ASSERT_EQ_WAIT(SctpTransportState::kConnected, observer_.State(),
kDefaultTimeout);
- EXPECT_THAT(observer_.States(), ElementsAre(SctpTransportState::kConnecting,
- SctpTransportState::kConnected));
+ EXPECT_THAT(observer_.States(), ElementsAre(SctpTransportState::kConnected));
}
TEST_F(SctpTransportTest, CloseWhenClearing) {
CreateTransport();
transport()->RegisterObserver(observer());
- AddDtlsTransport();
CompleteSctpHandshake();
ASSERT_EQ_WAIT(SctpTransportState::kConnected, observer_.State(),
kDefaultTimeout);
@@ -190,7 +191,6 @@
TEST_F(SctpTransportTest, MaxChannelsSignalled) {
CreateTransport();
transport()->RegisterObserver(observer());
- AddDtlsTransport();
EXPECT_FALSE(transport()->Information().MaxChannels());
EXPECT_FALSE(observer_.LastReceivedInformation().MaxChannels());
CompleteSctpHandshake();
@@ -206,7 +206,6 @@
TEST_F(SctpTransportTest, CloseWhenTransportCloses) {
CreateTransport();
transport()->RegisterObserver(observer());
- AddDtlsTransport();
CompleteSctpHandshake();
ASSERT_EQ_WAIT(SctpTransportState::kConnected, observer_.State(),
kDefaultTimeout);
