From 0e07f92043b333acfdaed8f22da5df903a70e0e9 Mon Sep 17 00:00:00 2001 From: Donald Curtis Date: Fri, 15 May 2015 09:21:23 -0700 Subject: [PATCH] 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 | 76 +++++++++++---------------- talk/app/webrtc/webrtcsdp_unittest.cc | 40 ++++++++++++-- webrtc/base/stringencode.cc | 19 +++++++ webrtc/base/stringencode.h | 6 +++ webrtc/base/stringencode_unittest.cc | 46 ++++++++++++++++ 5 files changed, 138 insertions(+), 49 deletions(-) diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc index a93977d6d..3ad6a92f4 100644 --- a/talk/app/webrtc/webrtcsdp.cc +++ b/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 :. 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:". - 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: // a=ssrc: : 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: =; =; ... - // 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 fields; - rtc::split(line.substr(kLinePrefixLength), - kSdpDelimiterSpace, &fields); + + std::string line_payload; + std::string line_params; // RFC 5576 // a=fmtp: // 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 fields; + rtc::split(line_params, kSdpDelimiterSemicolon, &fields); + cricket::CodecParameterMap codec_params; - for (std::vector::const_iterator iter = fields.begin() + 1; - iter != fields.end(); ++iter) { - std::string name; - std::string value; - if (iter->find(kSdpDelimiterEqual) == std::string::npos) { + for (auto &iter : fields) { + 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( media_desc, payload_type, codec_params); diff --git a/talk/app/webrtc/webrtcsdp_unittest.cc b/talk/app/webrtc/webrtcsdp_unittest.cc index 862e93c53..4fe18e896 100644 --- a/talk/app/webrtc/webrtcsdp_unittest.cc +++ b/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 @@ -2504,7 +2504,41 @@ TEST_F(WebRtcSdpTest, DeserializeVideoFmtp) { "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"; + "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(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, 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; diff --git a/webrtc/base/stringencode.cc b/webrtc/base/stringencode.cc index 52b75da70..5662040c6 100644 --- a/webrtc/base/stringencode.cc +++ b/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* fields) { DCHECK(fields); diff --git a/webrtc/base/stringencode.h b/webrtc/base/stringencode.h index 2e69a9c02..79af28509 100644 --- a/webrtc/base/stringencode.h +++ b/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* 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); diff --git a/webrtc/base/stringencode_unittest.cc b/webrtc/base/stringencode_unittest.cc index c9e726ecb..77fae35fb 100644 --- a/webrtc/base/stringencode_unittest.cc +++ b/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 fields;