From e48851d910cfa7dbe103c26582be51951132fbae Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Wed, 8 Jul 2020 09:06:00 +0200 Subject: [PATCH] red: only enable RED if its preferred as send codec only enables RFC 2198 redundancy if it has a higher preference than Opus. This means it not used by default but can be chosen with setCodecPreferences. BUG=webrtc:11640 Change-Id: I84ff2ca518da70440297a667dedba5cf4484eed7 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/178742 Commit-Queue: Niels Moller Reviewed-by: Henrik Lundin Reviewed-by: Niels Moller Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#31830} --- media/engine/webrtc_voice_engine.cc | 7 +++- media/engine/webrtc_voice_engine_unittest.cc | 36 +++++++++++--------- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index 016c00fbcc..590c31e409 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -1719,6 +1719,7 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs( send_codec_spec; webrtc::BitrateConstraints bitrate_config; absl::optional voice_codec_info; + size_t send_codec_position = 0; for (const AudioCodec& voice_codec : codecs) { if (!(IsCodec(voice_codec, kCnCodecName) || IsCodec(voice_codec, kDtmfCodecName) || @@ -1742,6 +1743,7 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs( bitrate_config = GetBitrateConfigForCodec(voice_codec); break; } + send_codec_position++; } if (!send_codec_spec) { @@ -1783,13 +1785,16 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs( if (IsAudioRedForOpusFieldTrialEnabled()) { // Loop through the codecs to find the RED codec that matches opus // with respect to clockrate and number of channels. + size_t red_codec_position = 0; for (const AudioCodec& red_codec : codecs) { - if (IsCodec(red_codec, kRedCodecName) && + if (red_codec_position < send_codec_position && + IsCodec(red_codec, kRedCodecName) && red_codec.clockrate == send_codec_spec->format.clockrate_hz && red_codec.channels == send_codec_spec->format.num_channels) { send_codec_spec->red_payload_type = red_codec.id; break; } + red_codec_position++; } } diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc index d70019e9f3..1bd631f14d 100644 --- a/media/engine/webrtc_voice_engine_unittest.cc +++ b/media/engine/webrtc_voice_engine_unittest.cc @@ -1507,33 +1507,35 @@ TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecs) { EXPECT_FALSE(channel_->CanInsertDtmf()); } -// Test that we set Opus/Red under the field trial. +// Test that we use Opus/Red under the field trial when it is +// listed as the first codec. TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecsRed) { webrtc::test::ScopedFieldTrials override_field_trials( "WebRTC-Audio-Red-For-Opus/Enabled/"); + EXPECT_TRUE(SetupSendStream()); + cricket::AudioSendParameters parameters; + parameters.codecs.push_back(kRed48000Codec); + parameters.codecs.push_back(kOpusCodec); + SetSendParameters(parameters); + const auto& send_codec_spec = *GetSendStreamConfig(kSsrcX).send_codec_spec; + EXPECT_EQ(111, send_codec_spec.payload_type); + EXPECT_STRCASEEQ("opus", send_codec_spec.format.name.c_str()); + EXPECT_EQ(112, send_codec_spec.red_payload_type); +} + +// Test that we do not use Opus/Red under the field trial by default. +TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecsRedDefault) { + webrtc::test::ScopedFieldTrials override_field_trials( + "WebRTC-Audio-Red-For-Opus/Enabled/"); + EXPECT_TRUE(SetupSendStream()); cricket::AudioSendParameters parameters; parameters.codecs.push_back(kOpusCodec); parameters.codecs.push_back(kRed48000Codec); - parameters.codecs[0].id = 96; SetSendParameters(parameters); const auto& send_codec_spec = *GetSendStreamConfig(kSsrcX).send_codec_spec; - EXPECT_EQ(96, send_codec_spec.payload_type); - EXPECT_STRCASEEQ("opus", send_codec_spec.format.name.c_str()); - EXPECT_EQ(112, send_codec_spec.red_payload_type); -} - -// Test that we set do not interpret Opus/Red by default. -TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecsRedDefault) { - EXPECT_TRUE(SetupSendStream()); - cricket::AudioSendParameters parameters; - parameters.codecs.push_back(kOpusCodec); - parameters.codecs.push_back(kRed48000Codec); - parameters.codecs[0].id = 96; - SetSendParameters(parameters); - const auto& send_codec_spec = *GetSendStreamConfig(kSsrcX).send_codec_spec; - EXPECT_EQ(96, send_codec_spec.payload_type); + EXPECT_EQ(111, send_codec_spec.payload_type); EXPECT_STRCASEEQ("opus", send_codec_spec.format.name.c_str()); EXPECT_EQ(absl::nullopt, send_codec_spec.red_payload_type); }