Reland "sdp: reject duplicate codecs with the same id but different name or clockrate"

This is a reland of commit ad6807805d12e48f11c3a68b4befaf8d7c23e8b5

Original change's description:
> sdp: reject duplicate codecs with the same id but different name or clockrate
>
> since something like
>   rtpmap:96 VP8/90000
>   rtpmap:96 VP9/90000
> or
>   rtpmap:97 ISAC/32000
>   rtpmap:97 ISAC/16000
> is wrong. Note that fmtp or rtcp-fb are not taken into account.
> Also note that sending invalid static payload types now throws an error.
>
> Drive-by: replace "RtpMap" with "Rtpmap" for consistency.
>
> BUG=None
>
> Change-Id: I2574b82a6f1a0afe3edc866e514a5dbca0798e8c
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/263641
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Philipp Hancke <philipp.hancke@googlemail.com>
> Cr-Commit-Position: refs/heads/main@{#37028}

Bug: webrtc:14140
Change-Id: I63a37aacea6b9e0a9d7570b8422849275eb69aae
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264544
Reviewed-by: Christoffer Jansson <jansson@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Philipp Hancke <philipp.hancke@googlemail.com>
Cr-Commit-Position: refs/heads/main@{#37066}

pc/test/test_sdp_strings.h

@@ -133,10 +133,10 @@ static const char kAudioSdpWithUnsupportedCodecsPlanB[] =
     "s=-\r\n"
     "c=IN IP4 192.168.30.208\r\n"
     "t=0 0\r\n"
-    "m=audio 16000 RTP/SAVPF 0 8 18 110 126\r\n"
+    "m=audio 16000 RTP/SAVPF 0 8 109 110 126\r\n"
     "a=rtpmap:0 PCMU/8000\r\n"
     "a=rtpmap:8 PCMA/8000\r\n"
-    "a=rtpmap:18 WeirdCodec1/8000\r\n"
+    "a=rtpmap:109 WeirdCodec1/8000\r\n"
     "a=rtpmap:110 WeirdCodec2/8000\r\n"
     "a=rtpmap:126 telephone-event/8000\r\n"
     "a=sendonly\r\n"
@@ -160,10 +160,10 @@ static const char kAudioSdpWithUnsupportedCodecsUnifiedPlan[] =
     "s=-\r\n"
     "c=IN IP4 192.168.30.208\r\n"
     "t=0 0\r\n"
-    "m=audio 16000 RTP/SAVPF 0 8 18 110 126\r\n"
+    "m=audio 16000 RTP/SAVPF 0 8 109 110 126\r\n"
     "a=rtpmap:0 PCMU/8000\r\n"
     "a=rtpmap:8 PCMA/8000\r\n"
-    "a=rtpmap:18 WeirdCodec1/8000\r\n"
+    "a=rtpmap:109 WeirdCodec1/8000\r\n"
     "a=rtpmap:110 WeirdCodec2/8000\r\n"
     "a=rtpmap:126 telephone-event/8000\r\n"
     "a=sendonly\r\n"

pc/webrtc_sdp.cc

@@ -280,7 +280,7 @@ static void BuildRtpContentAttributes(const MediaContentDescription* media_desc,
                                       const cricket::MediaType media_type,
                                       int msid_signaling,
                                       std::string* message);
-static void BuildRtpMap(const MediaContentDescription* media_desc,
+static void BuildRtpmap(const MediaContentDescription* media_desc,
                         const cricket::MediaType media_type,
                         std::string* message);
 static void BuildCandidate(const std::vector<Candidate>& candidates,
@@ -1682,7 +1682,7 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc,
   // RFC 4566
   // a=rtpmap:<payload type> <encoding name>/<clock rate>
   // [/<encodingparameters>]
-  BuildRtpMap(media_desc, media_type, message);
+  BuildRtpmap(media_desc, media_type, message);
 
   for (const StreamParams& track : media_desc->streams()) {
     // Build the ssrc-group lines.
@@ -1882,7 +1882,7 @@ bool GetParameter(const std::string& name,
   return true;
 }
 
-void BuildRtpMap(const MediaContentDescription* media_desc,
+void BuildRtpmap(const MediaContentDescription* media_desc,
                  const cricket::MediaType media_type,
                  std::string* message) {
   RTC_DCHECK(message != NULL);
@@ -2852,7 +2852,7 @@ T GetCodecWithPayloadType(const std::vector<T>& codecs, int payload_type) {
   return ret_val;
 }
 
-// Updates or creates a new codec entry in the audio description.
+// Updates or creates a new codec entry in the media description.
 template <class T, class U>
 void AddOrReplaceCodec(MediaContentDescription* content_desc, const U& codec) {
   T* desc = static_cast<T*>(content_desc);
@@ -3556,6 +3556,7 @@ bool ParseRtpmapAttribute(absl::string_view line,
                           const std::vector<int>& payload_types,
                           MediaContentDescription* media_desc,
                           SdpParseError* error) {
+  static const int kFirstDynamicPayloadTypeLowerRange = 35;
   std::vector<std::string> fields;
   rtc::split(line.substr(kLinePrefixLength), kSdpDelimiterSpaceChar, &fields);
   // RFC 4566
@@ -3596,8 +3597,23 @@ bool ParseRtpmapAttribute(absl::string_view line,
   if (!GetValueFromString(line, codec_params[1], &clock_rate, error)) {
     return false;
   }
+
   if (media_type == cricket::MEDIA_TYPE_VIDEO) {
     VideoContentDescription* video_desc = media_desc->as_video();
+    for (const cricket::VideoCodec& existing_codec : video_desc->codecs()) {
+      if (!existing_codec.name.empty() && payload_type == existing_codec.id &&
+          (encoding_name != existing_codec.name ||
+           clock_rate != existing_codec.clockrate)) {
+        rtc::StringBuilder description;
+        description
+            << "Duplicate "
+            << (payload_type < kFirstDynamicPayloadTypeLowerRange
+                    ? "statically assigned"
+                    : "")
+            << " payload type with conflicting codec name or clock rate.";
+        return ParseFailed(line, description.Release(), error);
+      }
+    }
     UpdateCodec(payload_type, encoding_name, video_desc);
   } else if (media_type == cricket::MEDIA_TYPE_AUDIO) {
     // RFC 4566
@@ -3616,6 +3632,21 @@ bool ParseRtpmapAttribute(absl::string_view line,
     }
 
     AudioContentDescription* audio_desc = media_desc->as_audio();
+    for (const cricket::AudioCodec& existing_codec : audio_desc->codecs()) {
+      if (!existing_codec.name.empty() && payload_type == existing_codec.id &&
+          (encoding_name != existing_codec.name ||
+           clock_rate != existing_codec.clockrate ||
+           channels != existing_codec.channels)) {
+        rtc::StringBuilder description;
+        description
+            << "Duplicate "
+            << (payload_type < kFirstDynamicPayloadTypeLowerRange
+                    ? "statically assigned"
+                    : "")
+            << " payload type with conflicting codec name or clock rate.";
+        return ParseFailed(line, description.Release(), error);
+      }
+    }
     UpdateCodec(payload_type, encoding_name, clock_rate, 0, channels,
                 audio_desc);
   }

pc/webrtc_sdp_unittest.cc

@@ -2427,7 +2427,7 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithoutRtpmap) {
       // Codec that doesn't appear in the m= line will be ignored.
       "a=rtpmap:104 ISAC/32000\r\n"
       // The rtpmap line for static payload codec is optional.
-      "a=rtpmap:18 G729/16000\r\n"
+      "a=rtpmap:18 G729/8000\r\n"
       "a=rtpmap:103 ISAC/16000\r\n";
 
   JsepSessionDescription jdesc(kDummyType);
@@ -2438,7 +2438,7 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithoutRtpmap) {
   // The codecs in the AudioContentDescription should be in the same order as
   // the payload types (<fmt>s) on the m= line.
   ref_codecs.push_back(AudioCodec(0, "PCMU", 8000, 0, 1));
-  ref_codecs.push_back(AudioCodec(18, "G729", 16000, 0, 1));
+  ref_codecs.push_back(AudioCodec(18, "G729", 8000, 0, 1));
   ref_codecs.push_back(AudioCodec(103, "ISAC", 16000, 0, 1));
   EXPECT_EQ(ref_codecs, audio->codecs());
 }
@@ -4658,6 +4658,47 @@ TEST_F(WebRtcSdpTest, MaxChannels) {
   ExpectParseFailure(sdp, "a=rtpmap:108 ISAC/16000/512");
 }
 
+TEST_F(WebRtcSdpTest, DuplicateAudioRtpmapWithConflict) {
+  std::string sdp =
+      "v=0\r\n"
+      "o=- 11 22 IN IP4 127.0.0.1\r\n"
+      "s=-\r\n"
+      "t=0 0\r\n"
+      "m=audio 49232 RTP/AVP 108\r\n"
+      // Same name but different payload type.
+      "a=rtpmap:108 ISAC/16000\r\n"
+      "a=rtpmap:108 ISAC/32000\r\n";
+
+  ExpectParseFailure(sdp, "a=rtpmap:108 ISAC/32000");
+}
+
+TEST_F(WebRtcSdpTest, DuplicateVideoRtpmapWithConflict) {
+  std::string sdp =
+      "v=0\r\n"
+      "o=- 11 22 IN IP4 127.0.0.1\r\n"
+      "s=-\r\n"
+      "t=0 0\r\n"
+      "m=video 49232 RTP/AVP 108\r\n"
+      "a=rtpmap:108 VP8/90000\r\n"
+      "a=rtpmap:108 VP9/90000\r\n";
+
+  ExpectParseFailure(sdp, "a=rtpmap:108 VP9/90000");
+}
+
+TEST_F(WebRtcSdpTest, FmtpBeforeRtpMap) {
+  std::string sdp =
+      "v=0\r\n"
+      "o=- 11 22 IN IP4 127.0.0.1\r\n"
+      "s=-\r\n"
+      "t=0 0\r\n"
+      "m=video 49232 RTP/AVP 108\r\n"
+      "a=fmtp:108 profile-level=1\r\n"
+      "a=rtpmap:108 VP9/90000\r\n";
+
+  JsepSessionDescription jdesc_output(kDummyType);
+  EXPECT_TRUE(SdpDeserialize(sdp, &jdesc_output));
+}
+
 // This tests parsing of SDP with unknown ssrc-specific attributes.
 TEST_F(WebRtcSdpTest, ParseIgnoreUnknownSsrcSpecificAttribute) {
   std::string sdp = kSdpString;