Implement the "needs-ice-restart" logic for SetConfiguration.

webrtc/api/peerconnectioninterface_unittest.cc

 /*
  *  Copyright 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 54 matching lines @@
 static const char kRecvonly[] = "recvonly";
 static const char kSendrecv[] = "sendrecv";
 
 // Reference SDP with a MediaStream with label "stream1" and audio track with
 // id "audio_1" and a video track with id "video_1;
 static const char kSdpStringWithStream1[] =
     "v=0\r\n"
     "o=- 0 0 IN IP4 127.0.0.1\r\n"
     "s=-\r\n"
     "t=0 0\r\n"
+    "m=audio 1 RTP/AVPF 103\r\n"
     "a=ice-ufrag:e5785931\r\n"
     "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
     "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
     "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
-    "m=audio 1 RTP/AVPF 103\r\n"
     "a=mid:audio\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:103 ISAC/16000\r\n"
     "a=ssrc:1 cname:stream1\r\n"
     "a=ssrc:1 mslabel:stream1\r\n"
     "a=ssrc:1 label:audiotrack0\r\n"
     "m=video 1 RTP/AVPF 120\r\n"
+    "a=ice-ufrag:e5785931\r\n"
+    "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
+    "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
+    "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
     "a=mid:video\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:120 VP8/90000\r\n"
     "a=ssrc:2 cname:stream1\r\n"
     "a=ssrc:2 mslabel:stream1\r\n"
     "a=ssrc:2 label:videotrack0\r\n";
 
 // Reference SDP with a MediaStream with label "stream1" and audio track with
 // id "audio_1";
 static const char kSdpStringWithStream1AudioTrackOnly[] =
     "v=0\r\n"
     "o=- 0 0 IN IP4 127.0.0.1\r\n"
     "s=-\r\n"
     "t=0 0\r\n"
+    "m=audio 1 RTP/AVPF 103\r\n"
     "a=ice-ufrag:e5785931\r\n"
     "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
     "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
     "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
-    "m=audio 1 RTP/AVPF 103\r\n"
     "a=mid:audio\r\n"
     "a=sendrecv\r\n"
     "a=rtpmap:103 ISAC/16000\r\n"
     "a=ssrc:1 cname:stream1\r\n"
     "a=ssrc:1 mslabel:stream1\r\n"
     "a=ssrc:1 label:audiotrack0\r\n"
     "a=rtcp-mux\r\n";
 
 // Reference SDP with two MediaStreams with label "stream1" and "stream2. Each
 // MediaStreams have one audio track and one video track.
 // This uses MSID.
 static const char kSdpStringWithStream1And2[] =
     "v=0\r\n"
     "o=- 0 0 IN IP4 127.0.0.1\r\n"
     "s=-\r\n"
     "t=0 0\r\n"
+    "a=msid-semantic: WMS stream1 stream2\r\n"
+    "m=audio 1 RTP/AVPF 103\r\n"
     "a=ice-ufrag:e5785931\r\n"
     "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
     "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
     "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
-    "a=msid-semantic: WMS stream1 stream2\r\n"
-    "m=audio 1 RTP/AVPF 103\r\n"
     "a=mid:audio\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:103 ISAC/16000\r\n"
     "a=ssrc:1 cname:stream1\r\n"
     "a=ssrc:1 msid:stream1 audiotrack0\r\n"
     "a=ssrc:3 cname:stream2\r\n"
     "a=ssrc:3 msid:stream2 audiotrack1\r\n"
     "m=video 1 RTP/AVPF 120\r\n"
+    "a=ice-ufrag:e5785931\r\n"
+    "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
+    "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
+    "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
     "a=mid:video\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:120 VP8/0\r\n"
     "a=ssrc:2 cname:stream1\r\n"
     "a=ssrc:2 msid:stream1 videotrack0\r\n"
     "a=ssrc:4 cname:stream2\r\n"
     "a=ssrc:4 msid:stream2 videotrack1\r\n";
 
 // Reference SDP without MediaStreams. Msid is not supported.
 static const char kSdpStringWithoutStreams[] =
     "v=0\r\n"
     "o=- 0 0 IN IP4 127.0.0.1\r\n"
     "s=-\r\n"
     "t=0 0\r\n"
+    "m=audio 1 RTP/AVPF 103\r\n"
     "a=ice-ufrag:e5785931\r\n"
     "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
     "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
     "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
-    "m=audio 1 RTP/AVPF 103\r\n"
     "a=mid:audio\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:103 ISAC/16000\r\n"
     "m=video 1 RTP/AVPF 120\r\n"
+    "a=ice-ufrag:e5785931\r\n"
+    "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
+    "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
+    "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
     "a=mid:video\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:120 VP8/90000\r\n";
 
 // Reference SDP without MediaStreams. Msid is supported.
 static const char kSdpStringWithMsidWithoutStreams[] =
     "v=0\r\n"
     "o=- 0 0 IN IP4 127.0.0.1\r\n"
     "s=-\r\n"
     "t=0 0\r\n"
+    "a=msid-semantic: WMS\r\n"
+    "m=audio 1 RTP/AVPF 103\r\n"
     "a=ice-ufrag:e5785931\r\n"
     "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
     "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
     "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
-    "a=msid-semantic: WMS\r\n"
-    "m=audio 1 RTP/AVPF 103\r\n"
     "a=mid:audio\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:103 ISAC/16000\r\n"
     "m=video 1 RTP/AVPF 120\r\n"
+    "a=ice-ufrag:e5785931\r\n"
+    "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
+    "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
+    "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
     "a=mid:video\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:120 VP8/90000\r\n";
 
 // Reference SDP without MediaStreams and audio only.
 static const char kSdpStringWithoutStreamsAudioOnly[] =
     "v=0\r\n"
     "o=- 0 0 IN IP4 127.0.0.1\r\n"
     "s=-\r\n"
     "t=0 0\r\n"
+    "m=audio 1 RTP/AVPF 103\r\n"
     "a=ice-ufrag:e5785931\r\n"
     "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
     "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
     "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
-    "m=audio 1 RTP/AVPF 103\r\n"
     "a=mid:audio\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:103 ISAC/16000\r\n";
 
 // Reference SENDONLY SDP without MediaStreams. Msid is not supported.
 static const char kSdpStringSendOnlyWithoutStreams[] =
     "v=0\r\n"
     "o=- 0 0 IN IP4 127.0.0.1\r\n"
     "s=-\r\n"
     "t=0 0\r\n"
+    "m=audio 1 RTP/AVPF 103\r\n"
     "a=ice-ufrag:e5785931\r\n"
     "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
     "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
     "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
-    "m=audio 1 RTP/AVPF 103\r\n"
     "a=mid:audio\r\n"
     "a=sendrecv\r\n"
     "a=sendonly\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:103 ISAC/16000\r\n"
     "m=video 1 RTP/AVPF 120\r\n"
+    "a=ice-ufrag:e5785931\r\n"
+    "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
+    "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
+    "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
     "a=mid:video\r\n"
     "a=sendrecv\r\n"
     "a=sendonly\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:120 VP8/90000\r\n";
 
 static const char kSdpStringInit[] =
     "v=0\r\n"
     "o=- 0 0 IN IP4 127.0.0.1\r\n"
     "s=-\r\n"
     "t=0 0\r\n"
+    "a=msid-semantic: WMS\r\n";
+
+static const char kSdpStringAudio[] =
+    "m=audio 1 RTP/AVPF 103\r\n"
     "a=ice-ufrag:e5785931\r\n"
     "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
     "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
     "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
-    "a=msid-semantic: WMS\r\n";
-
-static const char kSdpStringAudio[] =
-    "m=audio 1 RTP/AVPF 103\r\n"
     "a=mid:audio\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:103 ISAC/16000\r\n";
 
 static const char kSdpStringVideo[] =
     "m=video 1 RTP/AVPF 120\r\n"
+    "a=ice-ufrag:e5785931\r\n"
+    "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n"
+    "a=fingerprint:sha-256 58:AB:6E:F5:F1:E4:57:B7:E9:46:F4:86:04:28:F9:A7:ED:"
+    "BD:AB:AE:40:EF:CE:9A:51:2C:2A:B1:9B:8B:78:84\r\n"
     "a=mid:video\r\n"
     "a=sendrecv\r\n"
     "a=rtcp-mux\r\n"
     "a=rtpmap:120 VP8/90000\r\n";
 
 static const char kSdpStringMs1Audio0[] =
     "a=ssrc:1 cname:stream1\r\n"
     "a=ssrc:1 msid:stream1 audiotrack0\r\n";
 
 static const char kSdpStringMs1Video0[] =
@@ skipping 2459 matching lines @@
   CreateAndSetRemoteOffer(kSdpStringWithStream1AudioTrackOnly);
   EXPECT_EQ(observer_.num_added_tracks_, 1);
   EXPECT_EQ(observer_.last_added_track_label_, kAudioTracks[0]);
 
   // Create and set the updated remote SDP.
   CreateAndSetRemoteOffer(kSdpStringWithStream1);
   EXPECT_EQ(observer_.num_added_tracks_, 2);
   EXPECT_EQ(observer_.last_added_track_label_, kVideoTracks[0]);
 }
 
+// Test that when SetConfiguration is called and the configuration is
+// changing, the next offer causes an ICE restart.
+TEST_F(PeerConnectionInterfaceTest, SetConfigurationCausingIceRetart) {
+  PeerConnectionInterface::RTCConfiguration config;
+  config.type = PeerConnectionInterface::kRelay;
+  // Need to pass default constraints to prevent disabling of DTLS...
+  FakeConstraints default_constraints;
+  CreatePeerConnection(config, &default_constraints);
+  AddAudioVideoStream(kStreamLabel1, "audio_label", "video_label");
+
+  // Do initial offer/answer so there's something to restart.
+  CreateOfferAsLocalDescription();
+  CreateAnswerAsRemoteDescription(kSdpStringWithStream1);
+
+  // Grab the ufrags.
+  const cricket::SessionDescription* initial_offer =
+      pc_->local_description()->description();
+  std::vector<std::string> initial_ufrags;
+  for (const cricket::TransportInfo& info : initial_offer->transport_infos()) {
+    initial_ufrags.push_back(info.description.ice_ufrag);
+  }

[pthatcher1, 2016/12/10 02:26:31]

Unimportant idea: if you enabled max-bundle, couldn't you just have one ufrag? 
Would make this code slightly shorter.

[Taylor Brandstetter, 2016/12/10 19:00:35]

I'd prefer not to change what's tested just to shorten the code.

+
+  // Change ICE policy, which should trigger an ICE restart on the next offer.
+  config.type = PeerConnectionInterface::kAll;
+  EXPECT_TRUE(pc_->SetConfiguration(config));
+  CreateOfferAsLocalDescription();
+
+  // Grab the new ufrags.
+  const cricket::SessionDescription* subsequent_offer =
+      pc_->local_description()->description();
+  std::vector<std::string> subsequent_ufrags;
+  for (const cricket::TransportInfo& info :
+       subsequent_offer->transport_infos()) {
+    subsequent_ufrags.push_back(info.description.ice_ufrag);
+  }
+
+  // Sanity check.
+  EXPECT_EQ(initial_ufrags.size(), subsequent_ufrags.size());
+  // Check that each ufrag is different.
+  for (int i = 0; i < initial_ufrags.size(); ++i) {
+    EXPECT_NE(initial_ufrags[i], subsequent_ufrags[i]);
+  }
+}
+
+// Test that when SetConfiguration is called and the configuration *isn't*
+// changing, the next offer does *not* cause an ICE restart.
+TEST_F(PeerConnectionInterfaceTest, SetConfigurationNotCausingIceRetart) {
+  PeerConnectionInterface::RTCConfiguration config;
+  config.type = PeerConnectionInterface::kRelay;
+  // Need to pass default constraints to prevent disabling of DTLS...
+  FakeConstraints default_constraints;
+  CreatePeerConnection(config, &default_constraints);
+  AddAudioVideoStream(kStreamLabel1, "audio_label", "video_label");
+
+  // Do initial offer/answer so there's something to restart.
+  CreateOfferAsLocalDescription();
+  CreateAnswerAsRemoteDescription(kSdpStringWithStream1);

[pthatcher1, 2016/12/10 02:26:31]

Would it make sense to stick all of the above in some setup method?

[Taylor Brandstetter, 2016/12/10 19:00:34]

I don't think so, we already have way too many helper methods that make it hard
to see what tests are testing.

+
+  // Grab the ufrags.
+  const cricket::SessionDescription* initial_offer =
+      pc_->local_description()->description();
+  std::vector<std::string> initial_ufrags;
+  for (const cricket::TransportInfo& info : initial_offer->transport_infos()) {
+    initial_ufrags.push_back(info.description.ice_ufrag);
+  }

[pthatcher1, 2016/12/10 02:26:31]

Would it make sense to have helper method to grab ufrags?  Would make the actual
thing being tested a larger percentage of the code.

[Taylor Brandstetter, 2016/12/10 19:00:34]

In this case I agree; done.

+
+  // Call SetConfiguration with a config identical to what the PC was
+  // constructed with.
+  EXPECT_TRUE(pc_->SetConfiguration(config));
+  CreateOfferAsLocalDescription();
+
+  // Grab the new ufrags.
+  const cricket::SessionDescription* subsequent_offer =
+      pc_->local_description()->description();
+  std::vector<std::string> subsequent_ufrags;
+  for (const cricket::TransportInfo& info :
+       subsequent_offer->transport_infos()) {
+    subsequent_ufrags.push_back(info.description.ice_ufrag);
+  }
+
+  EXPECT_EQ(initial_ufrags, subsequent_ufrags);
+}
+
+// Test for a weird corner case scenario:
+// 1. Audio/video session established.
+// 2. SetConfiguration changes ICE config; ICE restart needed.
+// 3. ICE restart initiated by remote peer, but only for one m= section.
+// 4. Next createOffer should initiate an ICE restart, but only for the other
+//    m= section; it would be pointless to do an ICE restart for the m= section
+//    that was already restarted.
+TEST_F(PeerConnectionInterfaceTest, SetConfigurationCausingPartialIceRestart) {
+  PeerConnectionInterface::RTCConfiguration config;
+  config.type = PeerConnectionInterface::kRelay;
+  // Need to pass default constraints to prevent disabling of DTLS...
+  FakeConstraints default_constraints;
+  CreatePeerConnection(config, &default_constraints);
+  AddAudioVideoStream(kStreamLabel1, "audio_label", "video_label");
+
+  // Do initial offer/answer so there's something to restart.
+  CreateOfferAsLocalDescription();
+  CreateAnswerAsRemoteDescription(kSdpStringWithStream1);
+
+  // Change ICE policy, which should set the "needs-ice-restart" flag.
+  config.type = PeerConnectionInterface::kAll;
+  EXPECT_TRUE(pc_->SetConfiguration(config));
+
+  // Do ICE restart for the first m= section, initiated by remote peer.
+  webrtc::JsepSessionDescription* remote_offer =
+      new webrtc::JsepSessionDescription(SessionDescriptionInterface::kOffer);
+  EXPECT_TRUE(remote_offer->Initialize(kSdpStringWithStream1, nullptr));
+  remote_offer->description()->transport_infos()[0].description.ice_ufrag =
+      "modified";
+  EXPECT_TRUE(DoSetRemoteDescription(remote_offer));
+  CreateAnswerAsLocalDescription();
+
+  // Grab the ufrags.
+  const cricket::SessionDescription* local_answer =
+      pc_->local_description()->description();
+  std::vector<std::string> initial_ufrags;
+  for (const cricket::TransportInfo& info : local_answer->transport_infos()) {
+    initial_ufrags.push_back(info.description.ice_ufrag);
+  }
+
+  // Create offer and grab the new ufrags.
+  CreateOfferAsLocalDescription();
+  const cricket::SessionDescription* subsequent_offer =
+      pc_->local_description()->description();
+  std::vector<std::string> subsequent_ufrags;
+  for (const cricket::TransportInfo& info :
+       subsequent_offer->transport_infos()) {
+    subsequent_ufrags.push_back(info.description.ice_ufrag);
+  }
+
+  // Ensure that only the ufrag for the second m= section changed.
+  EXPECT_EQ(initial_ufrags[0], subsequent_ufrags[0]);
+  EXPECT_NE(initial_ufrags[1], subsequent_ufrags[1]);
+}
+
 class PeerConnectionMediaConfigTest : public testing::Test {
  protected:
   void SetUp() override {
     pcf_ = new rtc::RefCountedObject<PeerConnectionFactoryForTest>();
     pcf_->Initialize();
   }
   const cricket::MediaConfig& TestCreatePeerConnection(
       const PeerConnectionInterface::RTCConfiguration& config,
       const MediaConstraintsInterface *constraints) {
     pcf_->create_media_controller_called_ = false;
@@ skipping 243 matching lines @@
   FakeConstraints updated_answer_c;
   answer_c.SetMandatoryReceiveAudio(false);
   answer_c.SetMandatoryReceiveVideo(false);
 
   cricket::MediaSessionOptions updated_answer_options;
   EXPECT_TRUE(
       ParseConstraintsForAnswer(&updated_answer_c, &updated_answer_options));
   EXPECT_TRUE(updated_answer_options.has_audio());
   EXPECT_TRUE(updated_answer_options.has_video());
 }