sdp munging: detect audio nack and changes to opus FEC and DTX
FEC and DTX should not be modified by munging since those encoder
settings are controlled by the remote side.
audio nack is believed to be widely used.
BUG=chromium:40567530
Change-Id: I5dc6716b96d933ceb707b032d9897aa77891672e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/379460
Reviewed-by: Johannes Kron <kron@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@meta.com>
Cr-Commit-Position: refs/heads/main@{#44068}
diff --git a/api/uma_metrics.h b/api/uma_metrics.h
index 9ebb611..033c8f2 100644
--- a/api/uma_metrics.h
+++ b/api/uma_metrics.h
@@ -204,7 +204,11 @@
kAudioCodecsReordered = 62,
kAudioCodecsAddedMultiOpus = 63,
kAudioCodecsAddedL16 = 64,
+ kAudioCodecsRtcpFbAudioNack = 65,
+ kAudioCodecsFmtpOpusFec = 66,
+ kAudioCodecsFmtpOpusCbr = 67,
kAudioCodecsFmtpOpusStereo = 68,
+ kAudioCodecsFmtpOpusDtx = 69,
kAudioCodecsFmtp = 70,
kAudioCodecsRtcpFb = 71,
// Video-related munging.
diff --git a/media/base/media_constants.cc b/media/base/media_constants.cc
index 99ccddf..2a15745 100644
--- a/media/base/media_constants.cc
+++ b/media/base/media_constants.cc
@@ -10,6 +10,8 @@
#include "media/base/media_constants.h"
+#include <cstddef>
+
namespace cricket {
const int kVideoCodecClockrate = 90000;
@@ -58,6 +60,7 @@
const char kCodecParamStereo[] = "stereo";
const char kCodecParamUseInbandFec[] = "useinbandfec";
const char kCodecParamUseDtx[] = "usedtx";
+const char kCodecParamCbr[] = "cbr";
const char kCodecParamMaxAverageBitrate[] = "maxaveragebitrate";
const char kCodecParamMaxPlaybackRate[] = "maxplaybackrate";
diff --git a/media/base/media_constants.h b/media/base/media_constants.h
index 9ad8366..f44779e 100644
--- a/media/base/media_constants.h
+++ b/media/base/media_constants.h
@@ -61,6 +61,7 @@
extern const char kCodecParamStereo[];
extern const char kCodecParamUseInbandFec[];
extern const char kCodecParamUseDtx[];
+extern const char kCodecParamCbr[];
extern const char kCodecParamMaxAverageBitrate[];
extern const char kCodecParamMaxPlaybackRate[];
extern const char kCodecParamPerLayerPictureLossIndication[];
diff --git a/pc/sdp_munging_detector.cc b/pc/sdp_munging_detector.cc
index 052019e..f096f4a 100644
--- a/pc/sdp_munging_detector.cc
+++ b/pc/sdp_munging_detector.cc
@@ -143,11 +143,100 @@
return SdpMungingType::kAudioCodecsAddedL16;
}
+ // Audio NACK is not offered by default.
+ bool created_nack =
+ absl::c_find_if(last_created_media_description->codecs(),
+ [](const cricket::Codec codec) {
+ return codec.HasFeedbackParam(
+ cricket::FeedbackParam(cricket::kRtcpFbParamNack));
+ }) != last_created_media_description->codecs().end();
+ bool set_nack =
+ absl::c_find_if(media_description_to_set->codecs(),
+ [](const cricket::Codec codec) {
+ return codec.HasFeedbackParam(
+ cricket::FeedbackParam(cricket::kRtcpFbParamNack));
+ }) != media_description_to_set->codecs().end();
+ if (!created_nack && set_nack) {
+ RTC_LOG(LS_WARNING) << "SDP munging: audio nack enabled.";
+ return SdpMungingType::kAudioCodecsRtcpFbAudioNack;
+ }
+
if (last_created_media_description->codecs().size() <
media_description_to_set->codecs().size()) {
RTC_LOG(LS_WARNING) << "SDP munging: audio codecs added.";
return SdpMungingType::kAudioCodecsAdded;
}
+
+ // Opus FEC is on by default. Should not be munged, can be controlled by
+ // the other side.
+ bool created_opus_fec =
+ absl::c_find_if(last_created_media_description->codecs(),
+ [](const cricket::Codec codec) {
+ std::string value;
+ return codec.name == cricket::kOpusCodecName &&
+ codec.GetParam(cricket::kCodecParamUseInbandFec,
+ &value) &&
+ value == cricket::kParamValueTrue;
+ }) != last_created_media_description->codecs().end();
+ bool set_opus_fec =
+ absl::c_find_if(
+ media_description_to_set->codecs(), [](const cricket::Codec codec) {
+ std::string value;
+ return codec.name == cricket::kOpusCodecName &&
+ codec.GetParam(cricket::kCodecParamUseInbandFec, &value) &&
+ value == cricket::kParamValueTrue;
+ }) != media_description_to_set->codecs().end();
+ if (created_opus_fec && !set_opus_fec) {
+ RTC_LOG(LS_WARNING) << "SDP munging: Opus FEC disabled.";
+ return SdpMungingType::kAudioCodecsFmtpOpusFec;
+ }
+ // Opus DTX is off by default. Should not be munged, can be controlled by
+ // the other side.
+ bool created_opus_dtx =
+ absl::c_find_if(last_created_media_description->codecs(),
+ [](const cricket::Codec codec) {
+ std::string value;
+ return codec.name == cricket::kOpusCodecName &&
+ codec.GetParam(cricket::kCodecParamUseDtx,
+ &value) &&
+ value == cricket::kParamValueTrue;
+ }) != last_created_media_description->codecs().end();
+ bool set_opus_dtx =
+ absl::c_find_if(
+ media_description_to_set->codecs(), [](const cricket::Codec codec) {
+ std::string value;
+ return codec.name == cricket::kOpusCodecName &&
+ codec.GetParam(cricket::kCodecParamUseDtx, &value) &&
+ value == cricket::kParamValueTrue;
+ }) != media_description_to_set->codecs().end();
+ if (!created_opus_dtx && set_opus_dtx) {
+ RTC_LOG(LS_WARNING) << "SDP munging: Opus DTX enabled.";
+ return SdpMungingType::kAudioCodecsFmtpOpusDtx;
+ }
+
+ // Opus CBR is off by default. Should not be munged, can be controlled by
+ // the other side.
+ bool created_opus_cbr =
+ absl::c_find_if(last_created_media_description->codecs(),
+ [](const cricket::Codec codec) {
+ std::string value;
+ return codec.name == cricket::kOpusCodecName &&
+ codec.GetParam(cricket::kCodecParamCbr,
+ &value) &&
+ value == cricket::kParamValueTrue;
+ }) != last_created_media_description->codecs().end();
+ bool set_opus_cbr =
+ absl::c_find_if(
+ media_description_to_set->codecs(), [](const cricket::Codec codec) {
+ std::string value;
+ return codec.name == cricket::kOpusCodecName &&
+ codec.GetParam(cricket::kCodecParamCbr, &value) &&
+ value == cricket::kParamValueTrue;
+ }) != media_description_to_set->codecs().end();
+ if (!created_opus_cbr && set_opus_cbr) {
+ RTC_LOG(LS_WARNING) << "SDP munging: Opus CBR enabled.";
+ return SdpMungingType::kAudioCodecsFmtpOpusCbr;
+ }
return SdpMungingType::kNoModification;
}
diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc
index 7553009..b8410ef 100644
--- a/pc/sdp_offer_answer_unittest.cc
+++ b/pc/sdp_offer_answer_unittest.cc
@@ -2075,6 +2075,76 @@
ElementsAre(Pair(SdpMungingType::kAudioCodecsFmtpOpusStereo, 1)));
}
+TEST_F(SdpOfferAnswerMungingTest, OpusFec) {
+ auto pc = CreatePeerConnection();
+ pc->AddAudioTrack("audio_track", {});
+
+ auto offer = pc->CreateOffer();
+ auto& contents = offer->description()->contents();
+ ASSERT_EQ(contents.size(), 1u);
+ auto* media_description = contents[0].media_description();
+ ASSERT_TRUE(media_description);
+ std::vector<cricket::Codec> codecs = media_description->codecs();
+ for (auto& codec : codecs) {
+ if (codec.name == cricket::kOpusCodecName) {
+ // Enabled by default so we need to remove the parameter.
+ EXPECT_TRUE(codec.RemoveParam(cricket::kCodecParamUseInbandFec));
+ }
+ }
+ media_description->set_codecs(codecs);
+ RTCError error;
+ EXPECT_TRUE(pc->SetLocalDescription(std::move(offer), &error));
+ EXPECT_THAT(
+ metrics::Samples("WebRTC.PeerConnection.SdpMunging.Offer.Initial"),
+ ElementsAre(Pair(SdpMungingType::kAudioCodecsFmtpOpusFec, 1)));
+}
+
+TEST_F(SdpOfferAnswerMungingTest, OpusDtx) {
+ auto pc = CreatePeerConnection();
+ pc->AddAudioTrack("audio_track", {});
+
+ auto offer = pc->CreateOffer();
+ auto& contents = offer->description()->contents();
+ ASSERT_EQ(contents.size(), 1u);
+ auto* media_description = contents[0].media_description();
+ ASSERT_TRUE(media_description);
+ std::vector<cricket::Codec> codecs = media_description->codecs();
+ for (auto& codec : codecs) {
+ if (codec.name == cricket::kOpusCodecName) {
+ codec.SetParam(cricket::kCodecParamUseDtx, cricket::kParamValueTrue);
+ }
+ }
+ media_description->set_codecs(codecs);
+ RTCError error;
+ EXPECT_TRUE(pc->SetLocalDescription(std::move(offer), &error));
+ EXPECT_THAT(
+ metrics::Samples("WebRTC.PeerConnection.SdpMunging.Offer.Initial"),
+ ElementsAre(Pair(SdpMungingType::kAudioCodecsFmtpOpusDtx, 1)));
+}
+
+TEST_F(SdpOfferAnswerMungingTest, OpusCbr) {
+ auto pc = CreatePeerConnection();
+ pc->AddAudioTrack("audio_track", {});
+
+ auto offer = pc->CreateOffer();
+ auto& contents = offer->description()->contents();
+ ASSERT_EQ(contents.size(), 1u);
+ auto* media_description = contents[0].media_description();
+ ASSERT_TRUE(media_description);
+ std::vector<cricket::Codec> codecs = media_description->codecs();
+ for (auto& codec : codecs) {
+ if (codec.name == cricket::kOpusCodecName) {
+ codec.SetParam(cricket::kCodecParamCbr, cricket::kParamValueTrue);
+ }
+ }
+ media_description->set_codecs(codecs);
+ RTCError error;
+ EXPECT_TRUE(pc->SetLocalDescription(std::move(offer), &error));
+ EXPECT_THAT(
+ metrics::Samples("WebRTC.PeerConnection.SdpMunging.Offer.Initial"),
+ ElementsAre(Pair(SdpMungingType::kAudioCodecsFmtpOpusCbr, 1)));
+}
+
TEST_F(SdpOfferAnswerMungingTest, AudioCodecsRemoved) {
auto pc = CreatePeerConnection();
pc->AddAudioTrack("audio_track", {});
@@ -2407,6 +2477,27 @@
ElementsAre(Pair(SdpMungingType::kAudioCodecsRtcpFb, 1)));
}
+TEST_F(SdpOfferAnswerMungingTest, AudioCodecsRtcpFbNack) {
+ auto pc = CreatePeerConnection();
+ pc->AddAudioTrack("audio_track", {});
+
+ auto offer = pc->CreateOffer();
+ auto& contents = offer->description()->contents();
+ ASSERT_EQ(contents.size(), 1u);
+ auto* media_description = contents[0].media_description();
+ ASSERT_TRUE(media_description);
+ auto codecs = media_description->codecs();
+ ASSERT_GT(codecs.size(), 0u);
+ codecs[0].feedback_params.Add(cricket::FeedbackParam("nack"));
+ media_description->set_codecs(codecs);
+
+ RTCError error;
+ EXPECT_TRUE(pc->SetLocalDescription(std::move(offer), &error));
+ EXPECT_THAT(
+ metrics::Samples("WebRTC.PeerConnection.SdpMunging.Offer.Initial"),
+ ElementsAre(Pair(SdpMungingType::kAudioCodecsRtcpFbAudioNack, 1)));
+}
+
TEST_F(SdpOfferAnswerMungingTest, VideoCodecsRtcpFb) {
auto pc = CreatePeerConnection();
pc->AddVideoTrack("video_track", {});
