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..fcfd48804a792a422f8f704ed062629b6e53c09c 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,30 @@
 
 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;
+};
+
+const CngCodecSpec kCngCodecs[] = {{13, 8000},
+                                   {103, 16000},
+                                   {104, 32000},
+                                   {105, 48000}};
+
+bool IsComfortNoisePayload(uint8_t payloadType) {

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

s/payloadType/payload_type/

+  for (const auto& c : kCngCodecs) {
+    if (c.payload_type == payloadType)
+      return true;
+  }
+
+  return false;
+}
 
 class VerifyingAudioReceiver : public NullRtpData {
  public:
@@ -31,31 +54,17 @@ 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 payload_type = rtpHeader->header.payloadType;
+    if (payload_type == kPcmuPayloadType || payload_type == kDtmfPayloadType) {
+      EXPECT_EQ(sizeof(kTestPayload), payloadSize);
       // All our test vectors for payload type 96 and 97 even the stereo is on

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

may be update the comment with improvements to code:
All our test vectors for Pcmu and Dtmf payloads equal to |kTestPayload|

       // 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;
-    }
-    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;
-        }
-      }
-      ADD_FAILURE() << "This code path should never happen.";
-      return -1;
+      const size_t min_size = std::min(sizeof(kTestPayload), payloadSize);
+      EXPECT_EQ(0, memcmp(payloadData, kTestPayload, min_size));
+    } else if (IsComfortNoisePayload(payload_type)) {
+      // CNG types should be recognized properly.
+      EXPECT_EQ(kAudioFrameCN, rtpHeader->frameType);
+      EXPECT_TRUE(rtpHeader->type.Audio.isCNG);
     }
     return 0;
   }
@@ -68,14 +77,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()
@@ -89,11 +100,11 @@ class RtpRtcpAudioTest : public ::testing::Test {
   ~RtpRtcpAudioTest() {}
 
   void SetUp() override {
-    data_receiver1 = new VerifyingAudioReceiver();
-    data_receiver2 = new VerifyingAudioReceiver();
-    rtp_callback = new RTPCallback();
-    transport1 = new LoopBackTransport();
-    transport2 = new LoopBackTransport();
+    data_receiver1.reset(new VerifyingAudioReceiver());
+    data_receiver2.reset(new VerifyingAudioReceiver());
+    rtp_callback.reset(new RTPCallback());
+    transport1.reset(new LoopBackTransport());
+    transport2.reset(new LoopBackTransport());
 
     receive_statistics1_.reset(ReceiveStatistics::Create(&fake_clock));
     receive_statistics2_.reset(ReceiveStatistics::Create(&fake_clock));
@@ -107,49 +118,56 @@ class RtpRtcpAudioTest : public ::testing::Test {
     configuration.audio = true;
     configuration.clock = &fake_clock;
     configuration.receive_statistics = receive_statistics1_.get();
-    configuration.outgoing_transport = transport1;
+    configuration.outgoing_transport = transport1.get();
     configuration.retransmission_rate_limiter = &retransmission_rate_limiter_;
 
-    module1 = RtpRtcp::CreateRtpRtcp(configuration);
+    module1.reset(RtpRtcp::CreateRtpRtcp(configuration));
     rtp_receiver1_.reset(RtpReceiver::CreateAudioReceiver(
-        &fake_clock, data_receiver1, NULL, rtp_payload_registry1_.get()));
+        &fake_clock, data_receiver1.get(), NULL, rtp_payload_registry1_.get()));
 
     configuration.receive_statistics = receive_statistics2_.get();
-    configuration.outgoing_transport = transport2;
+    configuration.outgoing_transport = transport2.get();
 
-    module2 = RtpRtcp::CreateRtpRtcp(configuration);
+    module2.reset(RtpRtcp::CreateRtpRtcp(configuration));
     rtp_receiver2_.reset(RtpReceiver::CreateAudioReceiver(
-        &fake_clock, data_receiver2, NULL, rtp_payload_registry2_.get()));
+        &fake_clock, data_receiver2.get(), NULL, rtp_payload_registry2_.get()));
 
-    transport1->SetSendModule(module2, rtp_payload_registry2_.get(),
+    transport1->SetSendModule(module2.get(), rtp_payload_registry2_.get(),
                               rtp_receiver2_.get(), receive_statistics2_.get());
-    transport2->SetSendModule(module1, rtp_payload_registry1_.get(),
+    transport2->SetSendModule(module1.get(), rtp_payload_registry1_.get(),
                               rtp_receiver1_.get(), receive_statistics1_.get());
   }
 
-  void TearDown() override {
-    delete module1;
-    delete module2;
-    delete transport1;
-    delete transport2;
-    delete data_receiver1;
-    delete data_receiver2;
-    delete rtp_callback;
+  void RegisterPayload(const CodecInst& codec) {

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

nice helper. makes test below obviously clearer.

[ossu, 2016/10/05 09:42:29]

\o/

+    EXPECT_EQ(0, module1->RegisterSendPayload(codec));
+    EXPECT_EQ(0, rtp_receiver1_->RegisterReceivePayload(
+        codec.plname,
+        codec.pltype,
+        codec.plfreq,
+        codec.channels,
+        codec.rate));
+    EXPECT_EQ(0, module2->RegisterSendPayload(codec));
+    EXPECT_EQ(0, rtp_receiver2_->RegisterReceivePayload(
+        codec.plname,
+        codec.pltype,
+        codec.plfreq,
+        codec.channels,
+        codec.rate));
   }
 
-  RtpRtcp* module1;
-  RtpRtcp* module2;
+  std::unique_ptr<RtpRtcp> module1;

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

likely be better to put modules last so that they will be destroyed first.
Otherwise for a short while they will hold invalid pointers that might make some
future tests flaky.

[ossu, 2016/10/05 09:42:29]

Done.

+  std::unique_ptr<RtpRtcp> module2;
   std::unique_ptr<ReceiveStatistics> receive_statistics1_;
   std::unique_ptr<ReceiveStatistics> receive_statistics2_;
   std::unique_ptr<RtpReceiver> rtp_receiver1_;
   std::unique_ptr<RtpReceiver> rtp_receiver2_;
   std::unique_ptr<RTPPayloadRegistry> rtp_payload_registry1_;
   std::unique_ptr<RTPPayloadRegistry> rtp_payload_registry2_;
-  VerifyingAudioReceiver* data_receiver1;
-  VerifyingAudioReceiver* data_receiver2;
-  LoopBackTransport* transport1;
-  LoopBackTransport* transport2;
-  RTPCallback* rtp_callback;
+  std::unique_ptr<VerifyingAudioReceiver> data_receiver1;

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

might also turn this members from unique pointers to just members:
VerifyingAudioReceiver data_receiver1_; (since they are not created by a factory
and do not need to live longer than the fixture.

If you prefer pointers for consistency with other members, live them as
pointers.

[ossu, 2016/10/05 09:42:29]

Done. Turned out rtp_callback was never used, so I re-instated it. I expect
there was some point to creating it in the first place, though this whole tests
seems like it was only half finished way back when.

+  std::unique_ptr<VerifyingAudioReceiver> data_receiver2;
+  std::unique_ptr<LoopBackTransport> transport1;
+  std::unique_ptr<LoopBackTransport> transport2;
+  std::unique_ptr<RTPCallback> rtp_callback;
   uint32_t test_ssrc;
   uint32_t test_timestamp;
   uint16_t test_sequence_number;
@@ -171,36 +189,20 @@ 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);
+  RegisterPayload(voice_codec);
 
-  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));
-  EXPECT_EQ(0, module2->RegisterSendPayload(voice_codec));
-  voice_codec.rate = test_rate;
-  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));
-
-  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,32 +213,18 @@ 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;
+  voice_codec.rate = kTestRate;
   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));
-  EXPECT_EQ(0, module2->RegisterSendPayload(voice_codec));
-  voice_codec.rate = test_rate;
-  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));
+  RegisterPayload(voice_codec);
 
   module1->SetSSRC(test_ssrc);
   module1->SetStartTimestamp(test_timestamp);
   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);
 
@@ -246,7 +234,7 @@ TEST_F(RtpRtcpAudioTest, DTMF) {
         voice_codec.pltype,
         voice_codec.plfreq,
         voice_codec.channels,
-        (voice_codec.rate < 0) ? 0 : voice_codec.rate));
+        voice_codec.rate));
 
   // Start DTMF test.
   int timeStamp = 160;
@@ -257,27 +245,69 @@ 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));
+
+  // Register PCMU and all four comfort noise codecs
+  CodecInst voice_codec = {0};

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

Probably better to use older, primitive memset way of reseting the structure,
because it is clearer.
Or write factory helper that return a CodecInst with all members reset.

It seems this line would initialize 1st member (pltype) of CodecInst struct to
zero and use default values (i.e.  initialized to indeterminate values) for
other struct members.
(http://en.cppreference.com/w/cpp/language/aggregate_initialization)

[ossu, 2016/10/05 09:42:29]

I believe it will value initialize everything, just like if static, i.e.
primitives to zero. {0} is in fact the same as just writing {} but perhaps a bit
clearer (though actually slightly misleading since the 0 only initializes the
first value - the rest only happen to get initialized to zero as well, i.e.
putting a 1 there won't get everything initialized to ones).

See: https://akrzemi1.wordpress.com/2013/09/10/value-initialization-with-c/

I find memset on structs/classes an abomination in C++. :/

[danilchap, 2016/10/05 09:54:22]

According to the link I posted since C++11 the remaining fields use default
initialization, not value initialization.

If you do not like memset (agree it looks odd in c++ code) may be has default
values in CodecInst directly or, if you prefer this CL to be local, add a
factory above:
CodecInst CreateCodec() {
  CodecInst result;
  result.pltype = 0;
  result.plname[0] = 0;
  result.pltype = 0;
  ...
  return result;
}
(this might not be efficient in production code, but should be fine for test)

[ossu, 2016/10/05 10:00:57]

No, it doesn't. I quote (removing the C++17 specific blocks that don't apply to
us, for clarity):
"If the number of initializer clauses is less than the number of members or
initializer list is completely empty, the remaining members are initialized by
empty lists, in accordance with the usual list-initialization rules (which
performs value-initialization for non-class types and non-aggregate classes with
default constructors, and aggregate initialization for aggregates)."

So each of the members are either value-initialized or aggregate-initialized
themselves.

[danilchap, 2016/10/05 10:54:09]

Yes, you right, I didn't read that part careful enough and missed conditions
'since c++14' and 'for class types'

May be at least remove 0 and initialize with empty {} since I doubt misleading
initialization can be clearer.

[ossu, 2016/10/05 11:00:07]

Agreed!

+  voice_codec.pltype = kPcmuPayloadType;
+  voice_codec.plfreq = 8000;
+  memcpy(voice_codec.plname, "PCMU", 5);
+  RegisterPayload(voice_codec);
+
+  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);
+    RegisterPayload(cng_codec);
+  }
+
+  // Transmit comfort noise packets interleaved by PCMU packets.
+  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