From 885ba07b654e5874cbf2f8714563c5dc9263cc97 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Tue, 24 Aug 2021 15:56:03 +0200 Subject: [PATCH] Reland "red: generate and parse the red fmtp format" This is a reland of 9d0730942677a520ce7e184d081b4c5a2469fc48 Original change's description: > red: generate and parse the red fmtp format > > generates a fmtp line like > a=fmtp: / > and matches the incoming redundant payload types against the > send codec one. Offers without an FMTP line will not use RED. > Redundancy levels of 1 (plus main packet ) to 32 are accepted but > this is not wired up to the encoder since the O/A semantic of > RFC 2198 is not clear. > > This decreases the chance of a collision with the SATIN codec > which also runs on 48khz (but so far does not specify a channelCount of 2) > > BUG=webrtc:11640 > > Change-Id: I8755e5b1e944d105212f1bbe4f330cf4e0753e67 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/229583 > Reviewed-by: Harald Alvestrand > Reviewed-by: Henrik Lundin > Commit-Queue: Harald Alvestrand > Cr-Commit-Position: refs/heads/main@{#34848} Bug: webrtc:11640 Change-Id: I9465e489897a8ded9845592477fe14678af7ab61 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/230545 Commit-Queue: Harald Alvestrand Reviewed-by: Harald Alvestrand Reviewed-by: Mirko Bonadei Reviewed-by: Henrik Lundin Cr-Commit-Position: refs/heads/main@{#34965} --- media/engine/webrtc_voice_engine.cc | 38 +++++++++- media/engine/webrtc_voice_engine_unittest.cc | 78 +++++++++++++++++--- 2 files changed, 101 insertions(+), 15 deletions(-) diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index 75d1a5fe02..da5f594085 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -777,7 +777,9 @@ std::vector WebRtcVoiceEngine::CollectCodecs( out.push_back(codec); if (codec.name == kOpusCodecName && audio_red_for_opus_enabled_) { - map_format({kRedCodecName, 48000, 2}, &out); + std::string redFmtp = + rtc::ToString(codec.id) + "/" + rtc::ToString(codec.id); + map_format({kRedCodecName, 48000, 2, {{"", redFmtp}}}, &out); } } } @@ -1661,6 +1663,37 @@ bool WebRtcVoiceMediaChannel::SetRecvCodecs( return true; } +// Utility function to check if RED codec and its parameters match a codec spec. +bool CheckRedParameters( + const AudioCodec& red_codec, + const webrtc::AudioSendStream::Config::SendCodecSpec& send_codec_spec) { + if (red_codec.clockrate != send_codec_spec.format.clockrate_hz || + red_codec.channels != send_codec_spec.format.num_channels) { + return false; + } + + // Check the FMTP line for the empty parameter which should match + // /[/...] + auto red_parameters = red_codec.params.find(""); + if (red_parameters == red_codec.params.end()) { + RTC_LOG(LS_WARNING) << "audio/RED missing fmtp parameters."; + return false; + } + std::vector redundant_payloads; + rtc::split(red_parameters->second, '/', &redundant_payloads); + // 32 is chosen as a maximum upper bound for consistency with the + // red payload splitter. + if (redundant_payloads.size() < 2 || redundant_payloads.size() > 32) { + return false; + } + for (auto pt : redundant_payloads) { + if (pt != rtc::ToString(send_codec_spec.payload_type)) { + return false; + } + } + return true; +} + // Utility function called from SetSendParameters() to extract current send // codec settings from the given list of codecs (originally from SDP). Both send // and receive streams may be reconfigured based on the new settings. @@ -1772,8 +1805,7 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs( for (const AudioCodec& red_codec : codecs) { 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) { + CheckRedParameters(red_codec, *send_codec_spec)) { send_codec_spec->red_payload_type = red_codec.id; break; } diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc index f1c0c1eae4..8007210326 100644 --- a/media/engine/webrtc_voice_engine_unittest.cc +++ b/media/engine/webrtc_voice_engine_unittest.cc @@ -1028,10 +1028,12 @@ TEST_P(WebRtcVoiceEngineTestFake, RecvRedDefault) { cricket::AudioRecvParameters parameters; parameters.codecs.push_back(kOpusCodec); parameters.codecs.push_back(kRed48000Codec); + parameters.codecs[1].params[""] = "111/111"; EXPECT_TRUE(channel_->SetRecvParameters(parameters)); EXPECT_THAT(GetRecvStreamConfig(kSsrcX).decoder_map, (ContainerEq>( - {{111, {"opus", 48000, 2}}, {112, {"red", 48000, 2}}}))); + {{111, {"opus", 48000, 2}}, + {112, {"red", 48000, 2, {{"", "111/111"}}}}}))); } // Test that we disable Opus/Red with the kill switch. @@ -1495,15 +1497,13 @@ TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecs) { EXPECT_FALSE(channel_->CanInsertDtmf()); } -// Test that we use Opus/Red under the field trial when it is -// listed as the first codec. +// Test that we use Opus/Red by default when it is +// listed as the first codec and there is an fmtp line. 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[0].params[""] = "111/111"; parameters.codecs.push_back(kOpusCodec); SetSendParameters(parameters); const auto& send_codec_spec = *GetSendStreamConfig(kSsrcX).send_codec_spec; @@ -1512,15 +1512,13 @@ TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecsRed) { 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/"); - +// Test that we do not use Opus/Red by default when it is +// listed as the first codec but there is no fmtp line. +TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecsRedNoFmtp) { EXPECT_TRUE(SetupSendStream()); cricket::AudioSendParameters parameters; - parameters.codecs.push_back(kOpusCodec); 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); @@ -1528,6 +1526,62 @@ TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecsRedDefault) { EXPECT_EQ(absl::nullopt, send_codec_spec.red_payload_type); } +// Test that we do not use 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[1].params[""] = "111/111"; + 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(absl::nullopt, send_codec_spec.red_payload_type); +} + +// Test that the RED fmtp line must match the payload type. +TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecsRedFmtpMismatch) { + EXPECT_TRUE(SetupSendStream()); + cricket::AudioSendParameters parameters; + parameters.codecs.push_back(kRed48000Codec); + parameters.codecs[0].params[""] = "8/8"; + 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(absl::nullopt, send_codec_spec.red_payload_type); +} + +// Test that the RED fmtp line must show 2..32 payloads. +TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecsRedFmtpAmountOfRedundancy) { + EXPECT_TRUE(SetupSendStream()); + cricket::AudioSendParameters parameters; + parameters.codecs.push_back(kRed48000Codec); + parameters.codecs[0].params[""] = "111"; + 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(absl::nullopt, send_codec_spec.red_payload_type); + for (int i = 1; i < 32; i++) { + parameters.codecs[0].params[""] += "/111"; + 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); + } + parameters.codecs[0].params[""] += "/111"; + SetSendParameters(parameters); + const auto& send_codec_spec2 = *GetSendStreamConfig(kSsrcX).send_codec_spec; + EXPECT_EQ(111, send_codec_spec2.payload_type); + EXPECT_STRCASEEQ("opus", send_codec_spec2.format.name.c_str()); + EXPECT_EQ(absl::nullopt, send_codec_spec2.red_payload_type); +} + // Test that WebRtcVoiceEngine reconfigures, rather than recreates its // AudioSendStream. TEST_P(WebRtcVoiceEngineTestFake, DontRecreateSendStream) {