Always offer transport sequence number header extension for audio
If the extension is negotiated, it will only be used if
the field trial WebRTC-Audio-SendSideBwe is enabled.
This allows simpler experimentation if it should be used or not.
Bug: webrtc:10309 webrtc:10286
Change-Id: I797e6f14c06d46189e40f6d09805c2e09afc015b
Reviewed-on: https://webrtc-review.googlesource.com/c/122542
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26689}diff --git a/audio/BUILD.gn b/audio/BUILD.gn
index e4ab39a..80ce099 100644
--- a/audio/BUILD.gn
+++ b/audio/BUILD.gn
@@ -149,6 +149,7 @@
"../logging:rtc_event_log_api",
"../modules/audio_device:mock_audio_device",
"../rtc_base:rtc_base_tests_utils",
+ "../test:field_trial",
# For TestAudioDeviceModule
"../modules/audio_device:audio_device_impl",
diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc
index 0c597ed..2e4830c 100644
--- a/audio/audio_send_stream.cc
+++ b/audio/audio_send_stream.cc
@@ -264,8 +264,8 @@
RtcpBandwidthObserver* bandwidth_observer = nullptr;
- if (stream->allocation_settings_.IncludeAudioInFeedback(
- new_ids.transport_sequence_number != 0)) {
+ if (stream->allocation_settings_.ShouldSendTransportSequenceNumber(
+ new_ids.transport_sequence_number)) {
channel_send->EnableSendTransportSequenceNumber(
new_ids.transport_sequence_number);
// Probing in application limited region is only used in combination with
diff --git a/audio/audio_send_stream_tests.cc b/audio/audio_send_stream_tests.cc
index 7deeff3..55de03d 100644
--- a/audio/audio_send_stream_tests.cc
+++ b/audio/audio_send_stream_tests.cc
@@ -9,6 +9,8 @@
*/
#include "test/call_test.h"
+#include "test/constants.h"
+#include "test/field_trial.h"
#include "test/gtest.h"
#include "test/rtcp_packet_parser.h"
@@ -137,42 +139,54 @@
RunBaseTest(&test);
}
-TEST_F(AudioSendStreamCallTest, SupportsTransportWideSequenceNumbers) {
- static const uint8_t kExtensionId = test::kTransportSequenceNumberExtensionId;
- class TransportWideSequenceNumberObserver : public AudioSendTest {
- public:
- TransportWideSequenceNumberObserver() : AudioSendTest() {
- EXPECT_TRUE(parser_->RegisterRtpHeaderExtension(
- kRtpExtensionTransportSequenceNumber, kExtensionId));
- }
+class TransportWideSequenceNumberObserver : public AudioSendTest {
+ public:
+ explicit TransportWideSequenceNumberObserver(bool expect_sequence_number)
+ : AudioSendTest(), expect_sequence_number_(expect_sequence_number) {
+ EXPECT_TRUE(parser_->RegisterRtpHeaderExtension(
+ kRtpExtensionTransportSequenceNumber,
+ kTransportSequenceNumberExtensionId));
+ }
- private:
- Action OnSendRtp(const uint8_t* packet, size_t length) override {
- RTPHeader header;
- EXPECT_TRUE(parser_->Parse(packet, length, &header));
+ private:
+ Action OnSendRtp(const uint8_t* packet, size_t length) override {
+ RTPHeader header;
+ EXPECT_TRUE(parser_->Parse(packet, length, &header));
- EXPECT_TRUE(header.extension.hasTransportSequenceNumber);
- EXPECT_FALSE(header.extension.hasTransmissionTimeOffset);
- EXPECT_FALSE(header.extension.hasAbsoluteSendTime);
+ EXPECT_EQ(header.extension.hasTransportSequenceNumber,
+ expect_sequence_number_);
+ EXPECT_FALSE(header.extension.hasTransmissionTimeOffset);
+ EXPECT_FALSE(header.extension.hasAbsoluteSendTime);
- observation_complete_.Set();
+ observation_complete_.Set();
- return SEND_PACKET;
- }
+ return SEND_PACKET;
+ }
- void ModifyAudioConfigs(
- AudioSendStream::Config* send_config,
- std::vector<AudioReceiveStream::Config>* receive_configs) override {
- send_config->rtp.extensions.clear();
- send_config->rtp.extensions.push_back(RtpExtension(
- RtpExtension::kTransportSequenceNumberUri, kExtensionId));
- }
+ void ModifyAudioConfigs(
+ AudioSendStream::Config* send_config,
+ std::vector<AudioReceiveStream::Config>* receive_configs) override {
+ send_config->rtp.extensions.clear();
+ send_config->rtp.extensions.push_back(
+ RtpExtension(RtpExtension::kTransportSequenceNumberUri,
+ kTransportSequenceNumberExtensionId));
+ }
- void PerformTest() override {
- EXPECT_TRUE(Wait()) << "Timed out while waiting for a single RTP packet.";
- }
- } test;
+ void PerformTest() override {
+ EXPECT_TRUE(Wait()) << "Timed out while waiting for a single RTP packet.";
+ }
+ const bool expect_sequence_number_;
+};
+TEST_F(AudioSendStreamCallTest, SendsTransportWideSequenceNumbersInFieldTrial) {
+ ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/");
+ TransportWideSequenceNumberObserver test(/*expect_sequence_number=*/true);
+ RunBaseTest(&test);
+}
+
+TEST_F(AudioSendStreamCallTest,
+ DoesNotSendTransportWideSequenceNumbersPerDefault) {
+ TransportWideSequenceNumberObserver test(/*expect_sequence_number=*/false);
RunBaseTest(&test);
}
diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc
index 70f6c72..e20b031 100644
--- a/audio/audio_send_stream_unittest.cc
+++ b/audio/audio_send_stream_unittest.cc
@@ -28,6 +28,7 @@
#include "modules/rtp_rtcp/mocks/mock_rtcp_rtt_stats.h"
#include "modules/rtp_rtcp/mocks/mock_rtp_rtcp.h"
#include "rtc_base/task_queue.h"
+#include "test/field_trial.h"
#include "test/gtest.h"
#include "test/mock_audio_encoder.h"
#include "test/mock_audio_encoder_factory.h"
@@ -353,6 +354,7 @@
}
TEST(AudioSendStreamTest, AudioBweCorrectObjectsOnChannelProxy) {
+ ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/");
ConfigHelper helper(true, true);
auto send_stream = helper.CreateAudioSendStream();
}
@@ -504,6 +506,7 @@
}
TEST(AudioSendStreamTest, ReconfigureTransportCcResetsFirst) {
+ ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/");
ConfigHelper helper(false, true);
auto send_stream = helper.CreateAudioSendStream();
auto new_config = helper.config();
diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc
index 07a47d7..a055ae9 100644
--- a/media/engine/webrtc_voice_engine.cc
+++ b/media/engine/webrtc_voice_engine.cc
@@ -559,11 +559,8 @@
int id = 1;
capabilities.header_extensions.push_back(
webrtc::RtpExtension(webrtc::RtpExtension::kAudioLevelUri, id++));
- if (allocation_settings_.EnableTransportSequenceNumberExtension()) {
- capabilities.header_extensions.push_back(webrtc::RtpExtension(
- webrtc::RtpExtension::kTransportSequenceNumberUri, id++));
- }
-
+ capabilities.header_extensions.push_back(webrtc::RtpExtension(
+ webrtc::RtpExtension::kTransportSequenceNumberUri, id++));
return capabilities;
}
diff --git a/rtc_base/experiments/audio_allocation_settings.cc b/rtc_base/experiments/audio_allocation_settings.cc
index 79c5a0d..a505357 100644
--- a/rtc_base/experiments/audio_allocation_settings.cc
+++ b/rtc_base/experiments/audio_allocation_settings.cc
@@ -65,16 +65,12 @@
return audio_send_side_bwe_;
}
-bool AudioAllocationSettings::EnableTransportSequenceNumberExtension() const {
- // TODO(srte): Update this to be more accurate.
- return audio_send_side_bwe_ && !allocate_audio_without_feedback_;
-}
-
-bool AudioAllocationSettings::IncludeAudioInFeedback(
+bool AudioAllocationSettings::ShouldSendTransportSequenceNumber(
int transport_seq_num_extension_header_id) const {
if (force_no_audio_feedback_)
return false;
- return transport_seq_num_extension_header_id != 0;
+ return audio_send_side_bwe_ && !allocate_audio_without_feedback_ &&
+ transport_seq_num_extension_header_id != 0;
}
bool AudioAllocationSettings::UpdateAudioTargetBitrate(
diff --git a/rtc_base/experiments/audio_allocation_settings.h b/rtc_base/experiments/audio_allocation_settings.h
index bcc0f3e..f05b4a3 100644
--- a/rtc_base/experiments/audio_allocation_settings.h
+++ b/rtc_base/experiments/audio_allocation_settings.h
@@ -27,14 +27,13 @@
bool IgnoreSeqNumIdChange() const;
// Returns true if the bitrate allocation range should be configured.
bool ConfigureRateAllocationRange() const;
- // Returns true if the transport sequence number extension should be enabled.
- bool EnableTransportSequenceNumberExtension() const;
- // Returns true if audio traffic should be included in transport wide feedback
- // packets.
+ // Returns true if sent audio packets should have transport wide sequence
+ // numbers.
// |transport_seq_num_extension_header_id| the extension header id for
// transport sequence numbers. Set to 0 if not the extension is not
// configured.
- bool IncludeAudioInFeedback(int transport_seq_num_extension_header_id) const;
+ bool ShouldSendTransportSequenceNumber(
+ int transport_seq_num_extension_header_id) const;
// Returns true if target bitrate for audio streams should be updated.
// |transport_seq_num_extension_header_id| the extension header id for
// transport sequence numbers. Set to 0 if not the extension is not
diff --git a/video/end_to_end_tests/transport_feedback_tests.cc b/video/end_to_end_tests/transport_feedback_tests.cc
index cc0cb7b..9aacd9a 100644
--- a/video/end_to_end_tests/transport_feedback_tests.cc
+++ b/video/end_to_end_tests/transport_feedback_tests.cc
@@ -316,6 +316,7 @@
}
TEST_F(TransportFeedbackEndToEndTest, AudioReceivesTransportFeedback) {
+ test::ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/");
TransportFeedbackTester test(true, 0, 1);
RunBaseTest(&test);
}
@@ -424,6 +425,7 @@
}
TEST_F(TransportFeedbackEndToEndTest, TransportSeqNumOnAudioAndVideo) {
+ test::ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/");
static constexpr int kExtensionId = 8;
static constexpr size_t kMinPacketsToWaitFor = 50;
class TransportSequenceNumberTest : public test::EndToEndTest {
