DecoderDatabase: Made several methods nonvirtual to minimize mockable interface

webrtc/modules/audio_coding/neteq/decoder_database.h

 /*
  *  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.
  */
 
@@ skipping 23 matching lines @@
     kCodecNotSupported = -2,
     kInvalidSampleRate = -3,
     kDecoderExists = -4,
     kDecoderNotFound = -5,
     kInvalidPointer = -6
   };
 
   // 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,
+                rtc::scoped_refptr<AudioDecoderFactory> factory = nullptr);
     DecoderInfo(NetEqDecoder ct,
                 const std::string& nm,
                 AudioDecoder* ext_dec);
     DecoderInfo(DecoderInfo&&);
     ~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(); }

[kwiberg-webrtc, 2016/08/24 22:28:49]

Hmm. Why did you make these two const? It seems like they modify the state.

[ossu, 2016/08/25 10:23:47]

My reasoning is as follows:
- Some parts of NetEq uses DecoderDatabase through a const reference. They
access DecoderInfo (sometimes indirectly) to get info about a decoder.
- Getting the associated decoder object isn't materially different from getting
other info about the decoder. However, AudioDecoders are created on demand
(except when they're not, i.e. passed in directly through the constructor).
- Keeping the DecoderDatabase as const there seems like the right thing to do:
these methods may not change the database (i.e. add or remove codecs) - only
access what is already set up.

[kwiberg-webrtc, 2016/08/25 10:49:53]

Acknowledged.

 
     int SampleRateHz() const {
       RTC_DCHECK_EQ(1, !!decoder_ + !!external_decoder_ + !!cng_decoder_);
       return decoder_ ? decoder_->SampleRateHz()
                       : external_decoder_ ? external_decoder_->SampleRateHz()
                                           : cng_decoder_->sample_rate_hz;
     }
 
+    // Returns true if |code_type| is comfort noise.

[kwiberg-webrtc, 2016/08/24 22:28:49]

Spelling.

[ossu, 2016/08/25 10:23:47]

Ah! Now I see it! Thanks. :)

+    bool IsComfortNoise() const;
+
+    // Returns true if |code_type| is DTMF.

[kwiberg-webrtc, 2016/08/24 22:28:49]

Spelling.

[ossu, 2016/08/25 10:23:47]

Acknowledged.

+    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_;
+    rtc::scoped_refptr<AudioDecoderFactory> factory_;

[ossu, 2016/08/24 11:15:20]

If we can rely on DecoderInfo not outliving the respective DecoderDatabase (I
think we can), then AudioDecoderFactory could be a plain pointer instead. Since
DecoderInfo is part of DecoderDatabase, I don't think this is conceptually
wrong. Were we allowed to, I would've had it as a reference.

[kwiberg-webrtc, 2016/08/24 22:28:49]

I think you can have it as a reference. Mutable references are only disallowed
as function arguments IIRC.

[ossu, 2016/08/25 10:23:47]

Yeah, but then I'd pass it as a pointer to the constructor, which is the part I
don't like; but that's what the code style says. To me, passing a pointer looks
a bit like it might transfer ownership, maybe, but passing a reference clearly
doesn't. I'll keep it as a scoped_refptr for now, maybe degrade it to a plain
pointer later.

[kwiberg-webrtc, 2016/08/25 10:49:53]

Passing a raw pointer clearly doesn't transfer ownership. If you were
transferring ownership, you'd have used a smart pointer.

I know that not all of our code currently works this way, but new code should.

[ossu, 2016/08/25 13:43:04]

Agreed. But since it's not consistently like that in the code base, it  can be a
bit ambiguous. Oh, well!

+    mutable std::unique_ptr<AudioDecoder> decoder_;
 
     // Set iff this is an external decoder.
     AudioDecoder* const external_decoder_;
 
     // Set iff this is a comfort noise decoder.
     struct CngDecoder {
       static rtc::Optional<CngDecoder> Create(NetEqDecoder ct);
       int sample_rate_hz;
     };
     const rtc::Optional<CngDecoder> cng_decoder_;
@@ skipping 41 matching lines @@
   // Returns a pointer to the DecoderInfo struct for |rtp_payload_type|. If
   // no decoder is registered with that |rtp_payload_type|, NULL is returned.
   virtual const DecoderInfo* GetDecoderInfo(uint8_t rtp_payload_type) const;
 
   // Returns one RTP payload type associated with |codec_type|, or
   // kDecoderNotFound if no entry exists for that value. Note that one
   // |codec_type| may be registered with several RTP payload types, and the
   // 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();
 
   // 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
   // active decoder's AudioDecoder object is deleted.
   virtual int SetActiveCngDecoder(uint8_t rtp_payload_type);
 
   // Returns the current active comfort noise decoder, or NULL if no active
   // comfort noise decoder exists.
   virtual ComfortNoiseDecoder* GetActiveCngDecoder();
 
   // 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;
 
+  // The following are utility methods: they will look up DecoderInfo through

[ossu, 2016/08/24 11:15:20]

I moved these down so that everything virtual is kept together. Should help in
seeing what the mockable interface of DecoderDatabase is.

+  // 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);
+
+  // 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;
+
  private:
   typedef std::map<uint8_t, DecoderInfo> DecoderMap;
 
   DecoderMap decoders_;
   int active_decoder_type_;
   int active_cng_decoder_type_;
   std::unique_ptr<ComfortNoiseDecoder> active_cng_decoder_;
   rtc::scoped_refptr<AudioDecoderFactory> decoder_factory_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(DecoderDatabase);
 };
 
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_AUDIO_CODING_NETEQ_DECODER_DATABASE_H_