Fix flaky test SendRtpRtcpHeaderExtensionsTest.SentPackets*.

Flakiness was caused by a race condition between two atomic integers shared by two threads. Fixed by counting bad packets (those not containing the expected extension) instead of the good packets.

The CL also eliminates another possible flake by introducing a test fixture which doesn't automatically start sending audio packets when constructed.

BUG=3340,3356
R=tina.legrand@webrtc.org

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

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

webrtc/voice_engine/test/auto_test/fixtures/after_streaming_fixture.cc

@@ -10,65 +10,7 @@
 
 #include "webrtc/voice_engine/test/auto_test/fixtures/after_streaming_fixture.h"
 
-#include <string.h>
-
 AfterStreamingFixture::AfterStreamingFixture()
-    : channel_(voe_base_->CreateChannel()) {
-  EXPECT_GE(channel_, 0);
-
-  fake_microphone_input_file_ = resource_manager_.long_audio_file_path();
-  EXPECT_FALSE(fake_microphone_input_file_.empty());
-
-  SetUpLocalPlayback();
+    : BeforeStreamingFixture() {
   ResumePlaying();
-  RestartFakeMicrophone();
-}
-
-AfterStreamingFixture::~AfterStreamingFixture() {
-  voe_file_->StopPlayingFileAsMicrophone(channel_);
-  PausePlaying();
-
-  EXPECT_EQ(0, voe_network_->DeRegisterExternalTransport(channel_));
-  voe_base_->DeleteChannel(channel_);
-  delete transport_;
-}
-
-void AfterStreamingFixture::SwitchToManualMicrophone() {
-  EXPECT_EQ(0, voe_file_->StopPlayingFileAsMicrophone(channel_));
-
-  TEST_LOG("You need to speak manually into the microphone for this test.\n");
-  TEST_LOG("Please start speaking now.\n");
-  Sleep(1000);
-}
-
-void AfterStreamingFixture::RestartFakeMicrophone() {
-  EXPECT_EQ(0, voe_file_->StartPlayingFileAsMicrophone(
-        channel_, fake_microphone_input_file_.c_str(), true, true));
-}
-
-void AfterStreamingFixture::PausePlaying() {
-  EXPECT_EQ(0, voe_base_->StopSend(channel_));
-  EXPECT_EQ(0, voe_base_->StopPlayout(channel_));
-  EXPECT_EQ(0, voe_base_->StopReceive(channel_));
-}
-
-void AfterStreamingFixture::ResumePlaying() {
-  EXPECT_EQ(0, voe_base_->StartReceive(channel_));
-  EXPECT_EQ(0, voe_base_->StartPlayout(channel_));
-  EXPECT_EQ(0, voe_base_->StartSend(channel_));
-}
-
-void AfterStreamingFixture::SetUpLocalPlayback() {
-  transport_ = new LoopBackTransport(voe_network_);
-  EXPECT_EQ(0, voe_network_->RegisterExternalTransport(channel_, *transport_));
-
-  webrtc::CodecInst codec;
-  codec.channels = 1;
-  codec.pacsize = 160;
-  codec.plfreq = 8000;
-  codec.pltype = 0;
-  codec.rate = 64000;
-  strcpy(codec.plname, "PCMU");
-
-  voe_codec_->SetSendCodec(channel_, codec);
 }

webrtc/voice_engine/test/auto_test/fixtures/after_streaming_fixture.h

@@ -11,40 +11,14 @@
 #ifndef SRC_VOICE_ENGINE_MAIN_TEST_AUTO_TEST_STANDARD_AFTER_STREAMING_H_
 #define SRC_VOICE_ENGINE_MAIN_TEST_AUTO_TEST_STANDARD_AFTER_STREAMING_H_
 
-#include "webrtc/voice_engine/test/auto_test/fixtures/after_initialization_fixture.h"
-#include "webrtc/voice_engine/test/auto_test/resource_manager.h"
+#include "webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.h"
 
 // This fixture will, in addition to the work done by its superclasses,
-// create a channel and start playing a file through the fake microphone
-// to simulate microphone input. The purpose is to make it convenient
-// to write tests that require microphone input.
-class AfterStreamingFixture : public AfterInitializationFixture {
+// start play back on construction.
+class AfterStreamingFixture : public BeforeStreamingFixture {
  public:
   AfterStreamingFixture();
-  virtual ~AfterStreamingFixture();
-
- protected:
-  int             channel_;
-  ResourceManager resource_manager_;
-  std::string     fake_microphone_input_file_;
-
-  // Shuts off the fake microphone for this test.
-  void SwitchToManualMicrophone();
-
-  // Restarts the fake microphone if it's been shut off earlier.
-  void RestartFakeMicrophone();
-
-  // Stops all sending and playout.
-  void PausePlaying();
-
-  // Resumes all sending and playout.
-  void ResumePlaying();
-
- private:
-  void SetUpLocalPlayback();
-
-  LoopBackTransport* transport_;
+  virtual ~AfterStreamingFixture() {}
 };
 
-
 #endif  // SRC_VOICE_ENGINE_MAIN_TEST_AUTO_TEST_STANDARD_AFTER_STREAMING_H_

webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.cc

@@ -0,0 +1,75 @@
+/*
+ *  Copyright (c) 2014 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.
+ */
+
+#include "webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.h"
+
+BeforeStreamingFixture::BeforeStreamingFixture()
+    : channel_(voe_base_->CreateChannel()),
+      transport_(NULL) {
+  EXPECT_GE(channel_, 0);
+
+  fake_microphone_input_file_ = resource_manager_.long_audio_file_path();
+  EXPECT_FALSE(fake_microphone_input_file_.empty());
+
+  SetUpLocalPlayback();
+  RestartFakeMicrophone();
+}
+
+BeforeStreamingFixture::~BeforeStreamingFixture() {
+  voe_file_->StopPlayingFileAsMicrophone(channel_);
+  PausePlaying();
+
+  EXPECT_EQ(0, voe_network_->DeRegisterExternalTransport(channel_));
+  voe_base_->DeleteChannel(channel_);
+  delete transport_;
+}
+
+void BeforeStreamingFixture::SwitchToManualMicrophone() {
+  EXPECT_EQ(0, voe_file_->StopPlayingFileAsMicrophone(channel_));
+
+  TEST_LOG("You need to speak manually into the microphone for this test.\n");
+  TEST_LOG("Please start speaking now.\n");
+  Sleep(1000);
+}
+
+void BeforeStreamingFixture::RestartFakeMicrophone() {
+  EXPECT_EQ(0, voe_file_->StartPlayingFileAsMicrophone(
+        channel_, fake_microphone_input_file_.c_str(), true, true));
+}
+
+void BeforeStreamingFixture::PausePlaying() {
+  EXPECT_EQ(0, voe_base_->StopSend(channel_));
+  EXPECT_EQ(0, voe_base_->StopPlayout(channel_));
+  EXPECT_EQ(0, voe_base_->StopReceive(channel_));
+}
+
+void BeforeStreamingFixture::ResumePlaying() {
+  EXPECT_EQ(0, voe_base_->StartReceive(channel_));
+  EXPECT_EQ(0, voe_base_->StartPlayout(channel_));
+  EXPECT_EQ(0, voe_base_->StartSend(channel_));
+}
+
+void BeforeStreamingFixture::SetUpLocalPlayback() {
+  transport_ = new LoopBackTransport(voe_network_);
+  EXPECT_EQ(0, voe_network_->RegisterExternalTransport(channel_, *transport_));
+
+  webrtc::CodecInst codec;
+  codec.channels = 1;
+  codec.pacsize = 160;
+  codec.plfreq = 8000;
+  codec.pltype = 0;
+  codec.rate = 64000;
+#if defined(_MSC_VER) && defined(_WIN32)
+  _snprintf(codec.plname, RTP_PAYLOAD_NAME_SIZE - 1, "PCMU");
+#else
+  snprintf(codec.plname, RTP_PAYLOAD_NAME_SIZE, "PCMU");
+#endif
+  voe_codec_->SetSendCodec(channel_, codec);
+}

webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.h

@@ -0,0 +1,51 @@
+/*
+ *  Copyright (c) 2014 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.
+ */
+
+#ifndef SRC_VOICE_ENGINE_MAIN_TEST_AUTO_TEST_STANDARD_BEFORE_STREAMING_H_
+#define SRC_VOICE_ENGINE_MAIN_TEST_AUTO_TEST_STANDARD_BEFORE_STREAMING_H_
+
+#include <string>
+#include "webrtc/voice_engine/test/auto_test/fixtures/after_initialization_fixture.h"
+#include "webrtc/voice_engine/test/auto_test/resource_manager.h"
+
+// This fixture will, in addition to the work done by its superclasses,
+// create a channel and prepare playing a file through the fake microphone
+// to simulate microphone input. The purpose is to make it convenient
+// to write tests that require microphone input.
+class BeforeStreamingFixture : public AfterInitializationFixture {
+ public:
+  BeforeStreamingFixture();
+  virtual ~BeforeStreamingFixture();
+
+ protected:
+  int             channel_;
+  ResourceManager resource_manager_;
+  std::string     fake_microphone_input_file_;
+
+  // Shuts off the fake microphone for this test.
+  void SwitchToManualMicrophone();
+
+  // Restarts the fake microphone if it's been shut off earlier.
+  void RestartFakeMicrophone();
+
+  // Stops all sending and playout.
+  void PausePlaying();
+
+  // Resumes all sending and playout.
+  void ResumePlaying();
+
+ private:
+  void SetUpLocalPlayback();
+
+  LoopBackTransport* transport_;
+};
+
+
+#endif  // SRC_VOICE_ENGINE_MAIN_TEST_AUTO_TEST_STANDARD_BEFORE_STREAMING_H_

webrtc/voice_engine/test/auto_test/standard/rtp_rtcp_extensions.cc

@@ -13,7 +13,7 @@
 #include "webrtc/system_wrappers/interface/atomic32.h"
 #include "webrtc/system_wrappers/interface/sleep.h"
 #include "webrtc/video_engine/include/vie_network.h"
-#include "webrtc/voice_engine/test/auto_test/fixtures/after_streaming_fixture.h"
+#include "webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.h"
 
 using ::testing::_;
 using ::testing::AtLeast;
@@ -23,14 +23,13 @@ using ::testing::Field;
 class ExtensionVerifyTransport : public webrtc::Transport {
  public:
   ExtensionVerifyTransport()
-      : received_packets_(0),
-        ok_packets_(0),
-        parser_(webrtc::RtpHeaderParser::Create()),
+      : parser_(webrtc::RtpHeaderParser::Create()),
+        received_packets_(0),
+        bad_packets_(0),
         audio_level_id_(-1),
         absolute_sender_time_id_(-1) {}
 
   virtual int SendPacket(int channel, const void* data, int len) {
-    ++received_packets_;
     webrtc::RTPHeader header;
     if (parser_->Parse(static_cast<const uint8_t*>(data), len, &header)) {
       bool ok = true;
@@ -42,10 +41,14 @@ class ExtensionVerifyTransport : public webrtc::Transport {
           !header.extension.hasAbsoluteSendTime) {
         ok = false;
       }
-      if (ok) {
-        ++ok_packets_;
+      if (!ok) {
+        // bad_packets_ count packets we expected to have an extension but
+        // didn't have one.
+        ++bad_packets_;
       }
     }
+    // received_packets_ count all packets we receive.
+    ++received_packets_;
     return len;
   }
 
@@ -64,25 +67,31 @@ class ExtensionVerifyTransport : public webrtc::Transport {
                                         id);
   }
 
-  bool WaitForNPackets(int count) {
-    while (received_packets_.Value() < count) {
-      webrtc::SleepMs(10);
+  bool Wait() {
+    // Wait until we've received to specified number of packets.
+    while (received_packets_.Value() < kPacketsExpected) {
+      webrtc::SleepMs(kSleepIntervalMs);
     }
-    return (ok_packets_.Value() == count);
+    // Check whether any where 'bad' (didn't contain an extension when they
+    // where supposed to).
+    return bad_packets_.Value() == 0;
   }
 
  private:
-  webrtc::Atomic32 received_packets_;
-  webrtc::Atomic32 ok_packets_;
+  enum {
+    kPacketsExpected = 10,
+    kSleepIntervalMs = 10
+  };
   webrtc::scoped_ptr<webrtc::RtpHeaderParser> parser_;
+  webrtc::Atomic32 received_packets_;
+  webrtc::Atomic32 bad_packets_;
   int audio_level_id_;
   int absolute_sender_time_id_;
 };
 
-class SendRtpRtcpHeaderExtensionsTest : public AfterStreamingFixture {
+class SendRtpRtcpHeaderExtensionsTest : public BeforeStreamingFixture {
  protected:
   virtual void SetUp() {
-    PausePlaying();
     EXPECT_EQ(0, voe_network_->DeRegisterExternalTransport(channel_));
     EXPECT_EQ(0, voe_network_->RegisterExternalTransport(channel_,
                                                          verifying_transport_));
@@ -94,12 +103,25 @@ class SendRtpRtcpHeaderExtensionsTest : public AfterStreamingFixture {
   ExtensionVerifyTransport verifying_transport_;
 };
 
+TEST_F(SendRtpRtcpHeaderExtensionsTest, SentPacketsIncludeNoAudioLevel) {
+  verifying_transport_.SetAudioLevelId(0);
+  ResumePlaying();
+  EXPECT_FALSE(verifying_transport_.Wait());
+}
+
 TEST_F(SendRtpRtcpHeaderExtensionsTest, SentPacketsIncludeAudioLevel) {
   EXPECT_EQ(0, voe_rtp_rtcp_->SetSendAudioLevelIndicationStatus(channel_, true,
                                                                 9));
   verifying_transport_.SetAudioLevelId(9);
   ResumePlaying();
-  EXPECT_TRUE(verifying_transport_.WaitForNPackets(10));
+  EXPECT_TRUE(verifying_transport_.Wait());
+}
+
+TEST_F(SendRtpRtcpHeaderExtensionsTest, SentPacketsIncludeNoAbsoluteSenderTime)
+{
+  verifying_transport_.SetAbsoluteSenderTimeId(0);
+  ResumePlaying();
+  EXPECT_FALSE(verifying_transport_.Wait());
 }
 
 TEST_F(SendRtpRtcpHeaderExtensionsTest, SentPacketsIncludeAbsoluteSenderTime) {
@@ -107,7 +129,7 @@ TEST_F(SendRtpRtcpHeaderExtensionsTest, SentPacketsIncludeAbsoluteSenderTime) {
                                                               11));
   verifying_transport_.SetAbsoluteSenderTimeId(11);
   ResumePlaying();
-  EXPECT_TRUE(verifying_transport_.WaitForNPackets(10));
+  EXPECT_TRUE(verifying_transport_.Wait());
 }
 
 TEST_F(SendRtpRtcpHeaderExtensionsTest, SentPacketsIncludeAllExtensions1) {
@@ -118,7 +140,7 @@ TEST_F(SendRtpRtcpHeaderExtensionsTest, SentPacketsIncludeAllExtensions1) {
   verifying_transport_.SetAudioLevelId(9);
   verifying_transport_.SetAbsoluteSenderTimeId(11);
   ResumePlaying();
-  EXPECT_TRUE(verifying_transport_.WaitForNPackets(10));
+  EXPECT_TRUE(verifying_transport_.Wait());
 }
 
 TEST_F(SendRtpRtcpHeaderExtensionsTest, SentPacketsIncludeAllExtensions2) {
@@ -130,7 +152,7 @@ TEST_F(SendRtpRtcpHeaderExtensionsTest, SentPacketsIncludeAllExtensions2) {
   // Don't register audio level with header parser - unknown extensions should
   // be ignored when parsing.
   ResumePlaying();
-  EXPECT_TRUE(verifying_transport_.WaitForNPackets(10));
+  EXPECT_TRUE(verifying_transport_.Wait());
 }
 
 class MockViENetwork : public webrtc::ViENetwork {
@@ -150,10 +172,9 @@ class MockViENetwork : public webrtc::ViENetwork {
                                       const webrtc::RTPHeader&));
 };
 
-class ReceiveRtpRtcpHeaderExtensionsTest : public AfterStreamingFixture {
+class ReceiveRtpRtcpHeaderExtensionsTest : public BeforeStreamingFixture {
  protected:
   virtual void SetUp() {
-    PausePlaying();
     EXPECT_EQ(0,
         voe_rtp_rtcp_->SetSendAbsoluteSenderTimeStatus(channel_, true, 11));
     EXPECT_EQ(0,

webrtc/voice_engine/voice_engine.gyp

@@ -162,6 +162,8 @@
             'test/auto_test/fixtures/after_streaming_fixture.h',
             'test/auto_test/fixtures/before_initialization_fixture.cc',
             'test/auto_test/fixtures/before_initialization_fixture.h',
+            'test/auto_test/fixtures/before_streaming_fixture.cc',
+            'test/auto_test/fixtures/before_streaming_fixture.h',
             'test/auto_test/standard/audio_processing_test.cc',
             'test/auto_test/standard/codec_before_streaming_test.cc',
             'test/auto_test/standard/codec_test.cc',