Convert CNG into C++ and remove it from AudioDecoder

webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h

Index: webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h
diff --git a/webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h b/webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h
index 64bea1e26f6f225dedf6d72deeb7c8ab89a5f5f7..a1331a4004129e3682ba7e58bdf774ef43ea3592 100644
--- a/webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h
+++ b/webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h
@@ -12,152 +12,90 @@
 #ifndef WEBRTC_MODULES_AUDIO_CODING_CODECS_CNG_WEBRTC_CNG_H_
 #define WEBRTC_MODULES_AUDIO_CODING_CODECS_CNG_WEBRTC_CNG_H_
 
-#include <stddef.h>
-#include "webrtc/typedefs.h"
+#include <cstddef>
 
-#ifdef __cplusplus
-extern "C" {
-#endif
+#include "webrtc/base/array_view.h"
+#include "webrtc/typedefs.h"
 
 #define WEBRTC_CNG_MAX_LPC_ORDER 12
 #define WEBRTC_CNG_MAX_OUTSIZE_ORDER 640

[hlundin-webrtc, 2016/04/13 07:05:24]

This is only used inside the class, right? Make it a private const. Or is it
used inside both classes?

[ossu, 2016/04/13 11:57:07]

It is used inside both, to check that the data input and output (respectively)
is not too large. I also mention it in the comments in ComfortNoiseDecoder. We
should probably inform the user _somewhere_ about the required amount of data to
input / expect out. I guess this depends on the sample rate?

 
-/* Define Error codes. */
-
-/* 6100 Encoder */
-#define CNG_ENCODER_NOT_INITIATED               6120
-#define CNG_DISALLOWED_LPC_ORDER                6130
-#define CNG_DISALLOWED_FRAME_SIZE               6140
-#define CNG_DISALLOWED_SAMPLING_FREQUENCY       6150
-/* 6200 Decoder */
-#define CNG_DECODER_NOT_INITIATED               6220
-
-typedef struct WebRtcCngEncInst CNG_enc_inst;
-typedef struct WebRtcCngDecInst CNG_dec_inst;
-
-/****************************************************************************
- * WebRtcCng_CreateEnc/Dec(...)
- *
- * These functions create an instance to the specified structure
- *
- * Input:
- *    - XXX_inst      : Pointer to created instance that should be created
- *
- * Return value       :  0 - Ok
- *                      -1 - Error
- */
-int16_t WebRtcCng_CreateEnc(CNG_enc_inst** cng_inst);
-int16_t WebRtcCng_CreateDec(CNG_dec_inst** cng_inst);
-
-/****************************************************************************
- * WebRtcCng_InitEnc/Dec(...)
- *
- * This function initializes a instance
- *
- * Input:
- *    - cng_inst      : Instance that should be initialized
- *
- *    - fs            : 8000 for narrowband and 16000 for wideband
- *    - interval      : generate SID data every interval ms
- *    - quality       : Number of refl. coefs, maximum allowed is 12
- *
- * Output:
- *    - cng_inst      : Initialized instance
- *
- * Return value       :  0 - Ok
- *                      -1 - Error
- */
-
-int WebRtcCng_InitEnc(CNG_enc_inst* cng_inst, int fs, int16_t interval,
-                      int16_t quality);
-void WebRtcCng_InitDec(CNG_dec_inst* cng_inst);
-
-/****************************************************************************
- * WebRtcCng_FreeEnc/Dec(...)
- *
- * These functions frees the dynamic memory of a specified instance
- *
- * Input:
- *    - cng_inst      : Pointer to created instance that should be freed
- *
- * Return value       :  0 - Ok
- *                      -1 - Error
- */
-int16_t WebRtcCng_FreeEnc(CNG_enc_inst* cng_inst);
-int16_t WebRtcCng_FreeDec(CNG_dec_inst* cng_inst);
-
-/****************************************************************************
- * WebRtcCng_Encode(...)
- *
- * These functions analyzes background noise
- *
- * Input:
- *    - cng_inst      : Pointer to created instance
- *    - speech        : Signal to be analyzed
- *    - nrOfSamples   : Size of speech vector
- *    - forceSID      : not zero to force SID frame and reset
- *
- * Output:
- *    - bytesOut      : Nr of bytes to transmit, might be 0
- *
- * Return value       :  0 - Ok
- *                      -1 - Error
- */
-int WebRtcCng_Encode(CNG_enc_inst* cng_inst, int16_t* speech,
-                     size_t nrOfSamples, uint8_t* SIDdata,
-                     size_t* bytesOut, int16_t forceSID);
-
-/****************************************************************************
- * WebRtcCng_UpdateSid(...)
- *
- * These functions updates the CN state, when a new SID packet arrives
- *
- * Input:
- *    - cng_inst      : Pointer to created instance that should be freed
- *    - SID           : SID packet, all headers removed
- *    - length        : Length in bytes of SID packet
- *
- * Return value       :  0 - Ok
- *                      -1 - Error
- */
-int16_t WebRtcCng_UpdateSid(CNG_dec_inst* cng_inst, uint8_t* SID,
-                            size_t length);
-
-/****************************************************************************
- * WebRtcCng_Generate(...)
- *
- * These functions generates CN data when needed
- *
- * Input:
- *    - cng_inst      : Pointer to created instance that should be freed
- *    - outData       : pointer to area to write CN data
- *    - nrOfSamples   : How much data to generate
- *    - new_period    : >0 if a new period of CNG, will reset history
- *
- * Return value       :  0 - Ok
- *                      -1 - Error
- */
-int16_t WebRtcCng_Generate(CNG_dec_inst* cng_inst, int16_t* outData,
-                           size_t nrOfSamples, int16_t new_period);
-
-/*****************************************************************************
- * WebRtcCng_GetErrorCodeEnc/Dec(...)
- *
- * This functions can be used to check the error code of a CNG instance. When
- * a function returns -1 a error code will be set for that instance. The
- * function below extract the code of the last error that occurred in the
- * specified instance.
- *
- * Input:
- *    - CNG_inst    : CNG enc/dec instance
- *
- * Return value     : Error code
- */
-int16_t WebRtcCng_GetErrorCodeEnc(CNG_enc_inst* cng_inst);
-int16_t WebRtcCng_GetErrorCodeDec(CNG_dec_inst* cng_inst);
-
-#ifdef __cplusplus
-}
-#endif
+namespace webrtc {
+
+class ComfortNoiseDecoder {
+ public:
+  ComfortNoiseDecoder();
+  ~ComfortNoiseDecoder() = default;
+
+  ComfortNoiseDecoder(const ComfortNoiseDecoder&) = delete;
+  ComfortNoiseDecoder& operator=(const ComfortNoiseDecoder&) = delete;

[kwiberg-webrtc, 2016/04/12 13:35:31]

We have a macro for this in constructormagic.h: RTC_DISALLOW_COPY_AND_ASSIGN. I
haven't decided yet if I prefer deleting the methods manually or using that
macro...

[ossu, 2016/04/12 13:54:49]

There's been conversations on c-style, where using macros for this is
discouraged over doing it manually. I'm not sure it's made its way all through
to official Google C style yet, but it seems to be where we're headed.

On the other hand, I'm not sure they _need_ to be noncopyable. The state is just
POD. I could just leave them as copyable, for whenever that might be useful.

[hlundin-webrtc, 2016/04/13 07:05:24]

Don't leave them as copyable. Use either way to ban copying.

[ossu, 2016/04/13 11:57:07]

Alright, no copying!

+
+  void Reset();
+
+  /* Update the CN state when a new SID packet arrives.

[hlundin-webrtc, 2016/04/13 07:05:24]

Updates...
Also, I think you should make these block comments using // instead of /* */.

[ossu, 2016/04/13 11:57:07]

Acknowledged.

+   * |SID| is a view of the SID packet without the headers.
+   */
+  void UpdateSid(rtc::ArrayView<const uint8_t> SID);
+
+  /* Generate comfort noise.

[hlundin-webrtc, 2016/04/13 07:05:23]

Generates...

[ossu, 2016/04/13 11:57:06]

Acknowledged.

+   * |outData| will be filled with samples - its size determines the number of

[hlundin-webrtc, 2016/04/13 07:05:24]

out_data

[ossu, 2016/04/13 11:57:06]

Acknowledged.

+   * samples generated. When |new_period| is true, CNG history will be reset
+   * before any audio is generated.  Returns |false| if outData is too large
+   * (larger than WEBRTC_CNG_MAX_OUTSIZE_ORDER).
+   */
+  bool Generate(rtc::ArrayView<int16_t> outData, bool new_period);

[hlundin-webrtc, 2016/04/13 07:05:24]

Change order of parameters. Input params go before (input/)output params.

[hlundin-webrtc, 2016/04/13 07:05:24]

out_data

[ossu, 2016/04/13 11:57:07]

Acknowledged.

+
+ private:
+  uint32_t dec_seed_;
+  int32_t dec_target_energy_;
+  int32_t dec_used_energy_;
+  int16_t dec_target_reflCoefs_[WEBRTC_CNG_MAX_LPC_ORDER + 1];
+  int16_t dec_used_reflCoefs_[WEBRTC_CNG_MAX_LPC_ORDER + 1];
+  int16_t dec_filtstate_[WEBRTC_CNG_MAX_LPC_ORDER + 1];
+  int16_t dec_filtstateLow_[WEBRTC_CNG_MAX_LPC_ORDER + 1];
+  uint16_t dec_order_;
+  int16_t dec_target_scale_factor_;  /* Q29 */
+  int16_t dec_used_scale_factor_;  /* Q29 */
+};
+
+class ComfortNoiseEncoder {
+ public:
+  // Create a comfort noise encoder.

[hlundin-webrtc, 2016/04/13 07:05:24]

Creates...

[ossu, 2016/04/13 11:57:07]

Acknowledged.

+  // |fs| selects sample rate: 8000 for narrowband and 16000 for wideband

[hlundin-webrtc, 2016/04/13 07:05:24]

End all sentences with .

[ossu, 2016/04/13 11:57:06]

Acknowledged.

+  // |interval| sets the interval at which to generate SID data (in ms)
+  // |quality| selects the number of refl. coeffs. Maximum allowed is 12
+  ComfortNoiseEncoder(int fs, int interval, int quality);
+  ~ComfortNoiseEncoder() = default;
+
+  ComfortNoiseEncoder(const ComfortNoiseEncoder&) = delete;
+  ComfortNoiseEncoder& operator=(const ComfortNoiseEncoder&) = delete;
+
+  // Reset the comfort noise encoder to its initial state.

[hlundin-webrtc, 2016/04/13 07:05:23]

Resets...

[ossu, 2016/04/13 11:57:06]

Acknowledged.

+  // Parameters are set as during construction.
+  void Reset(int fs, int interval, int quality);

[kwiberg-webrtc, 2016/04/12 13:35:31]

Is this necessary? Can't the callers just make a new instance instead?

[ossu, 2016/04/12 13:54:49]

There was some code that used InitEnc before, so I thought I'd keep that
implementation around for now. I'm all for getting rid of Reset - preferably in
both Encoder and Decoder.

Hmm, maybe get them back as copyable and remove Reset? Would make it easy to
just reinitialize your local CNG-whatever without reallocations.

[kwiberg-webrtc, 2016/04/14 09:42:48]

Yes please.
I don't think resetting a CN encoder/decoder is a frequent enough event to
justify the extra effort.

+
+  // Analyze background noise from |speech| and put coefficients in |output|.

[hlundin-webrtc, 2016/04/13 07:05:23]

puts...

[hlundin-webrtc, 2016/04/13 07:05:23]

Analyzes...

+  // Returns the number of coefficients generated, or -1 if too much input is
+  // provided.  If |forceSID| is true, a SID frame is forced and the encoder

[hlundin-webrtc, 2016/04/13 07:05:24]

force_sid

+  // reset.
+  // TODO(ossu): Clarify this behavior. Does it reset before or after encoding?

[hlundin-webrtc, 2016/04/13 07:05:24]

I don't think it should reset all of the encoder state. What should happen is
that the internal SID interval counter should reset.

+  // TODO(ossu): Should it fail if given too much input? Can't it just ignore
+  // the rest?

[hlundin-webrtc, 2016/04/13 07:05:24]

Or maybe just DCHECK?

[ossu, 2016/04/13 11:57:06]

DCHECK works. Is the right size always 640 or does it depend on the sample rate?

+  int Encode(rtc::ArrayView<const int16_t> speech,
+             rtc::ArrayView<uint8_t> output,
+             bool forceSID);

[kwiberg-webrtc, 2016/04/12 13:35:31]

Consider outputting to an rtc::Buffer* instead, so that the caller doesn't have
to care how much space is needed.

[ossu, 2016/04/12 13:54:49]

Acknowledged.

[hlundin-webrtc, 2016/04/13 07:05:24]

force_sid

[ossu, 2016/04/13 11:57:07]

Acknowledged. Also, I guess force_sid should go before output.

+
+ private:
+  size_t enc_nrOfCoefs_;
+  int enc_sampfreq_;
+  int16_t enc_interval_;
+  int16_t enc_msSinceSID_;
+  int32_t enc_Energy_;
+  int16_t enc_reflCoefs_[WEBRTC_CNG_MAX_LPC_ORDER + 1];
+  int32_t enc_corrVector_[WEBRTC_CNG_MAX_LPC_ORDER + 1];
+  uint32_t enc_seed_;
+};
+
+}  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_CODING_CODECS_CNG_WEBRTC_CNG_H_