Fixing and re-enabling some flaky PeerConnection tests.

talk/app/webrtc/peerconnection_unittest.cc

 /*
  * libjingle
  * Copyright 2012 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 153 matching lines @@
       const PeerConnectionFactory::Options* options) {
     rtc::scoped_ptr<FakeDtlsIdentityStore> dtls_identity_store(
         rtc::SSLStreamAdapter::HaveDtlsSrtp() ? new FakeDtlsIdentityStore()
                                               : nullptr);
 
     return CreateClientWithDtlsIdentityStore(id, constraints, options,
                                              dtls_identity_store.Pass());
   }
 
   ~PeerConnectionTestClient() {
-    while (!fake_video_renderers_.empty()) {
-      RenderMap::iterator it = fake_video_renderers_.begin();
-      delete it->second;
-      fake_video_renderers_.erase(it);
-    }
   }
 
   void Negotiate() { Negotiate(true, true); }
 
   void Negotiate(bool audio, bool video) {
     rtc::scoped_ptr<SessionDescriptionInterface> offer;
     ASSERT_TRUE(DoCreateOffer(offer.use()));
 
     if (offer->description()->GetContentByName("audio")) {
       offer->description()->GetContentByName("audio")->rejected = !audio;
@@ skipping 33 matching lines @@
   void OnSignalingChange(
       webrtc::PeerConnectionInterface::SignalingState new_state) override {
     EXPECT_EQ(pc()->signaling_state(), new_state);
   }
   void OnAddStream(MediaStreamInterface* media_stream) override {
     media_stream->RegisterObserver(this);
     for (size_t i = 0; i < media_stream->GetVideoTracks().size(); ++i) {
       const std::string id = media_stream->GetVideoTracks()[i]->id();
       ASSERT_TRUE(fake_video_renderers_.find(id) ==
                   fake_video_renderers_.end());
-      fake_video_renderers_[id] =
-          new webrtc::FakeVideoTrackRenderer(media_stream->GetVideoTracks()[i]);
+      fake_video_renderers_[id].reset(new webrtc::FakeVideoTrackRenderer(
+          media_stream->GetVideoTracks()[i]));
     }
   }
   void OnRemoveStream(MediaStreamInterface* media_stream) override {}
   void OnRenegotiationNeeded() override {}
   void OnIceConnectionChange(
       webrtc::PeerConnectionInterface::IceConnectionState new_state) override {
     EXPECT_EQ(pc()->ice_connection_state(), new_state);
   }
   void OnIceGatheringChange(
       webrtc::PeerConnectionInterface::IceGatheringState new_state) override {
@@ skipping 14 matching lines @@
 
   // MediaStreamInterface callback
   void OnChanged() override {
     // Track added or removed from MediaStream, so update our renderers.
     rtc::scoped_refptr<StreamCollectionInterface> remote_streams =
         pc()->remote_streams();
     // Remove renderers for tracks that were removed.
     for (auto it = fake_video_renderers_.begin();
          it != fake_video_renderers_.end();) {
       if (remote_streams->FindVideoTrack(it->first) == nullptr) {
-        auto to_delete = it++;
-        delete to_delete->second;
-        fake_video_renderers_.erase(to_delete);
+        auto to_remove = it++;
+        removed_fake_video_renderers_.push_back(std::move(to_remove->second));
+        fake_video_renderers_.erase(to_remove);
       } else {
         ++it;
       }
     }
     // Create renderers for new video tracks.
     for (size_t stream_index = 0; stream_index < remote_streams->count();
          ++stream_index) {
       MediaStreamInterface* remote_stream = remote_streams->at(stream_index);
       for (size_t track_index = 0;
            track_index < remote_stream->GetVideoTracks().size();
            ++track_index) {
         const std::string id =
             remote_stream->GetVideoTracks()[track_index]->id();
         if (fake_video_renderers_.find(id) != fake_video_renderers_.end()) {
           continue;
         }
-        fake_video_renderers_[id] = new webrtc::FakeVideoTrackRenderer(
-            remote_stream->GetVideoTracks()[track_index]);
+        fake_video_renderers_[id].reset(new webrtc::FakeVideoTrackRenderer(
+            remote_stream->GetVideoTracks()[track_index]));
       }
     }
   }
 
   void SetVideoConstraints(const webrtc::FakeConstraints& video_constraint) {
     video_constraints_ = video_constraint;
   }
 
   void AddMediaStream(bool audio, bool video) {
     std::string stream_label =
@@ skipping 146 matching lines @@
              video_capturers_.begin();
          it != video_capturers_.end(); ++it) {
       (*it)->Stop();
     }
   }
 
   bool AudioFramesReceivedCheck(int number_of_frames) const {
     return number_of_frames <= fake_audio_capture_module_->frames_received();
   }
 
+  int TotalAudioFramesReceived() const {

[pthatcher1, 2015/12/11 02:10:13]

I'd just call this audio_frames_received()

+    return fake_audio_capture_module_->frames_received();
+  }
+
   bool VideoFramesReceivedCheck(int number_of_frames) {
     if (video_decoder_factory_enabled_) {
       const std::vector<FakeWebRtcVideoDecoder*>& decoders
           = fake_video_decoder_factory_->decoders();
       if (decoders.empty()) {
         return number_of_frames <= 0;
       }
 
-      for (std::vector<FakeWebRtcVideoDecoder*>::const_iterator
-           it = decoders.begin(); it != decoders.end(); ++it) {
-        if (number_of_frames > (*it)->GetNumFramesReceived()) {
+      for (FakeWebRtcVideoDecoder* decoder : decoders) {
+        if (number_of_frames > decoder->GetNumFramesReceived()) {
           return false;
         }
       }
       return true;
     } else {
       if (fake_video_renderers_.empty()) {
         return number_of_frames <= 0;
       }
 
-      for (RenderMap::const_iterator it = fake_video_renderers_.begin();
-           it != fake_video_renderers_.end(); ++it) {
-        if (number_of_frames > it->second->num_rendered_frames()) {
+      for (const auto& pair : fake_video_renderers_) {
+        if (number_of_frames > pair.second->num_rendered_frames()) {
           return false;
         }
       }
       return true;
     }
   }
 
+  int TotalVideoFramesReceived() const {

[pthatcher1, 2015/12/11 02:10:13]

Likewise, video_frames_received()

+    int total = 0;
+    if (video_decoder_factory_enabled_) {
+      const std::vector<FakeWebRtcVideoDecoder*>& decoders =
+          fake_video_decoder_factory_->decoders();
+      for (const FakeWebRtcVideoDecoder* decoder : decoders) {
+        total += decoder->GetNumFramesReceived();
+      }
+    } else {
+      for (const auto& pair : fake_video_renderers_) {
+        total += pair.second->num_rendered_frames();
+      }
+      for (const auto& renderer : removed_fake_video_renderers_) {
+        total += renderer->num_rendered_frames();
+      }
+    }
+    return total;
+  }
+
   // Verify the CreateDtmfSender interface
   void VerifyDtmf() {
     rtc::scoped_ptr<DummyDtmfObserver> observer(new DummyDtmfObserver());
     rtc::scoped_refptr<DtmfSenderInterface> dtmf_sender;
 
     // We can't create a DTMF sender with an invalid audio track or a non local
     // track.
     EXPECT_TRUE(peer_connection_->CreateDtmfSender(nullptr) == nullptr);
     rtc::scoped_refptr<webrtc::AudioTrackInterface> non_localtrack(
         peer_connection_factory_->CreateAudioTrack("dummy_track", nullptr));
@@ skipping 376 matching lines @@
   rtc::scoped_refptr<webrtc::PeerConnectionInterface> peer_connection_;
   rtc::scoped_refptr<webrtc::PeerConnectionFactoryInterface>
       peer_connection_factory_;
 
   bool auto_add_stream_ = true;
 
   typedef std::pair<std::string, std::string> IceUfragPwdPair;
   std::map<int, IceUfragPwdPair> ice_ufrag_pwd_;
   bool expect_ice_restart_ = false;
 
-  // Needed to keep track of number of frames send.
+  // Needed to keep track of number of frames sent.
   rtc::scoped_refptr<FakeAudioCaptureModule> fake_audio_capture_module_;
   // Needed to keep track of number of frames received.
-  typedef std::map<std::string, webrtc::FakeVideoTrackRenderer*> RenderMap;
-  RenderMap fake_video_renderers_;
+  std::map<std::string, rtc::scoped_ptr<webrtc::FakeVideoTrackRenderer>>
+      fake_video_renderers_;
+  // Needed to ensure frames aren't received for removed tracks.
+  std::vector<rtc::scoped_ptr<webrtc::FakeVideoTrackRenderer>>
+      removed_fake_video_renderers_;
   // Needed to keep track of number of frames received when external decoder
   // used.
   FakeWebRtcVideoDecoderFactory* fake_video_decoder_factory_ = nullptr;
   FakeWebRtcVideoEncoderFactory* fake_video_encoder_factory_ = nullptr;
   bool video_decoder_factory_enabled_ = false;
   webrtc::FakeConstraints video_constraints_;
 
   // For remote peer communication.
   SignalingMessageReceiver* signaling_message_receiver_ = nullptr;
 
@@ skipping 41 matching lines @@
   bool VideoFramesReceivedCheck(int frames_received) {
     return initiating_client_->VideoFramesReceivedCheck(frames_received) &&
         receiving_client_->VideoFramesReceivedCheck(frames_received);
   }
   void VerifyDtmf() {
     initiating_client_->VerifyDtmf();
     receiving_client_->VerifyDtmf();
   }
 
   void TestUpdateOfferWithRejectedContent() {
+    // Renegotiate, rejecting the video m-line.
     initiating_client_->Negotiate(true, false);
-    EXPECT_TRUE_WAIT(
-        FramesNotPending(kEndAudioFrameCount * 2, kEndVideoFrameCount),
-        kMaxWaitForFramesMs);
-    // There shouldn't be any more video frame after the new offer is
-    // negotiated.
-    EXPECT_FALSE(VideoFramesReceivedCheck(kEndVideoFrameCount + 1));
+    ASSERT_TRUE_WAIT(SessionActive(), kMaxWaitForActivationMs);
+
+    int pc1_audio_received = initiating_client_->TotalAudioFramesReceived();
+    int pc1_video_received = initiating_client_->TotalVideoFramesReceived();
+    int pc2_audio_received = receiving_client_->TotalAudioFramesReceived();
+    int pc2_video_received = receiving_client_->TotalVideoFramesReceived();
+
+    // Wait for some additional audio frames to be received.
+    EXPECT_TRUE_WAIT(initiating_client_->AudioFramesReceivedCheck(
+                         pc1_audio_received + kEndAudioFrameCount) &&
+                         receiving_client_->AudioFramesReceivedCheck(
+                             pc2_audio_received + kEndAudioFrameCount),
+                     kMaxWaitForFramesMs);
+
+    // During this time, we shouldn't have received any additional video frames
+    // for the rejected video tracks.
+    EXPECT_EQ(pc1_video_received,
+              initiating_client_->TotalVideoFramesReceived());
+    EXPECT_EQ(pc2_video_received,
+              receiving_client_->TotalVideoFramesReceived());
   }
 
   void VerifyRenderedSize(int width, int height) {
     EXPECT_EQ(width, receiving_client()->rendered_width());
     EXPECT_EQ(height, receiving_client()->rendered_height());
     EXPECT_EQ(width, initializing_client()->rendered_width());
     EXPECT_EQ(height, initializing_client()->rendered_height());
   }
 
   void VerifySessionDescriptions() {
@@ skipping 323 matching lines @@
 TEST_F(JsepPeerConnectionP2PTestClient, LocalP2PTestAnswerNone) {
   ASSERT_TRUE(CreateTestClients());
   receiving_client()->SetReceiveAudioVideo(false, false);
   LocalP2PTest();
 }
 
 // This test sets up an audio and video call between two parties. After the call
 // runs for a while (10 frames), the caller sends an update offer with video
 // being rejected. Once the re-negotiation is done, the video flow should stop
 // and the audio flow should continue.
-// Disabled due to b/14955157.
-TEST_F(JsepPeerConnectionP2PTestClient,
-       DISABLED_UpdateOfferWithRejectedContent) {
+TEST_F(JsepPeerConnectionP2PTestClient, UpdateOfferWithRejectedContent) {
   ASSERT_TRUE(CreateTestClients());
   LocalP2PTest();
   TestUpdateOfferWithRejectedContent();
 }
 
 // This test sets up a Jsep call between two parties. The MSID is removed from
 // the SDP strings from the caller.
-// Disabled due to b/14955157.
-TEST_F(JsepPeerConnectionP2PTestClient,
-       DISABLED_LocalP2PTestWithoutMsid) {
+TEST_F(JsepPeerConnectionP2PTestClient, LocalP2PTestWithoutMsid) {
   ASSERT_TRUE(CreateTestClients());
   receiving_client()->RemoveMsidFromReceivedSdp(true);
   // TODO(perkj): Currently there is a bug that cause audio to stop playing if
   // audio and video is muxed when MSID is disabled. Remove
   // SetRemoveBundleFromSdp once
   // https://code.google.com/p/webrtc/issues/detail?id=1193 is fixed.
   receiving_client()->RemoveBundleFromReceivedSdp(true);
   LocalP2PTest();
 }
 
@@ skipping 645 matching lines @@
   server.urls.push_back("stun:hostname");
   server.urls.push_back("turn:hostname");
   servers.push_back(server);
   EXPECT_TRUE(webrtc::ParseIceServers(servers, &stun_configurations_,
                                       &turn_configurations_));
   EXPECT_EQ(1U, stun_configurations_.size());
   EXPECT_EQ(1U, turn_configurations_.size());
 }
 
 #endif // if !defined(THREAD_SANITIZER)