From 914351de5c82bcb11da179bc65673fb28a5fa449 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Fri, 15 Feb 2019 10:54:55 +0100 Subject: [PATCH] Reland "Always offer transport sequence number header extension for audio"" (reverted in https://webrtc-review.googlesource.com/c/src/+/123182/1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original cl 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. Patchset 3 contain the only change: Add the field trial WebRTC-Audio-SendSideBwe to call/rampup_tests.cc TBR: srte@webrtc.org,ossu@webrtc.org Bug: webrtc:10309 webrtc:10286 Change-Id: I2c1224e8a9fab52c1030369c1364686322e88a0f Reviewed-on: https://webrtc-review.googlesource.com/c/123183 Commit-Queue: Per Kjellander Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#26706} --- audio/BUILD.gn | 1 + audio/audio_send_stream.cc | 4 +- audio/audio_send_stream_tests.cc | 72 +++++++++++-------- audio/audio_send_stream_unittest.cc | 3 + call/rampup_tests.cc | 3 + media/engine/webrtc_voice_engine.cc | 7 +- .../experiments/audio_allocation_settings.cc | 10 +-- .../experiments/audio_allocation_settings.h | 9 ++- .../transport_feedback_tests.cc | 2 + 9 files changed, 63 insertions(+), 48 deletions(-) diff --git a/audio/BUILD.gn b/audio/BUILD.gn index e4ab39a209..80ce0998f9 100644 --- a/audio/BUILD.gn +++ b/audio/BUILD.gn @@ -149,6 +149,7 @@ if (rtc_include_tests) { "../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 0c597ed321..2e4830c1f2 100644 --- a/audio/audio_send_stream.cc +++ b/audio/audio_send_stream.cc @@ -264,8 +264,8 @@ void AudioSendStream::ConfigureStream( 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 7deeff3b4d..55de03dd51 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 @@ TEST_F(AudioSendStreamCallTest, SupportsAudioLevel) { 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* 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* 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 70f6c7238e..e20b031079 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, SetMuted) { } 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, DontRecreateEncoder) { } 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/call/rampup_tests.cc b/call/rampup_tests.cc index 5917a6bc45..55d6a9b449 100644 --- a/call/rampup_tests.cc +++ b/call/rampup_tests.cc @@ -19,6 +19,7 @@ #include "rtc_base/platform_thread.h" #include "rtc_base/string_encode.h" #include "test/encoder_settings.h" +#include "test/field_trial.h" #include "test/gtest.h" #include "test/testsupport/perf_test.h" @@ -640,6 +641,7 @@ TEST_F(RampUpTest, DISABLED_UpDownUpTransportSequenceNumberPacketLoss) { UpDownUpAudioVideoTransportSequenceNumberRtx #endif TEST_F(RampUpTest, MAYBE_UpDownUpAudioVideoTransportSequenceNumberRtx) { + test::ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/"); std::vector loss_rates = {0, 0, 0, 0}; RampUpDownUpTester test(3, 1, 0, kStartBitrateBps, RtpExtension::kTransportSequenceNumberUri, true, @@ -648,6 +650,7 @@ TEST_F(RampUpTest, MAYBE_UpDownUpAudioVideoTransportSequenceNumberRtx) { } TEST_F(RampUpTest, UpDownUpAudioTransportSequenceNumberRtx) { + test::ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/"); std::vector loss_rates = {0, 0, 0, 0}; RampUpDownUpTester test(0, 1, 0, kStartBitrateBps, RtpExtension::kTransportSequenceNumberUri, true, diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index 07a47d789c..a055ae9ed3 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -559,11 +559,8 @@ RtpCapabilities WebRtcVoiceEngine::GetCapabilities() const { 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 79c5a0dfe7..a505357869 100644 --- a/rtc_base/experiments/audio_allocation_settings.cc +++ b/rtc_base/experiments/audio_allocation_settings.cc @@ -65,16 +65,12 @@ bool AudioAllocationSettings::ConfigureRateAllocationRange() const { 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 bcc0f3e90b..f05b4a33ed 100644 --- a/rtc_base/experiments/audio_allocation_settings.h +++ b/rtc_base/experiments/audio_allocation_settings.h @@ -27,14 +27,13 @@ class AudioAllocationSettings { 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 cc0cb7beb1..9aacd9a26f 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, VideoTransportFeedbackNotConfigured) { } 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, } 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 {