Split fmtp on semicolons not spaces as per RFC6871

BUG=4617
R=pthatcher@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/47169004

Cr-Commit-Position: refs/heads/master@{#9193}

talk/app/webrtc/webrtcsdp.cc

@@ -539,27 +539,11 @@ static bool AddSsrcLine(uint32 ssrc_id, const std::string& attribute,
   return AddLine(os.str(), message);
 }
 
-// Split the message into two parts by the first delimiter.
-static bool SplitByDelimiter(const std::string& message,
-                             const char delimiter,
-                             std::string* field1,
-                             std::string* field2) {
-  // Find the first delimiter
-  size_t pos = message.find(delimiter);
-  if (pos == std::string::npos) {
-    return false;
-  }
-  *field1 = message.substr(0, pos);
-  // The rest is the value.
-  *field2 = message.substr(pos + 1);
-  return true;
-}
-
 // Get value only from <attribute>:<value>.
 static bool GetValue(const std::string& message, const std::string& attribute,
                      std::string* value, SdpParseError* error) {
   std::string leftpart;
-  if (!SplitByDelimiter(message, kSdpDelimiterColon, &leftpart, value)) {
+  if (!rtc::tokenize_first(message, kSdpDelimiterColon, &leftpart, value)) {
     return ParseFailedGetValue(message, attribute, error);
   }
   // The left part should end with the expected attribute.
@@ -972,7 +956,8 @@ bool ParseCandidate(const std::string& message, Candidate* candidate,
   // Makes sure |message| contains only one line.
   if (message.size() > first_line.size()) {
     std::string left, right;
-    if (SplitByDelimiter(message, kNewLine, &left, &right) && !right.empty()) {
+    if (rtc::tokenize_first(message, kNewLine, &left, &right) &&
+        !right.empty()) {
       return ParseFailed(message, 0, "Expect one line only", error);
     }
   }
@@ -989,7 +974,7 @@ bool ParseCandidate(const std::string& message, Candidate* candidate,
   std::string candidate_value;
 
   // |first_line| must be in the form of "candidate:<value>".
-  if (!SplitByDelimiter(first_line, kSdpDelimiterColon,
+  if (!rtc::tokenize_first(first_line, kSdpDelimiterColon,
                         &attribute_candidate, &candidate_value) ||
       attribute_candidate != kAttributeCandidate) {
     if (is_raw) {
@@ -2749,7 +2734,7 @@ bool ParseSsrcAttribute(const std::string& line, SsrcInfoVec* ssrc_infos,
   // a=ssrc:<ssrc-id> <attribute>
   // a=ssrc:<ssrc-id> <attribute>:<value>
   std::string field1, field2;
-  if (!SplitByDelimiter(line.substr(kLinePrefixLength),
+  if (!rtc::tokenize_first(line.substr(kLinePrefixLength),
                         kSdpDelimiterSpace,
                         &field1,
                         &field2)) {
@@ -2769,7 +2754,7 @@ bool ParseSsrcAttribute(const std::string& line, SsrcInfoVec* ssrc_infos,
 
   std::string attribute;
   std::string value;
-  if (!SplitByDelimiter(field2, kSdpDelimiterColon,
+  if (!rtc::tokenize_first(field2, kSdpDelimiterColon,
                         &attribute, &value)) {
     std::ostringstream description;
     description << "Failed to get the ssrc attribute value from " << field2
@@ -3016,22 +3001,13 @@ bool ParseRtpmapAttribute(const std::string& line,
   return true;
 }
 
-void PruneRight(const char delimiter, std::string* message) {
-  size_t trailing = message->find(delimiter);
-  if (trailing != std::string::npos) {
-    *message = message->substr(0, trailing);
-  }
-}
-
 bool ParseFmtpParam(const std::string& line, std::string* parameter,
                     std::string* value, SdpParseError* error) {
-  if (!SplitByDelimiter(line, kSdpDelimiterEqual, parameter, value)) {
+  if (!rtc::tokenize_first(line, kSdpDelimiterEqual, parameter, value)) {
     ParseFailed(line, "Unable to parse fmtp parameter. \'=\' missing.", error);
     return false;
   }
   // a=fmtp:<payload_type> <param1>=<value1>; <param2>=<value2>; ...
-  // When parsing the values the trailing ";" gets picked up. Remove them.
-  PruneRight(kSdpDelimiterSemicolon, value);
   return true;
 }
 
@@ -3042,44 +3018,52 @@ bool ParseFmtpAttributes(const std::string& line, const MediaType media_type,
       media_type != cricket::MEDIA_TYPE_VIDEO) {
     return true;
   }
-  std::vector<std::string> fields;
+
-  rtc::split(line.substr(kLinePrefixLength),
+  std::string line_payload;
-                   kSdpDelimiterSpace, &fields);
+  std::string line_params;
 
   // RFC 5576
   // a=fmtp:<format> <format specific parameters>
   // At least two fields, whereas the second one is any of the optional
   // parameters.
-  if (fields.size() < 2) {
+  if(!rtc::tokenize_first(line.substr(kLinePrefixLength), kSdpDelimiterSpace,
+                       &line_payload, &line_params)) {
     ParseFailedExpectMinFieldNum(line, 2, error);
     return false;
   }
 
+  // Parse out the payload information.
   std::string payload_type_str;
-  if (!GetValue(fields[0], kAttributeFmtp, &payload_type_str, error)) {
+  if (!GetValue(line_payload, kAttributeFmtp, &payload_type_str, error)) {
     return false;
   }
 
+  int payload_type = 0;
+  if (!GetPayloadTypeFromString(line_payload, payload_type_str,
+                                &payload_type, error)) {
+    return false;
+  }
+
+  // Parse out format specific parameters.
+  std::vector<std::string> fields;
+  rtc::split(line_params, kSdpDelimiterSemicolon, &fields);
+
   cricket::CodecParameterMap codec_params;
-  for (std::vector<std::string>::const_iterator iter = fields.begin() + 1;
+  for (auto &iter : fields) {
-       iter != fields.end(); ++iter) {
+    if (iter.find(kSdpDelimiterEqual) == std::string::npos) {
-    std::string name;
-    std::string value;
-    if (iter->find(kSdpDelimiterEqual) == std::string::npos) {
       // Only fmtps with equals are currently supported. Other fmtp types
       // should be ignored. Unknown fmtps do not constitute an error.
       continue;
     }
-    if (!ParseFmtpParam(*iter, &name, &value, error)) {
+
+    std::string name;
+    std::string value;
+    if (!ParseFmtpParam(rtc::string_trim(iter), &name, &value, error)) {
       return false;
     }
     codec_params[name] = value;
   }
 
-  int payload_type = 0;
-  if (!GetPayloadTypeFromString(line, payload_type_str, &payload_type, error)) {
-    return false;
-  }
   if (media_type == cricket::MEDIA_TYPE_AUDIO) {
     UpdateCodec<AudioContentDescription, cricket::AudioCodec>(
         media_desc, payload_type, codec_params);

talk/app/webrtc/webrtcsdp_unittest.cc

@@ -1211,7 +1211,7 @@ class WebRtcSdpTest : public testing::Test {
        << "; stereo=" << params.stereo
        << "; sprop-stereo=" << params.sprop_stereo
        << "; useinbandfec=" << params.useinband
-       << " maxaveragebitrate=" << params.maxaveragebitrate << "\r\n"
+       << "; maxaveragebitrate=" << params.maxaveragebitrate << "\r\n"
        << "a=ptime:" << params.ptime << "\r\n"
        << "a=maxptime:" << params.max_ptime << "\r\n";
     sdp += os.str();
@@ -1222,7 +1222,7 @@ class WebRtcSdpTest : public testing::Test {
     os << "m=video 9 RTP/SAVPF 99 95\r\n"
        << "a=rtpmap:99 VP8/90000\r\n"
        << "a=rtpmap:95 RTX/90000\r\n"
-       << "a=fmtp:95 apt=99;rtx-time=1000\r\n";
+       << "a=fmtp:95 apt=99;\r\n";
     sdp += os.str();
 
     // Deserialize
@@ -2528,6 +2528,40 @@ TEST_F(WebRtcSdpTest, DeserializeVideoFmtp) {
   EXPECT_EQ(found->second, "40");
 }
 
+TEST_F(WebRtcSdpTest, DeserializeVideoFmtpWithSpace) {
+  JsepSessionDescription jdesc_output(kDummyString);
+
+  const char kSdpWithFmtpString[] =
+      "v=0\r\n"
+      "o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
+      "s=-\r\n"
+      "t=0 0\r\n"
+      "m=video 3457 RTP/SAVPF 120\r\n"
+      "a=rtpmap:120 VP8/90000\r\n"
+      "a=fmtp:120   x-google-min-bitrate=10;  x-google-max-quantization=40\r\n";
+
+  // Deserialize
+  SdpParseError error;
+  EXPECT_TRUE(webrtc::SdpDeserialize(kSdpWithFmtpString, &jdesc_output,
+                                     &error));
+
+  const ContentInfo* vc = GetFirstVideoContent(jdesc_output.description());
+  ASSERT_TRUE(vc != NULL);
+  const VideoContentDescription* vcd =
+      static_cast<const VideoContentDescription*>(vc->description);
+  ASSERT_FALSE(vcd->codecs().empty());
+  cricket::VideoCodec vp8 = vcd->codecs()[0];
+  EXPECT_EQ("VP8", vp8.name);
+  EXPECT_EQ(120, vp8.id);
+  cricket::CodecParameterMap::iterator found =
+      vp8.params.find("x-google-min-bitrate");
+  ASSERT_TRUE(found != vp8.params.end());
+  EXPECT_EQ(found->second, "10");
+  found = vp8.params.find("x-google-max-quantization");
+  ASSERT_TRUE(found != vp8.params.end());
+  EXPECT_EQ(found->second, "40");
+}
+
 TEST_F(WebRtcSdpTest, SerializeVideoFmtp) {
   VideoContentDescription* vcd = static_cast<VideoContentDescription*>(
       GetFirstVideoContent(&desc_)->description);

webrtc/base/stringencode.cc

@@ -607,6 +607,25 @@ size_t tokenize(const std::string& source, char delimiter, char start_mark,
   return tokenize_append(remain_source, delimiter, fields);
 }
 
+bool tokenize_first(const std::string& source, const char delimiter,
+                           std::string* token, std::string* rest) {
+  // Find the first delimiter
+  size_t left_pos = source.find(delimiter);
+  if (left_pos == std::string::npos) {
+    return false;
+  }
+
+  // Look for additional occurrances of delimiter.
+  size_t right_pos = left_pos + 1;
+  while(source[right_pos] == delimiter) {
+    right_pos++;
+  }
+
+  *token = source.substr(0, left_pos);
+  *rest = source.substr(right_pos);
+  return true;
+}
+
 size_t split(const std::string& source, char delimiter,
              std::vector<std::string>* fields) {
   DCHECK(fields);

webrtc/base/stringencode.h

@@ -159,6 +159,12 @@ size_t tokenize_append(const std::string& source, char delimiter,
 size_t tokenize(const std::string& source, char delimiter, char start_mark,
                 char end_mark, std::vector<std::string>* fields);
 
+// Extract the first token from source as separated by delimiter, with
+// duplicates of delimiter ignored. Return false if the delimiter could not be
+// found, otherwise return true.
+bool tokenize_first(const std::string& source, const char delimiter,
+                      std::string* token, std::string* rest);
+
 // Safe sprintf to std::string
 //void sprintf(std::string& value, size_t maxlen, const char * format, ...)
 //     PRINTF_FORMAT(3);

webrtc/base/stringencode_unittest.cc

@@ -298,6 +298,52 @@ TEST(TokenizeTest, TokenizeWithMarks) {
   ASSERT_STREQ("E F", fields.at(3).c_str());
 }
 
+TEST(TokenizeFirstTest, NoLeadingSpaces) {
+  std::string token;
+  std::string rest;
+
+  ASSERT_TRUE(tokenize_first("A &*${}", ' ', &token, &rest));
+  ASSERT_STREQ("A", token.c_str());
+  ASSERT_STREQ("&*${}", rest.c_str());
+
+  ASSERT_TRUE(tokenize_first("A B& *${}", ' ', &token, &rest));
+  ASSERT_STREQ("A", token.c_str());
+  ASSERT_STREQ("B& *${}", rest.c_str());
+
+  ASSERT_TRUE(tokenize_first("A    B& *${}    ", ' ', &token, &rest));
+  ASSERT_STREQ("A", token.c_str());
+  ASSERT_STREQ("B& *${}    ", rest.c_str());
+}
+
+TEST(TokenizeFirstTest, LeadingSpaces) {
+  std::string token;
+  std::string rest;
+
+  ASSERT_TRUE(tokenize_first("     A B C", ' ', &token, &rest));
+  ASSERT_STREQ("", token.c_str());
+  ASSERT_STREQ("A B C", rest.c_str());
+
+  ASSERT_TRUE(tokenize_first("     A    B   C    ", ' ', &token, &rest));
+  ASSERT_STREQ("", token.c_str());
+  ASSERT_STREQ("A    B   C    ", rest.c_str());
+}
+
+TEST(TokenizeFirstTest, SingleToken) {
+  std::string token;
+  std::string rest;
+
+  // In the case where we cannot find delimiter the whole string is a token.
+  ASSERT_FALSE(tokenize_first("ABC", ' ', &token, &rest));
+
+  ASSERT_TRUE(tokenize_first("ABC    ", ' ', &token, &rest));
+  ASSERT_STREQ("ABC", token.c_str());
+  ASSERT_STREQ("", rest.c_str());
+
+  ASSERT_TRUE(tokenize_first("    ABC    ", ' ', &token, &rest));
+  ASSERT_STREQ("", token.c_str());
+  ASSERT_STREQ("ABC    ", rest.c_str());
+}
+
 // Tests counting substrings.
 TEST(SplitTest, CountSubstrings) {
   std::vector<std::string> fields;