Implementing unified plan encoding of msid.

talk/app/webrtc/webrtcsdp.cc

Index: talk/app/webrtc/webrtcsdp.cc
diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc
index 16c0e6939ed4b65d15de44fda42ccaefb2cd5b85..83d56c1a61f45c41e4e9685130a6e9af38976400 100644
--- a/talk/app/webrtc/webrtcsdp.cc
+++ b/talk/app/webrtc/webrtcsdp.cc
@@ -101,7 +101,7 @@ namespace webrtc {
 // the form:
 // <type>=<value>
 // where <type> MUST be exactly one case-significant character.
-static const int kLinePrefixLength = 2;  // Lenght of <type>=
+static const int kLinePrefixLength = 2;  // Length of <type>=
 static const char kLineTypeVersion = 'v';
 static const char kLineTypeOrigin = 'o';
 static const char kLineTypeSessionName = 's';
@@ -121,6 +121,7 @@ static const char kLineTypeAttributes = 'a';
 // Attributes
 static const char kAttributeGroup[] = "group";
 static const char kAttributeMid[] = "mid";
+static const char kAttributeMsid[] = "msid";
 static const char kAttributeRtcpMux[] = "rtcp-mux";
 static const char kAttributeRtcpReducedSize[] = "rtcp-rsize";
 static const char kAttributeSsrc[] = "ssrc";
@@ -252,12 +253,13 @@ static void BuildMediaDescription(const ContentInfo* content_info,
                                   const TransportInfo* transport_info,
                                   const MediaType media_type,
                                   const std::vector<Candidate>& candidates,
+                                  bool unified_plan_sdp,
                                   std::string* message);
 static void BuildSctpContentAttributes(std::string* message, int sctp_port);
-static void BuildRtpContentAttributes(
-    const MediaContentDescription* media_desc,
-    const MediaType media_type,
-    std::string* message);
+static void BuildRtpContentAttributes(const MediaContentDescription* media_desc,
+                                      const MediaType media_type,
+                                      bool unified_plan_sdp,
+                                      std::string* message);
 static void BuildRtpMap(const MediaContentDescription* media_desc,
                         const MediaType media_type,
                         std::string* message);
@@ -334,6 +336,10 @@ static bool ParseFingerprintAttribute(const std::string& line,
 static bool ParseDtlsSetup(const std::string& line,
                            cricket::ConnectionRole* role,
                            SdpParseError* error);
+static bool ParseMsidAttribute(const std::string& line,
+                               std::string* stream_id,
+                               std::string* track_id,
+                               SdpParseError* error);
 
 // Helper functions
 
@@ -792,7 +798,8 @@ static void GetCandidatesByMindex(const SessionDescriptionInterface& desci,
   }
 }
 
-std::string SdpSerialize(const JsepSessionDescription& jdesc) {
+std::string SdpSerialize(const JsepSessionDescription& jdesc,
+                         bool unified_plan_sdp) {
   const cricket::SessionDescription* desc = jdesc.description();
   if (!desc) {
     return "";
@@ -863,10 +870,8 @@ std::string SdpSerialize(const JsepSessionDescription& jdesc) {
       static_cast<const MediaContentDescription*>(it->description);
     std::vector<Candidate> candidates;
     GetCandidatesByMindex(jdesc, ++mline_index, &candidates);
-    BuildMediaDescription(&*it,
-                          desc->GetTransportInfoByName(it->name),
-                          mdesc->type(),
-                          candidates,
+    BuildMediaDescription(&*it, desc->GetTransportInfoByName(it->name),
+                          mdesc->type(), candidates, unified_plan_sdp,
                           &message);
   }
   return message;
@@ -1172,6 +1177,7 @@ void BuildMediaDescription(const ContentInfo* content_info,
                            const TransportInfo* transport_info,
                            const MediaType media_type,
                            const std::vector<Candidate>& candidates,
+                           bool unified_plan_sdp,
                            std::string* message) {
   ASSERT(message != NULL);
   if (content_info == NULL || message == NULL) {
@@ -1347,7 +1353,8 @@ void BuildMediaDescription(const ContentInfo* content_info,
   if (IsDtlsSctp(media_desc->protocol())) {
     BuildSctpContentAttributes(message, sctp_port);
   } else if (IsRtp(media_desc->protocol())) {
-    BuildRtpContentAttributes(media_desc, media_type, message);
+    BuildRtpContentAttributes(media_desc, media_type, unified_plan_sdp,
+                              message);
   }
 }
 
@@ -1364,10 +1371,11 @@ void BuildSctpContentAttributes(std::string* message, int sctp_port) {
   AddLine(os.str(), message);
 }
 
-void BuildRtpContentAttributes(
-    const MediaContentDescription* media_desc,
-    const MediaType media_type,
-    std::string* message) {
+// If unified_plan_sdp is true, will use "a=msid".
+void BuildRtpContentAttributes(const MediaContentDescription* media_desc,
+                               const MediaType media_type,
+                               bool unified_plan_sdp,
+                               std::string* message) {
   std::ostringstream os;
   // RFC 5285
   // a=extmap:<value>["/"<direction>] <URI> <extensionattributes>
@@ -1399,6 +1407,23 @@ void BuildRtpContentAttributes(
   }
   AddLine(os.str(), message);
 
+  // draft-ietf-mmusic-msid-11
+  // a=msid:<stream id> <track id>
+  if (unified_plan_sdp && !media_desc->streams().empty()) {
+    if (media_desc->streams().size() > 1u) {
+      LOG(LS_WARNING) << "Trying to serialize unified plan SDP with more than "
+                      << "one track in a media section. Omitting 'a=msid'.";

[Taylor Brandstetter, 2016/02/12 02:20:10]

You may ask "why not throw an exception/return a warning here?", but this could
happen if you theoretically deserialize a Plan B description, then try to
serialize it as unified plan.

[pthatcher1, 2016/02/12 03:54:35]

Should we at least serialize the first media description, then?

[Taylor Brandstetter, 2016/02/12 20:44:57]

We do.

+    } else {
+      auto track = media_desc->streams().begin();
+      std::string appdata = track->id;

[pthatcher1, 2016/02/12 03:54:34]

1.  Why is this "appdata" instead of track_id?
2.  Why are you copying it?  Why not just use track->id below?
3.  Why not do "const std::string& stream_id = track->sync_label" to make it
more readable?

[Taylor Brandstetter, 2016/02/12 20:44:57]

1. That's what the code below does for the SSRC line. I assumed there was a
reason.
2. Same as above.
3. Same as above.

I cleaned it up in both places.

+      std::ostringstream os;
+      InitAttrLine(kAttributeMsid, &os);
+      os << kSdpDelimiterColon << track->sync_label << kSdpDelimiterSpace
+         << appdata;
+      AddLine(os.str(), message);
+    }
+  }
+
   // RFC 5761
   // a=rtcp-mux
   if (media_desc->rtcp_mux()) {
@@ -1467,8 +1492,8 @@ void BuildRtpContentAttributes(
 
       // draft-alvestrand-mmusic-msid-00
       // a=ssrc:<ssrc-id> msid:identifier [appdata]
-      // The appdata consists of the "id" attribute of a MediaStreamTrack, which
-      // is corresponding to the "name" attribute of StreamParams.
+      // The appdata consists of the "id" attribute of a MediaStreamTrack,
+      // which corresponds to the "id" attribute of StreamParams.
       std::string appdata = track->id;
       std::ostringstream os;
       InitAttrLine(kAttributeSsrc, &os);
@@ -1477,7 +1502,8 @@ void BuildRtpContentAttributes(
          << kSdpDelimiterSpace << appdata;
       AddLine(os.str(), message);
 
-      // TODO(ronghuawu): Remove below code which is for backward compatibility.
+      // TODO(ronghuawu): Remove below code which is for backward
+      // compatibility.
       // draft-alvestrand-rtcweb-mid-01
       // a=ssrc:<ssrc-id> mslabel:<value>
       // The label isn't yet defined.
@@ -2039,6 +2065,27 @@ static bool ParseDtlsSetup(const std::string& line,
   return true;
 }
 
+static bool ParseMsidAttribute(const std::string& line,
+                               std::string* stream_id,
+                               std::string* track_id,
+                               SdpParseError* error) {
+  // msid-value = msid-id [ SP msid-appdata ]
+  // msid-id = 1*64token-char ; see RFC 4566
+  // msid-appdata = 1*64token-char  ; see RFC 4566

[pthatcher1, 2016/02/12 03:54:34]

The parsing code and the serializing code don't seem to have the same comment. 
I think the other one was more clear.

[Taylor Brandstetter, 2016/02/12 20:44:56]

Again, was just copying the way it was done elsewhere. I included the other
comment, but kept this one, since it's how the attribute is actually defined in
the spec.

+  std::string field1;
+  if (!rtc::tokenize_first(line.substr(kLinePrefixLength), kSdpDelimiterSpace,
+                           &field1, track_id)) {
+    const size_t expected_fields = 2;
+    return ParseFailedExpectFieldNum(line, expected_fields, error);
+  }
+
+  // msid:<msid-id>
+  if (!GetValue(field1, kAttributeMsid, stream_id, error)) {
+    return false;
+  }
+  return true;
+}
+
 // RFC 3551
 //  PT   encoding    media type  clock rate   channels
 //                      name                    (Hz)
@@ -2463,6 +2510,8 @@ bool ParseContent(const std::string& message,
   SsrcGroupVec ssrc_groups;
   std::string maxptime_as_string;
   std::string ptime_as_string;
+  std::string stream_id;
+  std::string track_id;
 
   // Loop until the next m line
   while (!IsLineType(message, kLineTypeMedia, *pos)) {
@@ -2622,6 +2671,10 @@ bool ParseContent(const std::string& message,
         }
         if (flag_value.compare(kValueConference) == 0)
           media_desc->set_conference_mode(true);
+      } else if (HasAttribute(line, kAttributeMsid)) {
+        if (!ParseMsidAttribute(line, &stream_id, &track_id, error)) {
+          return false;
+        }
       }
     } else {
       // Only parse lines that we are interested of.
@@ -2630,6 +2683,16 @@ bool ParseContent(const std::string& message,
     }
   }
 
+  // Found an msid attribute.
+  // Setting the msid_identifier/msid_appdata will cause only one StreamParams
+  // to be created, containing all the SSRCs from the m= section.
+  if (!stream_id.empty() && !track_id.empty()) {
+    for (SsrcInfo& ssrc_info : ssrc_infos) {
+      ssrc_info.msid_identifier = stream_id;
+      ssrc_info.msid_appdata = track_id;

[pthatcher1, 2016/02/12 03:54:34]

Can we rename ssrc_info.msid_identifier and .msid_appdata to .stream_id and
.track_id?  It seems like it would make a lot more sense.

[Taylor Brandstetter, 2016/02/12 20:44:56]

Done.

+    }

[pthatcher1, 2016/02/12 03:54:35]

Do we need to merge all of the ssrc_infos into one big ssrc_info or does
CreateTracksFromSsrcInfos take care of that?  If it does, can you put a comment
here explaining so?

[Taylor Brandstetter, 2016/02/12 20:44:56]

Yes, CreateTracksFromSsrcInfos does that. I meant for the comment above to
convey that, but I made it more clear.

+  }
+
   // Create tracks from the |ssrc_infos|.
   CreateTracksFromSsrcInfos(ssrc_infos, &tracks);
 
@@ -2649,9 +2712,8 @@ bool ParseContent(const std::string& message,
   }
 
   // Add the new tracks to the |media_desc|.
-  for (StreamParamsVec::iterator track = tracks.begin();
-       track != tracks.end(); ++track) {
-    media_desc->AddStream(*track);
+  for (StreamParams& track : tracks) {
+    media_desc->AddStream(track);
   }
 
   if (media_type == cricket::MEDIA_TYPE_AUDIO) {