Enable test that there are no duplicates in a CodecList
This also required handling a case where an Android decoder factory
caused duplicate codec IDs to be generated. The video engine now
guards against this; the Android factory is not touched.
Bug: webrtc:42224689
Change-Id: Id448acb26914564b0ebb618f2ef12ee74941c793
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/379421
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Auto-Submit: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#44129}
diff --git a/api/video_codecs/sdp_video_format.h b/api/video_codecs/sdp_video_format.h
index 16b4fae..3fbcf0c 100644
--- a/api/video_codecs/sdp_video_format.h
+++ b/api/video_codecs/sdp_video_format.h
@@ -81,6 +81,11 @@
static const SdpVideoFormat VP9Profile3();
static const SdpVideoFormat AV1Profile0();
static const SdpVideoFormat AV1Profile1();
+
+ template <typename Sink>
+ friend void AbslStringify(Sink& sink, const SdpVideoFormat& format) {
+ sink.Append(format.ToString());
+ }
};
// For not so good reasons sometimes additional parameters are added to an
diff --git a/media/BUILD.gn b/media/BUILD.gn
index 5933d9e..ab9316c 100644
--- a/media/BUILD.gn
+++ b/media/BUILD.gn
@@ -876,6 +876,7 @@
"../rtc_base:copy_on_write_buffer",
"../rtc_base:dscp",
"../rtc_base:gunit_helpers",
+ "../rtc_base:logging",
"../rtc_base:macromagic",
"../rtc_base:network_route",
"../rtc_base:rtc_event",
diff --git a/media/base/codec_list.cc b/media/base/codec_list.cc
index 9fa1b3c..8efc94c 100644
--- a/media/base/codec_list.cc
+++ b/media/base/codec_list.cc
@@ -36,18 +36,11 @@
for (size_t i = 0; i < codecs.size(); i++) {
const Codec& codec = codecs[i];
if (codec.id != Codec::kIdNotSet) {
- // Not true - the test PeerConnectionMediaTest.RedFmtpPayloadMixed
- // fails this check. In that case, the duplicates are identical.
- // TODO: https://issues.webrtc.org/384756621 - fix test and enable check.
- // RTC_DCHECK(pt_to_index.count(codec.id) == 0);
- if (pt_to_index.count(codec.id) != 0) {
- RTC_LOG(LS_WARNING) << "Surprising condition: Two codecs on same PT. "
- << "First: " << codecs[pt_to_index[codec.id]]
- << " Second: " << codec;
- // Skip this codec in the map, and go on.
- continue;
+ bool inserted = pt_to_index.insert({codec.id, i}).second;
+ if (!inserted) {
+ LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
+ "Duplicate payload type in codec list");
}
- pt_to_index.insert({codec.id, i});
}
}
for (const Codec& codec : codecs) {
diff --git a/media/engine/fake_webrtc_video_engine.cc b/media/engine/fake_webrtc_video_engine.cc
index 18320f9..0fa87f5 100644
--- a/media/engine/fake_webrtc_video_engine.cc
+++ b/media/engine/fake_webrtc_video_engine.cc
@@ -19,6 +19,7 @@
#include "media/base/media_constants.h"
#include "media/engine/simulcast_encoder_adapter.h"
#include "modules/video_coding/include/video_error_codes.h"
+#include "rtc_base/logging.h"
#include "rtc_base/time_utils.h"
namespace cricket {
@@ -89,9 +90,13 @@
std::vector<webrtc::SdpVideoFormat> formats;
for (const webrtc::SdpVideoFormat& format : supported_codec_formats_) {
- // Don't add same codec twice.
- if (!format.IsCodecInList(formats))
- formats.push_back(format);
+ // We need to test erroneous scenarios, so just warn if there's
+ // a duplicate.
+ if (format.IsCodecInList(formats)) {
+ RTC_LOG(LS_WARNING) << "GetSupportedFormats found a duplicate format: "
+ << format << ", check that this is expected.";
+ }
+ formats.push_back(format);
}
return formats;
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc
index d2e69f1..60b2634 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -21,6 +21,7 @@
#include <optional>
#include <set>
#include <string>
+#include <unordered_set>
#include <utility>
#include <vector>
@@ -242,8 +243,8 @@
auto supported_formats =
GetDefaultSupportedFormats(factory, is_decoder_factory, trials);
- // Temporary: Use PayloadTypePicker for assignments.
webrtc::PayloadTypePicker pt_mapper;
+ std::unordered_set<int> used_payload_types;
std::vector<Codec> output_codecs;
for (const auto& supported_format : supported_formats) {
webrtc::RTCErrorOr<Codec> result =
@@ -253,6 +254,13 @@
// TODO: https://issues.webrtc.org/360058654 - Handle running out of IDs.
continue;
}
+ bool inserted = used_payload_types.insert(result.value().id).second;
+ if (!inserted) {
+ RTC_LOG(LS_WARNING) << "Factory produced duplicate codecs, ignoring "
+ << result.value() << " produced from "
+ << supported_format;
+ continue;
+ }
output_codecs.push_back(result.value());
if (include_rtx) {
Codec::ResiliencyType resiliency_type =
diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc
index d1657c4..23e85ab 100644
--- a/media/engine/webrtc_video_engine_unittest.cc
+++ b/media/engine/webrtc_video_engine_unittest.cc
@@ -21,6 +21,7 @@
#include <set>
#include <string>
#include <tuple>
+#include <unordered_set>
#include <utility>
#include <vector>
@@ -429,6 +430,8 @@
void AddSupportedVideoCodecType(
const std::string& name,
const std::vector<webrtc::ScalabilityMode>& scalability_modes = {});
+ void AddSupportedVideoCodec(webrtc::SdpVideoFormat format);
+
std::unique_ptr<VideoMediaSendChannelInterface>
SetSendParamsWithAllSupportedCodecs();
@@ -499,6 +502,32 @@
}
}
+MATCHER(HasUniquePtValues, "") {
+ std::unordered_set<int> seen_ids;
+ for (const auto& codec : arg) {
+ if (seen_ids.count(codec.id) > 0) {
+ *result_listener << "Duplicate id for " << absl::StrCat(codec);
+ return false;
+ }
+ seen_ids.insert(codec.id);
+ }
+ return true;
+}
+
+TEST_F(WebRtcVideoEngineTest, SupportingTwoKindsOfVp9IsOk) {
+ AddSupportedVideoCodecType("VP8");
+ AddSupportedVideoCodec(webrtc::SdpVideoFormat("VP9", {{"profile-id", "0"}}));
+ AddSupportedVideoCodec(webrtc::SdpVideoFormat("VP9", {{"profile-id", "1"}}));
+ AddSupportedVideoCodec(webrtc::SdpVideoFormat("VP9", {{"profile-id", "3"}}));
+ AddSupportedVideoCodec(webrtc::SdpVideoFormat(
+ "AV1", {{"level-idx", "5"}, {"profile", "1"}, {"tier", "0"}}));
+ AddSupportedVideoCodec(webrtc::SdpVideoFormat(
+ "AV1", {{"level-idx", "5"}, {"profile", "0"}, {"tier", "0"}}));
+ AddSupportedVideoCodec(webrtc::SdpVideoFormat("VP9")); // No parameters
+ ASSERT_THAT(engine_.LegacySendCodecs(), HasUniquePtValues());
+ ASSERT_THAT(engine_.LegacyRecvCodecs(), HasUniquePtValues());
+}
+
TEST_F(WebRtcVideoEngineTest, SupportsTimestampOffsetHeaderExtension) {
ExpectRtpCapabilitySupport(RtpExtension::kTimestampOffsetUri, true);
}
@@ -939,6 +968,12 @@
decoder_factory_->AddSupportedVideoCodecType(name);
}
+void WebRtcVideoEngineTest::AddSupportedVideoCodec(
+ webrtc::SdpVideoFormat format) {
+ encoder_factory_->AddSupportedVideoCodec(format);
+ decoder_factory_->AddSupportedVideoCodec(format);
+}
+
std::unique_ptr<VideoMediaSendChannelInterface>
WebRtcVideoEngineTest::SetSendParamsWithAllSupportedCodecs() {
std::unique_ptr<VideoMediaSendChannelInterface> channel =
diff --git a/pc/peer_connection_signaling_unittest.cc b/pc/peer_connection_signaling_unittest.cc
index 2214557..a580cd8 100644
--- a/pc/peer_connection_signaling_unittest.cc
+++ b/pc/peer_connection_signaling_unittest.cc
@@ -23,6 +23,7 @@
#include <utility>
#include <vector>
+#include "absl/strings/str_replace.h"
#include "api/audio/audio_device.h"
#include "api/audio_codecs/builtin_audio_decoder_factory.h"
#include "api/audio_codecs/builtin_audio_encoder_factory.h"
@@ -1372,4 +1373,22 @@
EXPECT_EQ(apt_it->second, "102");
}
+TEST_F(PeerConnectionSignalingUnifiedPlanTest, LoopbackSdpIsPossible) {
+ // This is not a recommended way of doing things.
+ // The test is added because an Android test tries to do it this way,
+ // and triggered surprising behavior.
+ auto caller = CreatePeerConnection();
+ auto transceiver =
+ caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, RtpTransceiverInit());
+
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+ std::string offer_sdp;
+ ASSERT_TRUE(offer->ToString(&offer_sdp));
+ std::string answer_sdp =
+ absl::StrReplaceAll(offer_sdp, {{"a=setup:actpass", "a=setup:active"}});
+ EXPECT_TRUE(caller->SetLocalDescription(std::move(offer)));
+ auto answer = CreateSessionDescription(SdpType::kAnswer, answer_sdp);
+ EXPECT_TRUE(caller->SetRemoteDescription(std::move(answer)));
+}
+
} // namespace webrtc
