From 3f1aee3cbb9fccc8f3deb8c88a39fdbeb00e5857 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Thu, 27 Feb 2020 11:59:23 -0800 Subject: [PATCH] Change network_priority from a double to an enum. It can only be one of four possible values, so it never made sense for it to be a double. Other than the fact that its neighbor bitrate_priority is a double, and they're both defined as the same enum in the web spec. However, while bitrate_priority being a double offers more flexibility than the web spec, network_priority being a double is only confusing. Bug: webrtc:5658 Change-Id: I0784c116f3260c4b3a8b99a3cd85c8d66017e46f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168840 Reviewed-by: Anders Carlsson Reviewed-by: Harald Alvestrand Reviewed-by: Karl Wiberg Commit-Queue: Taylor Cr-Commit-Position: refs/heads/master@{#30685} --- api/rtp_parameters.cc | 5 --- api/rtp_parameters.h | 15 ++++---- media/engine/webrtc_video_engine.cc | 31 +++++++++------- media/engine/webrtc_video_engine_unittest.cc | 31 +--------------- media/engine/webrtc_voice_engine.cc | 37 ++++++++++--------- media/engine/webrtc_voice_engine_unittest.cc | 31 +--------------- pc/rtp_sender.cc | 2 +- .../peerconnection/RTCRtpEncodingParameters.h | 10 ++++- .../RTCRtpEncodingParameters.mm | 32 +++++++++++++++- 9 files changed, 88 insertions(+), 106 deletions(-) diff --git a/api/rtp_parameters.cc b/api/rtp_parameters.cc index f09be189eb..2b580b1084 100644 --- a/api/rtp_parameters.cc +++ b/api/rtp_parameters.cc @@ -19,11 +19,6 @@ namespace webrtc { const double kDefaultBitratePriority = 1.0; -const double Priority::kVeryLow = 0.5; -const double Priority::kLow = 1.0; -const double Priority::kMedium = 2.0; -const double Priority::kHigh = 4.0; - RtcpFeedback::RtcpFeedback() = default; RtcpFeedback::RtcpFeedback(RtcpFeedbackType type) : type(type) {} RtcpFeedback::RtcpFeedback(RtcpFeedbackType type, diff --git a/api/rtp_parameters.h b/api/rtp_parameters.h index d9d4d5afa5..358a5820c3 100644 --- a/api/rtp_parameters.h +++ b/api/rtp_parameters.h @@ -91,12 +91,11 @@ enum class DegradationPreference { RTC_EXPORT extern const double kDefaultBitratePriority; -// TODO(deadbeef): Switch to an enum class. -struct RTC_EXPORT Priority { - static const double kVeryLow; - static const double kLow; - static const double kMedium; - static const double kHigh; +enum class Priority { + kVeryLow, + kLow, + kMedium, + kHigh, }; struct RTC_EXPORT RtcpFeedback { @@ -401,7 +400,9 @@ struct RTC_EXPORT RtpEncodingParameters { // we follow chromium's translation of the allowed string enum values for // this field to 1.0, 0.5, et cetera, similar to bitrate_priority above. // TODO(http://crbug.com/webrtc/8630): Implement this per encoding parameter. - double network_priority = Priority::kLow; + // TODO(http://crbug.com/webrtc/11379): TCP connections should use a single + // DSCP value even if shared by multiple senders; this is not implemented. + Priority network_priority = Priority::kLow; // If set, this represents the Transport Independent Application Specific // maximum bandwidth defined in RFC3890. If unset, there is no maximum diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index a93a509e0a..b2f842950e 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -944,22 +944,25 @@ webrtc::RTCError WebRtcVideoChannel::SetRtpSendParameters( } if (!parameters.encodings.empty()) { - const auto& priority = parameters.encodings[0].network_priority; + // Note that these values come from: + // https://tools.ietf.org/html/draft-ietf-tsvwg-rtcweb-qos-16#section-5 + // TODO(deadbeef): Change values depending on whether we are sending a + // keyframe or non-keyframe. rtc::DiffServCodePoint new_dscp = rtc::DSCP_DEFAULT; - if (priority == 0.5 * webrtc::kDefaultBitratePriority) { - new_dscp = rtc::DSCP_CS1; - } else if (priority == webrtc::kDefaultBitratePriority) { - new_dscp = rtc::DSCP_DEFAULT; - } else if (priority == 2.0 * webrtc::kDefaultBitratePriority) { - new_dscp = rtc::DSCP_AF42; - } else if (priority == 4.0 * webrtc::kDefaultBitratePriority) { - new_dscp = rtc::DSCP_AF41; - } else { - RTC_LOG(LS_WARNING) << "Received invalid send network priority: " - << priority; - return webrtc::RTCError(webrtc::RTCErrorType::INVALID_RANGE); + switch (parameters.encodings[0].network_priority) { + case webrtc::Priority::kVeryLow: + new_dscp = rtc::DSCP_CS1; + break; + case webrtc::Priority::kLow: + new_dscp = rtc::DSCP_DEFAULT; + break; + case webrtc::Priority::kMedium: + new_dscp = rtc::DSCP_AF42; + break; + case webrtc::Priority::kHigh: + new_dscp = rtc::DSCP_AF41; + break; } - SetPreferredDscp(new_dscp); } diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index a04f99ac75..d294d3153b 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -5061,18 +5061,13 @@ TEST_F(WebRtcVideoChannelTest, TestSetDscpOptions) { ASSERT_FALSE(parameters.encodings.empty()); // Various priorities map to various dscp values. - parameters.encodings[0].network_priority = 4.0; + parameters.encodings[0].network_priority = webrtc::Priority::kHigh; ASSERT_TRUE(channel->SetRtpSendParameters(kSsrc, parameters).ok()); EXPECT_EQ(rtc::DSCP_AF41, network_interface->dscp()); - parameters.encodings[0].network_priority = 0.5; + parameters.encodings[0].network_priority = webrtc::Priority::kVeryLow; ASSERT_TRUE(channel->SetRtpSendParameters(kSsrc, parameters).ok()); EXPECT_EQ(rtc::DSCP_CS1, network_interface->dscp()); - // A bad priority does not change the dscp value. - parameters.encodings[0].network_priority = 0.0; - ASSERT_FALSE(channel->SetRtpSendParameters(kSsrc, parameters).ok()); - EXPECT_EQ(rtc::DSCP_CS1, network_interface->dscp()); - // Packets should also self-identify their dscp in PacketOptions. const uint8_t kData[10] = {0}; EXPECT_TRUE(static_cast(channel.get()) @@ -6238,28 +6233,6 @@ TEST_F(WebRtcVideoChannelTest, SetRtpSendParametersPrioritySimulcastStreams) { EXPECT_TRUE(channel_->SetVideoSend(primary_ssrc, nullptr, nullptr)); } -// RTCRtpEncodingParameters.network_priority must be one of a few values -// derived from the default priority, corresponding to very-low, low, medium, -// or high. -TEST_F(WebRtcVideoChannelTest, SetRtpSendParametersInvalidNetworkPriority) { - AddSendStream(); - webrtc::RtpParameters parameters = channel_->GetRtpSendParameters(last_ssrc_); - EXPECT_EQ(1UL, parameters.encodings.size()); - EXPECT_EQ(webrtc::kDefaultBitratePriority, - parameters.encodings[0].network_priority); - - double good_values[] = {0.5, 1.0, 2.0, 4.0}; - double bad_values[] = {-1.0, 0.0, 0.49, 0.51, 1.1, 3.99, 4.1, 5.0}; - for (auto it : good_values) { - parameters.encodings[0].network_priority = it; - EXPECT_TRUE(channel_->SetRtpSendParameters(last_ssrc_, parameters).ok()); - } - for (auto it : bad_values) { - parameters.encodings[0].network_priority = it; - EXPECT_FALSE(channel_->SetRtpSendParameters(last_ssrc_, parameters).ok()); - } -} - TEST_F(WebRtcVideoChannelTest, GetAndSetRtpSendParametersScaleResolutionDownByVP8) { VideoSendParameters parameters; diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index b4b2b4a3ef..7da9abd384 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -711,8 +711,8 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream config_.rtp.c_name = c_name; config_.rtp.extmap_allow_mixed = extmap_allow_mixed; config_.rtp.extensions = extensions; - config_.has_dscp = rtp_parameters_.encodings[0].network_priority != - webrtc::kDefaultBitratePriority; + config_.has_dscp = + rtp_parameters_.encodings[0].network_priority != webrtc::Priority::kLow; config_.audio_network_adaptor_config = audio_network_adaptor_config; config_.encoder_factory = encoder_factory; config_.codec_pair_id = codec_pair_id; @@ -923,11 +923,11 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream const absl::optional old_rtp_max_bitrate = rtp_parameters_.encodings[0].max_bitrate_bps; double old_priority = rtp_parameters_.encodings[0].bitrate_priority; - double old_dscp = rtp_parameters_.encodings[0].network_priority; + webrtc::Priority old_dscp = rtp_parameters_.encodings[0].network_priority; rtp_parameters_ = parameters; config_.bitrate_priority = rtp_parameters_.encodings[0].bitrate_priority; config_.has_dscp = (rtp_parameters_.encodings[0].network_priority != - webrtc::kDefaultBitratePriority); + webrtc::Priority::kLow); bool reconfigure_send_stream = (rtp_parameters_.encodings[0].max_bitrate_bps != old_rtp_max_bitrate) || @@ -1389,22 +1389,23 @@ webrtc::RTCError WebRtcVoiceMediaChannel::SetRtpSendParameters( } if (!parameters.encodings.empty()) { - auto& priority = parameters.encodings[0].network_priority; + // Note that these values come from: + // https://tools.ietf.org/html/draft-ietf-tsvwg-rtcweb-qos-16#section-5 rtc::DiffServCodePoint new_dscp = rtc::DSCP_DEFAULT; - if (priority == 0.5 * webrtc::kDefaultBitratePriority) { - new_dscp = rtc::DSCP_CS1; - } else if (priority == 1.0 * webrtc::kDefaultBitratePriority) { - new_dscp = rtc::DSCP_DEFAULT; - } else if (priority == 2.0 * webrtc::kDefaultBitratePriority) { - new_dscp = rtc::DSCP_EF; - } else if (priority == 4.0 * webrtc::kDefaultBitratePriority) { - new_dscp = rtc::DSCP_EF; - } else { - RTC_LOG(LS_WARNING) << "Received invalid send network priority: " - << priority; - return webrtc::RTCError(webrtc::RTCErrorType::INVALID_RANGE); + switch (parameters.encodings[0].network_priority) { + case webrtc::Priority::kVeryLow: + new_dscp = rtc::DSCP_CS1; + break; + case webrtc::Priority::kLow: + new_dscp = rtc::DSCP_DEFAULT; + break; + case webrtc::Priority::kMedium: + new_dscp = rtc::DSCP_EF; + break; + case webrtc::Priority::kHigh: + new_dscp = rtc::DSCP_EF; + break; } - SetPreferredDscp(new_dscp); } diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc index 488683dbec..6cbbf0244b 100644 --- a/media/engine/webrtc_voice_engine_unittest.cc +++ b/media/engine/webrtc_voice_engine_unittest.cc @@ -1191,28 +1191,6 @@ TEST_F(WebRtcVoiceEngineTestFake, RtpParametersArePerStream) { EXPECT_EQ(32000, GetCodecBitrate(kSsrcs4[2])); } -// RTCRtpEncodingParameters.network_priority must be one of a few values -// derived from the default priority, corresponding to very-low, low, medium, -// or high. -TEST_F(WebRtcVoiceEngineTestFake, SetRtpSendParametersInvalidNetworkPriority) { - EXPECT_TRUE(SetupSendStream()); - webrtc::RtpParameters parameters = channel_->GetRtpSendParameters(kSsrcX); - EXPECT_EQ(1UL, parameters.encodings.size()); - EXPECT_EQ(webrtc::kDefaultBitratePriority, - parameters.encodings[0].network_priority); - - double good_values[] = {0.5, 1.0, 2.0, 4.0}; - double bad_values[] = {-1.0, 0.0, 0.49, 0.51, 1.1, 3.99, 4.1, 5.0}; - for (auto it : good_values) { - parameters.encodings[0].network_priority = it; - EXPECT_TRUE(channel_->SetRtpSendParameters(kSsrcX, parameters).ok()); - } - for (auto it : bad_values) { - parameters.encodings[0].network_priority = it; - EXPECT_FALSE(channel_->SetRtpSendParameters(kSsrcX, parameters).ok()); - } -} - // Test that GetRtpSendParameters returns the currently configured codecs. TEST_F(WebRtcVoiceEngineTestFake, GetRtpSendParametersCodecs) { EXPECT_TRUE(SetupSendStream()); @@ -3051,18 +3029,13 @@ TEST_F(WebRtcVoiceEngineTestFake, TestSetDscpOptions) { ASSERT_FALSE(parameters.encodings.empty()); // Various priorities map to various dscp values. - parameters.encodings[0].network_priority = 4.0; + parameters.encodings[0].network_priority = webrtc::Priority::kHigh; ASSERT_TRUE(channel->SetRtpSendParameters(kSsrcZ, parameters).ok()); EXPECT_EQ(rtc::DSCP_EF, network_interface.dscp()); - parameters.encodings[0].network_priority = 0.5; + parameters.encodings[0].network_priority = webrtc::Priority::kVeryLow; ASSERT_TRUE(channel->SetRtpSendParameters(kSsrcZ, parameters).ok()); EXPECT_EQ(rtc::DSCP_CS1, network_interface.dscp()); - // A bad priority does not change the dscp value. - parameters.encodings[0].network_priority = 0.0; - ASSERT_FALSE(channel->SetRtpSendParameters(kSsrcZ, parameters).ok()); - EXPECT_EQ(rtc::DSCP_CS1, network_interface.dscp()); - // Packets should also self-identify their dscp in PacketOptions. const uint8_t kData[10] = {0}; EXPECT_TRUE(channel->SendRtcp(kData, sizeof(kData))); diff --git a/pc/rtp_sender.cc b/pc/rtp_sender.cc index 8a1fa79dd6..5a955e39de 100644 --- a/pc/rtp_sender.cc +++ b/pc/rtp_sender.cc @@ -50,7 +50,7 @@ int GenerateUniqueId() { bool PerSenderRtpEncodingParameterHasValue( const RtpEncodingParameters& encoding_params) { if (encoding_params.bitrate_priority != kDefaultBitratePriority || - encoding_params.network_priority != kDefaultBitratePriority) { + encoding_params.network_priority != Priority::kLow) { return true; } return false; diff --git a/sdk/objc/api/peerconnection/RTCRtpEncodingParameters.h b/sdk/objc/api/peerconnection/RTCRtpEncodingParameters.h index 16eabf9b2f..bd4fe8e1ee 100644 --- a/sdk/objc/api/peerconnection/RTCRtpEncodingParameters.h +++ b/sdk/objc/api/peerconnection/RTCRtpEncodingParameters.h @@ -14,6 +14,14 @@ NS_ASSUME_NONNULL_BEGIN +/** Corresponds to webrtc::Priority. */ +typedef NS_ENUM(NSInteger, RTCPriority) { + RTCPriorityVeryLow, + RTCPriorityLow, + RTCPriorityMedium, + RTCPriorityHigh +}; + RTC_OBJC_EXPORT @interface RTCRtpEncodingParameters : NSObject @@ -52,7 +60,7 @@ RTC_OBJC_EXPORT @property(nonatomic, readonly, nullable) NSNumber *ssrc; /** The relative DiffServ Code Point priority. */ -@property(nonatomic, assign) double networkPriority; +@property(nonatomic, assign) RTCPriority networkPriority; - (instancetype)init NS_DESIGNATED_INITIALIZER; diff --git a/sdk/objc/api/peerconnection/RTCRtpEncodingParameters.mm b/sdk/objc/api/peerconnection/RTCRtpEncodingParameters.mm index 7378473f50..6fef212245 100644 --- a/sdk/objc/api/peerconnection/RTCRtpEncodingParameters.mm +++ b/sdk/objc/api/peerconnection/RTCRtpEncodingParameters.mm @@ -56,7 +56,8 @@ if (nativeParameters.ssrc) { _ssrc = [NSNumber numberWithUnsignedLong:*nativeParameters.ssrc]; } - _networkPriority = nativeParameters.network_priority; + _networkPriority = + [RTCRtpEncodingParameters priorityFromNativePriority:nativeParameters.network_priority]; } return self; } @@ -86,8 +87,35 @@ if (_ssrc != nil) { parameters.ssrc = absl::optional(_ssrc.unsignedLongValue); } - parameters.network_priority = _networkPriority; + parameters.network_priority = + [RTCRtpEncodingParameters nativePriorityFromPriority:_networkPriority]; return parameters; } ++ (webrtc::Priority)nativePriorityFromPriority:(RTCPriority)networkPriority { + switch (networkPriority) { + case RTCPriorityVeryLow: + return webrtc::Priority::kVeryLow; + case RTCPriorityLow: + return webrtc::Priority::kLow; + case RTCPriorityMedium: + return webrtc::Priority::kMedium; + case RTCPriorityHigh: + return webrtc::Priority::kHigh; + } +} + ++ (RTCPriority)priorityFromNativePriority:(webrtc::Priority)nativePriority { + switch (nativePriority) { + case webrtc::Priority::kVeryLow: + return RTCPriorityVeryLow; + case webrtc::Priority::kLow: + return RTCPriorityLow; + case webrtc::Priority::kMedium: + return RTCPriorityMedium; + case webrtc::Priority::kHigh: + return RTCPriorityHigh; + } +} + @end