Removed virtual from several methods in DecoderDatabase to minimize

the number of points that need to be mocked for testing.

For the now non-virtual methods, DecoderDatabase now does a lookup
through GetDecoderInfo and then delegates to the appropriate method in
the DecoderInfo object, if one is found.

A few other methods were also changed to look up through GetDecoderInfo.

Also moved the audio decoder factory into DecoderInfo, so that
DecoderInfo::GetDecoder can be used directly.

Review-Url: https://codereview.webrtc.org/2276913002
Cr-Commit-Position: refs/heads/master@{#13933}

webrtc/modules/audio_coding/neteq/decoder_database.cc

@@ -10,7 +10,6 @@
 
 #include "webrtc/modules/audio_coding/neteq/decoder_database.h"
 
-#include <assert.h>
 #include <utility>  // pair
 
 #include "webrtc/base/checks.h"
@@ -27,11 +26,14 @@ DecoderDatabase::DecoderDatabase(
 
 DecoderDatabase::~DecoderDatabase() = default;
 
-DecoderDatabase::DecoderInfo::DecoderInfo(NetEqDecoder ct,
+DecoderDatabase::DecoderInfo::DecoderInfo(
-                                          const std::string& nm)
+    NetEqDecoder ct,
+    const std::string& nm,
+    AudioDecoderFactory* factory)
     : codec_type(ct),
       name(nm),
       audio_format_(acm2::RentACodec::NetEqDecoderToSdpAudioFormat(ct)),
+      factory_(factory),
       external_decoder_(nullptr),
       cng_decoder_(CngDecoder::Create(ct)) {}
 
@@ -48,21 +50,39 @@ DecoderDatabase::DecoderInfo::DecoderInfo(NetEqDecoder ct,
 DecoderDatabase::DecoderInfo::DecoderInfo(DecoderInfo&&) = default;
 DecoderDatabase::DecoderInfo::~DecoderInfo() = default;
 
-AudioDecoder* DecoderDatabase::DecoderInfo::GetDecoder(
+AudioDecoder* DecoderDatabase::DecoderInfo::GetDecoder() const {
-    AudioDecoderFactory* factory) {
   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_) {
-    decoder_ = factory->MakeAudioDecoder(*audio_format_);
+    RTC_DCHECK(factory_);
+    decoder_ = factory_->MakeAudioDecoder(*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;
+}
+
+bool DecoderDatabase::DecoderInfo::IsDtmf() const {
+  return codec_type == NetEqDecoder::kDecoderAVT;
+}
+
+bool DecoderDatabase::DecoderInfo::IsRed() const {
+  return codec_type == NetEqDecoder::kDecoderRED;
+}
+
 rtc::Optional<DecoderDatabase::DecoderInfo::CngDecoder>
 DecoderDatabase::DecoderInfo::CngDecoder::Create(NetEqDecoder ct) {
   const auto cng = [](int sample_rate_hz) {
@@ -102,7 +122,7 @@ int DecoderDatabase::RegisterPayload(uint8_t rtp_payload_type,
   if (!CodecSupported(codec_type)) {
     return kCodecNotSupported;
   }
-  DecoderInfo info(codec_type, name);
+  DecoderInfo info(codec_type, name, decoder_factory_.get());
   auto ret =
       decoders_.insert(std::make_pair(rtp_payload_type, std::move(info)));
   if (ret.second == false) {
@@ -172,82 +192,32 @@ uint8_t DecoderDatabase::GetRtpPayloadType(
   return kRtpPayloadTypeError;
 }
 
-AudioDecoder* DecoderDatabase::GetDecoder(uint8_t rtp_payload_type) {
-  if (IsDtmf(rtp_payload_type) || IsRed(rtp_payload_type) ||
-      IsComfortNoise(rtp_payload_type)) {
-    // These are not real decoders.
-    return NULL;
-  }
-  DecoderMap::iterator it = decoders_.find(rtp_payload_type);
-  if (it == decoders_.end()) {
-    // Decoder not found.
-    return NULL;
-  }
-  DecoderInfo* info = &(*it).second;
-  return info->GetDecoder(decoder_factory_.get());
-}
-
-bool DecoderDatabase::IsType(uint8_t rtp_payload_type,
-                             NetEqDecoder codec_type) const {
-  DecoderMap::const_iterator it = decoders_.find(rtp_payload_type);
-  if (it == decoders_.end()) {
-    // Decoder not found.
-    return false;
-  }
-  return ((*it).second.codec_type == codec_type);
-}
-
-bool DecoderDatabase::IsComfortNoise(uint8_t rtp_payload_type) const {
-  DecoderMap::const_iterator it = decoders_.find(rtp_payload_type);
-  if (it == decoders_.end()) {
-    // Decoder not found.
-    return false;
-  }
-  const auto& type = it->second.codec_type;
-  return type == NetEqDecoder::kDecoderCNGnb
-      || type == NetEqDecoder::kDecoderCNGwb
-      || type == NetEqDecoder::kDecoderCNGswb32kHz
-      || type == NetEqDecoder::kDecoderCNGswb48kHz;
-}
-
-bool DecoderDatabase::IsDtmf(uint8_t rtp_payload_type) const {
-  return IsType(rtp_payload_type, NetEqDecoder::kDecoderAVT);
-}
-
-bool DecoderDatabase::IsRed(uint8_t rtp_payload_type) const {
-  return IsType(rtp_payload_type, NetEqDecoder::kDecoderRED);
-}
-
 int DecoderDatabase::SetActiveDecoder(uint8_t rtp_payload_type,
                                       bool* new_decoder) {
   // Check that |rtp_payload_type| exists in the database.
-  DecoderMap::const_iterator it = decoders_.find(rtp_payload_type);
+  const DecoderInfo *info = GetDecoderInfo(rtp_payload_type);
-  if (it == decoders_.end()) {
+  if (!info) {
     // Decoder not found.
     return kDecoderNotFound;
   }
-  RTC_CHECK(!IsComfortNoise(rtp_payload_type));
+  RTC_CHECK(!info->IsComfortNoise());
-  assert(new_decoder);
+  RTC_DCHECK(new_decoder);
   *new_decoder = false;
   if (active_decoder_type_ < 0) {
     // This is the first active decoder.
     *new_decoder = true;
   } else if (active_decoder_type_ != rtp_payload_type) {
     // Moving from one active decoder to another. Delete the first one.
-    DecoderMap::iterator it = decoders_.find(active_decoder_type_);
+    const DecoderInfo *old_info = GetDecoderInfo(active_decoder_type_);
-    if (it == decoders_.end()) {
+    RTC_DCHECK(old_info);
-      // Decoder not found. This should not be possible.
+    old_info->DropDecoder();
-      assert(false);
-      return kDecoderNotFound;
-    }
-    it->second.DropDecoder();
     *new_decoder = true;
   }
   active_decoder_type_ = rtp_payload_type;
   return kOK;
 }
 
-AudioDecoder* DecoderDatabase::GetActiveDecoder() {
+AudioDecoder* DecoderDatabase::GetActiveDecoder() const {
   if (active_decoder_type_ < 0) {
     // No active decoder.
     return NULL;
@@ -257,27 +227,22 @@ AudioDecoder* DecoderDatabase::GetActiveDecoder() {
 
 int DecoderDatabase::SetActiveCngDecoder(uint8_t rtp_payload_type) {
   // Check that |rtp_payload_type| exists in the database.
-  DecoderMap::const_iterator it = decoders_.find(rtp_payload_type);
+  const DecoderInfo *info = GetDecoderInfo(rtp_payload_type);
-  if (it == decoders_.end()) {
+  if (!info) {
     // Decoder not found.
     return kDecoderNotFound;
   }
   if (active_cng_decoder_type_ >= 0 &&
       active_cng_decoder_type_ != rtp_payload_type) {
     // Moving from one active CNG decoder to another. Delete the first one.
-    DecoderMap::iterator it = decoders_.find(active_cng_decoder_type_);
+    RTC_DCHECK(active_cng_decoder_);
-    if (it == decoders_.end()) {
-      // Decoder not found. This should not be possible.
-      assert(false);
-      return kDecoderNotFound;
-    }
     active_cng_decoder_.reset();
   }
   active_cng_decoder_type_ = rtp_payload_type;
   return kOK;
 }
 
-ComfortNoiseDecoder* DecoderDatabase::GetActiveCngDecoder() {
+ComfortNoiseDecoder* DecoderDatabase::GetActiveCngDecoder() const {
   if (active_cng_decoder_type_ < 0) {
     // No active CNG decoder.
     return NULL;
@@ -288,10 +253,36 @@ ComfortNoiseDecoder* DecoderDatabase::GetActiveCngDecoder() {
   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,
+                             NetEqDecoder codec_type) const {
+  const DecoderInfo *info = GetDecoderInfo(rtp_payload_type);
+  return info && info->codec_type == codec_type;
+}
+
+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();
+}
+
+bool DecoderDatabase::IsRed(uint8_t rtp_payload_type) const {
+  const DecoderInfo *info = GetDecoderInfo(rtp_payload_type);
+  return info && info->IsRed();
+}
+
 int DecoderDatabase::CheckPayloadTypes(const PacketList& packet_list) const {
   PacketList::const_iterator it;
   for (it = packet_list.begin(); it != packet_list.end(); ++it) {
-    if (decoders_.find((*it)->header.payloadType) == decoders_.end()) {
+    if (!GetDecoderInfo((*it)->header.payloadType)) {
       // Payload type is not found.
       LOG(LS_WARNING) << "CheckPayloadTypes: unknown RTP payload type "
                       << static_cast<int>((*it)->header.payloadType);

webrtc/modules/audio_coding/neteq/decoder_database.h

@@ -41,7 +41,10 @@ class DecoderDatabase {
   // Class that stores decoder info in the database.
   class DecoderInfo {
    public:
-    DecoderInfo(NetEqDecoder ct, const std::string& nm);
+    DecoderInfo(
+        NetEqDecoder ct,
+        const std::string& nm,
+        AudioDecoderFactory* factory = nullptr);
     DecoderInfo(NetEqDecoder ct,
                 const std::string& nm,
                 AudioDecoder* ext_dec);
@@ -49,11 +52,11 @@ class DecoderDatabase {
     ~DecoderInfo();
 
     // Get the AudioDecoder object, creating it first if necessary.
-    AudioDecoder* GetDecoder(AudioDecoderFactory* factory);
+    AudioDecoder* GetDecoder() const;
 
     // Delete the AudioDecoder object, unless it's external. (This means we can
     // always recreate it later if we need it.)
-    void DropDecoder() { decoder_.reset(); }
+    void DropDecoder() const { decoder_.reset(); }
 
     int SampleRateHz() const {
       RTC_DCHECK_EQ(1, !!decoder_ + !!external_decoder_ + !!cng_decoder_);
@@ -62,12 +65,22 @@ class DecoderDatabase {
                                           : cng_decoder_->sample_rate_hz;
     }
 
+    // Returns true if |codec_type| is comfort noise.
+    bool IsComfortNoise() const;
+
+    // Returns true if |codec_type| is DTMF.
+    bool IsDtmf() const;
+
+    // Returns true if |codec_type| is RED.
+    bool IsRed() const;
+
     const NetEqDecoder codec_type;
     const std::string name;
 
    private:
     const rtc::Optional<SdpAudioFormat> audio_format_;
-    std::unique_ptr<AudioDecoder> decoder_;
+    AudioDecoderFactory* factory_;
+    mutable std::unique_ptr<AudioDecoder> decoder_;
 
     // Set iff this is an external decoder.
     AudioDecoder* const external_decoder_;
@@ -129,31 +142,13 @@ class DecoderDatabase {
   // method may return any of them.
   virtual uint8_t GetRtpPayloadType(NetEqDecoder codec_type) const;
 
-  // Returns a pointer to the AudioDecoder object associated with
-  // |rtp_payload_type|, or NULL if none is registered. If the AudioDecoder
-  // object does not exist for that decoder, the object is created.
-  virtual AudioDecoder* GetDecoder(uint8_t rtp_payload_type);
-
-  // Returns true if |rtp_payload_type| is registered as a |codec_type|.
-  virtual bool IsType(uint8_t rtp_payload_type,
-                      NetEqDecoder codec_type) const;
-
-  // Returns true if |rtp_payload_type| is registered as comfort noise.
-  virtual bool IsComfortNoise(uint8_t rtp_payload_type) const;
-
-  // Returns true if |rtp_payload_type| is registered as DTMF.
-  virtual bool IsDtmf(uint8_t rtp_payload_type) const;
-
-  // Returns true if |rtp_payload_type| is registered as RED.
-  virtual bool IsRed(uint8_t rtp_payload_type) const;
-
   // Sets the active decoder to be |rtp_payload_type|. If this call results in a
   // change of active decoder, |new_decoder| is set to true. The previous active
   // decoder's AudioDecoder object is deleted.
   virtual int SetActiveDecoder(uint8_t rtp_payload_type, bool* new_decoder);
 
   // Returns the current active decoder, or NULL if no active decoder exists.
-  virtual AudioDecoder* GetActiveDecoder();
+  virtual AudioDecoder* GetActiveDecoder() const;
 
   // Sets the active comfort noise decoder to be |rtp_payload_type|. If this
   // call results in a change of active comfort noise decoder, the previous
@@ -162,11 +157,32 @@ class DecoderDatabase {
 
   // Returns the current active comfort noise decoder, or NULL if no active
   // comfort noise decoder exists.
-  virtual ComfortNoiseDecoder* GetActiveCngDecoder();
+  virtual ComfortNoiseDecoder* GetActiveCngDecoder() const;
+
+  // The following are utility methods: they will look up DecoderInfo through
+  // GetDecoderInfo and call the respective method on that info object, if it
+  // exists.
+
+  // Returns a pointer to the AudioDecoder object associated with
+  // |rtp_payload_type|, or NULL if none is registered. If the AudioDecoder
+  // object does not exist for that decoder, the object is created.
+  AudioDecoder* GetDecoder(uint8_t rtp_payload_type) const;
+
+  // Returns true if |rtp_payload_type| is registered as a |codec_type|.
+  bool IsType(uint8_t rtp_payload_type, NetEqDecoder codec_type) const;
+
+  // Returns true if |rtp_payload_type| is registered as comfort noise.
+  bool IsComfortNoise(uint8_t rtp_payload_type) const;
+
+  // Returns true if |rtp_payload_type| is registered as DTMF.
+  bool IsDtmf(uint8_t rtp_payload_type) const;
+
+  // Returns true if |rtp_payload_type| is registered as RED.
+  bool IsRed(uint8_t rtp_payload_type) const;
 
   // Returns kOK if all packets in |packet_list| carry payload types that are
   // registered in the database. Otherwise, returns kDecoderNotFound.
-  virtual int CheckPayloadTypes(const PacketList& packet_list) const;
+  int CheckPayloadTypes(const PacketList& packet_list) const;
 
  private:
   typedef std::map<uint8_t, DecoderInfo> DecoderMap;
@@ -174,7 +190,7 @@ class DecoderDatabase {
   DecoderMap decoders_;
   int active_decoder_type_;
   int active_cng_decoder_type_;
-  std::unique_ptr<ComfortNoiseDecoder> active_cng_decoder_;
+  mutable std::unique_ptr<ComfortNoiseDecoder> active_cng_decoder_;
   rtc::scoped_refptr<AudioDecoderFactory> decoder_factory_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(DecoderDatabase);

webrtc/modules/audio_coding/neteq/mock/mock_decoder_database.h

@@ -21,7 +21,9 @@ namespace webrtc {
 
 class MockDecoderDatabase : public DecoderDatabase {
  public:
-  MockDecoderDatabase() : DecoderDatabase(nullptr) {}
+  explicit MockDecoderDatabase(
+      rtc::scoped_refptr<AudioDecoderFactory> factory = nullptr)
+      : DecoderDatabase(factory) {}
   virtual ~MockDecoderDatabase() { Die(); }
   MOCK_METHOD0(Die, void());
   MOCK_CONST_METHOD0(Empty,
@@ -44,26 +46,14 @@ class MockDecoderDatabase : public DecoderDatabase {
       const DecoderInfo*(uint8_t rtp_payload_type));
   MOCK_CONST_METHOD1(GetRtpPayloadType,
       uint8_t(NetEqDecoder codec_type));
-  MOCK_METHOD1(GetDecoder,
-      AudioDecoder*(uint8_t rtp_payload_type));
-  MOCK_CONST_METHOD2(IsType,
-      bool(uint8_t rtp_payload_type, NetEqDecoder codec_type));
-  MOCK_CONST_METHOD1(IsComfortNoise,
-      bool(uint8_t rtp_payload_type));
-  MOCK_CONST_METHOD1(IsDtmf,
-      bool(uint8_t rtp_payload_type));
-  MOCK_CONST_METHOD1(IsRed,
-      bool(uint8_t rtp_payload_type));
   MOCK_METHOD2(SetActiveDecoder,
       int(uint8_t rtp_payload_type, bool* new_decoder));
-  MOCK_METHOD0(GetActiveDecoder,
+  MOCK_CONST_METHOD0(GetActiveDecoder,
       AudioDecoder*());
   MOCK_METHOD1(SetActiveCngDecoder,
       int(uint8_t rtp_payload_type));
-  MOCK_METHOD0(GetActiveCngDecoder,
+  MOCK_CONST_METHOD0(GetActiveCngDecoder,
       ComfortNoiseDecoder*());
-  MOCK_CONST_METHOD1(CheckPayloadTypes,
-      int(const PacketList& packet_list));
 };
 
 }  // namespace webrtc

webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc

@@ -269,24 +269,10 @@ TEST_F(NetEqImplTest, InsertPacket) {
 
         *dec = std::move(mock_decoder);
       }));
-  DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderPCMu, "");
+  DecoderDatabase::DecoderInfo info(NetEqDecoder::kDecoderPCMu, "",
+                                    mock_decoder_factory);
 
   // Expectations for decoder database.
-  EXPECT_CALL(*mock_decoder_database_, IsRed(kPayloadType))
-      .WillRepeatedly(Return(false));  // This is not RED.
-  EXPECT_CALL(*mock_decoder_database_, CheckPayloadTypes(_))
-      .Times(2)
-      .WillRepeatedly(Return(DecoderDatabase::kOK));  // Payload type is valid.
-  EXPECT_CALL(*mock_decoder_database_, IsDtmf(kPayloadType))
-      .WillRepeatedly(Return(false));  // This is not DTMF.
-  EXPECT_CALL(*mock_decoder_database_, GetDecoder(kPayloadType))
-      .Times(3)
-      .WillRepeatedly(
-          Invoke([&info, mock_decoder_factory](uint8_t payload_type) {
-            return info.GetDecoder(mock_decoder_factory);
-          }));
-  EXPECT_CALL(*mock_decoder_database_, IsComfortNoise(kPayloadType))
-      .WillRepeatedly(Return(false));  // This is not CNG.
   EXPECT_CALL(*mock_decoder_database_, GetDecoderInfo(kPayloadType))
       .WillRepeatedly(Return(&info));
 

webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc

@@ -174,10 +174,6 @@ TEST(PacketBuffer, InsertPacketList) {
   }
 
   MockDecoderDatabase decoder_database;
-  EXPECT_CALL(decoder_database, IsComfortNoise(0))
-      .WillRepeatedly(Return(false));
-  EXPECT_CALL(decoder_database, IsDtmf(0))
-      .WillRepeatedly(Return(false));
   uint8_t current_pt = 0xFF;
   uint8_t current_cng_pt = 0xFF;
   EXPECT_EQ(PacketBuffer::kOK, buffer.InsertPacketList(&list,
@@ -216,10 +212,6 @@ TEST(PacketBuffer, InsertPacketListChangePayloadType) {
 
 
   MockDecoderDatabase decoder_database;
-  EXPECT_CALL(decoder_database, IsComfortNoise(_))
-      .WillRepeatedly(Return(false));
-  EXPECT_CALL(decoder_database, IsDtmf(_))
-      .WillRepeatedly(Return(false));
   uint8_t current_pt = 0xFF;
   uint8_t current_cng_pt = 0xFF;
   EXPECT_EQ(PacketBuffer::kFlushed, buffer.InsertPacketList(&list,
@@ -349,10 +341,6 @@ TEST(PacketBuffer, Reordering) {
   }
 
   MockDecoderDatabase decoder_database;
-  EXPECT_CALL(decoder_database, IsComfortNoise(0))
-      .WillRepeatedly(Return(false));
-  EXPECT_CALL(decoder_database, IsDtmf(0))
-      .WillRepeatedly(Return(false));
   uint8_t current_pt = 0xFF;
   uint8_t current_cng_pt = 0xFF;
 
@@ -424,10 +412,6 @@ TEST(PacketBuffer, Failures) {
   list.push_back(packet);
   list.push_back(gen.NextPacket(payload_len));  // Valid packet.
   MockDecoderDatabase decoder_database;
-  EXPECT_CALL(decoder_database, IsComfortNoise(0))
-      .WillRepeatedly(Return(false));
-  EXPECT_CALL(decoder_database, IsDtmf(0))
-      .WillRepeatedly(Return(false));
   uint8_t current_pt = 0xFF;
   uint8_t current_cng_pt = 0xFF;
   EXPECT_EQ(PacketBuffer::kInvalidPacket,