AcmReceiver: index decoders by payload type instead of ACM codec ID

Change internal indexing of registered decoders. It makes sense because payload type is unique, while ACM codec ID may not be. This is a step towards allowing for addition of external decoders.

R=kwiberg@webrtc.org

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

Cr-Commit-Position: refs/heads/master@{#8867}

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

@@ -120,7 +120,7 @@ bool IsCng(int codec_id) {
 AcmReceiver::AcmReceiver(const AudioCodingModule::Config& config)
     : crit_sect_(CriticalSectionWrapper::CreateCriticalSection()),
       id_(config.id),
-      last_audio_decoder_(-1),  // Invalid value.
+      last_audio_decoder_(nullptr),
       previous_audio_activity_(AudioFrame::kVadPassive),
       current_sample_rate_hz_(config.neteq_config.sample_rate_hz),
       audio_buffer_(new int16_t[AudioFrame::kMaxDataSizeSamples]),
@@ -269,32 +269,29 @@ int AcmReceiver::InsertPacket(const WebRtcRTPHeader& rtp_header,
   {
     CriticalSectionScoped lock(crit_sect_.get());
 
-    int codec_id = RtpHeaderToCodecIndex(*header, incoming_payload);
+    const Decoder* decoder = RtpHeaderToDecoder(*header, incoming_payload);
-    if (codec_id < 0) {
+    if (!decoder) {
       LOG_F(LS_ERROR) << "Payload-type "
                       << static_cast<int>(header->payloadType)
                       << " is not registered.";
       return -1;
     }
-    const int sample_rate_hz = ACMCodecDB::CodecFreq(codec_id);
+    const int sample_rate_hz = ACMCodecDB::CodecFreq(decoder->acm_codec_id);
     receive_timestamp = NowInTimestamp(sample_rate_hz);
 
-    if (IsCng(codec_id)) {
+    if (IsCng(decoder->acm_codec_id)) {
       // If this is a CNG while the audio codec is not mono skip pushing in
       // packets into NetEq.
-      if (last_audio_decoder_ >= 0 &&
+      if (last_audio_decoder_ && last_audio_decoder_->channels > 1)
-        decoders_[last_audio_decoder_].channels > 1)
         return 0;
       packet_type = InitialDelayManager::kCngPacket;
-    } else if (codec_id == ACMCodecDB::kAVT) {
+    } else if (decoder->acm_codec_id == ACMCodecDB::kAVT) {
       packet_type = InitialDelayManager::kAvtPacket;
     } else {
-      if (codec_id != last_audio_decoder_) {
+      if (decoder != last_audio_decoder_) {
         // This is either the first audio packet or send codec is changed.
         // Therefore, either NetEq buffer is empty or will be flushed when this
-        // packet inserted. Note that |last_audio_decoder_| is initialized to
+        // packet is inserted.
-        // an invalid value (-1), hence, the above condition is true for the
-        // very first audio packet.
         new_codec = true;
 
         // Updating NACK'sampling rate is required, either first packet is
@@ -305,7 +302,7 @@ int AcmReceiver::InsertPacket(const WebRtcRTPHeader& rtp_header,
           nack_->Reset();
           nack_->UpdateSampleRate(sample_rate_hz);
         }
-        last_audio_decoder_ = codec_id;
+        last_audio_decoder_ = decoder;
       }
       packet_type = InitialDelayManager::kAudioPacket;
     }
@@ -491,22 +488,19 @@ int32_t AcmReceiver::AddCodec(int acm_codec_id,
 
   // The corresponding NetEq decoder ID.
   // If this codec has been registered before.
-  auto it = decoders_.find(acm_codec_id);
+  auto it = decoders_.find(payload_type);
   if (it != decoders_.end()) {
     const Decoder& decoder = it->second;
-    if (decoder.payload_type == payload_type &&
+    if (decoder.acm_codec_id == acm_codec_id && decoder.channels == channels) {
-        decoder.channels == channels) {
+      // Re-registering the same codec. Do nothing and return.
-      // Re-registering the same codec with the same payload-type. Do nothing
-      // and return.
       return 0;
     }
 
-    // Changing the payload-type or number of channels for this codec.
+    // Changing codec or number of channels. First unregister the old codec,
-    // First unregister. Then register with new payload-type/channels.
+    // then register the new one.
-    if (neteq_->RemovePayloadType(decoder.payload_type) !=
+    if (neteq_->RemovePayloadType(payload_type) != NetEq::kOK) {
-        NetEq::kOK) {
       LOG_F(LS_ERROR) << "Cannot remove payload "
-                      << static_cast<int>(decoder.payload_type);
+                      << static_cast<int>(payload_type);
       return -1;
     }
 
@@ -530,7 +524,7 @@ int32_t AcmReceiver::AddCodec(int acm_codec_id,
   decoder.acm_codec_id = acm_codec_id;
   decoder.payload_type = payload_type;
   decoder.channels = channels;
-  decoders_[acm_codec_id] = decoder;
+  decoders_[payload_type] = decoder;
   return 0;
 }
 
@@ -568,14 +562,14 @@ int AcmReceiver::RemoveAllCodecs() {
   }
 
   // No codec is registered, invalidate last audio decoder.
-  last_audio_decoder_ = -1;
+  last_audio_decoder_ = nullptr;
   return ret_val;
 }
 
 int AcmReceiver::RemoveCodec(uint8_t payload_type) {
   CriticalSectionScoped lock(crit_sect_.get());
-  int codec_index = PayloadType2CodecIndex(payload_type);
+  auto it = decoders_.find(payload_type);
-  if (codec_index < 0) {  // Such a payload-type is not registered.
+  if (it == decoders_.end()) {  // Such a payload-type is not registered.
     return 0;
   }
   if (neteq_->RemovePayloadType(payload_type) != NetEq::kOK) {
@@ -583,9 +577,9 @@ int AcmReceiver::RemoveCodec(uint8_t payload_type) {
               static_cast<int>(payload_type));
     return -1;
   }
-  decoders_.erase(codec_index);
+  if (last_audio_decoder_ == &it->second)
-  if (last_audio_decoder_ == codec_index)
+    last_audio_decoder_ = nullptr;
-    last_audio_decoder_ = -1;  // Codec is removed, invalidate last decoder.
+  decoders_.erase(it);
   return 0;
 }
 
@@ -606,29 +600,31 @@ bool AcmReceiver::GetPlayoutTimestamp(uint32_t* timestamp) {
 
 int AcmReceiver::last_audio_codec_id() const {
   CriticalSectionScoped lock(crit_sect_.get());
-  return last_audio_decoder_;
+  return last_audio_decoder_ ? last_audio_decoder_->acm_codec_id : -1;
 }
 
 int AcmReceiver::RedPayloadType() const {
-  CriticalSectionScoped lock(crit_sect_.get());
+  if (ACMCodecDB::kRED >= 0) {  // This ensures that RED is defined in WebRTC.
-  auto it = decoders_.find(ACMCodecDB::kRED);
+    CriticalSectionScoped lock(crit_sect_.get());
-  if (ACMCodecDB::kRED < 0 || it == decoders_.end()) {
+    for (const auto& decoder_pair : decoders_) {
-    LOG_F(LS_WARNING) << "RED is not registered.";
+      const Decoder& decoder = decoder_pair.second;
-    return -1;
+      if (decoder.acm_codec_id == ACMCodecDB::kRED)
+        return decoder.payload_type;
+    }
   }
-  return it->second.payload_type;
+  LOG_F(LS_WARNING) << "RED is not registered.";
+  return -1;
 }
 
 int AcmReceiver::LastAudioCodec(CodecInst* codec) const {
   CriticalSectionScoped lock(crit_sect_.get());
-  if (last_audio_decoder_ < 0) {
+  if (!last_audio_decoder_) {
     return -1;
   }
-  auto it = decoders_.find(last_audio_decoder_);
+  memcpy(codec, &ACMCodecDB::database_[last_audio_decoder_->acm_codec_id],
-  assert(it != decoders_.end());
+         sizeof(CodecInst));
-  memcpy(codec, &ACMCodecDB::database_[last_audio_decoder_], sizeof(CodecInst));
+  codec->pltype = last_audio_decoder_->payload_type;
-  codec->pltype = it->second.payload_type;
+  codec->channels = last_audio_decoder_->channels;
-  codec->channels = it->second.channels;
   return 0;
 }
 
@@ -679,29 +675,20 @@ void AcmReceiver::GetNetworkStatistics(NetworkStatistics* acm_stat) {
 int AcmReceiver::DecoderByPayloadType(uint8_t payload_type,
                                       CodecInst* codec) const {
   CriticalSectionScoped lock(crit_sect_.get());
-  int codec_index = PayloadType2CodecIndex(payload_type);
+  auto it = decoders_.find(payload_type);
-  if (codec_index < 0) {
+  if (it == decoders_.end()) {
     LOG_FERR1(LS_ERROR, "AcmReceiver::DecoderByPayloadType",
               static_cast<int>(payload_type));
     return -1;
   }
-  memcpy(codec, &ACMCodecDB::database_[codec_index], sizeof(CodecInst));
+  const Decoder& decoder = it->second;
-  // Safe not to check the iterator
+  memcpy(codec, &ACMCodecDB::database_[decoder.acm_codec_id],
-  const Decoder& decoder = decoders_.find(codec_index)->second;
+         sizeof(CodecInst));
   codec->pltype = decoder.payload_type;
   codec->channels = decoder.channels;
   return 0;
 }
 
-int AcmReceiver::PayloadType2CodecIndex(uint8_t payload_type) const {
-  for (const auto& decoder_pair : decoders_) {
-    const Decoder& decoder = decoder_pair.second;
-    if (decoder.payload_type == payload_type)
-      return decoder.acm_codec_id;
-  }
-  return -1;
-}
-
 int AcmReceiver::EnableNack(size_t max_nack_list_size) {
   // Don't do anything if |max_nack_list_size| is out of range.
   if (max_nack_list_size == 0 || max_nack_list_size > Nack::kNackListSizeLimit)
@@ -714,9 +701,9 @@ int AcmReceiver::EnableNack(size_t max_nack_list_size) {
 
     // Sampling rate might need to be updated if we change from disable to
     // enable. Do it if the receive codec is valid.
-    if (last_audio_decoder_ >= 0) {
+    if (last_audio_decoder_) {
       nack_->UpdateSampleRate(
-          ACMCodecDB::database_[last_audio_decoder_].plfreq);
+          ACMCodecDB::database_[last_audio_decoder_->acm_codec_id].plfreq);
     }
   }
   return nack_->SetMaxNackListSize(max_nack_list_size);
@@ -779,9 +766,10 @@ bool AcmReceiver::GetSilence(int desired_sample_rate_hz, AudioFrame* frame) {
   call_stats_.DecodedBySilenceGenerator();
 
   // Set the values if already got a packet, otherwise set to default values.
-  if (last_audio_decoder_ >= 0) {
+  if (last_audio_decoder_) {
-    current_sample_rate_hz_ = ACMCodecDB::database_[last_audio_decoder_].plfreq;
+    current_sample_rate_hz_ =
-    frame->num_channels_ = decoders_[last_audio_decoder_].channels;
+        ACMCodecDB::database_[last_audio_decoder_->acm_codec_id].plfreq;
+    frame->num_channels_ = last_audio_decoder_->channels;
   } else {
     frame->num_channels_ = 1;
   }
@@ -801,19 +789,18 @@ bool AcmReceiver::GetSilence(int desired_sample_rate_hz, AudioFrame* frame) {
   return true;
 }
 
-int AcmReceiver::RtpHeaderToCodecIndex(
+const AcmReceiver::Decoder* AcmReceiver::RtpHeaderToDecoder(
-    const RTPHeader &rtp_header, const uint8_t* payload) const {
+    const RTPHeader& rtp_header,
-  uint8_t payload_type = rtp_header.payloadType;
+    const uint8_t* payload) const {
-  auto it = decoders_.find(ACMCodecDB::kRED);
+  auto it = decoders_.find(rtp_header.payloadType);
   if (ACMCodecDB::kRED >= 0 &&  // This ensures that RED is defined in WebRTC.
-      it != decoders_.end() &&
+      it != decoders_.end() && ACMCodecDB::kRED == it->second.acm_codec_id) {
-      payload_type == it->second.payload_type) {
     // This is a RED packet, get the payload of the audio codec.
-    payload_type = payload[0] & 0x7F;
+    it = decoders_.find(payload[0] & 0x7F);
   }
 
   // Check if the payload is registered.
-  return PayloadType2CodecIndex(payload_type);
+  return it != decoders_.end() ? &it->second : nullptr;
 }
 
 uint32_t AcmReceiver::NowInTimestamp(int decoder_sampling_rate) const {

webrtc/modules/audio_coding/main/acm2/acm_receiver.h

@@ -307,15 +307,14 @@ class AcmReceiver {
   void GetDecodingCallStatistics(AudioDecodingCallStats* stats) const;
 
  private:
-  int PayloadType2CodecIndex(uint8_t payload_type) const;
-
   bool GetSilence(int desired_sample_rate_hz, AudioFrame* frame)
       EXCLUSIVE_LOCKS_REQUIRED(crit_sect_);
 
   int GetNumSyncPacketToInsert(uint16_t received_squence_number);
 
-  int RtpHeaderToCodecIndex(
+  const Decoder* RtpHeaderToDecoder(const RTPHeader& rtp_header,
-      const RTPHeader& rtp_header, const uint8_t* payload) const;
+                                    const uint8_t* payload) const
+      EXCLUSIVE_LOCKS_REQUIRED(crit_sect_);
 
   uint32_t NowInTimestamp(int decoder_sampling_rate) const;
 
@@ -323,7 +322,7 @@ class AcmReceiver {
 
   rtc::scoped_ptr<CriticalSectionWrapper> crit_sect_;
   int id_;  // TODO(henrik.lundin) Make const.
-  int last_audio_decoder_ GUARDED_BY(crit_sect_);
+  const Decoder* last_audio_decoder_ GUARDED_BY(crit_sect_);
   AudioFrame::VADActivity previous_audio_activity_ GUARDED_BY(crit_sect_);
   int current_sample_rate_hz_ GUARDED_BY(crit_sect_);
   ACMResampler resampler_ GUARDED_BY(crit_sect_);
@@ -335,7 +334,8 @@ class AcmReceiver {
   bool nack_enabled_ GUARDED_BY(crit_sect_);
   CallStatistics call_stats_ GUARDED_BY(crit_sect_);
   NetEq* neteq_;
-  std::map<int, Decoder> decoders_;  // keyed by ACM codec ID
+  // Decoders map is keyed by payload type
+  std::map<uint8_t, Decoder> decoders_ GUARDED_BY(crit_sect_);
   bool vad_enabled_;
   Clock* clock_;  // TODO(henrik.lundin) Make const if possible.
   bool resampled_last_output_frame_ GUARDED_BY(crit_sect_);

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

@@ -177,27 +177,45 @@ TEST_F(AcmReceiverTest, DISABLED_ON_ANDROID(AddCodecGetCodec)) {
 }
 
 TEST_F(AcmReceiverTest, DISABLED_ON_ANDROID(AddCodecChangePayloadType)) {
-  CodecInst ref_codec;
   const int codec_id = ACMCodecDB::kPCMA;
-  EXPECT_EQ(0, ACMCodecDB::Codec(codec_id, &ref_codec));
+  CodecInst ref_codec1;
-  const int payload_type = ref_codec.pltype;
+  EXPECT_EQ(0, ACMCodecDB::Codec(codec_id, &ref_codec1));
-  EXPECT_EQ(0, receiver_->AddCodec(codec_id, ref_codec.pltype,
+  CodecInst ref_codec2 = ref_codec1;
-                                   ref_codec.channels, NULL));
+  ++ref_codec2.pltype;
   CodecInst test_codec;
-  EXPECT_EQ(0, receiver_->DecoderByPayloadType(payload_type, &test_codec));
-  EXPECT_EQ(true, CodecsEqual(ref_codec, test_codec));
 
-  // Re-register the same codec with different payload.
+  // Register the same codec with different payload types.
-  ref_codec.pltype = payload_type + 1;
+  EXPECT_EQ(0, receiver_->AddCodec(codec_id, ref_codec1.pltype,
-  EXPECT_EQ(0, receiver_->AddCodec(codec_id, ref_codec.pltype,
+                                   ref_codec1.channels, NULL));
-                                   ref_codec.channels, NULL));
+  EXPECT_EQ(0, receiver_->AddCodec(codec_id, ref_codec2.pltype,
+                                   ref_codec2.channels, NULL));
 
-  // Payload type |payload_type| should not exist.
+  // Both payload types should exist.
-  EXPECT_EQ(-1, receiver_->DecoderByPayloadType(payload_type, &test_codec));
+  EXPECT_EQ(0, receiver_->DecoderByPayloadType(ref_codec1.pltype, &test_codec));
+  EXPECT_EQ(true, CodecsEqual(ref_codec1, test_codec));
+  EXPECT_EQ(0, receiver_->DecoderByPayloadType(ref_codec2.pltype, &test_codec));
+  EXPECT_EQ(true, CodecsEqual(ref_codec2, test_codec));
+}
 
-  // Payload type |payload_type + 1| should exist.
+TEST_F(AcmReceiverTest, DISABLED_ON_ANDROID(AddCodecChangeCodecId)) {
-  EXPECT_EQ(0, receiver_->DecoderByPayloadType(payload_type + 1, &test_codec));
+  const int codec_id1 = ACMCodecDB::kPCMU;
-  EXPECT_TRUE(CodecsEqual(test_codec, ref_codec));
+  CodecInst ref_codec1;
+  EXPECT_EQ(0, ACMCodecDB::Codec(codec_id1, &ref_codec1));
+  const int codec_id2 = ACMCodecDB::kPCMA;
+  CodecInst ref_codec2;
+  EXPECT_EQ(0, ACMCodecDB::Codec(codec_id2, &ref_codec2));
+  ref_codec2.pltype = ref_codec1.pltype;
+  CodecInst test_codec;
+
+  // Register the same payload type with different codec ID.
+  EXPECT_EQ(0, receiver_->AddCodec(codec_id1, ref_codec1.pltype,
+                                   ref_codec1.channels, NULL));
+  EXPECT_EQ(0, receiver_->AddCodec(codec_id2, ref_codec2.pltype,
+                                   ref_codec2.channels, NULL));
+
+  // Make sure that the last codec is used.
+  EXPECT_EQ(0, receiver_->DecoderByPayloadType(ref_codec2.pltype, &test_codec));
+  EXPECT_EQ(true, CodecsEqual(ref_codec2, test_codec));
 }
 
 TEST_F(AcmReceiverTest, DISABLED_ON_ANDROID(AddCodecRemoveCodec)) {

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

@@ -177,27 +177,45 @@ TEST_F(AcmReceiverTestOldApi, DISABLED_ON_ANDROID(AddCodecGetCodec)) {
 }
 
 TEST_F(AcmReceiverTestOldApi, DISABLED_ON_ANDROID(AddCodecChangePayloadType)) {
-  CodecInst ref_codec;
   const int codec_id = ACMCodecDB::kPCMA;
-  EXPECT_EQ(0, ACMCodecDB::Codec(codec_id, &ref_codec));
+  CodecInst ref_codec1;
-  const int payload_type = ref_codec.pltype;
+  EXPECT_EQ(0, ACMCodecDB::Codec(codec_id, &ref_codec1));
-  EXPECT_EQ(0, receiver_->AddCodec(codec_id, ref_codec.pltype,
+  CodecInst ref_codec2 = ref_codec1;
-                                   ref_codec.channels, NULL));
+  ++ref_codec2.pltype;
   CodecInst test_codec;
-  EXPECT_EQ(0, receiver_->DecoderByPayloadType(payload_type, &test_codec));
-  EXPECT_EQ(true, CodecsEqual(ref_codec, test_codec));
 
-  // Re-register the same codec with different payload.
+  // Register the same codec with different payloads.
-  ref_codec.pltype = payload_type + 1;
+  EXPECT_EQ(0, receiver_->AddCodec(codec_id, ref_codec1.pltype,
-  EXPECT_EQ(0, receiver_->AddCodec(codec_id, ref_codec.pltype,
+                                   ref_codec1.channels, NULL));
-                                   ref_codec.channels, NULL));
+  EXPECT_EQ(0, receiver_->AddCodec(codec_id, ref_codec2.pltype,
+                                   ref_codec2.channels, NULL));
 
-  // Payload type |payload_type| should not exist.
+  // Both payload types should exist.
-  EXPECT_EQ(-1, receiver_->DecoderByPayloadType(payload_type, &test_codec));
+  EXPECT_EQ(0, receiver_->DecoderByPayloadType(ref_codec1.pltype, &test_codec));
+  EXPECT_EQ(true, CodecsEqual(ref_codec1, test_codec));
+  EXPECT_EQ(0, receiver_->DecoderByPayloadType(ref_codec2.pltype, &test_codec));
+  EXPECT_EQ(true, CodecsEqual(ref_codec2, test_codec));
+}
 
-  // Payload type |payload_type + 1| should exist.
+TEST_F(AcmReceiverTestOldApi, DISABLED_ON_ANDROID(AddCodecChangeCodecId)) {
-  EXPECT_EQ(0, receiver_->DecoderByPayloadType(payload_type + 1, &test_codec));
+  const int codec_id1 = ACMCodecDB::kPCMU;
-  EXPECT_TRUE(CodecsEqual(test_codec, ref_codec));
+  CodecInst ref_codec1;
+  EXPECT_EQ(0, ACMCodecDB::Codec(codec_id1, &ref_codec1));
+  const int codec_id2 = ACMCodecDB::kPCMA;
+  CodecInst ref_codec2;
+  EXPECT_EQ(0, ACMCodecDB::Codec(codec_id2, &ref_codec2));
+  ref_codec2.pltype = ref_codec1.pltype;
+  CodecInst test_codec;
+
+  // Register the same payload type with different codec ID.
+  EXPECT_EQ(0, receiver_->AddCodec(codec_id1, ref_codec1.pltype,
+                                   ref_codec1.channels, NULL));
+  EXPECT_EQ(0, receiver_->AddCodec(codec_id2, ref_codec2.pltype,
+                                   ref_codec2.channels, NULL));
+
+  // Make sure that the last codec is used.
+  EXPECT_EQ(0, receiver_->DecoderByPayloadType(ref_codec2.pltype, &test_codec));
+  EXPECT_EQ(true, CodecsEqual(ref_codec2, test_codec));
 }
 
 TEST_F(AcmReceiverTestOldApi, DISABLED_ON_ANDROID(AddCodecRemoveCodec)) {