From 9f827f51f846ac4250078b66249dddfb179db474 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 19 Dec 2024 06:43:33 +0000 Subject: [PATCH] Extend FindMatchingCodec to support multi-codec RED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was exercised by a test, but multi-codec RED is not currently generated by WebRTC. RED spec allows it, so failing in comparator seems wrong. This was one of the cases where the referenced bug was triggered, but not the only one. Bug: webrtc:384756621 Change-Id: I28c101aa34a62083b72b5f7fc12d25fc637db209 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/372060 Commit-Queue: Harald Alvestrand Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/main@{#43607} --- media/base/codec_comparators.cc | 37 ++++++++++++------------ media/base/codec_comparators_unittest.cc | 35 ++++++++++++++++++++++ 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/media/base/codec_comparators.cc b/media/base/codec_comparators.cc index 45a1b9b8cf..bf26f17c63 100644 --- a/media/base/codec_comparators.cc +++ b/media/base/codec_comparators.cc @@ -9,6 +9,7 @@ */ #include "media/base/codec_comparators.h" +#include #include #include #include @@ -181,34 +182,34 @@ bool MatchesWithReferenceAttributesAndComparator( return true; } if (has_parameters_1 && has_parameters_2) { - // Different levels of redundancy between offer and answer are + // Different levels of redundancy between offer and answer are OK // since RED is considered to be declarative. std::vector redundant_payloads_1 = rtc::split(red_parameters_1->second, '/'); std::vector redundant_payloads_2 = rtc::split(red_parameters_2->second, '/'); - if (redundant_payloads_1.size() > 0 && redundant_payloads_2.size() > 0) { - // Mixed reference codecs (i.e. 111/112) are not supported. - for (size_t i = 1; i < redundant_payloads_1.size(); i++) { - if (redundant_payloads_1[i] != redundant_payloads_1[0]) { - return false; - } - } - for (size_t i = 1; i < redundant_payloads_2.size(); i++) { - if (redundant_payloads_2[i] != redundant_payloads_2[0]) { - return false; - } - } + // note: rtc::split returns at least 1 string even on empty strings. + size_t smallest_size = + std::min(redundant_payloads_1.size(), redundant_payloads_2.size()); + // If the smaller list is equivalent to the longer list, we consider them + // equivalent even if size differs. + for (size_t i = 0; i < smallest_size; i++) { int red_value_1; int red_value_2; - if (rtc::FromString(redundant_payloads_1[0], &red_value_1) && - rtc::FromString(redundant_payloads_2[0], &red_value_2)) { - if (reference_comparator(red_value_1, red_value_2)) { - return true; + if (rtc::FromString(redundant_payloads_1[i], &red_value_1) && + rtc::FromString(redundant_payloads_2[i], &red_value_2)) { + if (!reference_comparator(red_value_1, red_value_2)) { + return false; } + } else { + // At least one parameter was not an integer. + // This is a syntax error, but we allow it here if the whole parameter + // equals the other parameter, in order to not generate more errors + // by duplicating the bad parameter. + return red_parameters_1->second == red_parameters_2->second; } - return false; } + return true; } if (!has_parameters_1 && !has_parameters_2) { // Both parameters are missing. Happens for video RED. diff --git a/media/base/codec_comparators_unittest.cc b/media/base/codec_comparators_unittest.cc index 6f4a1990bf..f1286de977 100644 --- a/media/base/codec_comparators_unittest.cc +++ b/media/base/codec_comparators_unittest.cc @@ -80,6 +80,41 @@ TEST(CodecComparatorsTest, StaticPayloadTypesIgnoreName) { EXPECT_TRUE(MatchesWithCodecRules(codec_1, codec_2)); } +TEST(CodecComparatorsTest, MatchesWithReferenceAttributesRed) { + // Test that RED codecs' reference attributes get parsed correctly. + Codec codec_1 = + cricket::CreateAudioCodec(101, cricket::kRedCodecName, 48000, 2); + codec_1.SetParam(cricket::kCodecParamNotInNameValueFormat, "100/100"); + Codec codec_2 = + cricket::CreateAudioCodec(102, cricket::kRedCodecName, 48000, 2); + codec_2.SetParam(cricket::kCodecParamNotInNameValueFormat, "101/101"); + // Mixed codecs in RED + Codec codec_3 = + cricket::CreateAudioCodec(103, cricket::kRedCodecName, 48000, 2); + codec_3.SetParam(cricket::kCodecParamNotInNameValueFormat, "100/101"); + // Identical codecs always match. + EXPECT_TRUE(MatchesWithReferenceAttributes(codec_1, codec_1)); + EXPECT_TRUE(MatchesWithReferenceAttributes(codec_2, codec_2)); + EXPECT_TRUE(MatchesWithReferenceAttributes(codec_3, codec_3)); + // Mismatched reference codec lists. + EXPECT_FALSE(MatchesWithReferenceAttributes(codec_1, codec_2)); + EXPECT_FALSE(MatchesWithReferenceAttributes(codec_1, codec_3)); + EXPECT_FALSE(MatchesWithReferenceAttributes(codec_2, codec_3)); + // Overflow of longer lists are ignored. + // Overlong list - overflow should be ignored. + Codec codec_4 = + cricket::CreateAudioCodec(103, cricket::kRedCodecName, 48000, 2); + codec_4.SetParam(cricket::kCodecParamNotInNameValueFormat, "100/100/101/102"); + EXPECT_TRUE(MatchesWithReferenceAttributes(codec_4, codec_4)); + EXPECT_TRUE(MatchesWithReferenceAttributes(codec_1, codec_4)); + // Broken syntax will cause a non-match with anything except itself. + Codec codec_5 = + cricket::CreateAudioCodec(103, cricket::kRedCodecName, 48000, 2); + codec_5.SetParam(cricket::kCodecParamNotInNameValueFormat, ""); + EXPECT_TRUE(MatchesWithReferenceAttributes(codec_5, codec_5)); + EXPECT_FALSE(MatchesWithReferenceAttributes(codec_1, codec_5)); +} + struct TestParams { std::string name; SdpVideoFormat codec1;