Change codec merge to properly discard RTX codecs for discarded codecs.
There were particular issues with the situation where the remote side
contained duplicate codecs AND rtx for both the codecs, the error
was order dependent.
Sideswipe: Add AbslStringify for CodecList.
Bug: webrtc:384954756
Change-Id: I0d995c02f969a550504ab52500c585d1a38e49b1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/379140
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#44008}
diff --git a/media/base/codec_list.h b/media/base/codec_list.h
index 5f19583..c8a003a2 100644
--- a/media/base/codec_list.h
+++ b/media/base/codec_list.h
@@ -52,6 +52,15 @@
// The function will CHECK or DCHECK on inconsistencies.
void CheckConsistency();
+ template <typename Sink>
+ friend void AbslStringify(Sink& sink, const CodecList& list) {
+ absl::Format(&sink, "\n--- Codec list of size %d\n", list.size());
+ for (Codec codec : list) {
+ absl::Format(&sink, "%v\n", codec);
+ }
+ sink.Append("--- End\n");
+ }
+
private:
std::vector<Codec> codecs_;
};
diff --git a/pc/codec_vendor.cc b/pc/codec_vendor.cc
index 0a80dc8..6821b82 100644
--- a/pc/codec_vendor.cc
+++ b/pc/codec_vendor.cc
@@ -420,6 +420,7 @@
const CodecList& offered_codecs,
std::vector<Codec>* negotiated_codecs,
bool keep_offer_order) {
+ std::map<int, int> pt_mapping_table;
for (const Codec& ours : local_codecs) {
std::optional<Codec> theirs =
FindMatchingCodec(local_codecs, offered_codecs, ours);
@@ -430,13 +431,6 @@
NegotiatePacketization(ours, *theirs, &negotiated);
negotiated.IntersectFeedbackParams(*theirs);
if (negotiated.GetResiliencyType() == Codec::ResiliencyType::kRtx) {
- const auto apt_it =
- theirs->params.find(kCodecParamAssociatedPayloadType);
- // webrtc::FindMatchingCodec shouldn't return something with no apt
- // value.
- RTC_DCHECK(apt_it != theirs->params.end());
- negotiated.SetParam(kCodecParamAssociatedPayloadType, apt_it->second);
-
// We support parsing the declarative rtx-time parameter.
const auto rtx_time_it = theirs->params.find(kCodecParamRtxTime);
if (rtx_time_it != theirs->params.end()) {
@@ -461,11 +455,36 @@
NegotiateTxMode(ours, *theirs, &negotiated);
}
#endif
+ // Use their ID, if available.
+ pt_mapping_table.insert({negotiated.id, theirs->id});
negotiated.id = theirs->id;
negotiated.name = theirs->name;
negotiated_codecs->push_back(std::move(negotiated));
}
}
+ // Fix up apt parameters that point to other PTs.
+ for (Codec& negotiated : *negotiated_codecs) {
+ if (negotiated.GetResiliencyType() == Codec::ResiliencyType::kRtx) {
+ // Change the apt value according to the pt mapping table.
+ // This avoids changing to apt values that don't exist any more.
+ std::string apt_str;
+ if (!negotiated.GetParam(kCodecParamAssociatedPayloadType, &apt_str)) {
+ RTC_LOG(LS_WARNING) << "No apt value";
+ continue;
+ }
+ int apt_value;
+ if (!rtc::FromString(apt_str, &apt_value)) {
+ RTC_LOG(LS_WARNING) << "Unconvertable apt value";
+ continue;
+ }
+ if (pt_mapping_table.count(apt_value) != 1) {
+ RTC_LOG(LS_WARNING) << "Unmapped apt value " << apt_value;
+ continue;
+ }
+ negotiated.SetParam(kCodecParamAssociatedPayloadType,
+ pt_mapping_table.at(apt_value));
+ }
+ }
if (keep_offer_order) {
// RFC3264: Although the answerer MAY list the formats in their desired
// order of preference, it is RECOMMENDED that unless there is a
@@ -775,14 +794,31 @@
}
} else if (IsMediaContentOfType(&content, MEDIA_TYPE_VIDEO)) {
CodecList offered_codecs(content.media_description()->codecs());
+ std::vector<Codec> pending_rtx_codecs;
for (const Codec& offered_video_codec : offered_codecs) {
if (!FindMatchingCodec(offered_codecs, filtered_offered_video_codecs,
offered_video_codec) &&
FindMatchingCodec(offered_codecs, all_video_codecs(),
offered_video_codec)) {
+ // Special case: If it's an RTX codec, and the APT points to
+ // a codec that is not yet in the codec list, put it aside.
+ if (offered_video_codec.GetResiliencyType() ==
+ Codec::ResiliencyType::kRtx &&
+ !GetAssociatedCodecForRtx(filtered_offered_video_codecs,
+ offered_video_codec)) {
+ pending_rtx_codecs.push_back(offered_video_codec);
+ continue;
+ }
filtered_offered_video_codecs.push_back(offered_video_codec);
}
}
+ // If the associated codec showed up later in the codec list,
+ // append the corresponding RTX codec.
+ for (const Codec& codec : pending_rtx_codecs) {
+ if (GetAssociatedCodecForRtx(filtered_offered_video_codecs, codec)) {
+ filtered_offered_video_codecs.push_back(codec);
+ }
+ }
}
}
diff --git a/pc/media_session.cc b/pc/media_session.cc
index 5998278..9969d21 100644
--- a/pc/media_session.cc
+++ b/pc/media_session.cc
@@ -461,7 +461,6 @@
char buffer[100];
rtc::SimpleStringBuilder param(buffer);
param << codec_payload_type << "/" << codec_payload_type;
- RTC_LOG(LS_ERROR) << "DEBUG: Setting RED param to " << param.str();
codec.SetParam(kCodecParamNotInNameValueFormat, param.str());
}
}
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc
index 9ed4a5c..3923d6c 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -86,6 +86,7 @@
#include "rtc_base/ssl_fingerprint.h"
#include "rtc_base/ssl_identity.h"
#include "rtc_base/ssl_stream_adapter.h"
+#include "rtc_base/string_encode.h"
#include "rtc_base/task_queue_for_test.h"
#include "rtc_base/test_certificate_verifier.h"
#include "rtc_base/time_utils.h"
@@ -100,9 +101,12 @@
namespace {
using ::testing::AtLeast;
+using ::testing::Eq;
+using ::testing::Field;
using ::testing::InSequence;
using ::testing::MockFunction;
using ::testing::NiceMock;
+using ::testing::NotNull;
using ::testing::Return;
class PeerConnectionIntegrationTest
@@ -4820,6 +4824,71 @@
EXPECT_TRUE(caller()->SetLocalDescriptionAndSendSdpMessage(std::move(offer)));
}
+TEST_F(PeerConnectionIntegrationTestUnifiedPlan,
+ SensibleRtxWithDuplicateCodecs) {
+ ASSERT_TRUE(CreatePeerConnectionWrappers());
+ caller()->AddVideoTrack();
+ // Copied from WPT test webrtc/protocol/rtx-codecs.https.html
+ std::string remote_offer_string =
+ "v=0\r\n"
+ "o=- 1878890426675213188 2 IN IP4 127.0.0.1\r\n"
+ "s=-\r\n"
+ "t=0 0\r\n"
+ "a=group:BUNDLE video\r\n"
+ "a=msid-semantic: WMS\r\n"
+ "m=video 9 UDP/TLS/RTP/SAVPF 96 97 98 99\r\n"
+ "c=IN IP4 0.0.0.0\r\n"
+ "a=rtcp:9 IN IP4 0.0.0.0\r\n"
+ "a=ice-ufrag:RGPK\r\n"
+ "a=ice-pwd:rAyHEAKC7ckxQgWaRZXukz+Z\r\n"
+ "a=ice-options:trickle\r\n"
+ "a=fingerprint:sha-256 "
+ "8C:29:0A:8F:11:06:BF:1C:58:B3:CA:E6:F1:F1:DC:99:4C:6C:89:E9:FF:BC:D4:38:"
+ "11:18:1F:40:19:C8:49:37\r\n"
+ "a=setup:actpass\r\n"
+ "a=mid:video\r\n"
+ "a=recvonly\r\n"
+ "a=rtcp-mux\r\n"
+ "a=rtpmap:96 VP8/90000\r\n"
+ "a=rtpmap:97 rtx/90000\r\n"
+ "a=fmtp:97 apt=98\r\n"
+ "a=rtpmap:98 VP8/90000\r\n"
+ "a=rtcp-fb:98 ccm fir\r\n"
+ "a=rtcp-fb:98 nack\r\n"
+ "a=rtcp-fb:98 nack pli\r\n"
+ "a=rtcp-fb:98 goog-remb\r\n"
+ "a=rtcp-fb:98 transport-cc\r\n"
+ "a=rtpmap:99 rtx/90000\r\n"
+ "a=fmtp:99 apt=96\r\n";
+ auto srd_observer =
+ rtc::make_ref_counted<MockSetSessionDescriptionObserver>();
+ std::unique_ptr<SessionDescriptionInterface> remote_offer =
+ CreateSessionDescription(SdpType::kOffer, remote_offer_string);
+ EXPECT_TRUE(caller()->SetRemoteDescription(std::move(remote_offer)));
+ // The resulting SDP answer should have one video codec with a correctly
+ // associated RTX codec.
+ std::unique_ptr<SessionDescriptionInterface> answer =
+ caller()->CreateAnswerForTest();
+ ASSERT_THAT(answer, NotNull());
+ RTC_LOG(LS_ERROR) << "Answer is " << *answer;
+ ASSERT_THAT(answer->description()->contents().size(), Eq(1));
+ auto codecs =
+ answer->description()->contents()[0].media_description()->codecs();
+ std::vector<int> apt_values;
+ for (const cricket::Codec& codec : codecs) {
+ if (codec.GetResiliencyType() == cricket::Codec::ResiliencyType::kRtx) {
+ const auto apt_it =
+ codec.params.find(cricket::kCodecParamAssociatedPayloadType);
+ int apt_value;
+ ASSERT_TRUE(rtc::FromString(apt_it->second, &apt_value));
+ apt_values.push_back(apt_value);
+ }
+ }
+ for (int apt : apt_values) {
+ EXPECT_THAT(codecs, Contains(Field("id", &cricket::Codec::id, apt)));
+ }
+}
+
} // namespace
} // namespace webrtc
