Stopped using the NetEqDecoder enum internally in NetEq.

webrtc/modules/audio_coding/neteq/decoder_database.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/audio_coding/neteq/decoder_database.h"
 
 #include <utility>  // pair
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/modules/audio_coding/codecs/audio_decoder.h"
 
 namespace webrtc {
 
 DecoderDatabase::DecoderDatabase(
     const rtc::scoped_refptr<AudioDecoderFactory>& decoder_factory)
     : active_decoder_type_(-1),
       active_cng_decoder_type_(-1),
       decoder_factory_(decoder_factory) {}
 
 DecoderDatabase::~DecoderDatabase() = default;
 
 DecoderDatabase::DecoderInfo::DecoderInfo(
-    NetEqDecoder ct,
-    const std::string& nm,
+    const SdpAudioFormat& audio_format,
     AudioDecoderFactory* factory)
-    : codec_type(ct),
-      name(nm),
-      audio_format_(acm2::RentACodec::NetEqDecoderToSdpAudioFormat(ct)),
+    : audio_format_(audio_format),
       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(
+    NetEqDecoder ct,
+    AudioDecoderFactory* factory)
+    : 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(audio_format_)) {}
+
+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);
 }
 
 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");
 }
 
-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 char* name) const {
+  return STR_CASE_CMP(audio_format_.name.c_str(), name) == 0;
+}
+
+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>();
   }
 }
 
 bool DecoderDatabase::Empty() const { return decoders_.empty(); }
 
 int DecoderDatabase::Size() const { return static_cast<int>(decoders_.size()); }
 
 void DecoderDatabase::Reset() {
   decoders_.clear();
   active_decoder_type_ = -1;
   active_cng_decoder_type_ = -1;
 }
 
 int DecoderDatabase::RegisterPayload(uint8_t rtp_payload_type,
                                      NetEqDecoder codec_type,
                                      const std::string& name) {
   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) {
     // Database already contains a decoder with type |rtp_payload_type|.
     return kDecoderExists;
   }
   return kOK;
 }
 
 int DecoderDatabase::InsertExternal(uint8_t rtp_payload_type,
                                     NetEqDecoder codec_type,
                                     const std::string& codec_name,
                                     AudioDecoder* decoder) {
   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|.
     return kDecoderExists;
   }
   return kOK;
 }
 
 int DecoderDatabase::Remove(uint8_t rtp_payload_type) {
   if (decoders_.erase(rtp_payload_type) == 0) {
     // No decoder with that |rtp_payload_type|.
     return kDecoderNotFound;
   }
   if (active_decoder_type_ == rtp_payload_type) {
     active_decoder_type_ = -1;  // No active decoder.
   }
   if (active_cng_decoder_type_ == rtp_payload_type) {
     active_cng_decoder_type_ = -1;  // No active CNG decoder.
   }
   return kOK;
 }
 
 const DecoderDatabase::DecoderInfo* DecoderDatabase::GetDecoderInfo(
     uint8_t rtp_payload_type) const {
   DecoderMap::const_iterator it = decoders_.find(rtp_payload_type);
   if (it == decoders_.end()) {
     // 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,
                                       bool* new_decoder) {
   // Check that |rtp_payload_type| exists in the database.
   const DecoderInfo *info = GetDecoderInfo(rtp_payload_type);
   if (!info) {
     // Decoder not found.
     return kDecoderNotFound;
   }
@@ skipping 48 matching lines @@
     active_cng_decoder_.reset(new ComfortNoiseDecoder);
   }
   return active_cng_decoder_.get();
 }
 
 AudioDecoder* DecoderDatabase::GetDecoder(uint8_t rtp_payload_type) const {
   const DecoderInfo *info = GetDecoderInfo(rtp_payload_type);
   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 {
   const DecoderInfo *info = GetDecoderInfo(rtp_payload_type);
   return info && info->IsComfortNoise();
 }
 
 bool DecoderDatabase::IsDtmf(uint8_t rtp_payload_type) const {
   const DecoderInfo *info = GetDecoderInfo(rtp_payload_type);
   return info && info->IsDtmf();
@@ skipping 10 matching lines @@
     if (!GetDecoderInfo((*it)->header.payloadType)) {
       // Payload type is not found.
       LOG(LS_WARNING) << "CheckPayloadTypes: unknown RTP payload type "
                       << static_cast<int>((*it)->header.payloadType);
       return kDecoderNotFound;
     }
   }
   return kOK;
 }
 
-
 }  // namespace webrtc