Bug 822159 - Fix trickle ICE to start checking when new candidates come in. r=abr
authorEKR <ekr@rtfm.com>
Thu, 20 Dec 2012 08:12:45 -0800 (2012-12-20)
changeset 117545 f0c778cd83a1074ed337766501c55ba3336fe472
parent 117544 1024e72d4ef9e650369e99a80756cb0b09334554
child 117546 68f26a077ceeefd65192aa5b4c4321a85aea1c47
push id20565
push usererescorla@mozilla.com
push dateFri, 04 Jan 2013 02:41:20 +0000 (2013-01-04)
treeherdermozilla-inbound@f0c778cd83a1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersabr
bugs822159
milestone20.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 822159 - Fix trickle ICE to start checking when new candidates come in. r=abr
media/mtransport/nricemediastream.cpp
media/mtransport/test/ice_unittest.cpp
media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
--- a/media/mtransport/nricemediastream.cpp
+++ b/media/mtransport/nricemediastream.cpp
@@ -141,21 +141,16 @@ nsresult NrIceMediaStream::ParseTrickleC
 
     } else {
       MOZ_MTLOG(PR_LOG_ERROR, "Couldn't parse trickle candidate for stream '"
          << name_ << "'");
       return NS_ERROR_FAILURE;
     }
   }
 
-  if (ctx_->state() == NrIceCtx::ICE_CTX_GATHERED) {
-    // Try to start checks if they are not already started
-    return ctx_->StartChecks();
-  }
-
   return NS_OK;
 }
 
 
 void NrIceMediaStream::EmitAllCandidates() {
   char **attrs = 0;
   int attrct;
   int r;
--- a/media/mtransport/test/ice_unittest.cpp
+++ b/media/mtransport/test/ice_unittest.cpp
@@ -46,17 +46,18 @@ class IceTestPeer : public sigslot::has_
       name_(name),
       ice_ctx_(NrIceCtx::Create(name, offerer, set_priorities)),
       streams_(),
       candidates_(),
       gathering_complete_(false),
       ready_ct_(0),
       ice_complete_(false),
       received_(0),
-      sent_(0) {
+      sent_(0),
+      remote_(nullptr) {
     ice_ctx_->SignalGatheringCompleted.connect(this,
                                               &IceTestPeer::GatheringComplete);
     ice_ctx_->SignalCompleted.connect(this, &IceTestPeer::IceCompleted);
   }
 
   void AddStream(int components) {
     char name[100];
     snprintf(name, sizeof(name), "%s:stream%d", name_.c_str(), (int)streams_.size());
@@ -87,24 +88,29 @@ class IceTestPeer : public sigslot::has_
   }
 
   std::vector<std::string>& GetCandidates(const std::string &name) {
     return candidates_[name];
   }
 
   bool gathering_complete() { return gathering_complete_; }
   int ready_ct() { return ready_ct_; }
+  bool is_ready(size_t stream) {
+    return streams_[stream]->state() == NrIceMediaStream::ICE_OPEN;
+  }
   bool ice_complete() { return ice_complete_; }
   size_t received() { return received_; }
   size_t sent() { return sent_; }
 
   // Start connecting to another peer
   void Connect(IceTestPeer *remote, TrickleMode trickle_mode, bool start = true) {
     nsresult res;
 
+    remote_ = remote;
+
     test_utils->sts_target()->Dispatch(
       WrapRunnableRet(ice_ctx_,
         &NrIceCtx::ParseGlobalAttributes, remote->GetGlobalAttributes(), &res),
       NS_DISPATCH_SYNC);
     ASSERT_TRUE(NS_SUCCEEDED(res));
 
     if (trickle_mode == TRICKLE_NONE) {
       for (size_t i=0; i<streams_.size(); ++i) {
@@ -130,33 +136,37 @@ class IceTestPeer : public sigslot::has_
 
     if (start) {
       // Now start checks
       test_utils->sts_target()->Dispatch(
         WrapRunnableRet(ice_ctx_, &NrIceCtx::StartChecks, &res),
         NS_DISPATCH_SYNC);
       ASSERT_TRUE(NS_SUCCEEDED(res));
     }
+  }
 
-    if (trickle_mode == TRICKLE_DEFERRED) {
-      // If we are in trickle deferred mode, now trickle in the candidates
-      // after ICE has started
-      for (size_t i=0; i<streams_.size(); ++i) {
-        std::vector<std::string> candidates =
-            remote->GetCandidates(remote->streams_[i]->name());
+  void DoTrickle(size_t stream) {
+    std::cerr << "Doing trickle for stream " << stream << std::endl;
+    // If we are in trickle deferred mode, now trickle in the candidates
+    // for |stream}
+    nsresult res;
+
+    ASSERT_GT(remote_->streams_.size(), stream);
 
-        for (size_t j=0; j<candidates.size(); j++) {
-          test_utils->sts_target()->Dispatch(
-              WrapRunnableRet(streams_[i], &NrIceMediaStream::ParseTrickleCandidate,
-                              candidates[j],
-                              &res), NS_DISPATCH_SYNC);
+    std::vector<std::string> candidates =
+      remote_->GetCandidates(remote_->streams_[stream]->name());
 
-          ASSERT_TRUE(NS_SUCCEEDED(res));
-        }
-      }
+    for (size_t j=0; j<candidates.size(); j++) {
+      test_utils->sts_target()->Dispatch(
+        WrapRunnableRet(streams_[stream],
+                        &NrIceMediaStream::ParseTrickleCandidate,
+                        candidates[j],
+                        &res), NS_DISPATCH_SYNC);
+
+      ASSERT_TRUE(NS_SUCCEEDED(res));
     }
   }
 
   void Close() {
     ice_ctx_->destroy_peer_ctx();
   }
 
   void StartChecks() {
@@ -208,16 +218,17 @@ class IceTestPeer : public sigslot::has_
   nsRefPtr<NrIceCtx> ice_ctx_;
   std::vector<mozilla::RefPtr<NrIceMediaStream> > streams_;
   std::map<std::string, std::vector<std::string> > candidates_;
   bool gathering_complete_;
   int ready_ct_;
   bool ice_complete_;
   size_t received_;
   size_t sent_;
+  IceTestPeer *remote_;
 };
 
 class IceTest : public ::testing::Test {
  public:
   IceTest() : initted_(false) {}
 
   void SetUp() {
     nsresult rv;
@@ -249,32 +260,47 @@ class IceTest : public ::testing::Test {
       return false;
     EXPECT_TRUE_WAIT(p2_->gathering_complete(), 10000);
     if (!p2_->gathering_complete())
       return false;
 
     return true;
   }
 
-  void Connect(TrickleMode trickle_mode = TRICKLE_NONE) {
-    p1_->Connect(p2_, trickle_mode);
-    p2_->Connect(p1_, trickle_mode);
+  void Connect() {
+    p1_->Connect(p2_, TRICKLE_NONE);
+    p2_->Connect(p1_, TRICKLE_NONE);
 
     ASSERT_TRUE_WAIT(p1_->ready_ct() == 1 && p2_->ready_ct() == 1, 5000);
     ASSERT_TRUE_WAIT(p1_->ice_complete() && p2_->ice_complete(), 5000);
   }
 
+  void ConnectTrickle() {
+    p1_->Connect(p2_, TRICKLE_DEFERRED);
+    p2_->Connect(p1_, TRICKLE_DEFERRED);
+  }
+
+  void DoTrickle(size_t stream) {
+    p1_->DoTrickle(stream);
+    p2_->DoTrickle(stream);
+    ASSERT_TRUE_WAIT(p1_->is_ready(stream), 5000);
+    ASSERT_TRUE_WAIT(p2_->is_ready(stream), 5000);
+  }
+
+  void VerifyConnected() {
+  }
+
   void CloseP1() {
     p1_->Close();
   }
 
   void ConnectThenDelete() {
     p1_->Connect(p2_, TRICKLE_NONE, true);
     p2_->Connect(p1_, TRICKLE_NONE, false);
-    test_utils->sts_target()->Dispatch(WrapRunnable(this, 
+    test_utils->sts_target()->Dispatch(WrapRunnable(this,
                                                     &IceTest::CloseP1),
                                        NS_DISPATCH_SYNC);
     p2_->StartChecks();
 
     // Wait to see if we crash
     PR_Sleep(PR_MillisecondsToInterval(5000));
   }
 
@@ -315,22 +341,37 @@ TEST_F(IceTest, TestConnect) {
 
 TEST_F(IceTest, TestConnectAutoPrioritize) {
   Init(false);
   AddStream("first", 1);
   ASSERT_TRUE(Gather(true));
   Connect();
 }
 
-TEST_F(IceTest, TestConnectTrickle) {
+TEST_F(IceTest, TestConnectTrickleOneStreamOneComponent) {
   AddStream("first", 1);
   ASSERT_TRUE(Gather(true));
-  Connect(TRICKLE_DEFERRED);
+  ConnectTrickle();
+  DoTrickle(0);
+  ASSERT_TRUE_WAIT(p1_->ice_complete(), 1000);
+  ASSERT_TRUE_WAIT(p2_->ice_complete(), 1000);
 }
 
+TEST_F(IceTest, TestConnectTrickleTwoStreamsOneComponent) {
+  AddStream("first", 1);
+  AddStream("second", 1);
+  ASSERT_TRUE(Gather(true));
+  ConnectTrickle();
+  DoTrickle(0);
+  DoTrickle(1);
+  ASSERT_TRUE_WAIT(p1_->ice_complete(), 1000);
+  ASSERT_TRUE_WAIT(p2_->ice_complete(), 1000);
+}
+
+
 TEST_F(IceTest, TestSendReceive) {
   AddStream("first", 1);
   ASSERT_TRUE(Gather(true));
   Connect();
   SendReceive();
 }
 
 TEST_F(IceTest, TestConnectShutdownOneSide) {
--- a/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
+++ b/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ -328,28 +328,29 @@ static void nr_ice_media_stream_check_ti
         pair=TAILQ_NEXT(pair,entry);
       }
     }
 
     if(pair){
       nr_ice_candidate_pair_start(pair->pctx,pair); /* Ignore failures */
       NR_ASYNC_TIMER_SET(timer_val,nr_ice_media_stream_check_timer_cb,cb_arg,&stream->timer);
     }
-    /* TODO(ekr@rtfm.com): Report on the special case where there are no checks to
-       run at all */
+    else {
+      r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): no pairs for %s",stream->pctx->label,stream->label);
+    }
+
     _status=0;
   abort:
     return;
   }
 
 
 /* Start checks for this media stream (aka check list) */
 int nr_ice_media_stream_start_checks(nr_ice_peer_ctx *pctx, nr_ice_media_stream *stream)
   {
-    assert(stream->ice_state==NR_ICE_MEDIA_STREAM_CHECKS_FROZEN);
     nr_ice_media_stream_set_state(stream,NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE);
     nr_ice_media_stream_check_timer_cb(0,0,stream);
 
     return(0);
   }
   
 /* Start checks for this media stream (aka check list) S 5.7 */
 int nr_ice_media_stream_unfreeze_pairs(nr_ice_peer_ctx *pctx, nr_ice_media_stream *stream)
@@ -495,28 +496,33 @@ int nr_ice_media_stream_dump_state(nr_ic
       pair=TAILQ_NEXT(pair,entry);
     }
 
     return(0);
   }
 
 int nr_ice_media_stream_set_state(nr_ice_media_stream *str, int state)
   {
-    // removed, per EKR:  assert(state!=str->ice_state);
-      
+    /* Make no-change a no-op */
+    if (state == str->ice_state)
+      return 0;
+
     r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): stream %s state %s->%s",
       str->pctx->label,str->label,
       nr_ice_media_stream_states[str->ice_state],
       nr_ice_media_stream_states[state]);
     
-    if(str->ice_state != NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE)
+    if(state == NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE)
       str->pctx->active_streams++;
     if(str->ice_state == NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE)
       str->pctx->active_streams--;
 
+    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): %d active streams",
+      str->pctx->label, str->pctx->active_streams);
+
     str->ice_state=state;
     
     return(0);
   }
 
 /* S OK, this component has a nominated. If every component has a nominated, 
    the stream is ready */
 int nr_ice_media_stream_component_nominated(nr_ice_media_stream *stream,nr_ice_component *component)
--- a/media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
+++ b/media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
@@ -242,17 +242,37 @@ int nr_ice_peer_ctx_parse_trickle_candid
        fine because we suppress duplicate pairing */
     if (needs_pairing) {
       if(r=nr_ice_media_stream_pair_candidates(pctx, stream, pstream)) {
         r_log(LOG_ICE,LOG_ERR,"ICE(%s): peer (%s), stream(%s) failed to pair trickle ICE candidates",pctx->ctx->label,pctx->label,stream->label);
         ABORT(r);
       }
     }
 
-    _status =0;
+    /* Start checks if this stream is not checking yet or if it has checked
+       all the available candidates but not had a completed check for all
+       components.
+
+       Note that this is not compliant with RFC 5245, but consistent with
+       the libjingle trickle ICE behavior. Note that we will not restart
+       checks if either (a) the stream has failed or (b) all components
+       have a successful pair because the switch statement above jumps
+       will in both states.
+
+       TODO(ekr@rtfm.com): restart checks.
+       TODO(ekr@rtfm.com): update when the trickle ICE RFC is published
+    */
+    if (!pstream->timer) {
+      if(r=nr_ice_media_stream_start_checks(pctx, pstream)) {
+        r_log(LOG_ICE,LOG_ERR,"ICE(%s): peer (%s), stream(%s) failed to start checks",pctx->ctx->label,pctx->label,stream->label);
+        ABORT(r);
+      }
+    }
+
+    _status=0;
  abort:
     return(_status);
 
   }
 
 
 int nr_ice_peer_ctx_pair_candidates(nr_ice_peer_ctx *pctx)
   {