Sets the SCTP port codec in the native SessionDescription.

Previously it's only set when a SDP string is parsed into SessionDescription, causing failuring for native client.

BUG=3141
R=juberti@webrtc.org, wu@webrtc.org

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

git-svn-id: http://webrtc.googlecode.com/svn/trunk@6036 4adac7df-926f-26a2-2b94-8c16560cd09d

talk/app/webrtc/peerconnection_unittest.cc

@@ -1345,6 +1345,20 @@ TEST_F(JsepPeerConnectionP2PTestClient, AddDataChannelAfterRenegotiation) {
                    kMaxWaitMs);
 }
 
+// This test sets up a Jsep call with SCTP DataChannel and verifies the
+// negotiation is completed without error.
+#ifdef HAVE_SCTP
+TEST_F(JsepPeerConnectionP2PTestClient, CreateOfferWithSctpDataChannel) {
+  MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp);
+  FakeConstraints constraints;
+  constraints.SetMandatory(
+      MediaConstraintsInterface::kEnableDtlsSrtp, true);
+  ASSERT_TRUE(CreateTestClients(&constraints, &constraints));
+  initializing_client()->CreateDataChannel();
+  initializing_client()->Negotiate(false, false);
+}
+#endif
+
 // This test sets up a call between two parties with audio, and video.
 // During the call, the initializing side restart ice and the test verifies that
 // new ice candidates are generated and audio and video still can flow.
@@ -1406,6 +1420,4 @@ TEST_F(JsepPeerConnectionP2PTestClient,
   EnableVideoDecoderFactory();
   LocalP2PTest();
 }
-
 #endif // if !defined(THREAD_SANITIZER)
-

talk/app/webrtc/webrtcsdp.cc

@@ -207,7 +207,6 @@ static const char kIsacCodecName[] = "ISAC";  // From webrtcvoiceengine.cc
 static const int kIsacWbDefaultRate = 32000;  // From acm_common_defs.h
 static const int kIsacSwbDefaultRate = 56000;  // From acm_common_defs.h
 
-static const int kDefaultSctpFmt = 5000;
 static const char kDefaultSctpmapProtocol[] = "webrtc-datachannel";
 
 struct SsrcInfo {
@@ -240,7 +239,7 @@ static void BuildMediaDescription(const ContentInfo* content_info,
                                   const TransportInfo* transport_info,
                                   const MediaType media_type,
                                   std::string* message);
-static void BuildSctpContentAttributes(std::string* message);
+static void BuildSctpContentAttributes(std::string* message, int sctp_port);
 static void BuildRtpContentAttributes(
     const MediaContentDescription* media_desc,
     const MediaType media_type,
@@ -1166,6 +1165,7 @@ void BuildMediaDescription(const ContentInfo* content_info,
   ASSERT(media_desc != NULL);
 
   bool is_sctp = (media_desc->protocol() == cricket::kMediaProtocolDtlsSctp);
+  int sctp_port = cricket::kSctpDefaultPort;
 
   // RFC 4566
   // m=<media> <port> <proto> <fmt>
@@ -1200,14 +1200,22 @@ void BuildMediaDescription(const ContentInfo* content_info,
       fmt.append(talk_base::ToString<int>(it->id));
     }
   } else if (media_type == cricket::MEDIA_TYPE_DATA) {
+    const DataContentDescription* data_desc =
+          static_cast<const DataContentDescription*>(media_desc);
     if (is_sctp) {
       fmt.append(" ");
-      // TODO(jiayl): Replace the hard-coded string with the fmt read out of the
-      // ContentDescription.
-      fmt.append(talk_base::ToString<int>(kDefaultSctpFmt));
+
+      for (std::vector<cricket::DataCodec>::const_iterator it =
+           data_desc->codecs().begin();
+           it != data_desc->codecs().end(); ++it) {
+        if (it->id == cricket::kGoogleSctpDataCodecId &&
+            it->GetParam(cricket::kCodecParamPort, &sctp_port)) {
+          break;
+        }
+      }
+
+      fmt.append(talk_base::ToString<int>(sctp_port));
     } else {
-      const DataContentDescription* data_desc =
-          static_cast<const DataContentDescription*>(media_desc);
       for (std::vector<cricket::DataCodec>::const_iterator it =
            data_desc->codecs().begin();
            it != data_desc->codecs().end(); ++it) {
@@ -1289,18 +1297,18 @@ void BuildMediaDescription(const ContentInfo* content_info,
   AddLine(os.str(), message);
 
   if (is_sctp) {
-    BuildSctpContentAttributes(message);
+    BuildSctpContentAttributes(message, sctp_port);
   } else {
     BuildRtpContentAttributes(media_desc, media_type, message);
   }
 }
 
-void BuildSctpContentAttributes(std::string* message) {
+void BuildSctpContentAttributes(std::string* message, int sctp_port) {
   // draft-ietf-mmusic-sctp-sdp-04
   // a=sctpmap:sctpmap-number  protocol  [streams]
   std::ostringstream os;
   InitAttrLine(kAttributeSctpmap, &os);
-  os << kSdpDelimiterColon << kDefaultSctpFmt << kSdpDelimiterSpace
+  os << kSdpDelimiterColon << sctp_port << kSdpDelimiterSpace
      << kDefaultSctpmapProtocol << kSdpDelimiterSpace
      << (cricket::kMaxSctpSid + 1);
   AddLine(os.str(), message);
@@ -2166,6 +2174,7 @@ bool ParseMediaDescription(const std::string& message,
         codec_port.SetParam(cricket::kCodecParamPort, fields[3]);
         LOG(INFO) << "ParseMediaDescription: Got SCTP Port Number "
                   << fields[3];
+        ASSERT(!desc->HasCodec(cricket::kGoogleSctpDataCodecId));
         desc->AddCodec(codec_port);
       }
 

talk/app/webrtc/webrtcsdp_unittest.cc

@@ -299,7 +299,7 @@ static const char kSdpSctpDataChannelWithCandidatesString[] =
     "a=mid:data_content_name\r\n"
     "a=sctpmap:5000 webrtc-datachannel 1024\r\n";
 
-    static const char kSdpConferenceString[] =
+static const char kSdpConferenceString[] =
     "v=0\r\n"
     "o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n"
     "s=-\r\n"
@@ -1466,6 +1466,37 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithSctpDataChannel) {
   EXPECT_EQ(message, expected_sdp);
 }
 
+TEST_F(WebRtcSdpTest, SerializeWithSctpDataChannelAndNewPort) {
+  AddSctpDataChannel();
+  JsepSessionDescription jsep_desc(kDummyString);
+
+  ASSERT_TRUE(jsep_desc.Initialize(desc_.Copy(), kSessionId, kSessionVersion));
+  DataContentDescription* dcdesc = static_cast<DataContentDescription*>(
+      jsep_desc.description()->GetContentDescriptionByName(kDataContentName));
+
+  const int kNewPort = 1234;
+  cricket::DataCodec codec(
+        cricket::kGoogleSctpDataCodecId, cricket::kGoogleSctpDataCodecName, 0);
+  codec.SetParam(cricket::kCodecParamPort, kNewPort);
+  dcdesc->AddOrReplaceCodec(codec);
+
+  std::string message = webrtc::SdpSerialize(jsep_desc);
+
+  std::string expected_sdp = kSdpString;
+  expected_sdp.append(kSdpSctpDataChannelString);
+
+  char default_portstr[16];
+  char new_portstr[16];
+  talk_base::sprintfn(default_portstr, sizeof(default_portstr), "%d",
+                      kDefaultSctpPort);
+  talk_base::sprintfn(new_portstr, sizeof(new_portstr), "%d", kNewPort);
+  talk_base::replace_substrs(default_portstr, strlen(default_portstr),
+                             new_portstr, strlen(new_portstr),
+                             &expected_sdp);
+
+  EXPECT_EQ(expected_sdp, message);
+}
+
 TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithDataChannelAndBandwidth) {
   AddRtpDataChannel();
   data_desc_->set_bandwidth(100*1000);
@@ -1898,6 +1929,7 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannelAndNewPort) {
   talk_base::sprintfn(unusual_portstr, sizeof(unusual_portstr), "%d",
                       kUnusualSctpPort);
 
+  // First setup the expected JsepSessionDescription.
   JsepSessionDescription jdesc(kDummyString);
   // take our pre-built session description and change the SCTP port.
   cricket::SessionDescription* mutant = desc_.Copy();
@@ -1907,11 +1939,13 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannelAndNewPort) {
   EXPECT_EQ(codecs.size(), 1UL);
   EXPECT_EQ(codecs[0].id, cricket::kGoogleSctpDataCodecId);
   codecs[0].SetParam(cricket::kCodecParamPort, kUnusualSctpPort);
+  dcdesc->set_codecs(codecs);
 
   // note: mutant's owned by jdesc now.
   ASSERT_TRUE(jdesc.Initialize(mutant, kSessionId, kSessionVersion));
   mutant = NULL;
 
+  // Then get the deserialized JsepSessionDescription.
   std::string sdp_with_data = kSdpString;
   sdp_with_data.append(kSdpSctpDataChannelString);
   talk_base::replace_substrs(default_portstr, strlen(default_portstr),

talk/media/base/codec.h

@@ -120,6 +120,8 @@ struct Codec {
     name = c.name;
     clockrate = c.clockrate;
     preference = c.preference;
+    params = c.params;
+    feedback_params = c.feedback_params;
     return *this;
   }
 
@@ -127,7 +129,9 @@ struct Codec {
     return this->id == c.id &&  // id is reserved in objective-c
         name == c.name &&
         clockrate == c.clockrate &&
-        preference == c.preference;
+        preference == c.preference &&
+        params == c.params &&
+        feedback_params == c.feedback_params;
   }
 
   bool operator!=(const Codec& c) const {

talk/media/base/codec_unittest.cc

@@ -40,6 +40,43 @@ class CodecTest : public testing::Test {
   CodecTest() {}
 };
 
+TEST_F(CodecTest, TestCodecOperators) {
+  Codec c0(96, "D", 1000, 0);
+  c0.SetParam("a", 1);
+
+  Codec c1 = c0;
+  EXPECT_TRUE(c1 == c0);
+
+  int param_value0;
+  int param_value1;
+  EXPECT_TRUE(c0.GetParam("a", &param_value0));
+  EXPECT_TRUE(c1.GetParam("a", &param_value1));
+  EXPECT_EQ(param_value0, param_value1);
+
+  c1.id = 86;
+  EXPECT_TRUE(c0 != c1);
+
+  c1 = c0;
+  c1.name = "x";
+  EXPECT_TRUE(c0 != c1);
+
+  c1 = c0;
+  c1.clockrate = 2000;
+  EXPECT_TRUE(c0 != c1);
+
+  c1 = c0;
+  c1.preference = 1;
+  EXPECT_TRUE(c0 != c1);
+
+  c1 = c0;
+  c1.SetParam("a", 2);
+  EXPECT_TRUE(c0 != c1);
+
+  Codec c5;
+  Codec c6(0, "", 0, 0);
+  EXPECT_TRUE(c5 == c6);
+}
+
 TEST_F(CodecTest, TestAudioCodecOperators) {
   AudioCodec c0(96, "A", 44100, 20000, 2, 3);
   AudioCodec c1(95, "A", 44100, 20000, 2, 3);
@@ -238,23 +275,6 @@ TEST_F(CodecTest, TestDataCodecMatches) {
   EXPECT_FALSE(c1.Matches(DataCodec(95, "D", 0)));
 }
 
-TEST_F(CodecTest, TestDataCodecOperators) {
-  DataCodec c0(96, "D", 3);
-  DataCodec c1(95, "D", 3);
-  DataCodec c2(96, "x", 3);
-  DataCodec c3(96, "D", 1);
-  EXPECT_TRUE(c0 != c1);
-  EXPECT_TRUE(c0 != c2);
-  EXPECT_TRUE(c0 != c3);
-
-  DataCodec c4;
-  DataCodec c5(0, "", 0);
-  DataCodec c6 = c0;
-  EXPECT_TRUE(c5 == c4);
-  EXPECT_TRUE(c6 != c4);
-  EXPECT_TRUE(c6 == c0);
-}
-
 TEST_F(CodecTest, TestSetParamAndGetParam) {
   AudioCodec codec;
   codec.SetParam("a", "1");

talk/media/sctp/sctpdataengine.cc

@@ -105,12 +105,6 @@ namespace cricket {
 typedef talk_base::ScopedMessageData<SctpInboundPacket> InboundPacketMessage;
 typedef talk_base::ScopedMessageData<talk_base::Buffer> OutboundPacketMessage;
 
-// This is the SCTP port to use. It is passed along the wire and the listener
-// and connector must be using the same port. It is not related to the ports at
-// the IP level. (Corresponds to: sockaddr_conn.sconn_port in usrsctp.h)
-//
-// TODO(ldixon): Allow port to be set from higher level code.
-static const int kSctpDefaultPort = 5001;
 // TODO(ldixon): Find where this is defined, and also check is Sctp really
 // respects this.
 static const size_t kSctpMtu = 1280;
@@ -277,10 +271,9 @@ SctpDataEngine::SctpDataEngine() {
   }
   usrsctp_engines_count++;
 
-  // We don't put in a codec because we don't want one offered when we
-  // use the hybrid data engine.
-  // codecs_.push_back(cricket::DataCodec( kGoogleSctpDataCodecId,
-  // kGoogleSctpDataCodecName, 0));
+  cricket::DataCodec codec(kGoogleSctpDataCodecId, kGoogleSctpDataCodecName, 0);
+  codec.SetParam(kCodecParamPort, kSctpDefaultPort);
+  codecs_.push_back(codec);
 }
 
 SctpDataEngine::~SctpDataEngine() {
@@ -308,8 +301,8 @@ DataMediaChannel* SctpDataEngine::CreateChannel(
 
 SctpDataMediaChannel::SctpDataMediaChannel(talk_base::Thread* thread)
     : worker_thread_(thread),
-      local_port_(-1),
-      remote_port_(-1),
+      local_port_(kSctpDefaultPort),
+      remote_port_(kSctpDefaultPort),
       sock_(NULL),
       sending_(false),
       receiving_(false),
@@ -423,12 +416,6 @@ void SctpDataMediaChannel::CloseSctpSocket() {
 
 bool SctpDataMediaChannel::Connect() {
   LOG(LS_VERBOSE) << debug_name_ << "->Connect().";
-  if (remote_port_ < 0) {
-    remote_port_ = kSctpDefaultPort;
-  }
-  if (local_port_ < 0) {
-    local_port_ = kSctpDefaultPort;
-  }
 
   // If we already have a socket connection, just return.
   if (sock_) {

talk/media/sctp/sctpdataengine.h

@@ -58,6 +58,12 @@ namespace cricket {
 // tell SCTP we're going to use.
 const uint32 kMaxSctpSid = 1023;
 
+// This is the default SCTP port to use. It is passed along the wire and the
+// connectee and connector must be using the same port. It is not related to the
+// ports at the IP level. (Corresponds to: sockaddr_conn.sconn_port in
+// usrsctp.h)
+const int kSctpDefaultPort = 5000;
+
 // A DataEngine that interacts with usrsctp.
 //
 // From channel calls, data flows like this:

talk/session/media/mediasession.cc

@@ -308,6 +308,20 @@ static void GetCurrentStreamParams(const SessionDescription* sdesc,
   }
 }
 
+// Filters the data codecs for the data channel type.
+void FilterDataCodecs(std::vector<DataCodec>* codecs, bool sctp) {
+  // Filter RTP codec for SCTP and vice versa.
+  int codec_id = sctp ? kGoogleRtpDataCodecId : kGoogleSctpDataCodecId;
+  for (std::vector<DataCodec>::iterator iter = codecs->begin();
+       iter != codecs->end();) {
+    if (iter->id == codec_id) {
+      iter = codecs->erase(iter);
+    } else {
+      ++iter;
+    }
+  }
+}
+
 template <typename IdStruct>
 class UsedIds {
  public:
@@ -1204,6 +1218,8 @@ SessionDescription* MediaSessionDescriptionFactory::CreateOffer(
     scoped_ptr<DataContentDescription> data(new DataContentDescription());
     bool is_sctp = (options.data_channel_type == DCT_SCTP);
 
+    FilterDataCodecs(&data_codecs, is_sctp);
+
     cricket::SecurePolicy sdes_policy =
         IsDtlsActive(CN_DATA, current_description) ?
             cricket::SEC_DISABLED : secure();
@@ -1397,6 +1413,10 @@ SessionDescription* MediaSessionDescriptionFactory::CreateAnswer(
     if (!data_transport) {
       return NULL;
     }
+    bool is_sctp = (options.data_channel_type == DCT_SCTP);
+    std::vector<DataCodec> data_codecs(data_codecs_);
+    FilterDataCodecs(&data_codecs, is_sctp);
+
     scoped_ptr<DataContentDescription> data_answer(
         new DataContentDescription());
     // Do not require or create SDES cryptos if DTLS is used.

talk/session/media/mediasession.h

@@ -318,6 +318,16 @@ class MediaContentDescriptionImpl : public MediaContentDescription {
   void AddCodec(const C& codec) {
     codecs_.push_back(codec);
   }
+  void AddOrReplaceCodec(const C& codec) {
+    for (typename std::vector<C>::iterator iter = codecs_.begin();
+         iter != codecs_.end(); ++iter) {
+      if (iter->id == codec.id) {
+        *iter = codec;
+        return;
+      }
+    }
+    AddCodec(codec);
+  }
   void AddCodecs(const std::vector<C>& codecs) {
     typename std::vector<C>::const_iterator codec;
     for (codec = codecs.begin(); codec != codecs.end(); ++codec) {