Adding unittests to AudioConferenceMixer.

webrtc/modules/audio_conference_mixer/test/audio_conference_mixer_unittest.cc

+/*
+ *  Copyright (c) 2015 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/base/scoped_ptr.h"
+#include "webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer.h"
+#include "webrtc/modules/audio_conference_mixer/interface/audio_conference_mixer_defines.h"
+
+#include "testing/gmock/include/gmock/gmock.h"

[Andrew MacDonald, 2015/08/18 04:03:48]

This can go with the other includes.

+
+namespace webrtc {
+
+using testing::_;
+using testing::AtLeast;
+using testing::Invoke;
+using testing::Return;
+
+class MockAudioMixerOutputReceiver : public AudioMixerOutputReceiver {
+ public:
+  MOCK_METHOD4(NewMixedAudio, void(const int32_t id,
+                                   const AudioFrame& general_audio_frame,
+                                   const AudioFrame** unique_audio_frames,
+                                   const uint32_t size));
+};
+
+class MockMixerParticipant : public MixerParticipant {
+ public:
+  MockMixerParticipant() {
+    ON_CALL(*this, GetAudioFrame(_, _))
+        .WillByDefault(Invoke(this, &MockMixerParticipant::FakeAudioFrame));

[Andrew MacDonald, 2015/08/18 04:03:48]

If you always want this behavior, don't mock it, but "fake" it. Move
FakeAudioFrame into GetAudioFrame's implementation:

int32_t GetAudioFrame(const int32_t id, AudioFrame& audio_frame) override {
  audio_frame.CopyFrom(fake_frame_);
  return 0;
}

BTW, there are so many style violations in this stuff. Do you have any interest
in fixing them? Just to take this method, the interface should be:
int GetAudioFrame(int id, AudioFrame* audio_frame);

[minyue-webrtc, 2015/08/25 15:36:12]

The reason that I mock GetAudioFrame is that I want to check whether it is
called with EXPECT_CALL.
Yes, I'd like to do this. Maybe in a separate CL to be cleaner.

[ajm, 2015/08/25 16:46:03]

Ah OK. You can often define the methods actions directly via the mock, but looks
like needing to use AudioFrame::CopyFrom would make that difficult.
Yep, sounds good.

+  }
+  MOCK_METHOD2(GetAudioFrame,
+               int32_t(const int32_t id, AudioFrame& audio_frame));
+  MOCK_METHOD1(NeededFrequency, int32_t(const int32_t id));
+  AudioFrame fake_frame_;

[Andrew MacDonald, 2015/08/18 04:03:48]

This should be private with a setter.

[minyue-webrtc, 2015/08/25 15:36:12]

Yes, it can be nicer but the it is more complicated to change its value, you may
need to write more functions to modify its data value, VAD flag, etc.

[ajm, 2015/08/25 16:46:03]

The typical way to do this is:
AudioFrame* fake_frame() { return &fake_frame_; }

Since this is a test class, I won't insist that you change it, but it's odd to
see a public data member.

[minyue-webrtc, 2015/08/31 14:28:39]

I like the proposal, changes made accordingly.

+ private:
+  int32_t FakeAudioFrame(const int32_t id, AudioFrame& audio_frame) {
+    audio_frame.CopyFrom(fake_frame_);
+    return 0;
+  }
+};
+
+TEST(AudioConferenceMixer, AnonymousAndUnanonymous) {

[Andrew MacDonald, 2015/08/18 04:03:48]

Nonanonymous is a bit more natural (but also weird).

[minyue-webrtc, 2015/08/25 15:36:12]

How about "Named"?

[ajm, 2015/08/25 16:46:03]

Nice.

+  const int kId = 1;
+  // Should not matter even if partipants are more than
+  // kMaximumAmountOfMixedParticipants.
+  const int kUnanonymous =
+      AudioConferenceMixer::kMaximumAmountOfMixedParticipants + 1;
+  const int kAnonymous =
+      AudioConferenceMixer::kMaximumAmountOfMixedParticipants + 1;
+
+  rtc::scoped_ptr<AudioConferenceMixer> mixer;
+  mixer.reset(AudioConferenceMixer::Create(kId));

[Andrew MacDonald, 2015/08/18 04:03:48]

Don't reset, assign directly:
rtc::scoped_ptr<AudioConferenceMixer> mixer(AudioConferenceMixer::Create(kId));

[minyue-webrtc, 2015/08/25 15:36:12]

Done.

+
+  MockMixerParticipant unanonymous[kUnanonymous];
+  MockMixerParticipant anonymous[kAnonymous];

[Andrew MacDonald, 2015/08/18 04:03:48]

Use vectors.

[minyue-webrtc, 2015/08/25 15:36:12]

Done.

+
+  bool ret;
+  for (int i = 0; i < kUnanonymous; i++) {

[Andrew MacDonald, 2015/08/18 04:03:48]

nit: ++i and below

[minyue-webrtc, 2015/08/25 15:36:12]

Done.

+    EXPECT_EQ(0, mixer->SetMixabilityStatus(unanonymous[i], true));
+    EXPECT_EQ(0, mixer->MixabilityStatus(unanonymous[i], ret));

[Andrew MacDonald, 2015/08/18 04:03:48]

Is there any value in testing this with more than one participant?

[minyue-webrtc, 2015/08/25 15:36:12]

May be not very needed. But I made the number of "named" to be larger than
kMaximumAmountOfMixedParticipants (see line 55). To see that it won't be limited
by the kMaximumAmountOfMixedParticipants

+    EXPECT_TRUE(ret);
+  }
+
+  for (int i = 0; i < kAnonymous; i++) {

[Andrew MacDonald, 2015/08/18 04:03:48]

Same here, is this loop adding any value? And the other loops in this test.

[minyue-webrtc, 2015/08/25 15:36:12]

I made the number of "named" to be larger than kMaximumAmountOfMixedParticipants
(see line 55). To see that it won't be limited by the
kMaximumAmountOfMixedParticipants

+    // Participant must be registered before turning it into anonymous.
+    EXPECT_EQ(-1, mixer->SetAnonymousMixabilityStatus(anonymous[i], true));
+    EXPECT_EQ(0, mixer->SetMixabilityStatus(anonymous[i], true));
+    EXPECT_EQ(0, mixer->MixabilityStatus(anonymous[i], ret));
+    EXPECT_TRUE(ret);
+    EXPECT_EQ(0, mixer->AnonymousMixabilityStatus(anonymous[i], ret));
+    EXPECT_FALSE(ret);
+
+    EXPECT_EQ(0, mixer->SetAnonymousMixabilityStatus(anonymous[i], true));
+    EXPECT_EQ(0, mixer->AnonymousMixabilityStatus(anonymous[i], ret));
+    EXPECT_TRUE(ret);
+
+    // Anonymous participants do not show status by MixabilityStatus.
+    EXPECT_EQ(0, mixer->MixabilityStatus(anonymous[i], ret));
+    EXPECT_FALSE(ret);
+  }
+
+  for (int i = 0; i < kUnanonymous; i++) {
+    EXPECT_EQ(0, mixer->SetMixabilityStatus(unanonymous[i], false));
+    EXPECT_EQ(0, mixer->MixabilityStatus(unanonymous[i], ret));
+    EXPECT_FALSE(ret);
+  }
+
+  for (int i = 0; i < kAnonymous - 1; i++) {
+    EXPECT_EQ(0, mixer->SetAnonymousMixabilityStatus(anonymous[i], false));
+    EXPECT_EQ(0, mixer->AnonymousMixabilityStatus(anonymous[i], ret));
+    EXPECT_FALSE(ret);
+    // SetAnonymousMixabilityStatus(anonymous, false) moves anonymous to the
+    // unanonymous group.
+    EXPECT_EQ(0, mixer->MixabilityStatus(anonymous[i], ret));
+    EXPECT_TRUE(ret);
+  }
+
+  // SetMixabilityStatus(anonymous, false) will remove anonymous from both
+  // anonymous and unanonymous groups.
+  EXPECT_EQ(0, mixer->SetMixabilityStatus(anonymous[kAnonymous - 1], false));
+  EXPECT_EQ(0, mixer->AnonymousMixabilityStatus(anonymous[kAnonymous - 1],
+                                                ret));
+  EXPECT_FALSE(ret);
+  EXPECT_EQ(0, mixer->MixabilityStatus(anonymous[kAnonymous - 1], ret));
+  EXPECT_FALSE(ret);
+}
+
+TEST(AudioConferenceMixer, LargestEnergyVadActiveMixed) {
+  const int kId = 1;
+  const int kParticipants =
+      AudioConferenceMixer::kMaximumAmountOfMixedParticipants + 3;
+
+  rtc::scoped_ptr<AudioConferenceMixer> mixer;
+  mixer.reset(AudioConferenceMixer::Create(kId));

[Andrew MacDonald, 2015/08/18 04:03:48]

As above, reset is not needed.

[minyue-webrtc, 2015/08/25 15:36:12]

Done.

+
+  MockAudioMixerOutputReceiver output_receiver;
+  EXPECT_EQ(0, mixer->RegisterMixedStreamCallback(output_receiver));
+
+  MockMixerParticipant participants[kParticipants];

[Andrew MacDonald, 2015/08/18 04:03:48]

vector

[minyue-webrtc, 2015/08/25 15:36:12]

Done.

+
+  for (int i = 0; i < kParticipants; i++) {
+    participants[i].fake_frame_.UpdateFrame(i, 0, nullptr, 320, 32000,

[Andrew MacDonald, 2015/08/18 04:03:48]

This is a rarely used method. Could you just set the members you want to change
directly?

[minyue-webrtc, 2015/08/25 15:36:12]

I searched for its usage, and found that it is not very rarely used. I also feel
that it fits here nicely, otherwise, one needs to understand the default value
and modify one-by-one, making it more lines and less readible.

[ajm, 2015/08/25 16:46:03]

I'd argue that this is _less_ readable since you have a bunch of literals
assigned to who-knows-what.

Two suggestions:
- Set these default values in the MockMixerParticipant constructor.
- Replace 320 and 32000 literals with constants to be used anywhere else they're
needed (e.g. line 139).

[minyue-webrtc, 2015/08/31 14:28:39]

I'd take your initial suggestion.

+                                            AudioFrame::kNormalSpeech,
+                                            AudioFrame::kVadActive, 1, 0);
+    // Use 80 to escape from ramped-in window.

[Andrew MacDonald, 2015/08/18 04:03:48]

Do you mean that you need to choose a late enough sample to avoid ramping? I
think you should elaborate a bit.

[minyue-webrtc, 2015/08/25 15:36:12]

Done.

+    participants[i].fake_frame_.data_[80] = i;
+
+    EXPECT_EQ(0, mixer->SetMixabilityStatus(participants[i], true));
+    EXPECT_CALL(participants[i], GetAudioFrame(_, _))
+        .Times(AtLeast(1));
+    EXPECT_CALL(participants[i], NeededFrequency(_))
+        .WillRepeatedly(Return(32000));
+  }
+
+  // Last participant gives audio frame with passive VAD, although it has the
+  // largest energy.
+  participants[kParticipants - 1].fake_frame_.vad_activity_ =
+      AudioFrame::kVadPassive;
+
+  EXPECT_CALL(output_receiver, NewMixedAudio(_, _, _, _))
+      .Times(AtLeast(1));
+
+  EXPECT_EQ(0, mixer->Process());
+
+  bool ret;

[Andrew MacDonald, 2015/08/18 04:03:48]

Instead of ret, name this what it is: is_mixed.

[minyue-webrtc, 2015/08/25 15:36:12]

Done.

+  for (int i = 0; i < kParticipants; i++) {
+    EXPECT_EQ(0, participants[i].IsMixed(ret));
+    if (i == kParticipants - 1 || i < kParticipants - 1 -
+        AudioConferenceMixer::kMaximumAmountOfMixedParticipants) {
+      EXPECT_FALSE(ret) << "Mixing status of Participant #" << i << " wrong.";
+    } else {
+      EXPECT_TRUE(ret) << "Mixing status of Participant #" << i << " wrong.";
+    }
+  }
+
+  EXPECT_EQ(0, mixer->UnRegisterMixedStreamCallback());
+}
+
+}  // namespace webrtc