Revert "Always offer transport sequence number header extension for audio"
This reverts commit fd965c008c7bc395bb276f260262ac11ccd25406.
Reason for revert: Cause test failure.
Original change's description:
> 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}
TBR=ossu@webrtc.org,sprang@webrtc.org,srte@webrtc.org,perkj@webrtc.org
Change-Id: I1b7d3fa5c282a5bf049ca54695ad16c8278a2698
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:10309 webrtc:10286
Reviewed-on: https://webrtc-review.googlesource.com/c/123182
Reviewed-by: Ying Wang <yinwa@webrtc.org>
Commit-Queue: Ying Wang <yinwa@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26703}diff --git a/audio/BUILD.gn b/audio/BUILD.gn
index 80ce099..e4ab39a 100644
--- a/audio/BUILD.gn
+++ b/audio/BUILD.gn
@@ -149,7 +149,6 @@
"../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 2e4830c..0c597ed 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_.ShouldSendTransportSequenceNumber(
- new_ids.transport_sequence_number)) {
+ if (stream->allocation_settings_.IncludeAudioInFeedback(
+ new_ids.transport_sequence_number != 0)) {
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 55de03d..7deeff3 100644
--- a/audio/audio_send_stream_tests.cc
+++ b/audio/audio_send_stream_tests.cc
@@ -9,8 +9,6 @@
*/
#include "test/call_test.h"
-#include "test/constants.h"
-#include "test/field_trial.h"
#include "test/gtest.h"
#include "test/rtcp_packet_parser.h"
@@ -139,54 +137,42 @@
RunBaseTest(&test);
}
-class TransportWideSequenceNumberObserver : public AudioSendTest {
- public:
- explicit TransportWideSequenceNumberObserver(bool expect_sequence_number)
- : AudioSendTest(), expect_sequence_number_(expect_sequence_number) {
- EXPECT_TRUE(parser_->RegisterRtpHeaderExtension(
- kRtpExtensionTransportSequenceNumber,
- kTransportSequenceNumberExtensionId));
- }
+TEST_F(AudioSendStreamCallTest, SupportsTransportWideSequenceNumbers) {
+ static const uint8_t kExtensionId = test::kTransportSequenceNumberExtensionId;
+ class TransportWideSequenceNumberObserver : public AudioSendTest {
+ public:
+ TransportWideSequenceNumberObserver() : AudioSendTest() {
+ EXPECT_TRUE(parser_->RegisterRtpHeaderExtension(
+ kRtpExtensionTransportSequenceNumber, kExtensionId));
+ }
- 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_EQ(header.extension.hasTransportSequenceNumber,
- expect_sequence_number_);
- EXPECT_FALSE(header.extension.hasTransmissionTimeOffset);
- EXPECT_FALSE(header.extension.hasAbsoluteSendTime);
+ EXPECT_TRUE(header.extension.hasTransportSequenceNumber);
+ 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,
- kTransportSequenceNumberExtensionId));
- }
+ 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 PerformTest() override {
- EXPECT_TRUE(Wait()) << "Timed out while waiting for a single RTP packet.";
- }
- const bool expect_sequence_number_;
-};
+ void PerformTest() override {
+ EXPECT_TRUE(Wait()) << "Timed out while waiting for a single RTP packet.";
+ }
+ } test;
-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 e20b031..70f6c72 100644
--- a/audio/audio_send_stream_unittest.cc
+++ b/audio/audio_send_stream_unittest.cc
@@ -28,7 +28,6 @@
#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"
@@ -354,7 +353,6 @@
}
TEST(AudioSendStreamTest, AudioBweCorrectObjectsOnChannelProxy) {
- ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/");
ConfigHelper helper(true, true);
auto send_stream = helper.CreateAudioSendStream();
}
@@ -506,7 +504,6 @@
}
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 a055ae9..07a47d7 100644
--- a/media/engine/webrtc_voice_engine.cc
+++ b/media/engine/webrtc_voice_engine.cc
@@ -559,8 +559,11 @@
int id = 1;
capabilities.header_extensions.push_back(
webrtc::RtpExtension(webrtc::RtpExtension::kAudioLevelUri, id++));
- capabilities.header_extensions.push_back(webrtc::RtpExtension(
- webrtc::RtpExtension::kTransportSequenceNumberUri, id++));
+ if (allocation_settings_.EnableTransportSequenceNumberExtension()) {
+ 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 a505357..79c5a0d 100644
--- a/rtc_base/experiments/audio_allocation_settings.cc
+++ b/rtc_base/experiments/audio_allocation_settings.cc
@@ -65,12 +65,16 @@
return audio_send_side_bwe_;
}
-bool AudioAllocationSettings::ShouldSendTransportSequenceNumber(
+bool AudioAllocationSettings::EnableTransportSequenceNumberExtension() const {
+ // TODO(srte): Update this to be more accurate.
+ return audio_send_side_bwe_ && !allocate_audio_without_feedback_;
+}
+
+bool AudioAllocationSettings::IncludeAudioInFeedback(
int transport_seq_num_extension_header_id) const {
if (force_no_audio_feedback_)
return false;
- return audio_send_side_bwe_ && !allocate_audio_without_feedback_ &&
- transport_seq_num_extension_header_id != 0;
+ return 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 f05b4a3..bcc0f3e 100644
--- a/rtc_base/experiments/audio_allocation_settings.h
+++ b/rtc_base/experiments/audio_allocation_settings.h
@@ -27,13 +27,14 @@
bool IgnoreSeqNumIdChange() const;
// Returns true if the bitrate allocation range should be configured.
bool ConfigureRateAllocationRange() const;
- // Returns true if sent audio packets should have transport wide sequence
- // numbers.
+ // 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.
// |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 ShouldSendTransportSequenceNumber(
- int transport_seq_num_extension_header_id) const;
+ bool IncludeAudioInFeedback(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 9aacd9a..cc0cb7b 100644
--- a/video/end_to_end_tests/transport_feedback_tests.cc
+++ b/video/end_to_end_tests/transport_feedback_tests.cc
@@ -316,7 +316,6 @@
}
TEST_F(TransportFeedbackEndToEndTest, AudioReceivesTransportFeedback) {
- test::ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/");
TransportFeedbackTester test(true, 0, 1);
RunBaseTest(&test);
}
@@ -425,7 +424,6 @@
}
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 {
