Implement AudioReceiveStream::GetStats().

talk/media/webrtc/webrtcvoiceengine_unittest.cc

Index: talk/media/webrtc/webrtcvoiceengine_unittest.cc
diff --git a/talk/media/webrtc/webrtcvoiceengine_unittest.cc b/talk/media/webrtc/webrtcvoiceengine_unittest.cc
index b0fc2bb8d7b4c4c8a0316c5dfcfc1bd5c0e797a7..ac6e49ee3c405162c38533e8582ee4abeee512d7 100644
--- a/talk/media/webrtc/webrtcvoiceengine_unittest.cc
+++ b/talk/media/webrtc/webrtcvoiceengine_unittest.cc
@@ -38,27 +38,27 @@
 #include "webrtc/p2p/base/faketransportcontroller.h"
 #include "talk/session/media/channel.h"
 
-// Tests for the WebRtcVoiceEngine/VoiceChannel code.
-
 using cricket::kRtpAudioLevelHeaderExtension;
 using cricket::kRtpAbsoluteSenderTimeHeaderExtension;
 
-static const cricket::AudioCodec kPcmuCodec(0, "PCMU", 8000, 64000, 1, 0);
-static const cricket::AudioCodec kIsacCodec(103, "ISAC", 16000, 32000, 1, 0);
-static const cricket::AudioCodec kOpusCodec(111, "opus", 48000, 64000, 2, 0);
-static const cricket::AudioCodec kG722CodecVoE(9, "G722", 16000, 64000, 1, 0);
-static const cricket::AudioCodec kG722CodecSdp(9, "G722", 8000, 64000, 1, 0);
-static const cricket::AudioCodec kRedCodec(117, "red", 8000, 0, 1, 0);
-static const cricket::AudioCodec kCn8000Codec(13, "CN", 8000, 0, 1, 0);
-static const cricket::AudioCodec kCn16000Codec(105, "CN", 16000, 0, 1, 0);
-static const cricket::AudioCodec
-    kTelephoneEventCodec(106, "telephone-event", 8000, 0, 1, 0);
-static const cricket::AudioCodec* const kAudioCodecs[] = {
+namespace {
+
+const cricket::AudioCodec kPcmuCodec(0, "PCMU", 8000, 64000, 1, 0);
+const cricket::AudioCodec kIsacCodec(103, "ISAC", 16000, 32000, 1, 0);
+const cricket::AudioCodec kOpusCodec(111, "opus", 48000, 64000, 2, 0);
+const cricket::AudioCodec kG722CodecVoE(9, "G722", 16000, 64000, 1, 0);
+const cricket::AudioCodec kG722CodecSdp(9, "G722", 8000, 64000, 1, 0);
+const cricket::AudioCodec kRedCodec(117, "red", 8000, 0, 1, 0);
+const cricket::AudioCodec kCn8000Codec(13, "CN", 8000, 0, 1, 0);
+const cricket::AudioCodec kCn16000Codec(105, "CN", 16000, 0, 1, 0);
+const cricket::AudioCodec kTelephoneEventCodec(106, "telephone-event", 8000, 0,
+                                               1, 0);
+const cricket::AudioCodec* const kAudioCodecs[] = {
     &kPcmuCodec, &kIsacCodec, &kOpusCodec, &kG722CodecVoE, &kRedCodec,
     &kCn8000Codec, &kCn16000Codec, &kTelephoneEventCodec,
 };
-static uint32_t kSsrc1 = 0x99;
-static uint32_t kSsrc2 = 0x98;
+const uint32_t kSsrc1 = 0x99;
+const uint32_t kSsrc2 = 0x98;
 
 class FakeVoEWrapper : public cricket::VoEWrapper {
  public:
@@ -68,10 +68,8 @@ class FakeVoEWrapper : public cricket::VoEWrapper {
                             engine,  // codec
                             engine,  // dtmf
                             engine,  // hw
-                            engine,  // neteq
                             engine,  // network
                             engine,  // rtp
-                            engine,  // sync
                             engine) {  // volume
   }
 };
@@ -86,6 +84,7 @@ class FakeVoETraceWrapper : public cricket::VoETraceWrapper {
   int SetTraceCallback(webrtc::TraceCallback* callback) override { return 0; }
   unsigned int filter_;
 };
+} // namespace

[tommi, 2015/10/19 12:36:24]

nit: 2 spaces

[the sun, 2015/10/19 14:25:02]

Done.

 
 class WebRtcVoiceEngineTestFake : public testing::Test {
  public:
@@ -293,6 +292,71 @@ class WebRtcVoiceEngineTestFake : public testing::Test {
     EXPECT_EQ(-1, voe_.GetReceiveRtpExtensionId(new_channel_num, ext));
   }
 
+  const webrtc::AudioReceiveStream::Stats& GetAudioReceiveStreamStats() const {
+    static webrtc::AudioReceiveStream::Stats stats;
+    if (stats.remote_ssrc == 0) {
+      stats.remote_ssrc = 123;
+      stats.bytes_rcvd = 456;
+      stats.packets_rcvd = 768;
+      stats.packets_lost = 101;
+      stats.fraction_lost = 23.45f;
+      stats.codec_name = "codec_name";
+      stats.ext_seqnum = 678;
+      stats.jitter_ms = 901;
+      stats.jitter_buffer_ms = 234;
+      stats.jitter_buffer_preferred_ms = 567;
+      stats.delay_estimate_ms = 890;
+      stats.audio_level = 1234;
+      stats.expand_rate = 5.67f;
+      stats.speech_expand_rate = 8.90f;
+      stats.secondary_decoded_rate = 1.23f;
+      stats.accelerate_rate = 4.56f;
+      stats.preemptive_expand_rate = 7.89f;
+      stats.decoding_calls_to_silence_generator = 012;
+      stats.decoding_calls_to_neteq = 345;
+      stats.decoding_normal = 67890;
+      stats.decoding_plc = 1234;
+      stats.decoding_cng = 5678;
+      stats.decoding_plc_cng = 9012;
+      stats.capture_start_ntp_time_ms = 3456;
+    }
+    return stats;
+  }
+  void SetAudioReceiveStreamStats() {
+    for (auto s : call_.GetAudioReceiveStreams()) {

[tommi, 2015/10/19 12:36:24]

nit: no curlies.
also, just to make it clear that this is not by-value (i.e. creating a copy each
time), use auto*

[the sun, 2015/10/19 14:25:02]

Yes, curlies.

1. Style guide avoids the issue.
2. Surrounding code (libjingle) generally use curlies for one-liners; style
guide says prefer consistency.
3. Arguments in favor of using curlies (consistency, less risk of copy/paste
bugs, bad formatting doesn't cause reader misinterpretation) trumps arguments
against (an extra line).

:)

auto* is more readable, thanks.

[tommi, 2015/10/19 14:55:44]

Re 2. I thought the opposite.
Re 3. (I'll need to break that down)
  - consistency - I don't think that's in favor or against {} in general
  - less risk of copy/paste bugs - nah where there's a will, there's a way, see
next line.
  - bad formatting doesn't cause reader misinterpretation - you'd think not :) 
see slide 33 here:
https://docs.google.com/presentation/d/1WAa-k6n9v9XEUPRxYODrCH5Bvng6KckGkr-7Z...
(it's not clear where the author went wrong, but for sure it fooled the
reviewer)

I understand your point though and don't want to keep you occupied with bike
shedding.  In the end, you're right since this file uses curly brackets for
single line statements like this.

[the sun, 2015/10/20 08:31:29]

Wonder how often these debates pop up? Goes to show that there should just be a
decision in the style guide...

But since we're on the topic, allow me to shoot back:

- Consistency - yes, if consistently using brackets, there is one less
possibility for the reader to take into account when mentally parsing the code.
- Copy/paste - agree the risk isn't big, but how about badly written macros?
Consistent bracketing avoids such issues.
- As I see it, in the example you give, the problem *is* mixed bracketing. It's
more likely the reviewer (and writer) would have spotted a misaligned } even if
the formatting was messed up.

As a friend of mine once said: "Religions? All good. Just don't try to convert
me." :)

+      s->SetStats(GetAudioReceiveStreamStats());
+    }
+  }
+  void VerifyVoiceReceiverInfo(const cricket::VoiceReceiverInfo& info) {
+    const auto& kStats = GetAudioReceiveStreamStats();
+    EXPECT_EQ(info.local_stats.front().ssrc, kStats.remote_ssrc);
+    EXPECT_EQ(info.bytes_rcvd, kStats.bytes_rcvd);
+    EXPECT_EQ(info.packets_rcvd, kStats.packets_rcvd);
+    EXPECT_EQ(info.packets_lost, kStats.packets_lost);
+    EXPECT_EQ(info.fraction_lost, kStats.fraction_lost);
+    EXPECT_EQ(info.codec_name, kStats.codec_name);
+    EXPECT_EQ(info.ext_seqnum, kStats.ext_seqnum);
+    EXPECT_EQ(info.jitter_ms, kStats.jitter_ms);
+    EXPECT_EQ(info.jitter_buffer_ms, kStats.jitter_buffer_ms);
+    EXPECT_EQ(info.jitter_buffer_preferred_ms,
+              kStats.jitter_buffer_preferred_ms);
+    EXPECT_EQ(info.delay_estimate_ms, kStats.delay_estimate_ms);
+    EXPECT_EQ(info.audio_level, kStats.audio_level);
+    EXPECT_EQ(info.expand_rate, kStats.expand_rate);
+    EXPECT_EQ(info.speech_expand_rate, kStats.speech_expand_rate);
+    EXPECT_EQ(info.secondary_decoded_rate, kStats.secondary_decoded_rate);
+    EXPECT_EQ(info.accelerate_rate, kStats.accelerate_rate);
+    EXPECT_EQ(info.preemptive_expand_rate, kStats.preemptive_expand_rate);
+    EXPECT_EQ(info.decoding_calls_to_silence_generator,
+              kStats.decoding_calls_to_silence_generator);
+    EXPECT_EQ(info.decoding_calls_to_neteq, kStats.decoding_calls_to_neteq);
+    EXPECT_EQ(info.decoding_normal, kStats.decoding_normal);
+    EXPECT_EQ(info.decoding_plc, kStats.decoding_plc);
+    EXPECT_EQ(info.decoding_cng, kStats.decoding_cng);
+    EXPECT_EQ(info.decoding_plc_cng, kStats.decoding_plc_cng);
+    EXPECT_EQ(info.capture_start_ntp_time_ms, kStats.capture_start_ntp_time_ms);
+  }
+
  protected:
   cricket::FakeCall call_;
   cricket::FakeWebRtcVoiceEngine voe_;
@@ -2008,38 +2072,30 @@ TEST_F(WebRtcVoiceEngineTestFake, GetStatsWithMultipleSendStreams) {
     EXPECT_EQ(cricket::kIntStatValue, info.senders[i].jitter_ms);
     EXPECT_EQ(kPcmuCodec.name, info.senders[i].codec_name);
   }
-  EXPECT_EQ(0u, info.receivers.size());
+
+  // We have added one receive stream, but its stats should be empty.
+  EXPECT_EQ(info.receivers.size(), 1u);
+  EXPECT_EQ(info.receivers[0].local_stats.front().ssrc, 0);
 
   // Registered stream's remote SSRC is kSsrc2. Send a packet with SSRC=1.
   // We should drop the packet and no stats should be available.
   DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame));
   EXPECT_EQ(true, channel_->GetStats(&info));
-  EXPECT_EQ(0u, info.receivers.size());
+  EXPECT_EQ(info.receivers.size(), 1u);
+  EXPECT_EQ(info.receivers[0].local_stats.front().ssrc, 0);
 
-  // Remove the kSsrc2 stream and deliver a new packet - a default receive
-  // stream should be created and we should see stats.
+  // Remove the kSsrc2 stream.
   EXPECT_TRUE(channel_->RemoveRecvStream(kSsrc2));
+  EXPECT_EQ(true, channel_->GetStats(&info));
+  EXPECT_EQ(0u, info.receivers.size());
+
+  // Deliver a new packet - a default receive stream should be created and we
+  // should see stats.
   DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame));
+  SetAudioReceiveStreamStats();
   EXPECT_EQ(true, channel_->GetStats(&info));
   EXPECT_EQ(1u, info.receivers.size());
-
-  EXPECT_EQ(cricket::kIntStatValue, info.receivers[0].bytes_rcvd);
-  EXPECT_EQ(cricket::kIntStatValue, info.receivers[0].packets_rcvd);
-  EXPECT_EQ(cricket::kIntStatValue, info.receivers[0].packets_lost);
-  EXPECT_EQ(cricket::kIntStatValue, info.receivers[0].ext_seqnum);
-  EXPECT_EQ(kPcmuCodec.name, info.receivers[0].codec_name);
-  EXPECT_EQ(static_cast<float>(cricket::kNetStats.currentExpandRate) /
-      (1 << 14), info.receivers[0].expand_rate);
-  EXPECT_EQ(static_cast<float>(cricket::kNetStats.currentSpeechExpandRate) /
-      (1 << 14), info.receivers[0].speech_expand_rate);
-  EXPECT_EQ(static_cast<float>(cricket::kNetStats.currentSecondaryDecodedRate) /
-      (1 << 14), info.receivers[0].secondary_decoded_rate);
-  EXPECT_EQ(
-      static_cast<float>(cricket::kNetStats.currentAccelerateRate) / (1 << 14),
-      info.receivers[0].accelerate_rate);
-  EXPECT_EQ(
-      static_cast<float>(cricket::kNetStats.currentPreemptiveRate) / (1 << 14),
-      info.receivers[0].preemptive_expand_rate);
+  VerifyVoiceReceiverInfo(info.receivers[0]);
 }
 
 // Test that we can add and remove receive streams, and do proper send/playout.
@@ -2300,33 +2356,29 @@ TEST_F(WebRtcVoiceEngineTestFake, GetStats) {
   // EXPECT_EQ(cricket::kIntStatValue, info.senders[0].echo_return_loss);
   // EXPECT_EQ(cricket::kIntStatValue,
   //           info.senders[0].echo_return_loss_enhancement);
-  EXPECT_EQ(0u, info.receivers.size());
+  // We have added one receive stream, but its stats should be empty.
+  EXPECT_EQ(info.receivers.size(), 1u);
+  EXPECT_EQ(info.receivers[0].local_stats.front().ssrc, 0);
 
   // Registered stream's remote SSRC is kSsrc2. Send a packet with SSRC=1.
   // We should drop the packet and no stats should be available.
   DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame));
   EXPECT_EQ(true, channel_->GetStats(&info));
-  EXPECT_EQ(0u, info.receivers.size());
+  EXPECT_EQ(info.receivers.size(), 1u);
+  EXPECT_EQ(info.receivers[0].local_stats.front().ssrc, 0);
 
-  // Remove the kSsrc2 stream and deliver a new packet - a default receive
-  // stream should be created and we should see stats.
+  // Remove the kSsrc2 stream.
   EXPECT_TRUE(channel_->RemoveRecvStream(kSsrc2));
+  EXPECT_EQ(true, channel_->GetStats(&info));
+  EXPECT_EQ(0u, info.receivers.size());
+
+  // Deliver a new packet - a default receive stream should be created and we
+  // should see stats.
   DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame));
+  SetAudioReceiveStreamStats();
   EXPECT_EQ(true, channel_->GetStats(&info));
   EXPECT_EQ(1u, info.receivers.size());
-
-  EXPECT_EQ(cricket::kIntStatValue, info.receivers[0].bytes_rcvd);
-  EXPECT_EQ(cricket::kIntStatValue, info.receivers[0].packets_rcvd);
-  EXPECT_EQ(cricket::kIntStatValue, info.receivers[0].packets_lost);
-  EXPECT_EQ(cricket::kIntStatValue, info.receivers[0].ext_seqnum);
-  EXPECT_EQ(kPcmuCodec.name, info.receivers[0].codec_name);
-  EXPECT_EQ(static_cast<float>(cricket::kNetStats.currentExpandRate) /
-      (1 << 14), info.receivers[0].expand_rate);
-  EXPECT_EQ(static_cast<float>(cricket::kNetStats.currentSpeechExpandRate) /
-      (1 << 14), info.receivers[0].speech_expand_rate);
-  EXPECT_EQ(static_cast<float>(cricket::kNetStats.currentSecondaryDecodedRate) /
-      (1 << 14), info.receivers[0].secondary_decoded_rate);
-  // TODO(sriniv): Add testing for more receiver fields.
+  VerifyVoiceReceiverInfo(info.receivers[0]);
 }
 
 // Test that we can set the outgoing SSRC properly with multiple streams.