Change ACM's CodecManager to hold one encoder instead of an array

With this change, the currently used encoder is held in a scoped_ptr.
iSAC is a special case, since the encoder instance is also a decoder
instance, so it may have to be available also if another send codec is
used. This is accomplished by having a separate scoped_ptr for iSAC.

Remove mirror ID from ACM codec database functions, and remove unused
functions from the database.

COAUTHOR=kwiberg@webrtc.org
BUG=4228
R=tina.legrand@webrtc.org

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

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

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

@@ -227,7 +227,7 @@ enum {
 // Gets the codec id number from the database. If there is some mismatch in
 // the codec settings, the function will return an error code.
 // NOTE! The first mismatch found will generate the return value.
-int ACMCodecDB::CodecNumber(const CodecInst& codec_inst, int* mirror_id) {
+int ACMCodecDB::CodecNumber(const CodecInst& codec_inst) {
   // Look for a matching codec in the database.
   int codec_id = CodecId(codec_inst);
 
@@ -243,13 +243,11 @@ int ACMCodecDB::CodecNumber(const CodecInst& codec_inst, int* mirror_id) {
 
   // Comfort Noise is special case, packet-size & rate is not checked.
   if (STR_CASE_CMP(database_[codec_id].plname, "CN") == 0) {
-    *mirror_id = codec_id;
     return codec_id;
   }
 
   // RED is special case, packet-size & rate is not checked.
   if (STR_CASE_CMP(database_[codec_id].plname, "red") == 0) {
-    *mirror_id = codec_id;
     return codec_id;
   }
 
@@ -278,16 +276,8 @@ int ACMCodecDB::CodecNumber(const CodecInst& codec_inst, int* mirror_id) {
 
   // Check the validity of rate. Codecs with multiple rates have their own
   // function for this.
-  *mirror_id = codec_id;
   if (STR_CASE_CMP("isac", codec_inst.plname) == 0) {
-    if (IsISACRateValid(codec_inst.rate)) {
+    return IsISACRateValid(codec_inst.rate) ? codec_id : kInvalidRate;
-      // Set mirrorID to iSAC WB which is only created once to be used both for
-      // iSAC WB and SWB, because they need to share struct.
-      *mirror_id = kISAC;
-      return  codec_id;
-    } else {
-      return kInvalidRate;
-    }
   } else if (STR_CASE_CMP("ilbc", codec_inst.plname) == 0) {
     return IsILBCRateValid(codec_inst.rate, codec_inst.pacsize)
         ? codec_id : kInvalidRate;
@@ -350,22 +340,10 @@ int ACMCodecDB::CodecId(const char* payload_name, int frequency, int channels) {
   // We didn't find a matching codec.
   return -1;
 }
-// Gets codec id number, and mirror id, from database for the receiver.
+// Gets codec id number from database for the receiver.
-int ACMCodecDB::ReceiverCodecNumber(const CodecInst& codec_inst,
+int ACMCodecDB::ReceiverCodecNumber(const CodecInst& codec_inst) {
-                                    int* mirror_id) {
   // Look for a matching codec in the database.
-  int codec_id = CodecId(codec_inst);
+  return CodecId(codec_inst);
-
-  // Set |mirror_id| to |codec_id|, except for iSAC. In case of iSAC we always
-  // set |mirror_id| to iSAC WB (kISAC) which is only created once to be used
-  // both for iSAC WB and SWB, because they need to share struct.
-  if (STR_CASE_CMP(codec_inst.plname, "ISAC") != 0) {
-    *mirror_id = codec_id;
-  } else {
-    *mirror_id = kISAC;
-  }
-
-  return codec_id;
 }
 
 // Returns the codec sampling frequency for codec with id = "codec_id" in
@@ -394,16 +372,6 @@ const NetEqDecoder* ACMCodecDB::NetEQDecoders() {
   return neteq_decoders_;
 }
 
-// Gets mirror id. The Id is used for codecs sharing struct for settings that
-// need different payload types.
-int ACMCodecDB::MirrorID(int codec_id) {
-  if (STR_CASE_CMP(database_[codec_id].plname, "isac") == 0) {
-    return kISAC;
-  } else {
-    return codec_id;
-  }
-}
-
 // Creates memory/instance for storing codec state.
 ACMGenericCodec* ACMCodecDB::CreateCodecInstance(const CodecInst& codec_inst,
                                                  int cng_pt_nb,

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

@@ -166,36 +166,17 @@ class ACMCodecDB {
   //   0 if successful, otherwise -1.
   static int Codec(int codec_id, CodecInst* codec_inst);
 
-  // Returns codec id and mirror id from database, given the information
+  // Returns codec id from database, given the information received in the input
-  // received in the input [codec_inst]. Mirror id is a number that tells
+  // [codec_inst].
-  // where to find the codec's memory (instance). The number is either the
-  // same as codec id (most common), or a number pointing at a different
-  // entry in the database, if the codec has several entries with different
-  // payload types. This is used for codecs that must share one struct even if
-  // the payload type differs.
-  // One example is the codec iSAC which has the same struct for both 16 and
-  // 32 khz, but they have different entries in the database. Let's say the
-  // function is called with iSAC 32kHz. The function will return 1 as that is
-  // the entry in the data base, and [mirror_id] = 0, as that is the entry for
-  // iSAC 16 kHz, which holds the shared memory.
   // Input:
   //   [codec_inst] - Information about the codec for which we require the
   //                  database id.
-  // Output:
-  //   [mirror_id] - mirror id, which most often is the same as the return
-  //                 value, see above.
-  //   [err_message] - if present, in the event of a mismatch found between the
-  //                   input and the database, a descriptive error message is
-  //                   written here.
-  //   [err_message] - if present, the length of error message is returned here.
   // Return:
   //   codec id if successful, otherwise < 0.
-  static int CodecNumber(const CodecInst& codec_inst, int* mirror_id,
+  static int CodecNumber(const CodecInst& codec_inst);
-                         char* err_message, int max_message_len_byte);
-  static int CodecNumber(const CodecInst& codec_inst, int* mirror_id);
   static int CodecId(const CodecInst& codec_inst);
   static int CodecId(const char* payload_name, int frequency, int channels);
-  static int ReceiverCodecNumber(const CodecInst& codec_inst, int* mirror_id);
+  static int ReceiverCodecNumber(const CodecInst& codec_inst);
 
   // Returns the codec sampling frequency for codec with id = "codec_id" in
   // database.
@@ -221,19 +202,6 @@ class ACMCodecDB {
   // Returns the NetEQ decoder database.
   static const NetEqDecoder* NetEQDecoders();
 
-  // Returns mirror id, which is a number that tells where to find the codec's
-  // memory (instance). It is either the same as codec id (most common), or a
-  // number pointing at a different entry in the database, if the codec have
-  // several entries with different payload types. This is used for codecs that
-  // must share struct even if the payload type differs.
-  // TODO(tlegrand): Check if function is needed, or if we can change
-  // to access database directly.
-  // Input:
-  //   [codec_id] - number that specifies codec's position in the database.
-  // Return:
-  //   Mirror id on success, otherwise -1.
-  static int MirrorID(int codec_id);
-
   // Creates a codec wrapper containing an AudioEncoder object (or an
   // ACMGenericCodec subclass during the refactoring time). The type of
   // AudioEncoder is decided by looking at the information in |codec_inst|.

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

@@ -81,9 +81,7 @@ int AudioCodingModule::Codec(const char* payload_name,
 
 // Checks the validity of the parameters of the given codec
 bool AudioCodingModule::IsCodecValid(const CodecInst& codec) {
-  int mirror_id;
+  int codec_number = acm2::ACMCodecDB::CodecNumber(codec);
-
-  int codec_number = acm2::ACMCodecDB::CodecNumber(codec, &mirror_id);
 
   if (codec_number < 0) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, -1,

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

@@ -34,9 +34,7 @@ bool IsCodecCN(int index) {
 }
 
 // Check if the given codec is a valid to be registered as send codec.
-int IsValidSendCodec(const CodecInst& send_codec,
+int IsValidSendCodec(const CodecInst& send_codec, bool is_primary_encoder) {
-                     bool is_primary_encoder,
-                     int* mirror_id) {
   int dummy_id = 0;
   if ((send_codec.channels != 1) && (send_codec.channels != 2)) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
@@ -47,7 +45,7 @@ int IsValidSendCodec(const CodecInst& send_codec,
     return -1;
   }
 
-  int codec_id = ACMCodecDB::CodecNumber(send_codec, mirror_id);
+  int codec_id = ACMCodecDB::CodecNumber(send_codec);
   if (codec_id < 0) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
                  "Invalid codec setting for the send codec.");
@@ -68,7 +66,6 @@ int IsValidSendCodec(const CodecInst& send_codec,
   if (!STR_CASE_CMP(send_codec.plname, "telephone-event")) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
                  "telephone-event cannot be a send codec");
-    *mirror_id = -1;
     return -1;
   }
 
@@ -77,7 +74,6 @@ int IsValidSendCodec(const CodecInst& send_codec,
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
                  "%d number of channels not supportedn for %s.",
                  send_codec.channels, send_codec.plname);
-    *mirror_id = -1;
     return -1;
   }
 
@@ -87,20 +83,22 @@ int IsValidSendCodec(const CodecInst& send_codec,
     if (IsCodecRED(&send_codec)) {
       WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
                    "RED cannot be secondary codec");
-      *mirror_id = -1;
       return -1;
     }
 
     if (IsCodecCN(&send_codec)) {
       WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
                    "DTX cannot be secondary codec");
-      *mirror_id = -1;
       return -1;
     }
   }
   return codec_id;
 }
 
+bool IsIsac(const CodecInst& codec) {
+  return !STR_CASE_CMP(codec.plname, "isac");
+}
+
 const CodecInst kEmptyCodecInst = {-1, "noCodecRegistered", 0, 0, 0, 0};
 }  // namespace
 
@@ -119,11 +117,6 @@ CodecManager::CodecManager(AudioCodingModuleImpl* acm)
       send_codec_inst_(kEmptyCodecInst),
       red_enabled_(false),
       codec_fec_enabled_(false) {
-  for (int i = 0; i < ACMCodecDB::kMaxNumCodecs; i++) {
-    codecs_[i] = nullptr;
-    mirror_codec_idx_[i] = -1;
-  }
-
   // Register the default payload type for RED and for CNG at sampling rates of
   // 8, 16, 32 and 48 kHz.
   for (int i = (ACMCodecDB::kNumCodecs - 1); i >= 0; i--) {
@@ -144,25 +137,11 @@ CodecManager::CodecManager(AudioCodingModuleImpl* acm)
   thread_checker_.DetachFromThread();
 }
 
-CodecManager::~CodecManager() {
+CodecManager::~CodecManager() = default;
-  for (int i = 0; i < ACMCodecDB::kMaxNumCodecs; i++) {
-    if (codecs_[i] != NULL) {
-      // Mirror index holds the address of the codec memory.
-      assert(mirror_codec_idx_[i] > -1);
-      if (codecs_[mirror_codec_idx_[i]] != NULL) {
-        delete codecs_[mirror_codec_idx_[i]];
-        codecs_[mirror_codec_idx_[i]] = NULL;
-      }
-
-      codecs_[i] = NULL;
-    }
-  }
-}
 
 int CodecManager::RegisterSendCodec(const CodecInst& send_codec) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  int mirror_id;
+  int codec_id = IsValidSendCodec(send_codec, true);
-  int codec_id = IsValidSendCodec(send_codec, true, &mirror_id);
 
   // Check for reported errors from function IsValidSendCodec().
   if (codec_id < 0) {
@@ -243,36 +222,31 @@ int CodecManager::RegisterSendCodec(const CodecInst& send_codec) {
   // Check if the codec is already registered as send codec.
   bool is_send_codec;
   if (current_encoder_) {
-    int send_codec_mirror_id;
+    int send_codec_id = ACMCodecDB::CodecNumber(send_codec_inst_);
-    int send_codec_id =
-        ACMCodecDB::CodecNumber(send_codec_inst_, &send_codec_mirror_id);
     assert(send_codec_id >= 0);
-    is_send_codec =
+    is_send_codec = send_codec_id == codec_id;
-        (send_codec_id == codec_id) || (mirror_id == send_codec_mirror_id);
   } else {
     is_send_codec = false;
   }
 
   // If new codec, or new settings, register.
   if (!is_send_codec) {
-    if (!codecs_[mirror_id]) {
+    ACMGenericCodec* new_codec;
-      codecs_[mirror_id] = ACMCodecDB::CreateCodecInstance(
+    if (!IsIsac(send_codec)) {
+      encoder_.reset(ACMCodecDB::CreateCodecInstance(
           send_codec, cng_nb_pltype_, cng_wb_pltype_, cng_swb_pltype_,
-          cng_fb_pltype_, red_enabled_, red_nb_pltype_);
+          cng_fb_pltype_, red_enabled_, red_nb_pltype_));
-      if (!codecs_[mirror_id]) {
+      new_codec = encoder_.get();
-        WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
+    } else {
-                     "Cannot Create the codec");
+      if (!isac_enc_dec_) {
-        return -1;
+        isac_enc_dec_.reset(ACMCodecDB::CreateCodecInstance(
+            send_codec, cng_nb_pltype_, cng_wb_pltype_, cng_swb_pltype_,
+            cng_fb_pltype_, red_enabled_, red_nb_pltype_));
       }
-      mirror_codec_idx_[mirror_id] = mirror_id;
+      new_codec = isac_enc_dec_.get();
     }
+    DCHECK(new_codec);
 
-    if (mirror_id != codec_id) {
-      codecs_[codec_id] = codecs_[mirror_id];
-      mirror_codec_idx_[codec_id] = mirror_id;
-    }
-
-    ACMGenericCodec* codec_ptr = codecs_[codec_id];
     WebRtcACMCodecParams codec_params;
 
     memcpy(&(codec_params.codec_inst), &send_codec, sizeof(CodecInst));
@@ -280,7 +254,7 @@ int CodecManager::RegisterSendCodec(const CodecInst& send_codec) {
     codec_params.enable_dtx = dtx_enabled_;
     codec_params.vad_mode = vad_mode_;
     // Force initialization.
-    if (codec_ptr->InitEncoder(&codec_params, true) < 0) {
+    if (new_codec->InitEncoder(&codec_params, true) < 0) {
       // Could not initialize the encoder.
 
       // Check if already have a registered codec.
@@ -306,18 +280,18 @@ int CodecManager::RegisterSendCodec(const CodecInst& send_codec) {
       // If we change codec we start fresh with RED.
       // This is not strictly required by the standard.
 
-      if (codec_ptr->SetCopyRed(red_enabled_) < 0) {
+      if (new_codec->SetCopyRed(red_enabled_) < 0) {
         // We tried to preserve the old red status, if failed, it means the
         // red status has to be flipped.
         red_enabled_ = !red_enabled_;
       }
 
-      codec_ptr->SetVAD(&dtx_enabled_, &vad_enabled_, &vad_mode_);
+      new_codec->SetVAD(&dtx_enabled_, &vad_enabled_, &vad_mode_);
 
-      if (!codec_ptr->HasInternalFEC()) {
+      if (!new_codec->HasInternalFEC()) {
         codec_fec_enabled_ = false;
       } else {
-        if (codec_ptr->SetFEC(codec_fec_enabled_) < 0) {
+        if (new_codec->SetFEC(codec_fec_enabled_) < 0) {
           WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
                        "Cannot set codec FEC");
           return -1;
@@ -325,7 +299,7 @@ int CodecManager::RegisterSendCodec(const CodecInst& send_codec) {
       }
     }
 
-    current_encoder_ = codecs_[codec_id];
+    current_encoder_ = new_codec;
     DCHECK(current_encoder_);
     memcpy(&send_codec_inst_, &send_codec, sizeof(CodecInst));
     return 0;
@@ -335,11 +309,6 @@ int CodecManager::RegisterSendCodec(const CodecInst& send_codec) {
     // If any parameter is valid then apply it and record.
     bool force_init = false;
 
-    if (mirror_id != codec_id) {
-      codecs_[codec_id] = codecs_[mirror_id];
-      mirror_codec_idx_[codec_id] = mirror_id;
-    }
-
     // Check the payload type.
     if (send_codec.pltype != send_codec_inst_.pltype) {
       // At this point check if the given payload type is valid.
@@ -396,7 +365,7 @@ int CodecManager::RegisterSendCodec(const CodecInst& send_codec) {
 
     // Check if a change in Rate is required.
     if (send_codec.rate != send_codec_inst_.rate) {
-      if (codecs_[codec_id]->SetBitRate(send_codec.rate) < 0) {
+      if (current_encoder_->SetBitRate(send_codec.rate) < 0) {
         WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
                      "Could not change the codec rate.");
         return -1;
@@ -404,10 +373,10 @@ int CodecManager::RegisterSendCodec(const CodecInst& send_codec) {
       send_codec_inst_.rate = send_codec.rate;
     }
 
-    if (!codecs_[codec_id]->HasInternalFEC()) {
+    if (!current_encoder_->HasInternalFEC()) {
       codec_fec_enabled_ = false;
     } else {
-      if (codecs_[codec_id]->SetFEC(codec_fec_enabled_) < 0) {
+      if (current_encoder_->SetFEC(codec_fec_enabled_) < 0) {
         WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, dummy_id,
                      "Cannot set codec FEC");
         return -1;
@@ -445,8 +414,7 @@ int CodecManager::RegisterReceiveCodec(const CodecInst& codec) {
     return -1;
   }
 
-  int mirror_id;
+  int codec_id = ACMCodecDB::ReceiverCodecNumber(codec);
-  int codec_id = ACMCodecDB::ReceiverCodecNumber(codec, &mirror_id);
 
   if (codec_id < 0 || codec_id >= ACMCodecDB::kNumCodecs) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, 0,
@@ -464,7 +432,7 @@ int CodecManager::RegisterReceiveCodec(const CodecInst& codec) {
   AudioDecoder* decoder = NULL;
   // Get |decoder| associated with |codec|. |decoder| can be NULL if |codec|
   // does not own its decoder.
-  if (GetAudioDecoder(codec, codec_id, mirror_id, &decoder) < 0) {
+  if (GetAudioDecoder(codec, codec_id, &decoder) < 0) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, 0,
                  "Wrong codec params to be registered as receive codec");
     return -1;
@@ -554,57 +522,43 @@ int CodecManager::SetCodecFEC(bool enable_codec_fec) {
 }
 
 void CodecManager::SetCngPayloadType(int sample_rate_hz, int payload_type) {
-  for (auto* codec : codecs_) {
+  if (isac_enc_dec_)
-    if (codec) {
+    isac_enc_dec_->SetCngPt(sample_rate_hz, payload_type);
-      codec->SetCngPt(sample_rate_hz, payload_type);
+  if (encoder_)
-    }
+    encoder_->SetCngPt(sample_rate_hz, payload_type);
-  }
 }
 
 void CodecManager::SetRedPayloadType(int sample_rate_hz, int payload_type) {
-  for (auto* codec : codecs_) {
+  if (isac_enc_dec_)
-    if (codec) {
+    isac_enc_dec_->SetRedPt(sample_rate_hz, payload_type);
-      codec->SetRedPt(sample_rate_hz, payload_type);
+  if (encoder_)
-    }
+    encoder_->SetRedPt(sample_rate_hz, payload_type);
-  }
 }
 
 int CodecManager::GetAudioDecoder(const CodecInst& codec,
                                   int codec_id,
-                                  int mirror_id,
                                   AudioDecoder** decoder) {
-  if (ACMCodecDB::OwnsDecoder(codec_id)) {
+  if (!ACMCodecDB::OwnsDecoder(codec_id)) {
-    // This codec has to own its own decoder. Therefore, it should create the
+    DCHECK(!IsIsac(codec)) << "Codec must not be iSAC at this point.";
-    // corresponding AudioDecoder class and insert it into NetEq. If the codec
+    *decoder = nullptr;
-    // does not exist create it.
+    return 0;
-    //
-    // TODO(turajs): this part of the code is common with RegisterSendCodec(),
-    //               make a method for it.
-    if (codecs_[mirror_id] == NULL) {
-      codecs_[mirror_id] = ACMCodecDB::CreateCodecInstance(
-          codec, cng_nb_pltype_, cng_wb_pltype_, cng_swb_pltype_,
-          cng_fb_pltype_, red_enabled_, red_nb_pltype_);
-      if (codecs_[mirror_id] == NULL) {
-        WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, 0,
-                     "Cannot Create the codec");
-        return -1;
-      }
-      mirror_codec_idx_[mirror_id] = mirror_id;
-    }
-
-    if (mirror_id != codec_id) {
-      codecs_[codec_id] = codecs_[mirror_id];
-      mirror_codec_idx_[codec_id] = mirror_id;
-    }
-    *decoder = codecs_[codec_id]->Decoder();
-    if (!*decoder) {
-      assert(false);
-      return -1;
-    }
-  } else {
-    *decoder = NULL;
   }
-
+  DCHECK(IsIsac(codec)) << "Codec must be iSAC at this point.";
+  // This codec has to own its own decoder. Therefore, it should create the
+  // corresponding AudioDecoder class and insert it into NetEq. If the codec
+  // does not exist create it.
+  //
+  // TODO(turajs): this part of the code is common with RegisterSendCodec(),
+  //               make a method for it.
+  if (!isac_enc_dec_) {
+    isac_enc_dec_.reset(ACMCodecDB::CreateCodecInstance(
+        codec, cng_nb_pltype_, cng_wb_pltype_, cng_swb_pltype_, cng_fb_pltype_,
+        red_enabled_, red_nb_pltype_));
+    if (!isac_enc_dec_)
+      return -1;
+  }
+  *decoder = isac_enc_dec_->Decoder();
+  DCHECK(*decoder);
   return 0;
 }
 

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

@@ -72,7 +72,6 @@ class CodecManager final {
   // valid pointer, otherwise it will be NULL.
   int GetAudioDecoder(const CodecInst& codec,
                       int codec_id,
-                      int mirror_id,
                       AudioDecoder** decoder);
 
   AudioCodingModuleImpl* acm_;
@@ -90,8 +89,8 @@ class CodecManager final {
   CodecInst send_codec_inst_;
   bool red_enabled_;
   bool codec_fec_enabled_;
-  ACMGenericCodec* codecs_[ACMCodecDB::kMaxNumCodecs];
+  rtc::scoped_ptr<ACMGenericCodec> isac_enc_dec_;
-  int mirror_codec_idx_[ACMCodecDB::kMaxNumCodecs];
+  rtc::scoped_ptr<ACMGenericCodec> encoder_;
 
   DISALLOW_COPY_AND_ASSIGN(CodecManager);
 };