From 57e060251af925c3c12693b7f8598c85c97aa9a4 Mon Sep 17 00:00:00 2001 From: "solenberg@webrtc.org" Date: Fri, 16 May 2014 11:27:09 +0000 Subject: [PATCH] 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 --- .../fixtures/after_streaming_fixture.cc | 60 +-------------- .../fixtures/after_streaming_fixture.h | 34 +-------- .../fixtures/before_streaming_fixture.cc | 75 +++++++++++++++++++ .../fixtures/before_streaming_fixture.h | 51 +++++++++++++ .../auto_test/standard/rtp_rtcp_extensions.cc | 63 ++++++++++------ webrtc/voice_engine/voice_engine.gyp | 2 + 6 files changed, 175 insertions(+), 110 deletions(-) create mode 100644 webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.cc create mode 100644 webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.h diff --git a/webrtc/voice_engine/test/auto_test/fixtures/after_streaming_fixture.cc b/webrtc/voice_engine/test/auto_test/fixtures/after_streaming_fixture.cc index 8cd489425..c688e4827 100644 --- a/webrtc/voice_engine/test/auto_test/fixtures/after_streaming_fixture.cc +++ b/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 - 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); } diff --git a/webrtc/voice_engine/test/auto_test/fixtures/after_streaming_fixture.h b/webrtc/voice_engine/test/auto_test/fixtures/after_streaming_fixture.h index f9980e113..f81716fab 100644 --- a/webrtc/voice_engine/test/auto_test/fixtures/after_streaming_fixture.h +++ b/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_ diff --git a/webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.cc b/webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.cc new file mode 100644 index 000000000..8a7c4c3b4 --- /dev/null +++ b/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); +} diff --git a/webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.h b/webrtc/voice_engine/test/auto_test/fixtures/before_streaming_fixture.h new file mode 100644 index 000000000..6de583c7e --- /dev/null +++ b/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 +#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_ diff --git a/webrtc/voice_engine/test/auto_test/standard/rtp_rtcp_extensions.cc b/webrtc/voice_engine/test/auto_test/standard/rtp_rtcp_extensions.cc index eaa7f50f0..cf33adf18 100644 --- a/webrtc/voice_engine/test/auto_test/standard/rtp_rtcp_extensions.cc +++ b/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(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 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, diff --git a/webrtc/voice_engine/voice_engine.gyp b/webrtc/voice_engine/voice_engine.gyp index a2de46df9..8e8b98f74 100644 --- a/webrtc/voice_engine/voice_engine.gyp +++ b/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',