Avoid that previous settings in APM are overwritten by WebRtcVoiceEngine

webrtc/media/engine/webrtcvoiceengine_unittest.cc

 /*
  *  Copyright (c) 2008 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.
  */
 
@@ skipping 16 matching lines @@
 #include "webrtc/rtc_base/arraysize.h"
 #include "webrtc/rtc_base/byteorder.h"
 #include "webrtc/rtc_base/safe_conversions.h"
 #include "webrtc/rtc_base/scoped_ref_ptr.h"
 #include "webrtc/test/field_trial.h"
 #include "webrtc/test/gtest.h"
 #include "webrtc/test/mock_audio_decoder_factory.h"
 #include "webrtc/test/mock_audio_encoder_factory.h"
 #include "webrtc/voice_engine/transmit_mixer.h"
 
+using testing::_;
 using testing::ContainerEq;
 using testing::Return;
+using testing::ReturnPointee;
+using testing::SaveArg;
 using testing::StrictMock;
 
 namespace {
 
 constexpr uint32_t kMaxUnsignaledRecvStreams = 4;
 
 const cricket::AudioCodec kPcmuCodec(0, "PCMU", 8000, 64000, 1);
 const cricket::AudioCodec kIsacCodec(103, "ISAC", 16000, 32000, 1);
 const cricket::AudioCodec kOpusCodec(111, "opus", 48000, 32000, 2);
 const cricket::AudioCodec kG722CodecVoE(9, "G722", 16000, 64000, 1);
@@ skipping 69 matching lines @@
 }
 }  // namespace
 
 // Tests that our stub library "works".
 TEST(WebRtcVoiceEngineTestStubLibrary, StartupShutdown) {
   StrictMock<webrtc::test::MockAudioDeviceModule> adm;
   AdmSetupExpectations(&adm);
   rtc::scoped_refptr<StrictMock<webrtc::test::MockAudioProcessing>> apm =
       new rtc::RefCountedObject<
           StrictMock<webrtc::test::MockAudioProcessing>>();
-  EXPECT_CALL(*apm, ApplyConfig(testing::_));
+  webrtc::AudioProcessing::Config apm_config;
+  EXPECT_CALL(*apm, GetConfig()).WillRepeatedly(ReturnPointee(&apm_config));
+  EXPECT_CALL(*apm, ApplyConfig(_)).WillRepeatedly(SaveArg<0>(&apm_config));
   EXPECT_CALL(*apm, SetExtraOptions(testing::_));
   EXPECT_CALL(*apm, Initialize()).WillOnce(Return(0));
   EXPECT_CALL(*apm, DetachAecDump());
   StrictMock<MockTransmitMixer> transmit_mixer;
   EXPECT_CALL(transmit_mixer, EnableStereoChannelSwapping(false));
   cricket::FakeWebRtcVoiceEngine voe(&transmit_mixer);
   EXPECT_FALSE(voe.IsInited());
   {
     cricket::WebRtcVoiceEngine engine(
         &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(),
@@ skipping 24 matching lines @@
         apm_gc_(*apm_->gain_control()),
         apm_ec_(*apm_->echo_cancellation()),
         apm_ns_(*apm_->noise_suppression()),
         apm_vd_(*apm_->voice_detection()),
         call_(webrtc::Call::Config(&event_log_)),
         voe_(&transmit_mixer_),
         override_field_trials_(field_trials) {
     // AudioDeviceModule.
     AdmSetupExpectations(&adm_);
     // AudioProcessing.
-    EXPECT_CALL(*apm_, ApplyConfig(testing::_));
+    EXPECT_CALL(*apm_, GetConfig()).WillRepeatedly(ReturnPointee(&apm_config_));
+    EXPECT_CALL(*apm_, ApplyConfig(_)).WillRepeatedly(SaveArg<0>(&apm_config_));
     EXPECT_CALL(*apm_, SetExtraOptions(testing::_));
     EXPECT_CALL(*apm_, Initialize()).WillOnce(Return(0));
     EXPECT_CALL(*apm_, DetachAecDump());
     // Default Options.
     EXPECT_CALL(apm_ec_, Enable(true)).WillOnce(Return(0));
     EXPECT_CALL(apm_ec_, enable_metrics(true)).WillOnce(Return(0));
     EXPECT_CALL(apm_gc_, Enable(true)).WillOnce(Return(0));
     EXPECT_CALL(apm_ns_, Enable(true)).WillOnce(Return(0));
     EXPECT_CALL(apm_vd_, Enable(true)).WillOnce(Return(0));
     EXPECT_CALL(transmit_mixer_, EnableStereoChannelSwapping(false));
@@ skipping 13 matching lines @@
                                                  decoder_factory, nullptr, apm_,
                                                  new FakeVoEWrapper(&voe_)));
     engine_->Init();
     send_parameters_.codecs.push_back(kPcmuCodec);
     recv_parameters_.codecs.push_back(kPcmuCodec);
     // Default Options.
     EXPECT_TRUE(IsHighPassFilterEnabled());
   }
 
   bool SetupChannel() {
-    EXPECT_CALL(*apm_, ApplyConfig(testing::_));

[peah-webrtc, 2017/07/25 21:40:21]

This EXPECT_CALL is actually redundant (and thereby misleading), as it is also
placed in the constructor. I removed it as the new testing code otherwise would
have required this redundant code to be extended (in a redundant manner).

     EXPECT_CALL(*apm_, SetExtraOptions(testing::_));
     channel_ = engine_->CreateChannel(&call_, cricket::MediaConfig(),
                                       cricket::AudioOptions());
     return (channel_ != nullptr);
   }
 
   bool SetupRecvStream() {
     if (!SetupChannel()) {
       return false;
     }
@@ skipping 53 matching lines @@
   const webrtc::AudioReceiveStream::Config& GetRecvStreamConfig(uint32_t ssrc) {
     return GetRecvStream(ssrc).GetConfig();
   }
 
   void SetSend(bool enable) {
     ASSERT_TRUE(channel_);
     if (enable) {
       EXPECT_CALL(adm_, RecordingIsInitialized()).WillOnce(Return(false));
       EXPECT_CALL(adm_, Recording()).WillOnce(Return(false));
       EXPECT_CALL(adm_, InitRecording()).WillOnce(Return(0));
-      EXPECT_CALL(*apm_, ApplyConfig(testing::_));

[peah-webrtc, 2017/07/25 21:40:21]

Same comment as above regarding redundant code.

       EXPECT_CALL(*apm_, SetExtraOptions(testing::_));
     }
     channel_->SetSend(enable);
   }
 
   void SetSendParameters(const cricket::AudioSendParameters& params) {
-    EXPECT_CALL(*apm_, ApplyConfig(testing::_));

[peah-webrtc, 2017/07/25 21:40:21]

Same comment as above regarding redundant code.

     EXPECT_CALL(*apm_, SetExtraOptions(testing::_));
     ASSERT_TRUE(channel_);
     EXPECT_TRUE(channel_->SetSendParameters(params));
   }
 
   void SetAudioSend(uint32_t ssrc, bool enable, cricket::AudioSource* source,
                     const cricket::AudioOptions* options = nullptr) {
     EXPECT_CALL(*apm_, set_output_will_be_muted(!enable));
     ASSERT_TRUE(channel_);
     if (enable && options) {
-      EXPECT_CALL(*apm_, ApplyConfig(testing::_));

[peah-webrtc, 2017/07/25 21:40:21]

Same comment as above regarding redundant code.

       EXPECT_CALL(*apm_, SetExtraOptions(testing::_));
     }
     EXPECT_TRUE(channel_->SetAudioSend(ssrc, enable, options, source));
   }
 
   void TestInsertDtmf(uint32_t ssrc, bool caller,
                       const cricket::AudioCodec& codec) {
     EXPECT_TRUE(SetupChannel());
     if (caller) {
       // If this is a caller, local description will be applied and add the
@@ skipping 344 matching lines @@
     }
     EXPECT_EQ(recv_parameters_.codecs.size(), info.receive_codecs.size());
     for (const cricket::AudioCodec& codec : recv_parameters_.codecs) {
       ASSERT_EQ(info.receive_codecs.count(codec.id), 1U);
       EXPECT_EQ(info.receive_codecs.find(codec.id)->second,
                 codec.ToCodecParameters());
     }
   }
 
   bool IsHighPassFilterEnabled() {
     return engine_->GetApmConfigForTest().high_pass_filter.enabled;

[Taylor Brandstetter, 2017/07/25 22:24:00]

nit: Can this now just use "apm_config_" directly, removing GetApmConfigForTest?
Getting rid of "for test" methods makes me happy.

[peah-webrtc, 2017/07/25 22:38:29]

That sounds great!
I'll do it in another CL though. I created an issue for it.

   }
 
  protected:
   StrictMock<webrtc::test::MockAudioDeviceModule> adm_;
   rtc::scoped_refptr<StrictMock<webrtc::test::MockAudioProcessing>> apm_;
   webrtc::test::MockGainControl& apm_gc_;
   webrtc::test::MockEchoCancellation& apm_ec_;
   webrtc::test::MockNoiseSuppression& apm_ns_;
   webrtc::test::MockVoiceDetection& apm_vd_;
   StrictMock<MockTransmitMixer> transmit_mixer_;
   webrtc::RtcEventLogNullImpl event_log_;
   cricket::FakeCall call_;
   cricket::FakeWebRtcVoiceEngine voe_;
   std::unique_ptr<cricket::WebRtcVoiceEngine> engine_;
   cricket::VoiceMediaChannel* channel_ = nullptr;
   cricket::AudioSendParameters send_parameters_;
   cricket::AudioRecvParameters recv_parameters_;
   FakeAudioSource fake_source_;
+  webrtc::AudioProcessing::Config apm_config_;
+
  private:
   webrtc::test::ScopedFieldTrials override_field_trials_;
 };
 
 // Tests that we can create and destroy a channel.
 TEST_F(WebRtcVoiceEngineTestFake, CreateChannel) {
   EXPECT_TRUE(SetupChannel());
 }
 
 // Test that we can add a send stream and that it has the correct defaults.
@@ skipping 2102 matching lines @@
   EXPECT_CALL(adm_,
               BuiltInAECIsAvailable()).Times(8).WillRepeatedly(Return(false));
   EXPECT_CALL(adm_,
               BuiltInAGCIsAvailable()).Times(8).WillRepeatedly(Return(false));
   EXPECT_CALL(adm_,
               BuiltInNSIsAvailable()).Times(8).WillRepeatedly(Return(false));
   EXPECT_CALL(adm_,
               RecordingIsInitialized()).Times(2).WillRepeatedly(Return(false));
   EXPECT_CALL(adm_, Recording()).Times(2).WillRepeatedly(Return(false));
   EXPECT_CALL(adm_, InitRecording()).Times(2).WillRepeatedly(Return(0));
-  EXPECT_CALL(*apm_, ApplyConfig(testing::_)).Times(10);
+  webrtc::AudioProcessing::Config apm_config;
+  EXPECT_CALL(*apm_, GetConfig())
+      .Times(10)

[Taylor Brandstetter, 2017/07/25 22:23:59]

nit: I think this should be a "WillRepeatedly"; if WebRtcVoiceEngine is
refactored such that it ends up calling GetConfig twice per cycle for some
reason, this test shouldn't fail.

[peah-webrtc, 2017/07/25 22:38:29]

I partly (or mostly) agree. I think that a main part of the mock usage in this
test is to hardcode the behavior of webrtcvoiceengine by specifying the number
of calls. I like the aspect of this which is like a soft bitexactness test, but
I don't at all like that there is apparent logic in the test to the number of
methods calls that is specified.

As the rest of the code in these tests is like this, I'll keep it as it is.

+      .WillRepeatedly(ReturnPointee(&apm_config));
+  EXPECT_CALL(*apm_, ApplyConfig(_))
+      .Times(10)
+      .WillRepeatedly(SaveArg<0>(&apm_config));
   EXPECT_CALL(*apm_, SetExtraOptions(testing::_)).Times(10);
 
   std::unique_ptr<cricket::WebRtcVoiceMediaChannel> channel1(
       static_cast<cricket::WebRtcVoiceMediaChannel*>(engine_->CreateChannel(
           &call_, cricket::MediaConfig(), cricket::AudioOptions())));
   std::unique_ptr<cricket::WebRtcVoiceMediaChannel> channel2(
       static_cast<cricket::WebRtcVoiceMediaChannel*>(engine_->CreateChannel(
           &call_, cricket::MediaConfig(), cricket::AudioOptions())));
 
   // Have to add a stream to make SetSend work.
@@ skipping 90 matching lines @@
   EXPECT_EQ(expected_options, channel2->options());
 }
 
 // This test verifies DSCP settings are properly applied on voice media channel.
 TEST_F(WebRtcVoiceEngineTestFake, TestSetDscpOptions) {
   EXPECT_TRUE(SetupSendStream());
   cricket::FakeNetworkInterface network_interface;
   cricket::MediaConfig config;
   std::unique_ptr<cricket::VoiceMediaChannel> channel;
 
-  EXPECT_CALL(*apm_, ApplyConfig(testing::_)).Times(3);
+  webrtc::AudioProcessing::Config apm_config;
+  EXPECT_CALL(*apm_, GetConfig())
+      .Times(3)

[Taylor Brandstetter, 2017/07/25 22:23:59]

Same comment here.

[peah-webrtc, 2017/07/25 22:38:29]

I agree, but as described above I'll keep it as it is for now at least.

+      .WillRepeatedly(ReturnPointee(&apm_config));
+  EXPECT_CALL(*apm_, ApplyConfig(_))
+      .Times(3)
+      .WillRepeatedly(SaveArg<0>(&apm_config));
   EXPECT_CALL(*apm_, SetExtraOptions(testing::_)).Times(3);
 
   channel.reset(
       engine_->CreateChannel(&call_, config, cricket::AudioOptions()));
   channel->SetInterface(&network_interface);
   // Default value when DSCP is disabled should be DSCP_DEFAULT.
   EXPECT_EQ(rtc::DSCP_DEFAULT, network_interface.dscp());
 
   config.enable_dscp = true;
   channel.reset(
@@ skipping 555 matching lines @@
   // Without this cast, the comparison turned unsigned and, thus, failed for -1.
   const int num_specs = static_cast<int>(specs.size());
   EXPECT_GE(find_codec({"cn", 8000, 1}), num_specs);
   EXPECT_GE(find_codec({"cn", 16000, 1}), num_specs);
   EXPECT_EQ(find_codec({"cn", 32000, 1}), -1);
   EXPECT_GE(find_codec({"telephone-event", 8000, 1}), num_specs);
   EXPECT_GE(find_codec({"telephone-event", 16000, 1}), num_specs);
   EXPECT_GE(find_codec({"telephone-event", 32000, 1}), num_specs);
   EXPECT_GE(find_codec({"telephone-event", 48000, 1}), num_specs);
 }