Stopped using the NetEqDecoder enum internally in NetEq.

webrtc/modules/audio_coding/neteq/decoder_database.cc

Index: webrtc/modules/audio_coding/neteq/decoder_database.cc
diff --git a/webrtc/modules/audio_coding/neteq/decoder_database.cc b/webrtc/modules/audio_coding/neteq/decoder_database.cc
index b189e4bb562ae77872ce3380dfbb977d3bd4756a..2a2e24e27c3482178ff9fd3fbff6d7aee389f87b 100644
--- a/webrtc/modules/audio_coding/neteq/decoder_database.cc
+++ b/webrtc/modules/audio_coding/neteq/decoder_database.cc
@@ -27,22 +27,24 @@ DecoderDatabase::DecoderDatabase(
 DecoderDatabase::~DecoderDatabase() = default;
 
 DecoderDatabase::DecoderInfo::DecoderInfo(
+    const SdpAudioFormat& audio_format,
+    AudioDecoderFactory* factory)
+    : audio_format_(audio_format),
+      factory_(factory),
+      external_decoder_(nullptr),
+      cng_decoder_(CngDecoder::Create(audio_format)) {}
+
+DecoderDatabase::DecoderInfo::DecoderInfo(
     NetEqDecoder ct,
-    const std::string& nm,
     AudioDecoderFactory* factory)
-    : codec_type(ct),
-      name(nm),
-      audio_format_(acm2::RentACodec::NetEqDecoderToSdpAudioFormat(ct)),
+    : audio_format_(*acm2::RentACodec::NetEqDecoderToSdpAudioFormat(ct)),

[kwiberg-webrtc, 2016/09/20 16:05:43]

This conversion can still fail, right? Maybe DCHECK that it doesn't?

[ossu, 2016/09/21 09:10:55]

Well if it fails, it'll do an RTC_DCHECK in Optional (RTC_DCHECK(has_value_)),
so it's already covered.

[kwiberg-webrtc, 2016/09/21 09:21:19]

Yes, but a DCHECK here will make the problem much easier to diagnose. But since
you'll have to introduce a lambda just to have somewhere to put the DCHECK
statement, I guess it might not be worth it.

       factory_(factory),
       external_decoder_(nullptr),
-      cng_decoder_(CngDecoder::Create(ct)) {}
+      cng_decoder_(CngDecoder::Create(audio_format_)) {}
 
-DecoderDatabase::DecoderInfo::DecoderInfo(NetEqDecoder ct,
-                                          const std::string& nm,
+DecoderDatabase::DecoderInfo::DecoderInfo(const SdpAudioFormat& audio_format,
                                           AudioDecoder* ext_dec)
-    : codec_type(ct),
-      name(nm),
-      audio_format_(acm2::RentACodec::NetEqDecoderToSdpAudioFormat(ct)),
+    : audio_format_(audio_format),
       external_decoder_(ext_dec) {
   RTC_CHECK(ext_dec);
 }
@@ -51,55 +53,51 @@ DecoderDatabase::DecoderInfo::DecoderInfo(DecoderInfo&&) = default;
 DecoderDatabase::DecoderInfo::~DecoderInfo() = default;
 
 AudioDecoder* DecoderDatabase::DecoderInfo::GetDecoder() const {
+  if (IsDtmf() || IsRed() || IsComfortNoise()) {
+    // These are not real decoders.

[kwiberg-webrtc, 2016/09/20 16:05:43]

// These are not the decoders you are looking for.

[ossu, 2016/09/21 09:10:55]

Yeah, I should phrase this differently. It's disparaging to call them "not
real". :(

[kwiberg-webrtc, 2016/09/21 09:21:19]

Well, that too. But my suggestion was because we compile with -Werror
-Wmissing-star-wars-reference.

[ossu, 2016/09/21 15:46:30]

:)

+    return nullptr;
+  }
   if (external_decoder_) {
     RTC_DCHECK(!decoder_);
     RTC_DCHECK(!cng_decoder_);
     return external_decoder_;
   }
-  if (IsRed() || IsComfortNoise() || IsDtmf())
-    return nullptr;
-  RTC_DCHECK(audio_format_);
   if (!decoder_) {
+    // TODO(ossu): Keep a check here for now, since a number of tests create
+    // DecoderInfos without factories.
     RTC_DCHECK(factory_);
-    decoder_ = factory_->MakeAudioDecoder(*audio_format_);
+    decoder_ = factory_->MakeAudioDecoder(audio_format_);
   }
-  RTC_DCHECK(decoder_) << "Failed to create: " << *audio_format_;
+  RTC_DCHECK(decoder_) << "Failed to create: " << audio_format_;
   return decoder_.get();
 }
 
-
 bool DecoderDatabase::DecoderInfo::IsComfortNoise() const {
-  return codec_type == NetEqDecoder::kDecoderCNGnb
-      || codec_type == NetEqDecoder::kDecoderCNGwb
-      || codec_type == NetEqDecoder::kDecoderCNGswb32kHz
-      || codec_type == NetEqDecoder::kDecoderCNGswb48kHz;
+  return IsType("CN");

[kwiberg-webrtc, 2016/09/20 16:05:43]

Or

  RTC_DCHECK_EQ(!!cng_decoder_, IsType("CN"));
  return !!cng_decoder_;

It's faster, and makes use of (and documents and checks) one of the invariants
you establish.

[ossu, 2016/09/21 09:10:55]

I guess that's fair. We've already done the string check when setting up
cng_decoder_, so might as well use that information directly.

[kwiberg-webrtc, 2016/09/21 09:21:19]

Yes. And by relying on it, we strengthen the invariant.

 }
 
 bool DecoderDatabase::DecoderInfo::IsDtmf() const {
-  return codec_type == NetEqDecoder::kDecoderAVT;
+  return IsType("telephone-event");
 }
 
 bool DecoderDatabase::DecoderInfo::IsRed() const {
-  return codec_type == NetEqDecoder::kDecoderRED;
+  return IsType("red");
+}
+
+bool DecoderDatabase::DecoderInfo::IsType(const char* name) const {
+  return STR_CASE_CMP(audio_format_.name.c_str(), name) == 0;
 }
 
-rtc::Optional<DecoderDatabase::DecoderInfo::CngDecoder>
-DecoderDatabase::DecoderInfo::CngDecoder::Create(NetEqDecoder ct) {
-  const auto cng = [](int sample_rate_hz) {
-    return rtc::Optional<DecoderDatabase::DecoderInfo::CngDecoder>(
-        {sample_rate_hz});
-  };
-  switch (ct) {
-    case NetEqDecoder::kDecoderCNGnb:
-      return cng(8000);
-    case NetEqDecoder::kDecoderCNGwb:
-      return cng(16000);
-    case NetEqDecoder::kDecoderCNGswb32kHz:
-      return cng(32000);
-    case NetEqDecoder::kDecoderCNGswb48kHz:
-      return cng(48000);
-    default:
-      return rtc::Optional<DecoderDatabase::DecoderInfo::CngDecoder>();
+bool DecoderDatabase::DecoderInfo::IsType(const std::string& name) const {
+  return IsType(name.c_str());
+}
+
+rtc::Optional<DecoderDatabase::DecoderInfo::CngDecoder> DecoderDatabase::
+        DecoderInfo::CngDecoder::Create(const SdpAudioFormat& format) {
+  if (STR_CASE_CMP(format.name.c_str(), "CN") == 0) {
+    return rtc::Optional<CngDecoder>({format.clockrate_hz});
+  } else {
+    return rtc::Optional<CngDecoder>();
   }
 }
 
@@ -119,10 +117,17 @@ int DecoderDatabase::RegisterPayload(uint8_t rtp_payload_type,
   if (rtp_payload_type > 0x7F) {
     return kInvalidRtpPayloadType;
   }
-  if (!CodecSupported(codec_type)) {
+  // kCodecArbitrary is only supported through InsertExternal.
+  if (codec_type == NetEqDecoder::kDecoderArbitrary ||
+      !CodecSupported(codec_type)) {
     return kCodecNotSupported;
   }
-  DecoderInfo info(codec_type, name, decoder_factory_.get());
+  auto opt_format = acm2::RentACodec::NetEqDecoderToSdpAudioFormat(codec_type);
+  if (!opt_format) {
+    return kCodecNotSupported;
+  }
+  DecoderInfo info(*opt_format, decoder_factory_);
+  info.name = name;
   auto ret =
       decoders_.insert(std::make_pair(rtp_payload_type, std::move(info)));
   if (ret.second == false) {
@@ -139,14 +144,17 @@ int DecoderDatabase::InsertExternal(uint8_t rtp_payload_type,
   if (rtp_payload_type > 0x7F) {
     return kInvalidRtpPayloadType;
   }
-  if (!CodecSupported(codec_type)) {
-    return kCodecNotSupported;
-  }
   if (!decoder) {
     return kInvalidPointer;
   }
+
+  auto opt_db_format =

[kwiberg-webrtc, 2016/09/20 16:05:43]

const

[ossu, 2016/09/21 09:10:55]

Acknowledged.

+      acm2::RentACodec::NetEqDecoderToSdpAudioFormat(codec_type);
+  SdpAudioFormat format = opt_db_format.value_or({"arbitrary", 0, 0});

[kwiberg-webrtc, 2016/09/20 16:05:43]

const

[ossu, 2016/09/21 09:10:55]

Acknowledged.

+
   std::pair<DecoderMap::iterator, bool> ret;
-  DecoderInfo info(codec_type, codec_name, decoder);
+  DecoderInfo info(format, decoder);
+  info.name = codec_name;
   ret = decoders_.insert(std::make_pair(rtp_payload_type, std::move(info)));
   if (ret.second == false) {
     // Database already contains a decoder with type |rtp_payload_type|.
@@ -176,20 +184,7 @@ const DecoderDatabase::DecoderInfo* DecoderDatabase::GetDecoderInfo(
     // Decoder not found.
     return NULL;
   }
-  return &(*it).second;
-}
-
-uint8_t DecoderDatabase::GetRtpPayloadType(
-    NetEqDecoder codec_type) const {
-  DecoderMap::const_iterator it;
-  for (it = decoders_.begin(); it != decoders_.end(); ++it) {
-    if ((*it).second.codec_type == codec_type) {
-      // Match found.
-      return (*it).first;
-    }
-  }
-  // No match.
-  return kRtpPayloadTypeError;
+  return &it->second;
 }
 
 int DecoderDatabase::SetActiveDecoder(uint8_t rtp_payload_type,
@@ -258,10 +253,14 @@ AudioDecoder* DecoderDatabase::GetDecoder(uint8_t rtp_payload_type) const {
   return info ? info->GetDecoder() : nullptr;
 }
 
+bool DecoderDatabase::IsType(uint8_t rtp_payload_type, const char* name) const {
+  const DecoderInfo* info = GetDecoderInfo(rtp_payload_type);
+  return info && info->IsType(name);
+}
+
 bool DecoderDatabase::IsType(uint8_t rtp_payload_type,
-                             NetEqDecoder codec_type) const {
-  const DecoderInfo *info = GetDecoderInfo(rtp_payload_type);
-  return info && info->codec_type == codec_type;
+                             const std::string& name) const {
+  return IsType(rtp_payload_type, name.c_str());
 }
 
 bool DecoderDatabase::IsComfortNoise(uint8_t rtp_payload_type) const {
@@ -292,5 +291,4 @@ int DecoderDatabase::CheckPayloadTypes(const PacketList& packet_list) const {
   return kOK;
 }
 
-
 }  // namespace webrtc