Add a unit test for callbacks with empty frames and fix bug in code

This change adds a couple of new tests that verify that callbacks
with frame type kFrameEmpty are sent in between comfort noise packets.
This used to be the case until r8268, and with the fix included in
this CL is once again so.

COAUTHOR=kwiberg@webrtc.org
R=minyue@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/37229004

Cr-Commit-Position: refs/heads/master@{#8353}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8353 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/audio_coding/codecs/audio_encoder.h

@@ -24,11 +24,15 @@ class AudioEncoder {
  public:
   struct EncodedInfoLeaf {
     EncodedInfoLeaf()
-        : encoded_bytes(0), encoded_timestamp(0), payload_type(0) {}
+        : encoded_bytes(0),
+          encoded_timestamp(0),
+          payload_type(0),
+          send_even_if_empty(false) {}
 
     size_t encoded_bytes;
     uint32_t encoded_timestamp;
     int payload_type;
+    bool send_even_if_empty;
   };
 
   // This is the main struct for auxiliary encoding information. Each encoded

webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc

@@ -154,6 +154,7 @@ bool AudioEncoderCng::EncodeInternal(uint32_t rtp_timestamp,
       return_val = EncodePassive(encoded, &info->encoded_bytes);
       info->encoded_timestamp = first_timestamp_in_buffer_;
       info->payload_type = cng_payload_type_;
+      info->send_even_if_empty = true;
       last_frame_active_ = false;
       break;
     }

webrtc/modules/audio_coding/main/acm2/acm_generic_codec.cc

@@ -1211,7 +1211,7 @@ int16_t ACMGenericCodecWrapper::Encode(
   *bitstream_len_byte = static_cast<int16_t>(encoded_info->encoded_bytes);
   if (encoded_info->encoded_bytes == 0) {
     *encoding_type = kNoEncoding;
-    return 0;
+    return encoded_info->send_even_if_empty ? 1 : 0;
   }
   *timestamp = encoded_info->encoded_timestamp;
 

webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc

@@ -294,7 +294,7 @@ int32_t AudioCodingModuleImpl::Process() {
     } else {
       switch (encoding_type) {
         case kNoEncoding: {
-          FATAL() << "This case is no longer valid.";
+          current_payload_type = previous_pltype_;
           frame_type = kFrameEmpty;
           length_bytes = 0;
           break;

webrtc/modules/audio_coding/main/acm2/audio_coding_module_unittest_oldapi.cc

@@ -37,12 +37,15 @@
 
 namespace webrtc {
 
+namespace {
 const int kSampleRateHz = 16000;
 const int kNumSamples10ms = kSampleRateHz / 100;
 const int kFrameSizeMs = 10;  // Multiple of 10.
 const int kFrameSizeSamples = kFrameSizeMs / 10 * kNumSamples10ms;
 const int kPayloadSizeBytes = kFrameSizeSamples * sizeof(int16_t);
 const uint8_t kPayloadType = 111;
+const int kUseDefaultPacketSize = -1;
+}  // namespace
 
 class RtpUtility {
  public:
@@ -75,10 +78,12 @@ class RtpUtility {
   uint8_t payload_type_;
 };
 
-class PacketizationCallbackStub : public AudioPacketizationCallback {
+class PacketizationCallbackStubOldApi : public AudioPacketizationCallback {
  public:
-  PacketizationCallbackStub()
+  PacketizationCallbackStubOldApi()
       : num_calls_(0),
+        last_frame_type_(kFrameEmpty),
+        last_payload_type_(-1),
         crit_sect_(CriticalSectionWrapper::CreateCriticalSection()) {}
 
   virtual int32_t SendData(
@@ -90,6 +95,8 @@ class PacketizationCallbackStub : public AudioPacketizationCallback {
       const RTPFragmentationHeader* fragmentation) OVERRIDE {
     CriticalSectionScoped lock(crit_sect_.get());
     ++num_calls_;
+    last_frame_type_ = frame_type;
+    last_payload_type_ = payload_type;
     last_payload_vec_.assign(payload_data, payload_data + payload_len_bytes);
     return 0;
   }
@@ -104,6 +111,16 @@ class PacketizationCallbackStub : public AudioPacketizationCallback {
     return last_payload_vec_.size();
   }
 
+  FrameType last_frame_type() const {
+    CriticalSectionScoped lock(crit_sect_.get());
+    return last_frame_type_;
+  }
+
+  int last_payload_type() const {
+    CriticalSectionScoped lock(crit_sect_.get());
+    return last_payload_type_;
+  }
+
   void SwapBuffers(std::vector<uint8_t>* payload) {
     CriticalSectionScoped lock(crit_sect_.get());
     last_payload_vec_.swap(*payload);
@@ -111,6 +128,8 @@ class PacketizationCallbackStub : public AudioPacketizationCallback {
 
  private:
   int num_calls_ GUARDED_BY(crit_sect_);
+  FrameType last_frame_type_ GUARDED_BY(crit_sect_);
+  int last_payload_type_ GUARDED_BY(crit_sect_);
   std::vector<uint8_t> last_payload_vec_ GUARDED_BY(crit_sect_);
   const scoped_ptr<CriticalSectionWrapper> crit_sect_;
 };
@@ -120,7 +139,8 @@ class AudioCodingModuleTestOldApi : public ::testing::Test {
   AudioCodingModuleTestOldApi()
       : id_(1),
         rtp_utility_(new RtpUtility(kFrameSizeSamples, kPayloadType)),
-        clock_(Clock::GetRealTimeClock()) {}
+        clock_(Clock::GetRealTimeClock()),
+        packet_size_samples_(kUseDefaultPacketSize) {}
 
   ~AudioCodingModuleTestOldApi() {}
 
@@ -129,8 +149,6 @@ class AudioCodingModuleTestOldApi : public ::testing::Test {
   void SetUp() {
     acm_.reset(AudioCodingModule::Create(id_, clock_));
 
-    RegisterCodec();
-
     rtp_utility_->Populate(&rtp_header_);
 
     input_frame_.sample_rate_hz_ = kSampleRateHz;
@@ -148,6 +166,9 @@ class AudioCodingModuleTestOldApi : public ::testing::Test {
   virtual void RegisterCodec() {
     AudioCodingModule::Codec("L16", &codec_, kSampleRateHz, 1);
     codec_.pltype = kPayloadType;
+    if (packet_size_samples_ != kUseDefaultPacketSize) {
+      codec_.pacsize = packet_size_samples_;
+    }
 
     // Register L16 codec in ACM.
     ASSERT_EQ(0, acm_->RegisterReceiveCodec(codec_));
@@ -186,16 +207,18 @@ class AudioCodingModuleTestOldApi : public ::testing::Test {
   const int id_;
   scoped_ptr<RtpUtility> rtp_utility_;
   scoped_ptr<AudioCodingModule> acm_;
-  PacketizationCallbackStub packet_cb_;
+  PacketizationCallbackStubOldApi packet_cb_;
   WebRtcRTPHeader rtp_header_;
   AudioFrame input_frame_;
   CodecInst codec_;
   Clock* clock_;
+  int packet_size_samples_;
 };
 
 // Check if the statistics are initialized correctly. Before any call to ACM
 // all fields have to be zero.
 TEST_F(AudioCodingModuleTestOldApi, DISABLED_ON_ANDROID(InitializedToZero)) {
+  RegisterCodec();
   AudioDecodingCallStats stats;
   acm_->GetDecodingCallStatistics(&stats);
   EXPECT_EQ(0, stats.calls_to_neteq);
@@ -210,6 +233,7 @@ TEST_F(AudioCodingModuleTestOldApi, DISABLED_ON_ANDROID(InitializedToZero)) {
 // should result in generating silence, check the associated field.
 TEST_F(AudioCodingModuleTestOldApi,
        DISABLED_ON_ANDROID(SilenceGeneratorCalled)) {
+  RegisterCodec();
   AudioDecodingCallStats stats;
   const int kInitialDelay = 100;
 
@@ -233,6 +257,7 @@ TEST_F(AudioCodingModuleTestOldApi,
 // simulate packet loss and check if PLC and PLC-to-CNG statistics are
 // correctly updated.
 TEST_F(AudioCodingModuleTestOldApi, DISABLED_ON_ANDROID(NetEqCalls)) {
+  RegisterCodec();
   AudioDecodingCallStats stats;
   const int kNumNormalCalls = 10;
 
@@ -279,6 +304,86 @@ TEST_F(AudioCodingModuleTestOldApi, FailOnZeroDesiredFrequency) {
   EXPECT_EQ(-1, acm_->PlayoutData10Ms(0, &audio_frame));
 }
 
+// Checks that the transport callback is invoked once for each speech packet.
+// Also checks that the frame type is kAudioFrameSpeech.
+TEST_F(AudioCodingModuleTestOldApi, TransportCallbackIsInvokedForEachPacket) {
+  const int k10MsBlocksPerPacket = 3;
+  packet_size_samples_ = k10MsBlocksPerPacket * kSampleRateHz / 100;
+  RegisterCodec();
+  const int kLoops = 10;
+  for (int i = 0; i < kLoops; ++i) {
+    EXPECT_EQ(i / k10MsBlocksPerPacket, packet_cb_.num_calls());
+    if (packet_cb_.num_calls() > 0)
+      EXPECT_EQ(kAudioFrameSpeech, packet_cb_.last_frame_type());
+    InsertAudio();
+    Encode();
+  }
+  EXPECT_EQ(kLoops / k10MsBlocksPerPacket, packet_cb_.num_calls());
+  EXPECT_EQ(kAudioFrameSpeech, packet_cb_.last_frame_type());
+}
+
+// Introduce this class to set different expectations on the number of encoded
+// bytes. This class expects all encoded packets to be 9 bytes (matching one
+// CNG SID frame) or 0 bytes. This test depends on |input_frame_| containing
+// (near-)zero values.
+class AudioCodingModuleTestWithComfortNoiseOldApi
+    : public AudioCodingModuleTestOldApi {
+ protected:
+  void Encode() override {
+    int32_t encoded_bytes = acm_->Process();
+    // Expect either one packet with 9 comfort noise parameters, or no packet
+    // at all.
+    EXPECT_TRUE(encoded_bytes == 9 || encoded_bytes == 0);
+  }
+};
+
+// Checks that the transport callback is invoked once for frame period of the
+// underlying speech encoder, even when comfort noise is produced.
+// Also checks that the frame type is kAudioFrameCN or kFrameEmpty.
+TEST_F(AudioCodingModuleTestWithComfortNoiseOldApi,
+       TransportCallbackTestForComfortNoise) {
+  const int k10MsBlocksPerPacket = 3;
+  packet_size_samples_ = k10MsBlocksPerPacket * kSampleRateHz / 100;
+  RegisterCodec();
+  ASSERT_EQ(0, acm_->SetVAD(true, true));
+  const int kLoops = 40;
+  // This array defines the expected frame types, and when they should arrive.
+  // We expect a frame to arrive each time the speech encoder would have
+  // produced a packet, and once every 100 ms the frame should be non-empty,
+  // that is contain comfort noise.
+  const struct {
+    int ix;
+    FrameType type;
+  } expectation[] = {{2, kAudioFrameCN},
+                     {5, kFrameEmpty},
+                     {8, kFrameEmpty},
+                     {11, kAudioFrameCN},
+                     {14, kFrameEmpty},
+                     {17, kFrameEmpty},
+                     {20, kAudioFrameCN},
+                     {23, kFrameEmpty},
+                     {26, kFrameEmpty},
+                     {29, kFrameEmpty},
+                     {32, kAudioFrameCN},
+                     {35, kFrameEmpty},
+                     {38, kFrameEmpty}};
+  for (int i = 0; i < kLoops; ++i) {
+    int num_calls_before = packet_cb_.num_calls();
+    EXPECT_EQ(i / k10MsBlocksPerPacket, num_calls_before);
+    InsertAudio();
+    Encode();
+    int num_calls = packet_cb_.num_calls();
+    if (num_calls == num_calls_before + 1) {
+      EXPECT_EQ(expectation[num_calls - 1].ix, i);
+      EXPECT_EQ(expectation[num_calls - 1].type, packet_cb_.last_frame_type())
+          << "Wrong frame type for lap " << i;
+      EXPECT_EQ(98, packet_cb_.last_payload_type());  // Default CNG-wb type.
+    } else {
+      EXPECT_EQ(num_calls, num_calls_before);
+    }
+  }
+}
+
 // A multi-threaded test for ACM. This base class is using the PCM16b 16 kHz
 // codec, while the derive class AcmIsacMtTest is using iSAC.
 class AudioCodingModuleMtTestOldApi : public AudioCodingModuleTestOldApi {
@@ -312,6 +417,7 @@ class AudioCodingModuleMtTestOldApi : public AudioCodingModuleTestOldApi {
 
   void SetUp() {
     AudioCodingModuleTestOldApi::SetUp();
+    RegisterCodec();  // Must be called before the threads start below.
     StartThreads();
   }
 
@@ -440,6 +546,7 @@ class AcmIsacMtTestOldApi : public AudioCodingModuleMtTestOldApi {
 
   void SetUp() {
     AudioCodingModuleTestOldApi::SetUp();
+    RegisterCodec();  // Must be called before the threads start below.
 
     // Set up input audio source to read from specified file, loop after 5
     // seconds, and deliver blocks of 10 ms.
@@ -461,10 +568,11 @@ class AcmIsacMtTestOldApi : public AudioCodingModuleMtTestOldApi {
     StartThreads();
   }
 
-  virtual void RegisterCodec() {
+  void RegisterCodec() override {
     static_assert(kSampleRateHz == 16000, "test designed for iSAC 16 kHz");
     AudioCodingModule::Codec("ISAC", &codec_, kSampleRateHz, 1);
     codec_.pltype = kPayloadType;
+    ASSERT_EQ(kUseDefaultPacketSize, packet_size_samples_);
 
     // Register iSAC codec in ACM, effectively unregistering the PCM16B codec
     // registered in AudioCodingModuleTestOldApi::SetUp();