From 919dc2e843ed4d0e276a88ef5b4abda1a51c2f3f Mon Sep 17 00:00:00 2001 From: henrika Date: Thu, 12 Oct 2017 14:24:55 +0200 Subject: [PATCH] Removes fallback from Linux PulseAudio to ALSA. Unit test now checks that ADM:Init() works before any test runs. It means that all tests will be skipped on bots that lack Pulse support which is as how it worked before this CL as well. But then, we detected the lack of support by checking that the audio layer had changed from Pulse to Alsa. As a consequence, I also decided to inject fake/mock ADMs in more unit tests. One was actually already injected for other reasons (see https://codereview.webrtc.org/2997383002/) but it had accidentally been "reverted" later in combination with other changes. To summarize: before this change we had a set of unit tests where real audio was tested but it was not the intention of the test or required. In addition, some Linux bots (VM:s) did not support PulseAudio and on them the tests relied on a fallback mechanism to ALSA to work, i.e., we had a rather complex dependency on hardware. This dependency has now been removed and it should result in more stable tests. Bug: webrtc:7306, webrtc:7806 Change-Id: Ia0485658c04a4ef3b3f2dc0d557d73738067304b Reviewed-on: https://webrtc-review.googlesource.com/8640 Commit-Queue: Henrik Andreassson Reviewed-by: Fredrik Solenberg Cr-Commit-Position: refs/heads/master@{#20307} --- media/engine/nullwebrtcvideoengine_unittest.cc | 4 +++- media/engine/webrtcvoiceengine_unittest.cc | 15 ++++++++++----- modules/audio_device/audio_device_impl.cc | 15 +-------------- modules/audio_device/audio_device_unittest.cc | 10 +++++++--- pc/peerconnectionfactory_unittest.cc | 14 +++++++++++--- pc/peerconnectioninterface_unittest.cc | 7 +++++-- 6 files changed, 37 insertions(+), 28 deletions(-) diff --git a/media/engine/nullwebrtcvideoengine_unittest.cc b/media/engine/nullwebrtcvideoengine_unittest.cc index 84f330a9e7..a53f773fec 100644 --- a/media/engine/nullwebrtcvideoengine_unittest.cc +++ b/media/engine/nullwebrtcvideoengine_unittest.cc @@ -10,6 +10,7 @@ #include "media/engine/nullwebrtcvideoengine.h" #include "media/engine/webrtcvoiceengine.h" +#include "modules/audio_device/include/mock_audio_device.h" #include "modules/audio_processing/include/audio_processing.h" #include "test/gtest.h" #include "test/mock_audio_decoder_factory.h" @@ -38,8 +39,9 @@ class WebRtcMediaEngineNullVideo // Simple test to check if NullWebRtcVideoEngine implements the methods // required by CompositeMediaEngine. TEST(NullWebRtcVideoEngineTest, CheckInterface) { + testing::NiceMock adm; WebRtcMediaEngineNullVideo engine( - nullptr, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::MockAudioDecoderFactory::CreateUnusedFactory()); EXPECT_TRUE(engine.Init()); } diff --git a/media/engine/webrtcvoiceengine_unittest.cc b/media/engine/webrtcvoiceengine_unittest.cc index cf155e5869..1c81f33cbd 100644 --- a/media/engine/webrtcvoiceengine_unittest.cc +++ b/media/engine/webrtcvoiceengine_unittest.cc @@ -3321,10 +3321,11 @@ TEST_F(WebRtcVoiceEngineTestFake, PreservePlayoutWhenRecreateRecvStream) { TEST(WebRtcVoiceEngineTest, StartupShutdown) { // If the VoiceEngine wants to gather available codecs early, that's fine but // we never want it to create a decoder at this stage. + testing::NiceMock adm; rtc::scoped_refptr apm = webrtc::AudioProcessing::Create(); cricket::WebRtcVoiceEngine engine( - nullptr, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::MockAudioDecoderFactory::CreateUnusedFactory(), nullptr, apm); engine.Init(); webrtc::RtcEventLogNullImpl event_log; @@ -3362,10 +3363,11 @@ TEST(WebRtcVoiceEngineTest, StartupShutdownWithExternalADM) { TEST(WebRtcVoiceEngineTest, HasCorrectPayloadTypeMapping) { // TODO(ossu): Why are the payload types of codecs with non-static payload // type assignments checked here? It shouldn't really matter. + testing::NiceMock adm; rtc::scoped_refptr apm = webrtc::AudioProcessing::Create(); cricket::WebRtcVoiceEngine engine( - nullptr, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::MockAudioDecoderFactory::CreateUnusedFactory(), nullptr, apm); engine.Init(); for (const cricket::AudioCodec& codec : engine.send_codecs()) { @@ -3406,10 +3408,11 @@ TEST(WebRtcVoiceEngineTest, HasCorrectPayloadTypeMapping) { // Tests that VoE supports at least 32 channels TEST(WebRtcVoiceEngineTest, Has32Channels) { + testing::NiceMock adm; rtc::scoped_refptr apm = webrtc::AudioProcessing::Create(); cricket::WebRtcVoiceEngine engine( - nullptr, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::MockAudioDecoderFactory::CreateUnusedFactory(), nullptr, apm); engine.Init(); webrtc::RtcEventLogNullImpl event_log; @@ -3443,10 +3446,11 @@ TEST(WebRtcVoiceEngineTest, SetRecvCodecs) { // what we sent in - though it's probably reasonable to expect so, if // SetRecvParameters returns true. // I think it will become clear once audio decoder injection is completed. + testing::NiceMock adm; rtc::scoped_refptr apm = webrtc::AudioProcessing::Create(); cricket::WebRtcVoiceEngine engine( - nullptr, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::CreateBuiltinAudioDecoderFactory(), nullptr, apm); engine.Init(); webrtc::RtcEventLogNullImpl event_log; @@ -3483,10 +3487,11 @@ TEST(WebRtcVoiceEngineTest, CollectRecvCodecs) { new rtc::RefCountedObject; EXPECT_CALL(*mock_decoder_factory.get(), GetSupportedDecoders()) .WillOnce(Return(specs)); + testing::NiceMock adm; rtc::scoped_refptr apm = webrtc::AudioProcessing::Create(); - cricket::WebRtcVoiceEngine engine(nullptr, unused_encoder_factory, + cricket::WebRtcVoiceEngine engine(&adm, unused_encoder_factory, mock_decoder_factory, nullptr, apm); engine.Init(); auto codecs = engine.recv_codecs(); diff --git a/modules/audio_device/audio_device_impl.cc b/modules/audio_device/audio_device_impl.cc index 2cfae0a569..ccbfb18679 100644 --- a/modules/audio_device/audio_device_impl.cc +++ b/modules/audio_device/audio_device_impl.cc @@ -208,22 +208,9 @@ int32_t AudioDeviceModuleImpl::CreatePlatformSpecificObjects() { LOG(INFO) << "Attempting to use Linux PulseAudio APIs..."; // Linux PulseAudio implementation. audio_device_.reset(new AudioDeviceLinuxPulse()); - if (audio_device_->Init() == AudioDeviceGeneric::InitStatus::OK) { - LOG(INFO) << "Linux PulseAudio APIs will be utilized"; - } else { - LOG(WARNING) << "Failed to initialize Linux PulseAudio " - "implementation."; - audio_device_.reset(nullptr); -#endif -#if defined(LINUX_ALSA) - // Revert to Linux ALSA implementation instead. - audio_device_.reset(new AudioDeviceLinuxALSA()); - audio_layer_ = kLinuxAlsaAudio; - LOG(WARNING) << "Linux PulseAudio is not supported => ALSA APIs will " - "be utilized instead."; + LOG(INFO) << "Linux PulseAudio APIs will be utilized"; #endif #if defined(LINUX_PULSE) - } #endif } else if (audio_layer == kLinuxAlsaAudio) { #if defined(LINUX_ALSA) diff --git a/modules/audio_device/audio_device_unittest.cc b/modules/audio_device/audio_device_unittest.cc index e1f0c1564e..5af91922e7 100644 --- a/modules/audio_device/audio_device_unittest.cc +++ b/modules/audio_device/audio_device_unittest.cc @@ -466,12 +466,16 @@ class AudioDeviceTest : public ::testing::Test { AudioDeviceModule::AudioLayer audio_layer; int got_platform_audio_layer = audio_device_->ActiveAudioLayer(&audio_layer); - if (got_platform_audio_layer != 0 || - audio_layer == AudioDeviceModule::kLinuxAlsaAudio) { + // First, ensure that a valid audio layer can be activated. + if (got_platform_audio_layer != 0) { requirements_satisfied_ = false; } + // Next, verify that the ADM can be initialized. + if (requirements_satisfied_) { + requirements_satisfied_ = (audio_device_->Init() == 0); + } + // Finally, ensure that at least one valid device exists in each direction. if (requirements_satisfied_) { - EXPECT_EQ(0, audio_device_->Init()); const int16_t num_playout_devices = audio_device_->PlayoutDevices(); const int16_t num_record_devices = audio_device_->RecordingDevices(); requirements_satisfied_ = diff --git a/pc/peerconnectionfactory_unittest.cc b/pc/peerconnectionfactory_unittest.cc index 5688d1540b..59f6cde707 100644 --- a/pc/peerconnectionfactory_unittest.cc +++ b/pc/peerconnectionfactory_unittest.cc @@ -16,6 +16,7 @@ #include "media/base/fakevideocapturer.h" #include "p2p/base/fakeportallocator.h" #include "pc/peerconnectionfactory.h" +#include "pc/test/fakeaudiocapturemodule.h" #include "rtc_base/gunit.h" #ifdef WEBRTC_ANDROID @@ -85,9 +86,12 @@ class PeerConnectionFactoryTest : public testing::Test { #ifdef WEBRTC_ANDROID webrtc::InitializeAndroidObjects(); #endif + // Use fake audio device module since we're only testing the interface + // level, and using a real one could make tests flaky e.g. when run in + // parallel. factory_ = webrtc::CreatePeerConnectionFactory( - rtc::Thread::Current(), rtc::Thread::Current(), rtc::Thread::Current(), - nullptr, nullptr, nullptr); + rtc::Thread::Current(), rtc::Thread::Current(), + FakeAudioCaptureModule::Create(), nullptr, nullptr); ASSERT_TRUE(factory_.get() != NULL); port_allocator_.reset( @@ -127,7 +131,11 @@ class PeerConnectionFactoryTest : public testing::Test { // Verify creation of PeerConnection using internal ADM, video factory and // internal libjingle threads. -TEST(PeerConnectionFactoryTestInternal, CreatePCUsingInternalModules) { +// TODO(henrika): disabling this test since relying on real audio can result in +// flaky tests and focus on details that are out of scope for you might expect +// for a PeerConnectionFactory unit test. +// See https://bugs.chromium.org/p/webrtc/issues/detail?id=7806 for details. +TEST(PeerConnectionFactoryTestInternal, DISABLED_CreatePCUsingInternalModules) { #ifdef WEBRTC_ANDROID webrtc::InitializeAndroidObjects(); #endif diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc index 7a275e5b89..084126b2ee 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -544,10 +544,13 @@ class PeerConnectionFactoryForTest : public webrtc::PeerConnectionFactory { auto audio_encoder_factory = webrtc::CreateBuiltinAudioEncoderFactory(); auto audio_decoder_factory = webrtc::CreateBuiltinAudioDecoderFactory(); + // Use fake audio device module since we're only testing the interface + // level, and using a real one could make tests flaky when run in parallel. auto media_engine = std::unique_ptr( cricket::WebRtcMediaEngineFactory::Create( - nullptr, audio_encoder_factory, audio_decoder_factory, nullptr, - nullptr, nullptr, webrtc::AudioProcessing::Create())); + FakeAudioCaptureModule::Create(), audio_encoder_factory, + audio_decoder_factory, nullptr, nullptr, nullptr, + webrtc::AudioProcessing::Create())); std::unique_ptr call_factory = webrtc::CreateCallFactory();