From 0012bfa128c0cd6911a526471f92cd1b0587f35e Mon Sep 17 00:00:00 2001 From: Florent Castelli Date: Fri, 26 Jul 2024 16:16:41 +0000 Subject: [PATCH] Change DataChannelInit::priority to integer and forward to SCTP transport The new type PriorityValue is a strong 16-bit integer matching RFC 8831 requirements that can be built from a Priority enum. The value is now propagated and used by the SCTP transport, but enabling the feature still requires a field trial for now. Bug: webrtc:42225365 Change-Id: I56c9f48744c70999a8c2d01415a08a0b6761df4b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/357941 Commit-Queue: Florent Castelli Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#42695} --- api/BUILD.gn | 5 ++ api/DEPS | 4 ++ api/data_channel_interface.cc | 5 ++ api/data_channel_interface.h | 4 +- api/priority.h | 32 +++++++++++++ api/test/mock_data_channel.h | 3 +- api/transport/BUILD.gn | 1 + .../data_channel_transport_interface.h | 3 +- media/BUILD.gn | 4 ++ media/sctp/dcsctp_transport.cc | 16 ++++++- media/sctp/dcsctp_transport.h | 6 ++- media/sctp/dcsctp_transport_unittest.cc | 47 +++++++++++++++---- media/sctp/sctp_transport_internal.h | 3 +- pc/BUILD.gn | 5 ++ pc/data_channel_controller.cc | 14 ++++-- pc/data_channel_controller.h | 3 +- pc/data_channel_controller_unittest.cc | 6 ++- pc/data_channel_unittest.cc | 7 +-- pc/sctp_data_channel.cc | 7 +-- pc/sctp_data_channel.h | 6 +-- pc/sctp_transport.cc | 5 +- pc/sctp_transport.h | 2 +- pc/sctp_transport_unittest.cc | 3 +- pc/sctp_utils.cc | 34 ++------------ pc/sctp_utils.h | 3 +- pc/sctp_utils_unittest.cc | 7 +-- pc/test/fake_data_channel_controller.h | 6 ++- test/pc/sctp/BUILD.gn | 1 + test/pc/sctp/fake_sctp_transport.h | 5 +- 29 files changed, 174 insertions(+), 73 deletions(-) diff --git a/api/BUILD.gn b/api/BUILD.gn index 074c5a398a..c8ff1dd282 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -515,6 +515,10 @@ rtc_source_set("rtp_transceiver_direction") { rtc_source_set("priority") { visibility = [ "*" ] sources = [ "priority.h" ] + deps = [ + "../rtc_base:checks", + "../rtc_base:strong_alias", + ] } rtc_library("rtp_parameters") { @@ -1131,6 +1135,7 @@ if (rtc_include_tests) { deps = [ ":libjingle_peerconnection_api", + ":priority", "../test:test_support", ] } diff --git a/api/DEPS b/api/DEPS index b07180e763..129cc4a185 100644 --- a/api/DEPS +++ b/api/DEPS @@ -173,6 +173,10 @@ specific_include_rules = { "+rtc_base/system/no_unique_address.h", ], + "priority\.h": [ + "+rtc_base/strong_alias.h", + ], + "simulated_network\.h": [ "+rtc_base/random.h", "+rtc_base/thread_annotations.h", diff --git a/api/data_channel_interface.cc b/api/data_channel_interface.cc index 970f53b4bd..8e040099f1 100644 --- a/api/data_channel_interface.cc +++ b/api/data_channel_interface.cc @@ -10,6 +10,7 @@ #include "api/data_channel_interface.h" +#include "api/priority.h" #include "rtc_base/checks.h" namespace webrtc { @@ -42,6 +43,10 @@ bool DataChannelInterface::negotiated() const { return false; } +PriorityValue DataChannelInterface::priority() const { + return PriorityValue(Priority::kLow); +} + uint64_t DataChannelInterface::MaxSendQueueSize() { return 16 * 1024 * 1024; // 16 MiB } diff --git a/api/data_channel_interface.h b/api/data_channel_interface.h index d2deace2e6..300012e52f 100644 --- a/api/data_channel_interface.h +++ b/api/data_channel_interface.h @@ -67,7 +67,7 @@ struct DataChannelInit { int id = -1; // https://w3c.github.io/webrtc-priority/#new-rtcdatachannelinit-member - absl::optional priority; + absl::optional priority; }; // At the JavaScript level, data can be passed in as a string or a blob, so @@ -172,7 +172,7 @@ class RTC_EXPORT DataChannelInterface : public RefCountInterface { // If negotiated in-band, this ID will be populated once the DTLS role is // determined, and until then this will return -1. virtual int id() const = 0; - virtual Priority priority() const { return Priority::kLow; } + virtual PriorityValue priority() const; virtual DataState state() const = 0; // When state is kClosed, and the DataChannel was not closed using // the closing procedure, returns the error information about the closing. diff --git a/api/priority.h b/api/priority.h index 4953e453a3..da845f7be4 100644 --- a/api/priority.h +++ b/api/priority.h @@ -11,6 +11,11 @@ #ifndef API_PRIORITY_H_ #define API_PRIORITY_H_ +#include + +#include "rtc_base/checks.h" +#include "rtc_base/strong_alias.h" + namespace webrtc { // GENERATED_JAVA_ENUM_PACKAGE: org.webrtc @@ -21,6 +26,33 @@ enum class Priority { kHigh, }; +class PriorityValue + : public webrtc::StrongAlias { + public: + // TODO(bugs.webrtc.org/42225365): Make explicit after downstream projects + // have updated + PriorityValue(Priority priority) { // NOLINT(runtime/explicit) + switch (priority) { + case Priority::kVeryLow: + value_ = 128; + break; + case Priority::kLow: + value_ = 256; + break; + case Priority::kMedium: + value_ = 512; + break; + case Priority::kHigh: + value_ = 1024; + break; + default: + RTC_CHECK_NOTREACHED(); + } + } + + explicit PriorityValue(uint16_t priority) : StrongAlias(priority) {} +}; + } // namespace webrtc #endif // API_PRIORITY_H_ diff --git a/api/test/mock_data_channel.h b/api/test/mock_data_channel.h index 5d38ec1375..d8e19b0bae 100644 --- a/api/test/mock_data_channel.h +++ b/api/test/mock_data_channel.h @@ -14,6 +14,7 @@ #include #include "api/data_channel_interface.h" +#include "api/priority.h" #include "test/gmock.h" namespace webrtc { @@ -41,7 +42,7 @@ class MockDataChannelInterface MOCK_METHOD(std::string, protocol, (), (const, override)); MOCK_METHOD(bool, negotiated, (), (const, override)); MOCK_METHOD(int, id, (), (const, override)); - MOCK_METHOD(Priority, priority, (), (const, override)); + MOCK_METHOD(PriorityValue, priority, (), (const, override)); MOCK_METHOD(DataState, state, (), (const, override)); MOCK_METHOD(RTCError, error, (), (const, override)); MOCK_METHOD(uint32_t, messages_sent, (), (const, override)); diff --git a/api/transport/BUILD.gn b/api/transport/BUILD.gn index 3b76f0a28b..cf8768ee88 100644 --- a/api/transport/BUILD.gn +++ b/api/transport/BUILD.gn @@ -76,6 +76,7 @@ rtc_source_set("datagram_transport_interface") { sources = [ "data_channel_transport_interface.h" ] deps = [ "..:array_view", + "..:priority", "..:rtc_error", "../../rtc_base:copy_on_write_buffer", "//third_party/abseil-cpp/absl/types:optional", diff --git a/api/transport/data_channel_transport_interface.h b/api/transport/data_channel_transport_interface.h index 2476166c7b..751f79278d 100644 --- a/api/transport/data_channel_transport_interface.h +++ b/api/transport/data_channel_transport_interface.h @@ -13,6 +13,7 @@ #define API_TRANSPORT_DATA_CHANNEL_TRANSPORT_INTERFACE_H_ #include "absl/types/optional.h" +#include "api/priority.h" #include "api/rtc_error.h" #include "rtc_base/copy_on_write_buffer.h" @@ -99,7 +100,7 @@ class DataChannelTransportInterface { // Opens a data `channel_id` for sending. May return an error if the // specified `channel_id` is unusable. Must be called before `SendData`. - virtual RTCError OpenChannel(int channel_id) = 0; + virtual RTCError OpenChannel(int channel_id, PriorityValue priority) = 0; // Sends a data buffer to the remote endpoint using the given send parameters. // `buffer` may not be larger than 256 KiB. Returns an error if the send diff --git a/media/BUILD.gn b/media/BUILD.gn index 3870a3b735..79662b04bf 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -687,6 +687,7 @@ rtc_source_set("rtc_data_sctp_transport_internal") { sources = [ "sctp/sctp_transport_internal.h" ] deps = [ ":media_channel", + "../api:priority", "../api:rtc_error", "../api/transport:datagram_transport_interface", "../p2p:packet_transport_internal", @@ -707,6 +708,7 @@ if (rtc_build_dcsctp) { ":rtc_data_sctp_transport_internal", "../api:array_view", "../api:libjingle_peerconnection_api", + "../api:priority", "../api/environment", "../api/task_queue:pending_task_safety_flag", "../api/task_queue:task_queue", @@ -903,6 +905,7 @@ if (rtc_include_tests) { "../api:mock_video_bitrate_allocator_factory", "../api:mock_video_codec_factory", "../api:mock_video_encoder", + "../api:priority", "../api:rtp_parameters", "../api:scoped_refptr", "../api:simulcast_test_fixture_api", @@ -948,6 +951,7 @@ if (rtc_include_tests) { "../modules/video_coding:webrtc_h264", "../modules/video_coding:webrtc_vp8", "../modules/video_coding/svc:scalability_mode_util", + "../net/dcsctp/public:types", "../p2p:p2p_test_utils", "../rtc_base:async_packet_socket", "../rtc_base:byte_order", diff --git a/media/sctp/dcsctp_transport.cc b/media/sctp/dcsctp_transport.cc index 3d54b7d742..4623435ea9 100644 --- a/media/sctp/dcsctp_transport.cc +++ b/media/sctp/dcsctp_transport.cc @@ -21,6 +21,7 @@ #include "api/array_view.h" #include "api/data_channel_interface.h" #include "api/environment/environment.h" +#include "api/priority.h" #include "media/base/media_channel.h" #include "net/dcsctp/public/dcsctp_socket_factory.h" #include "net/dcsctp/public/packet_observer.h" @@ -222,16 +223,27 @@ bool DcSctpTransport::Start(int local_sctp_port, MaybeConnectSocket(); + for (const auto& [sid, stream_state] : stream_states_) { + socket_->SetStreamPriority(sid, stream_state.priority); + } + return true; } -bool DcSctpTransport::OpenStream(int sid) { +bool DcSctpTransport::OpenStream(int sid, PriorityValue priority) { RTC_DCHECK_RUN_ON(network_thread_); - RTC_DLOG(LS_INFO) << debug_name_ << "->OpenStream(" << sid << ")."; + RTC_DLOG(LS_INFO) << debug_name_ << "->OpenStream(" << sid << ", " + << priority.value() << ")."; StreamState stream_state; + stream_state.priority = dcsctp::StreamPriority(priority.value()); stream_states_.insert_or_assign(dcsctp::StreamID(static_cast(sid)), stream_state); + if (socket_) { + socket_->SetStreamPriority(dcsctp::StreamID(sid), + dcsctp::StreamPriority(priority.value())); + } + return true; } diff --git a/media/sctp/dcsctp_transport.h b/media/sctp/dcsctp_transport.h index 958c54bb69..7f3e7df1d5 100644 --- a/media/sctp/dcsctp_transport.h +++ b/media/sctp/dcsctp_transport.h @@ -18,6 +18,7 @@ #include "absl/types/optional.h" #include "api/array_view.h" #include "api/environment/environment.h" +#include "api/priority.h" #include "api/task_queue/task_queue_base.h" #include "media/sctp/sctp_transport_internal.h" #include "net/dcsctp/public/dcsctp_options.h" @@ -57,7 +58,7 @@ class DcSctpTransport : public cricket::SctpTransportInternal, bool Start(int local_sctp_port, int remote_sctp_port, int max_message_size) override; - bool OpenStream(int sid) override; + bool OpenStream(int sid, PriorityValue priority) override; bool ResetStream(int sid) override; RTCError SendData(int sid, const SendDataParams& params, @@ -127,6 +128,9 @@ class DcSctpTransport : public cricket::SctpTransportInternal, bool incoming_reset_done = false; // True when the local connection received OnStreamsResetPerformed bool outgoing_reset_done = false; + // Priority of the stream according to RFC 8831, section 6.4 + dcsctp::StreamPriority priority = + dcsctp::StreamPriority(PriorityValue(webrtc::Priority::kLow).value()); }; // Map of all currently open or closing data channels diff --git a/media/sctp/dcsctp_transport_unittest.cc b/media/sctp/dcsctp_transport_unittest.cc index adc8c125da..eb9dc67d71 100644 --- a/media/sctp/dcsctp_transport_unittest.cc +++ b/media/sctp/dcsctp_transport_unittest.cc @@ -15,8 +15,10 @@ #include "api/environment/environment.h" #include "api/environment/environment_factory.h" +#include "api/priority.h" #include "net/dcsctp/public/mock_dcsctp_socket.h" #include "net/dcsctp/public/mock_dcsctp_socket_factory.h" +#include "net/dcsctp/public/types.h" #include "p2p/base/fake_packet_transport.h" #include "test/gtest.h" @@ -33,6 +35,9 @@ using ::testing::ReturnPointee; namespace webrtc { namespace { + +const PriorityValue kDefaultPriority = PriorityValue(Priority::kLow); + class MockDataChannelSink : public DataChannelSink { public: MOCK_METHOD(void, OnConnected, ()); @@ -107,6 +112,10 @@ TEST(DcSctpTransportTest, CloseSequence) { { InSequence sequence; + EXPECT_CALL( + *peer_a.socket_, + SetStreamPriority(dcsctp::StreamID(1), + dcsctp::StreamPriority(kDefaultPriority.value()))); EXPECT_CALL(*peer_a.socket_, ResetStreams(ElementsAre(dcsctp::StreamID(1)))) .WillOnce(Return(dcsctp::ResetStreamsStatus::kPerformed)); @@ -121,8 +130,8 @@ TEST(DcSctpTransportTest, CloseSequence) { peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024); peer_b.sctp_transport_->Start(5000, 5000, 256 * 1024); - peer_a.sctp_transport_->OpenStream(1); - peer_b.sctp_transport_->OpenStream(1); + peer_a.sctp_transport_->OpenStream(1, kDefaultPriority); + peer_b.sctp_transport_->OpenStream(1, kDefaultPriority); peer_a.sctp_transport_->ResetStream(1); // Simulate the callbacks from the stream resets @@ -163,8 +172,8 @@ TEST(DcSctpTransportTest, CloseSequenceSimultaneous) { peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024); peer_b.sctp_transport_->Start(5000, 5000, 256 * 1024); - peer_a.sctp_transport_->OpenStream(1); - peer_b.sctp_transport_->OpenStream(1); + peer_a.sctp_transport_->OpenStream(1, kDefaultPriority); + peer_b.sctp_transport_->OpenStream(1, kDefaultPriority); peer_a.sctp_transport_->ResetStream(1); peer_b.sctp_transport_->ResetStream(1); @@ -180,6 +189,28 @@ TEST(DcSctpTransportTest, CloseSequenceSimultaneous) { ->OnIncomingStreamsReset(streams); } +TEST(DcSctpTransportTest, SetStreamPriority) { + rtc::AutoThread main_thread; + Peer peer_a; + + { + InSequence sequence; + + EXPECT_CALL( + *peer_a.socket_, + SetStreamPriority(dcsctp::StreamID(1), dcsctp::StreamPriority(1337))); + EXPECT_CALL( + *peer_a.socket_, + SetStreamPriority(dcsctp::StreamID(2), dcsctp::StreamPriority(3141))); + } + + EXPECT_CALL(*peer_a.socket_, Send(_, _)).Times(0); + + peer_a.sctp_transport_->OpenStream(1, PriorityValue(1337)); + peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024); + peer_a.sctp_transport_->OpenStream(2, PriorityValue(3141)); +} + TEST(DcSctpTransportTest, DiscardMessageClosedChannel) { rtc::AutoThread main_thread; Peer peer_a; @@ -200,7 +231,7 @@ TEST(DcSctpTransportTest, DiscardMessageClosingChannel) { EXPECT_CALL(*peer_a.socket_, Send(_, _)).Times(0); - peer_a.sctp_transport_->OpenStream(1); + peer_a.sctp_transport_->OpenStream(1, kDefaultPriority); peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024); peer_a.sctp_transport_->ResetStream(1); @@ -218,7 +249,7 @@ TEST(DcSctpTransportTest, SendDataOpenChannel) { EXPECT_CALL(*peer_a.socket_, Send(_, _)).Times(1); EXPECT_CALL(*peer_a.socket_, options()).WillOnce(ReturnPointee(&options)); - peer_a.sctp_transport_->OpenStream(1); + peer_a.sctp_transport_->OpenStream(1, kDefaultPriority); peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024); SendDataParams params; @@ -234,7 +265,7 @@ TEST(DcSctpTransportTest, DeliversMessage) { OnDataReceived(1, webrtc::DataMessageType::kBinary, _)) .Times(1); - peer_a.sctp_transport_->OpenStream(1); + peer_a.sctp_transport_->OpenStream(1, kDefaultPriority); peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024); static_cast(peer_a.sctp_transport_.get()) @@ -248,7 +279,7 @@ TEST(DcSctpTransportTest, DropMessageWithUnknownPpid) { EXPECT_CALL(peer_a.sink_, OnDataReceived(_, _, _)).Times(0); - peer_a.sctp_transport_->OpenStream(1); + peer_a.sctp_transport_->OpenStream(1, kDefaultPriority); peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024); static_cast(peer_a.sctp_transport_.get()) diff --git a/media/sctp/sctp_transport_internal.h b/media/sctp/sctp_transport_internal.h index 62bb5e5f26..7596d67b10 100644 --- a/media/sctp/sctp_transport_internal.h +++ b/media/sctp/sctp_transport_internal.h @@ -18,6 +18,7 @@ #include #include +#include "api/priority.h" #include "api/rtc_error.h" #include "api/transport/data_channel_transport_interface.h" #include "media/base/media_channel.h" @@ -113,7 +114,7 @@ class SctpTransportInternal { // TODO(deadbeef): Actually implement the "returns false if `sid` can't be // used" part. See: // https://bugs.chromium.org/p/chromium/issues/detail?id=619849 - virtual bool OpenStream(int sid) = 0; + virtual bool OpenStream(int sid, webrtc::PriorityValue priority) = 0; // The inverse of OpenStream. Begins the closing procedure, which will // eventually result in SignalClosingProcedureComplete on the side that // initiates it, and both SignalClosingProcedureStartedRemotely and diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 062980c19d..61cb0f3854 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -533,6 +533,7 @@ rtc_source_set("sctp_transport") { ":dtls_transport", "../api:dtls_transport_interface", "../api:libjingle_peerconnection_api", + "../api:priority", "../api:scoped_refptr", "../api:sequence_checker", "../api/transport:datagram_transport_interface", @@ -835,6 +836,7 @@ rtc_source_set("data_channel_controller") { ":sctp_data_channel", ":sctp_utils", "../api:libjingle_peerconnection_api", + "../api:priority", "../api:rtc_error", "../api:scoped_refptr", "../api:sequence_checker", @@ -2011,6 +2013,7 @@ if (rtc_include_tests && !build_with_chromium) { "../api:libjingle_peerconnection_api", "../api:make_ref_counted", "../api:make_ref_counted", + "../api:priority", "../api:rtc_error", "../api:rtp_headers", "../api:rtp_parameters", @@ -2488,6 +2491,7 @@ if (rtc_include_tests && !build_with_chromium) { ":pc_test_utils", ":peer_connection_internal", ":sctp_data_channel", + "../api:priority", "../rtc_base:null_socket_server", "../test:run_loop", "../test:test_support", @@ -2712,6 +2716,7 @@ if (rtc_include_tests && !build_with_chromium) { "../api:libjingle_peerconnection_api", "../api:make_ref_counted", "../api:media_stream_interface", + "../api:priority", "../api:rtc_error", "../api:rtc_stats_api", "../api:rtp_parameters", diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index b95ee8d4a3..e252e8f5e6 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -10,11 +10,13 @@ #include "pc/data_channel_controller.h" +#include #include #include "absl/algorithm/container.h" #include "absl/types/optional.h" #include "api/peer_connection_interface.h" +#include "api/priority.h" #include "api/rtc_error.h" #include "pc/peer_connection_internal.h" #include "pc/sctp_utils.h" @@ -52,10 +54,11 @@ RTCError DataChannelController::SendData( payload); } -void DataChannelController::AddSctpDataStream(StreamId sid) { +void DataChannelController::AddSctpDataStream(StreamId sid, + PriorityValue priority) { RTC_DCHECK_RUN_ON(network_thread()); if (data_channel_transport_) { - data_channel_transport_->OpenChannel(sid.stream_id_int()); + data_channel_transport_->OpenChannel(sid.stream_id_int(), priority); } } @@ -349,7 +352,8 @@ DataChannelController::CreateDataChannel(const std::string& label, // If we have an id already, notify the transport. if (sid.has_value()) - AddSctpDataStream(*sid); + AddSctpDataStream(*sid, + config.priority.value_or(PriorityValue(Priority::kLow))); return channel; } @@ -412,7 +416,7 @@ void DataChannelController::AllocateSctpSids(rtc::SSLRole role) { absl::optional sid = sid_allocator_.AllocateSid(role); if (sid.has_value()) { (*it)->SetSctpSid_n(*sid); - AddSctpDataStream(*sid); + AddSctpDataStream(*sid, (*it)->priority()); if (ready_to_send) { RTC_LOG(LS_INFO) << "AllocateSctpSids: Id assigned, ready to send."; (*it)->OnTransportReady(); @@ -471,7 +475,7 @@ void DataChannelController::NotifyDataChannelsOfTransportCreated() { for (const auto& channel : sctp_data_channels_n_) { if (channel->sid_n().has_value()) - AddSctpDataStream(*channel->sid_n()); + AddSctpDataStream(*channel->sid_n(), channel->priority()); channel->OnTransportChannelCreated(); } } diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h index fe1024db6d..74e00e097e 100644 --- a/pc/data_channel_controller.h +++ b/pc/data_channel_controller.h @@ -15,6 +15,7 @@ #include #include "api/data_channel_interface.h" +#include "api/priority.h" #include "api/rtc_error.h" #include "api/scoped_refptr.h" #include "api/sequence_checker.h" @@ -50,7 +51,7 @@ class DataChannelController : public SctpDataChannelControllerInterface, RTCError SendData(StreamId sid, const SendDataParams& params, const rtc::CopyOnWriteBuffer& payload) override; - void AddSctpDataStream(StreamId sid) override; + void AddSctpDataStream(StreamId sid, PriorityValue priority) override; void RemoveSctpDataStream(StreamId sid) override; void OnChannelStateChanged(SctpDataChannel* channel, DataChannelInterface::DataState state) override; diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc index f49d2e6db5..f8850794bd 100644 --- a/pc/data_channel_controller_unittest.cc +++ b/pc/data_channel_controller_unittest.cc @@ -12,6 +12,7 @@ #include +#include "api/priority.h" #include "pc/peer_connection_internal.h" #include "pc/sctp_data_channel.h" #include "pc/test/mock_peer_connection_internal.h" @@ -31,7 +32,10 @@ class MockDataChannelTransport : public DataChannelTransportInterface { public: ~MockDataChannelTransport() override {} - MOCK_METHOD(RTCError, OpenChannel, (int channel_id), (override)); + MOCK_METHOD(RTCError, + OpenChannel, + (int channel_id, PriorityValue priority), + (override)); MOCK_METHOD(RTCError, SendData, (int channel_id, diff --git a/pc/data_channel_unittest.cc b/pc/data_channel_unittest.cc index ac229d6f9b..c5f6525f0d 100644 --- a/pc/data_channel_unittest.cc +++ b/pc/data_channel_unittest.cc @@ -16,6 +16,7 @@ #include #include "api/data_channel_interface.h" +#include "api/priority.h" #include "api/rtc_error.h" #include "api/scoped_refptr.h" #include "api/transport/data_channel_transport_interface.h" @@ -100,7 +101,7 @@ class SctpDataChannelTest : public ::testing::Test { RTC_DCHECK_RUN_ON(&network_thread_); if (!inner_channel_->sid_n().has_value()) { inner_channel_->SetSctpSid_n(sid); - controller_->AddSctpDataStream(sid); + controller_->AddSctpDataStream(sid, inner_channel_->priority()); } inner_channel_->OnTransportChannelCreated(); }); @@ -117,7 +118,7 @@ class SctpDataChannelTest : public ::testing::Test { StreamId sid) { network_thread_.BlockingCall([&]() { channel->SetSctpSid_n(sid); - controller_->AddSctpDataStream(sid); + controller_->AddSctpDataStream(sid, channel->priority()); }); } @@ -162,7 +163,7 @@ TEST_F(SctpDataChannelTest, VerifyConfigurationGetters) { EXPECT_EQ(channel_->reliable(), init_.reliable); EXPECT_EQ(channel_->ordered(), init_.ordered); EXPECT_EQ(channel_->negotiated(), init_.negotiated); - EXPECT_EQ(channel_->priority(), Priority::kLow); + EXPECT_EQ(channel_->priority(), PriorityValue(Priority::kLow)); EXPECT_EQ(channel_->maxRetransmitTime(), static_cast(-1)); EXPECT_EQ(channel_->maxPacketLifeTime(), init_.maxRetransmitTime); EXPECT_EQ(channel_->maxRetransmits(), static_cast(-1)); diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc index afc9488e63..6c2fb0b9cd 100644 --- a/pc/sctp_data_channel.cc +++ b/pc/sctp_data_channel.cc @@ -15,6 +15,7 @@ #include #include +#include "api/priority.h" #include "media/sctp/sctp_transport_internal.h" #include "pc/proxy.h" #include "rtc_base/checks.h" @@ -50,7 +51,7 @@ BYPASS_PROXY_CONSTMETHOD0(std::string, protocol) BYPASS_PROXY_CONSTMETHOD0(bool, negotiated) // Can't bypass the proxy since the id may change. PROXY_SECONDARY_CONSTMETHOD0(int, id) -BYPASS_PROXY_CONSTMETHOD0(Priority, priority) +BYPASS_PROXY_CONSTMETHOD0(PriorityValue, priority) BYPASS_PROXY_CONSTMETHOD0(DataState, state) BYPASS_PROXY_CONSTMETHOD0(RTCError, error) PROXY_SECONDARY_CONSTMETHOD0(uint32_t, messages_sent) @@ -479,8 +480,8 @@ int SctpDataChannel::id() const { return id_n_.has_value() ? id_n_->stream_id_int() : -1; } -Priority SctpDataChannel::priority() const { - return priority_ ? *priority_ : Priority::kLow; +PriorityValue SctpDataChannel::priority() const { + return priority_.value_or(PriorityValue(Priority::kLow)); } uint64_t SctpDataChannel::buffered_amount() const { diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h index e1bd461a8e..ea8e255786 100644 --- a/pc/sctp_data_channel.h +++ b/pc/sctp_data_channel.h @@ -48,7 +48,7 @@ class SctpDataChannelControllerInterface { const SendDataParams& params, const rtc::CopyOnWriteBuffer& payload) = 0; // Adds the data channel SID to the transport for SCTP. - virtual void AddSctpDataStream(StreamId sid) = 0; + virtual void AddSctpDataStream(StreamId sid, PriorityValue priority) = 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(StreamId sid) = 0; @@ -164,7 +164,7 @@ class SctpDataChannel : public DataChannelInterface { std::string protocol() const override; bool negotiated() const override; int id() const override; - Priority priority() const override; + PriorityValue priority() const override; uint64_t buffered_amount() const override; void Close() override; @@ -277,7 +277,7 @@ class SctpDataChannel : public DataChannelInterface { const std::string protocol_; const absl::optional max_retransmit_time_; const absl::optional max_retransmits_; - const absl::optional priority_; + const absl::optional priority_; const bool negotiated_; const bool ordered_; // See the body of `MaybeSendOnBufferedAmountChanged`. diff --git a/pc/sctp_transport.cc b/pc/sctp_transport.cc index 6c5e66fe6c..6473bf485d 100644 --- a/pc/sctp_transport.cc +++ b/pc/sctp_transport.cc @@ -15,6 +15,7 @@ #include "absl/types/optional.h" #include "api/dtls_transport_interface.h" +#include "api/priority.h" #include "api/sequence_checker.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" @@ -75,10 +76,10 @@ void SctpTransport::UnregisterObserver() { observer_ = nullptr; } -RTCError SctpTransport::OpenChannel(int channel_id) { +RTCError SctpTransport::OpenChannel(int channel_id, PriorityValue priority) { RTC_DCHECK_RUN_ON(owner_thread_); RTC_DCHECK(internal_sctp_transport_); - internal_sctp_transport_->OpenStream(channel_id); + internal_sctp_transport_->OpenStream(channel_id, priority); return RTCError::OK(); } diff --git a/pc/sctp_transport.h b/pc/sctp_transport.h index 5508843162..b8f9de532e 100644 --- a/pc/sctp_transport.h +++ b/pc/sctp_transport.h @@ -45,7 +45,7 @@ class SctpTransport : public SctpTransportInterface, void UnregisterObserver() override; // DataChannelTransportInterface - RTCError OpenChannel(int channel_id) override; + RTCError OpenChannel(int channel_id, PriorityValue priority) override; RTCError SendData(int channel_id, const SendDataParams& params, const rtc::CopyOnWriteBuffer& buffer) override; diff --git a/pc/sctp_transport_unittest.cc b/pc/sctp_transport_unittest.cc index 4eb83d375a..049cf41956 100644 --- a/pc/sctp_transport_unittest.cc +++ b/pc/sctp_transport_unittest.cc @@ -16,6 +16,7 @@ #include "absl/memory/memory.h" #include "absl/types/optional.h" #include "api/dtls_transport_interface.h" +#include "api/priority.h" #include "api/transport/data_channel_transport_interface.h" #include "media/base/media_channel.h" #include "p2p/base/fake_dtls_transport.h" @@ -47,7 +48,7 @@ class FakeCricketSctpTransport : public cricket::SctpTransportInternal { bool Start(int local_port, int remote_port, int max_message_size) override { return true; } - bool OpenStream(int sid) override { return true; } + bool OpenStream(int sid, PriorityValue priority) override { return true; } bool ResetStream(int sid) override { return true; } RTCError SendData(int sid, const SendDataParams& params, diff --git a/pc/sctp_utils.cc b/pc/sctp_utils.cc index 60ffdc7919..df1da4d31a 100644 --- a/pc/sctp_utils.cc +++ b/pc/sctp_utils.cc @@ -88,17 +88,7 @@ bool ParseDataChannelOpenMessage(const rtc::CopyOnWriteBuffer& payload, << "Could not read OPEN message reliabilility prioirty."; return false; } - // Parse priority as defined in - // https://w3c.github.io/webrtc-priority/#rtcdatachannel-processing-steps - if (priority <= DCO_PRIORITY_VERY_LOW) { - config->priority = Priority::kVeryLow; - } else if (priority <= DCO_PRIORITY_LOW) { - config->priority = Priority::kLow; - } else if (priority <= DCO_PRIORITY_MEDIUM) { - config->priority = Priority::kMedium; - } else { - config->priority = Priority::kHigh; - } + config->priority = PriorityValue(priority); uint32_t reliability_param; if (!buffer.ReadUInt32(&reliability_param)) { @@ -172,7 +162,7 @@ bool WriteDataChannelOpenMessage(const std::string& label, bool WriteDataChannelOpenMessage(const std::string& label, const std::string& protocol, - absl::optional opt_priority, + absl::optional opt_priority, bool ordered, absl::optional max_retransmits, absl::optional max_retransmit_time, @@ -181,25 +171,9 @@ bool WriteDataChannelOpenMessage(const std::string& label, // http://tools.ietf.org/html/draft-ietf-rtcweb-data-protocol-09#section-5.1 uint8_t channel_type = 0; uint32_t reliability_param = 0; - uint16_t priority = 0; // Set priority according to // https://tools.ietf.org/html/draft-ietf-rtcweb-data-channel-12#section-6.4 - if (opt_priority) { - switch (*opt_priority) { - case Priority::kVeryLow: - priority = DCO_PRIORITY_VERY_LOW; - break; - case Priority::kLow: - priority = DCO_PRIORITY_LOW; - break; - case Priority::kMedium: - priority = DCO_PRIORITY_MEDIUM; - break; - case Priority::kHigh: - priority = DCO_PRIORITY_HIGH; - break; - } - } + PriorityValue priority = opt_priority.value_or(PriorityValue(Priority::kLow)); if (ordered) { if (max_retransmits) { channel_type = DCOMCT_ORDERED_PARTIAL_RTXS; @@ -226,7 +200,7 @@ bool WriteDataChannelOpenMessage(const std::string& label, // TODO(tommi): Add error handling and check resulting length. buffer.WriteUInt8(DATA_CHANNEL_OPEN_MESSAGE_TYPE); buffer.WriteUInt8(channel_type); - buffer.WriteUInt16(priority); + buffer.WriteUInt16(priority.value()); buffer.WriteUInt32(reliability_param); buffer.WriteUInt16(static_cast(label.length())); buffer.WriteUInt16(static_cast(protocol.length())); diff --git a/pc/sctp_utils.h b/pc/sctp_utils.h index c1bf23dd98..2f4b1f09a0 100644 --- a/pc/sctp_utils.h +++ b/pc/sctp_utils.h @@ -14,6 +14,7 @@ #include #include "api/data_channel_interface.h" +#include "api/priority.h" #include "api/transport/data_channel_transport_interface.h" #include "media/base/media_channel.h" #include "media/sctp/sctp_transport_internal.h" @@ -63,7 +64,7 @@ bool ParseDataChannelOpenAckMessage(const rtc::CopyOnWriteBuffer& payload); bool WriteDataChannelOpenMessage(const std::string& label, const std::string& protocol, - absl::optional priority, + absl::optional priority, bool ordered, absl::optional max_retransmits, absl::optional max_retransmit_time, diff --git a/pc/sctp_utils_unittest.cc b/pc/sctp_utils_unittest.cc index 49d6b09206..0907b9033f 100644 --- a/pc/sctp_utils_unittest.cc +++ b/pc/sctp_utils_unittest.cc @@ -55,9 +55,10 @@ class SctpUtilsTest : public ::testing::Test { if (config.priority) { // Exact values are checked by round-trip conversion, but // all values defined are greater than zero. - EXPECT_GT(priority, 0); + EXPECT_EQ(priority, config.priority->value()); } else { - EXPECT_EQ(priority, 0); + EXPECT_EQ(priority, + webrtc::PriorityValue(webrtc::Priority::kLow).value()); } ASSERT_TRUE(buffer.ReadUInt32(&reliability)); @@ -154,7 +155,7 @@ TEST_F(SctpUtilsTest, WriteParseOpenMessageWithPriority) { webrtc::DataChannelInit config; std::string label = "abc"; config.protocol = "y"; - config.priority = webrtc::Priority::kVeryLow; + config.priority = webrtc::PriorityValue(webrtc::Priority::kVeryLow); rtc::CopyOnWriteBuffer packet; ASSERT_TRUE(webrtc::WriteDataChannelOpenMessage(label, config, &packet)); diff --git a/pc/test/fake_data_channel_controller.h b/pc/test/fake_data_channel_controller.h index 3f62660922..6ef8a57a59 100644 --- a/pc/test/fake_data_channel_controller.h +++ b/pc/test/fake_data_channel_controller.h @@ -15,6 +15,7 @@ #include #include +#include "api/priority.h" #include "pc/sctp_data_channel.h" #include "rtc_base/checks.h" #include "rtc_base/weak_ptr.h" @@ -60,7 +61,7 @@ class FakeDataChannelController transport_available_, init, signaling_thread_, network_thread_); if (transport_available_ && channel->sid_n().has_value()) { - AddSctpDataStream(*channel->sid_n()); + AddSctpDataStream(*channel->sid_n(), channel->priority()); } if (ready_to_send_) { network_thread_->PostTask([channel = channel] { @@ -95,7 +96,8 @@ class FakeDataChannelController return webrtc::RTCError::OK(); } - void AddSctpDataStream(webrtc::StreamId sid) override { + void AddSctpDataStream(webrtc::StreamId sid, + webrtc::PriorityValue priority) override { RTC_DCHECK_RUN_ON(network_thread_); if (!transport_available_) { return; diff --git a/test/pc/sctp/BUILD.gn b/test/pc/sctp/BUILD.gn index 4a8eecc9bd..24004d6c5a 100644 --- a/test/pc/sctp/BUILD.gn +++ b/test/pc/sctp/BUILD.gn @@ -12,6 +12,7 @@ rtc_source_set("fake_sctp_transport") { visibility = [ "*" ] sources = [ "fake_sctp_transport.h" ] deps = [ + "../../../api:priority", "../../../api/environment", "../../../api/transport:sctp_transport_factory_interface", "../../../media:rtc_data_sctp_transport_internal", diff --git a/test/pc/sctp/fake_sctp_transport.h b/test/pc/sctp/fake_sctp_transport.h index 41b7a962f1..8aacd3b497 100644 --- a/test/pc/sctp/fake_sctp_transport.h +++ b/test/pc/sctp/fake_sctp_transport.h @@ -14,6 +14,7 @@ #include #include "api/environment/environment.h" +#include "api/priority.h" #include "api/transport/sctp_transport_factory_interface.h" #include "media/sctp/sctp_transport_internal.h" @@ -31,7 +32,9 @@ class FakeSctpTransport : public cricket::SctpTransportInternal { max_message_size_ = max_message_size; return true; } - bool OpenStream(int sid) override { return true; } + bool OpenStream(int sid, webrtc::PriorityValue priority) override { + return true; + } bool ResetStream(int sid) override { return true; } webrtc::RTCError SendData(int sid, const webrtc::SendDataParams& params,