Resurrected test_api_audio.cc

webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc

Index: webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc
diff --git a/webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc b/webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc
index 7d72478df0516df12dffdf25fe4dc701c540c049..0c30504d77690384abd47892c7918ae9df028ce9 100644
--- a/webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc
+++ b/webrtc/modules/rtp_rtcp/test/testAPI/test_api_audio.cc
@@ -23,7 +23,21 @@
 
 namespace webrtc {
 namespace {
-#define test_rate 64000u
+
+const uint32_t kTestRate = 64000u;
+const uint8_t kTestPayload[] = { 't', 'e', 's', 't' };
+const uint8_t kPcmuPayloadType = 96;
+const uint8_t kDtmfPayloadType = 97;
+
+struct CngCodecSpec {
+  int payload_type;
+  int clockrate_hz;
+};
+
+CngCodecSpec kCngCodecs[] = {{13, 8000},

[danilchap, 2016/10/04 13:00:32]

add const (or constexpr), to be safe.

[hlundin-webrtc, 2016/10/04 13:45:03]

+1

[ossu, 2016/10/04 14:55:29]

Sure thing!

+                             {103, 16000},
+                             {104, 32000},
+                             {105, 48000}};
 
 class VerifyingAudioReceiver : public NullRtpData {
  public:
@@ -31,31 +45,27 @@ class VerifyingAudioReceiver : public NullRtpData {
       const uint8_t* payloadData,
       size_t payloadSize,
       const webrtc::WebRtcRTPHeader* rtpHeader) override {
-    if (rtpHeader->header.payloadType == 98 ||
-        rtpHeader->header.payloadType == 99) {
-      EXPECT_EQ(4u, payloadSize);
-      char str[5];
-      memcpy(str, payloadData, payloadSize);
-      str[4] = 0;
+    const uint8_t payloadType = rtpHeader->header.payloadType;

[danilchap, 2016/10/04 13:00:32]

payload_type = ...

[ossu, 2016/10/04 14:55:29]

Done.

+    if (payloadType == kPcmuPayloadType || payloadType == kDtmfPayloadType) {
+      EXPECT_EQ(sizeof(kTestPayload), payloadSize);
       // All our test vectors for payload type 96 and 97 even the stereo is on
       // a per channel base equal to the 4 chars "test".
-      // Note there is no null termination so we add that to use the
-      // test EXPECT_STRCASEEQ.
-      EXPECT_STRCASEEQ("test", str);
-      return 0;
+      const size_t min_size = std::min(sizeof(kTestPayload), payloadSize);

[hlundin-webrtc, 2016/10/04 13:45:03]

Shouldn't it be an error if payloadSize != sizeof(kTestPayload)? If so, I
suggest you replace this line with
ASSERT_EQ(sizeof(kTestPayload), payloadSize);
and do the memcmp over sizeof(kTestPayload) unconditionally.

[ossu, 2016/10/04 14:55:30]

Unfortunately, I can't use ASSERT because the function doesn't return void. The
previous test code exploded here when memcpy:ing unconditionally (due to
payloadSize including the size of the header as well). However, that never
happened because the payload types were wrong so the test never ran. :)

I could skip the rest if the payload size is off, but I believe it can be useful
for debugging to also compare the data. Maybe the size is off but the data is
correct. 

[danilchap, 2016/10/05 08:12:57]

Another solution is include gmock and use matchers from it:
EXPECT_THAT(::testing::make_tuple(payloadData, payloadSize),
::testing::ElementsAreArray(kTestPayload));
that test size and data together.

(just a suggestion. personally I'm fine both with current way and with removing
min_size and comparing with sizeof(kTestPayload) unconditionally. Worst case
failing test will crash, but still will catch a problem).

[hlundin-webrtc, 2016/10/05 13:52:42]

Right, I didn't think of the non-void return type. Keep it as it is.

+      EXPECT_EQ(0, memcmp(payloadData, kTestPayload, min_size));
     }
-    if (rtpHeader->header.payloadType == 100 ||
-        rtpHeader->header.payloadType == 101 ||
-        rtpHeader->header.payloadType == 102) {
-      if (rtpHeader->type.Audio.channel == 1) {
-        if (payloadData[0] == 0xff) {
-          // All our test vectors for payload type 100, 101 and 102 have the
-          // first channel data being equal to 0xff.
-          return 0;
-        }
+
+    auto is_cng_codec = [] (uint8_t payloadType) {

[danilchap, 2016/10/04 13:00:32]

may be put this helper function near kCngCodecs defined instead of creating
lambda:
bool IsCngCodec(int payload_type) {
  ...
}

[ossu, 2016/10/04 14:55:29]

Done.

+      for (const auto& c : kCngCodecs) {
+        if (c.payload_type == payloadType)
+          return true;
       }
-      ADD_FAILURE() << "This code path should never happen.";
-      return -1;
+
+      return false;
+    };
+    if (is_cng_codec(payloadType)) {
+      // CNG types should be recognized properly.
+      EXPECT_EQ(kAudioFrameCN, rtpHeader->frameType);
+      EXPECT_TRUE(rtpHeader->type.Audio.isCNG);
     }
     return 0;
   }
@@ -68,14 +78,16 @@ class RTPCallback : public NullRtpFeedback {
                               const int frequency,
                               const size_t channels,
                               const uint32_t rate) override {
-    if (payloadType == 96) {
-      EXPECT_EQ(test_rate, rate) <<
+    if (payloadType == kPcmuPayloadType) {
+      EXPECT_EQ(kTestRate, rate) <<
           "The rate should be 64K for this payloadType";
     }
     return 0;
   }
 };
 
+}  // namespace
+
 class RtpRtcpAudioTest : public ::testing::Test {
  protected:
   RtpRtcpAudioTest()
@@ -171,13 +183,13 @@ TEST_F(RtpRtcpAudioTest, Basic) {
 
   // Send an empty RTP packet.
   // Should fail since we have not registered the payload type.
-  EXPECT_FALSE(module1->SendOutgoingData(webrtc::kAudioFrameSpeech, 96, 0, -1,
-                                         nullptr, 0, nullptr, nullptr,
-                                         nullptr));
+  EXPECT_FALSE(module1->SendOutgoingData(webrtc::kAudioFrameSpeech,
+                                         kPcmuPayloadType, 0, -1, nullptr, 0,
+                                         nullptr, nullptr, nullptr));
 
   CodecInst voice_codec;
   memset(&voice_codec, 0, sizeof(voice_codec));
-  voice_codec.pltype = 96;
+  voice_codec.pltype = kPcmuPayloadType;
   voice_codec.plfreq = 8000;
   memcpy(voice_codec.plname, "PCMU", 5);
 
@@ -189,7 +201,7 @@ TEST_F(RtpRtcpAudioTest, Basic) {
       voice_codec.channels,
       (voice_codec.rate < 0) ? 0 : voice_codec.rate));
   EXPECT_EQ(0, module2->RegisterSendPayload(voice_codec));
-  voice_codec.rate = test_rate;
+  voice_codec.rate = kTestRate;
   EXPECT_EQ(0, rtp_receiver2_->RegisterReceivePayload(
       voice_codec.plname,
       voice_codec.pltype,
@@ -197,10 +209,9 @@ TEST_F(RtpRtcpAudioTest, Basic) {
       voice_codec.channels,
       (voice_codec.rate < 0) ? 0 : voice_codec.rate));
 
-  const uint8_t test[5] = "test";
-  EXPECT_EQ(true,
-            module1->SendOutgoingData(webrtc::kAudioFrameSpeech, 96, 0, -1,
-                                      test, 4, nullptr, nullptr, nullptr));
+  EXPECT_TRUE(module1->SendOutgoingData(webrtc::kAudioFrameSpeech,
+                                        kPcmuPayloadType, 0, -1, kTestPayload,
+                                        4, nullptr, nullptr, nullptr));
 
   EXPECT_EQ(test_ssrc, rtp_receiver2_->SSRC());
   uint32_t timestamp;
@@ -211,7 +222,7 @@ TEST_F(RtpRtcpAudioTest, Basic) {
 TEST_F(RtpRtcpAudioTest, DTMF) {
   CodecInst voice_codec;
   memset(&voice_codec, 0, sizeof(voice_codec));
-  voice_codec.pltype = 96;
+  voice_codec.pltype = kPcmuPayloadType;
   voice_codec.plfreq = 8000;
   memcpy(voice_codec.plname, "PCMU", 5);
 
@@ -223,7 +234,7 @@ TEST_F(RtpRtcpAudioTest, DTMF) {
       voice_codec.channels,
       (voice_codec.rate < 0) ? 0 : voice_codec.rate));
   EXPECT_EQ(0, module2->RegisterSendPayload(voice_codec));
-  voice_codec.rate = test_rate;
+  voice_codec.rate = kTestRate;
   EXPECT_EQ(0, rtp_receiver2_->RegisterReceivePayload(
       voice_codec.plname,
       voice_codec.pltype,
@@ -236,7 +247,7 @@ TEST_F(RtpRtcpAudioTest, DTMF) {
   EXPECT_EQ(0, module1->SetSendingStatus(true));
 
   // Prepare for DTMF.
-  voice_codec.pltype = 97;
+  voice_codec.pltype = kDtmfPayloadType;
   voice_codec.plfreq = 8000;
   memcpy(voice_codec.plname, "telephone-event", 16);
 
@@ -257,27 +268,100 @@ TEST_F(RtpRtcpAudioTest, DTMF) {
   }
   timeStamp += 160;  // Prepare for next packet.
 
-  const uint8_t test[9] = "test";
-
   // Send RTP packets for 16 tones a 160 ms  100ms
   // pause between = 2560ms + 1600ms = 4160ms
   for (; timeStamp <= 250 * 160; timeStamp += 160) {
-    EXPECT_TRUE(module1->SendOutgoingData(webrtc::kAudioFrameSpeech, 96,
-                                          timeStamp, -1, test, 4, nullptr,
-                                          nullptr, nullptr));
+    EXPECT_TRUE(module1->SendOutgoingData(
+        webrtc::kAudioFrameSpeech, kPcmuPayloadType, timeStamp, -1,
+        kTestPayload, 4, nullptr, nullptr, nullptr));
     fake_clock.AdvanceTimeMilliseconds(20);
     module1->Process();
   }
   EXPECT_EQ(0, module1->SendTelephoneEventOutband(32, 9000, 10));
 
   for (; timeStamp <= 740 * 160; timeStamp += 160) {
-    EXPECT_TRUE(module1->SendOutgoingData(webrtc::kAudioFrameSpeech, 96,
-                                          timeStamp, -1, test, 4, nullptr,
-                                          nullptr, nullptr));
+    EXPECT_TRUE(module1->SendOutgoingData(
+        webrtc::kAudioFrameSpeech, kPcmuPayloadType, timeStamp, -1,
+        kTestPayload, 4, nullptr, nullptr, nullptr));
     fake_clock.AdvanceTimeMilliseconds(20);
     module1->Process();
   }
 }
 
-}  // namespace
+TEST_F(RtpRtcpAudioTest, ComfortNoise) {
+  module1->SetSSRC(test_ssrc);
+  module1->SetStartTimestamp(test_timestamp);
+
+  EXPECT_EQ(0, module1->SetSendingStatus(true));
+
+  // Start basic RTP test.
+
+  CodecInst voice_codec;
+  memset(&voice_codec, 0, sizeof(voice_codec));
+  voice_codec.pltype = kPcmuPayloadType;
+  voice_codec.plfreq = 8000;
+  memcpy(voice_codec.plname, "PCMU", 5);
+
+  EXPECT_EQ(0, module1->RegisterSendPayload(voice_codec));
+  EXPECT_EQ(0, rtp_receiver1_->RegisterReceivePayload(
+      voice_codec.plname,
+      voice_codec.pltype,
+      voice_codec.plfreq,
+      voice_codec.channels,
+      (voice_codec.rate < 0) ? 0 : voice_codec.rate));

[hlundin-webrtc, 2016/10/04 13:45:02]

voice_codec.rate should be deterministically known at this point, right? So why
the conditional?

[ossu, 2016/10/04 14:55:29]

I don't know. It was like this when I got here. :)

+  EXPECT_EQ(0, module2->RegisterSendPayload(voice_codec));
+  voice_codec.rate = kTestRate;
+  EXPECT_EQ(0, rtp_receiver2_->RegisterReceivePayload(
+      voice_codec.plname,
+      voice_codec.pltype,
+      voice_codec.plfreq,
+      voice_codec.channels,
+      (voice_codec.rate < 0) ? 0 : voice_codec.rate));

[hlundin-webrtc, 2016/10/04 13:45:02]

Same here.

[ossu, 2016/10/04 14:55:30]

Well, alright, it was like this in the test I stole it from. I don't think rate
will ever be < 0 in any of the tests. I'll clean it up.

+
+  for (const auto& c : kCngCodecs) {
+    CodecInst cng_codec = {0};
+    cng_codec.pltype = c.payload_type;
+    cng_codec.plfreq = c.clockrate_hz;
+    memcpy(cng_codec.plname, "CN", 3);
+
+    EXPECT_EQ(0, module1->RegisterSendPayload(cng_codec));
+    EXPECT_EQ(0, rtp_receiver1_->RegisterReceivePayload(
+        cng_codec.plname,
+        cng_codec.pltype,
+        cng_codec.plfreq,
+        cng_codec.channels,
+        cng_codec.rate));
+    EXPECT_EQ(0, module2->RegisterSendPayload(cng_codec));
+    EXPECT_EQ(0, rtp_receiver2_->RegisterReceivePayload(
+        cng_codec.plname,
+        cng_codec.pltype,
+        cng_codec.plfreq,
+        cng_codec.channels,
+        cng_codec.rate));
+  }
+
+  uint32_t in_timestamp = 0;
+
+  for (const auto& c : kCngCodecs) {
+    uint32_t timestamp;
+    EXPECT_TRUE(module1->SendOutgoingData(
+        webrtc::kAudioFrameSpeech, kPcmuPayloadType, in_timestamp, -1,
+        kTestPayload, 4, nullptr, nullptr, nullptr));
+
+    EXPECT_EQ(test_ssrc, rtp_receiver2_->SSRC());
+    EXPECT_TRUE(rtp_receiver2_->Timestamp(&timestamp));
+    EXPECT_EQ(test_timestamp + in_timestamp, timestamp);
+    in_timestamp += 10;
+
+    EXPECT_TRUE(module1->SendOutgoingData(webrtc::kAudioFrameCN, c.payload_type,
+                                          in_timestamp, -1, kTestPayload, 1,
+                                          nullptr, nullptr, nullptr));
+
+    EXPECT_EQ(test_ssrc, rtp_receiver2_->SSRC());
+    EXPECT_TRUE(rtp_receiver2_->Timestamp(&timestamp));
+    EXPECT_EQ(test_timestamp + in_timestamp, timestamp);
+    in_timestamp += 10;
+  }
+}
+
 }  // namespace webrtc