Get rid of the manual destructor in AudioCodingModuleImpl

webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc

Index: webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc
diff --git a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc
index a3542cef335a31969e8c9ec726faf9f8cec72139..86a6e814883e5069512d6022b9b2887004d4185f 100644
--- a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc
+++ b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc
@@ -139,7 +139,6 @@ AudioCodingModuleImpl::AudioCodingModuleImpl(
       receiver_(config),
       bitrate_logger_("WebRTC.Audio.TargetBitrateInKbps"),
       previous_pltype_(255),
-      aux_rtp_header_(NULL),
       receiver_initialized_(false),
       first_10ms_data_(false),
       first_frame_(true),
@@ -153,21 +152,6 @@ AudioCodingModuleImpl::AudioCodingModuleImpl(
   WEBRTC_TRACE(webrtc::kTraceMemory, webrtc::kTraceAudioCoding, id_, "Created");
 }
 
-AudioCodingModuleImpl::~AudioCodingModuleImpl() {
-  if (aux_rtp_header_ != NULL) {
-    delete aux_rtp_header_;
-    aux_rtp_header_ = NULL;
-  }
-
-  delete callback_crit_sect_;
-  callback_crit_sect_ = NULL;
-
-  delete acm_crit_sect_;
-  acm_crit_sect_ = NULL;
-  WEBRTC_TRACE(webrtc::kTraceMemory, webrtc::kTraceAudioCoding, id_,
-               "Destroyed");
-}
-
 int32_t AudioCodingModuleImpl::Encode(const InputData& input_data) {
   uint8_t stream[2 * MAX_PAYLOAD_SIZE_BYTE];  // Make room for 1 RED payload.
   AudioEncoder::EncodedInfo encoded_info;
@@ -213,7 +197,7 @@ int32_t AudioCodingModuleImpl::Encode(const InputData& input_data) {
   }
 
   {
-    CriticalSectionScoped lock(callback_crit_sect_);
+    CriticalSectionScoped lock(callback_crit_sect_.get());
     if (packetization_callback_) {
       packetization_callback_->SendData(
           frame_type, encoded_info.payload_type, encoded_info.encoded_timestamp,
@@ -237,19 +221,19 @@ int32_t AudioCodingModuleImpl::Encode(const InputData& input_data) {
 
 // Can be called multiple times for Codec, CNG, RED.
 int AudioCodingModuleImpl::RegisterSendCodec(const CodecInst& send_codec) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return codec_manager_.RegisterEncoder(send_codec);
 }
 
 void AudioCodingModuleImpl::RegisterExternalSendCodec(
     AudioEncoderMutable* external_speech_encoder) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   codec_manager_.RegisterEncoder(external_speech_encoder);
 }
 
 // Get current send codec.
 int AudioCodingModuleImpl::SendCodec(CodecInst* current_codec) const {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return codec_manager_.GetCodecInst(current_codec);
 }
 
@@ -257,7 +241,7 @@ int AudioCodingModuleImpl::SendCodec(CodecInst* current_codec) const {
 int AudioCodingModuleImpl::SendFrequency() const {
   WEBRTC_TRACE(webrtc::kTraceStream, webrtc::kTraceAudioCoding, id_,
                "SendFrequency()");
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
 
   if (!codec_manager_.CurrentEncoder()) {
     WEBRTC_TRACE(webrtc::kTraceStream, webrtc::kTraceAudioCoding, id_,
@@ -269,7 +253,7 @@ int AudioCodingModuleImpl::SendFrequency() const {
 }
 
 void AudioCodingModuleImpl::SetBitRate(int bitrate_bps) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   if (codec_manager_.CurrentEncoder()) {
     codec_manager_.CurrentEncoder()->SetTargetBitrate(bitrate_bps);
   }
@@ -279,7 +263,7 @@ void AudioCodingModuleImpl::SetBitRate(int bitrate_bps) {
 // the encoded buffers.
 int AudioCodingModuleImpl::RegisterTransportCallback(
     AudioPacketizationCallback* transport) {
-  CriticalSectionScoped lock(callback_crit_sect_);
+  CriticalSectionScoped lock(callback_crit_sect_.get());
   packetization_callback_ = transport;
   return 0;
 }
@@ -287,7 +271,7 @@ int AudioCodingModuleImpl::RegisterTransportCallback(
 // Add 10MS of raw (PCM) audio data to the encoder.
 int AudioCodingModuleImpl::Add10MsData(const AudioFrame& audio_frame) {
   InputData input_data;
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   int r = Add10MsDataInternal(audio_frame, &input_data);
   return r < 0 ? r : Encode(input_data);
 }
@@ -460,7 +444,7 @@ int AudioCodingModuleImpl::PreprocessToAddData(const AudioFrame& in_frame,
 //
 
 bool AudioCodingModuleImpl::REDStatus() const {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return codec_manager_.red_enabled();
 }
 
@@ -468,7 +452,7 @@ bool AudioCodingModuleImpl::REDStatus() const {
 int AudioCodingModuleImpl::SetREDStatus(
 #ifdef WEBRTC_CODEC_RED
     bool enable_red) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return codec_manager_.SetCopyRed(enable_red) ? 0 : -1;
 #else
     bool /* enable_red */) {
@@ -483,17 +467,17 @@ int AudioCodingModuleImpl::SetREDStatus(
 //
 
 bool AudioCodingModuleImpl::CodecFEC() const {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return codec_manager_.codec_fec_enabled();
 }
 
 int AudioCodingModuleImpl::SetCodecFEC(bool enable_codec_fec) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return codec_manager_.SetCodecFEC(enable_codec_fec);
 }
 
 int AudioCodingModuleImpl::SetPacketLossRate(int loss_rate) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   if (HaveValidEncoder("SetPacketLossRate")) {
     codec_manager_.CurrentSpeechEncoder()->SetProjectedPacketLossRate(
         loss_rate / 100.0);
@@ -509,14 +493,14 @@ int AudioCodingModuleImpl::SetVAD(bool enable_dtx,
                                   ACMVADMode mode) {
   // Note: |enable_vad| is not used; VAD is enabled based on the DTX setting.
   DCHECK_EQ(enable_dtx, enable_vad);
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return codec_manager_.SetVAD(enable_dtx, mode);
 }
 
 // Get VAD/DTX settings.
 int AudioCodingModuleImpl::VAD(bool* dtx_enabled, bool* vad_enabled,
                                ACMVADMode* mode) const {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   codec_manager_.VAD(dtx_enabled, vad_enabled, mode);
   return 0;
 }
@@ -526,7 +510,7 @@ int AudioCodingModuleImpl::VAD(bool* dtx_enabled, bool* vad_enabled,
 //
 
 int AudioCodingModuleImpl::InitializeReceiver() {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return InitializeReceiverSafe();
 }
 
@@ -566,7 +550,7 @@ int AudioCodingModuleImpl::ReceiveFrequency() const {
   WEBRTC_TRACE(webrtc::kTraceStream, webrtc::kTraceAudioCoding, id_,
                "ReceiveFrequency()");
 
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
 
   int codec_id = receiver_.last_audio_codec_id();
 
@@ -579,7 +563,7 @@ int AudioCodingModuleImpl::PlayoutFrequency() const {
   WEBRTC_TRACE(webrtc::kTraceStream, webrtc::kTraceAudioCoding, id_,
                "PlayoutFrequency()");
 
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
 
   return receiver_.current_sample_rate_hz();
 }
@@ -587,7 +571,7 @@ int AudioCodingModuleImpl::PlayoutFrequency() const {
 // Register possible receive codecs, can be called multiple times,
 // for codecs, CNG (NB, WB and SWB), DTMF, RED.
 int AudioCodingModuleImpl::RegisterReceiveCodec(const CodecInst& codec) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   DCHECK(receiver_initialized_);
   if (codec.channels > 2 || codec.channels < 0) {
     LOG_F(LS_ERROR) << "Unsupported number of channels: " << codec.channels;
@@ -619,7 +603,7 @@ int AudioCodingModuleImpl::RegisterExternalReceiveCodec(
     AudioDecoder* external_decoder,
     int sample_rate_hz,
     int num_channels) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   DCHECK(receiver_initialized_);
   if (num_channels > 2 || num_channels < 0) {
     LOG_F(LS_ERROR) << "Unsupported number of channels: " << num_channels;
@@ -639,7 +623,7 @@ int AudioCodingModuleImpl::RegisterExternalReceiveCodec(
 
 // Get current received codec.
 int AudioCodingModuleImpl::ReceiveCodec(CodecInst* current_codec) const {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return receiver_.LastAudioCodec(current_codec);
 }
 
@@ -709,7 +693,7 @@ int AudioCodingModuleImpl::GetNetworkStatistics(NetworkStatistics* statistics) {
 int AudioCodingModuleImpl::RegisterVADCallback(ACMVADCallback* vad_callback) {
   WEBRTC_TRACE(webrtc::kTraceDebug, webrtc::kTraceAudioCoding, id_,
                "RegisterVADCallback()");
-  CriticalSectionScoped lock(callback_crit_sect_);
+  CriticalSectionScoped lock(callback_crit_sect_.get());
   vad_callback_ = vad_callback;
   return 0;
 }
@@ -721,10 +705,10 @@ int AudioCodingModuleImpl::IncomingPayload(const uint8_t* incoming_payload,
                                            uint32_t timestamp) {
   // We are not acquiring any lock when interacting with |aux_rtp_header_| no
   // other method uses this member variable.
-  if (aux_rtp_header_ == NULL) {
+  if (!aux_rtp_header_) {

[hlundin-webrtc, 2015/08/24 13:29:21]

Why is this even a pointer? Why not just a plain member variable?

[kwiberg-webrtc, 2015/08/24 13:51:46]

That's a good question...

1. One interesting fact is that aux_rtp_header_->header.payloadType is set to
payload_type on line 712. Is it really right to set that only when initializing,
and not on every packet? To preserve the existing behavior with a plain member
variable, I'd need a separate flag to keep track of whether it's been
initialized or not, but if I could set payload_type on each IncomingPayload call
I wouldn't need that.

2. One possible justification is that the indirection saves memory (the size of
a WebRtcRTPHeader) if we often don't end up calling IncomingPayload at all.
Might that be the case?

What do you think?

[hlundin-webrtc, 2015/08/24 14:01:12]

I agree that it seems odd not to set the payload type on each call. But in the
spirit of keeping this CL virtually no-op, I'd refrain from that change.
Perhaps.
I think that this method should be deleted. It is used only by the FilePlayer
utility. That piece of code should keep track of its own fake RTP header instead
of having ACM bend over backwards like this.

Keep the code as is, with a scoped_ptr, but add a TODO to remove this method.

[kwiberg-webrtc, 2015/08/25 07:47:34]

TODO added.

     // This is the first time that we are using |dummy_rtp_header_|
     // so we have to create it.
-    aux_rtp_header_ = new WebRtcRTPHeader;
+    aux_rtp_header_.reset(new WebRtcRTPHeader);
     aux_rtp_header_->header.payloadType = payload_type;
     // Don't matter in this case.
     aux_rtp_header_->header.ssrc = 0;
@@ -743,7 +727,7 @@ int AudioCodingModuleImpl::IncomingPayload(const uint8_t* incoming_payload,
 
 // TODO(henrik.lundin): Remove? Only used in tests. Deprecated in VoiceEngine.
 int AudioCodingModuleImpl::SetISACMaxRate(int max_bit_per_sec) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
 
   if (!HaveValidEncoder("SetISACMaxRate")) {
     return -1;
@@ -755,7 +739,7 @@ int AudioCodingModuleImpl::SetISACMaxRate(int max_bit_per_sec) {
 
 // TODO(henrik.lundin): Remove? Only used in tests. Deprecated in VoiceEngine.
 int AudioCodingModuleImpl::SetISACMaxPayloadSize(int max_size_bytes) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
 
   if (!HaveValidEncoder("SetISACMaxPayloadSize")) {
     return -1;
@@ -766,7 +750,7 @@ int AudioCodingModuleImpl::SetISACMaxPayloadSize(int max_size_bytes) {
 }
 
 int AudioCodingModuleImpl::SetOpusApplication(OpusApplicationMode application) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   if (!HaveValidEncoder("SetOpusApplication")) {
     return -1;
   }
@@ -787,7 +771,7 @@ int AudioCodingModuleImpl::SetOpusApplication(OpusApplicationMode application) {
 
 // Informs Opus encoder of the maximum playback rate the receiver will render.
 int AudioCodingModuleImpl::SetOpusMaxPlaybackRate(int frequency_hz) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   if (!HaveValidEncoder("SetOpusMaxPlaybackRate")) {
     return -1;
   }
@@ -797,7 +781,7 @@ int AudioCodingModuleImpl::SetOpusMaxPlaybackRate(int frequency_hz) {
 }
 
 int AudioCodingModuleImpl::EnableOpusDtx() {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   if (!HaveValidEncoder("EnableOpusDtx")) {
     return -1;
   }
@@ -805,7 +789,7 @@ int AudioCodingModuleImpl::EnableOpusDtx() {
 }
 
 int AudioCodingModuleImpl::DisableOpusDtx() {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   if (!HaveValidEncoder("DisableOpusDtx")) {
     return -1;
   }
@@ -831,7 +815,7 @@ int AudioCodingModuleImpl::UnregisterReceiveCodec(uint8_t payload_type) {
 
 int AudioCodingModuleImpl::SetInitialPlayoutDelay(int delay_ms) {
   {
-    CriticalSectionScoped lock(acm_crit_sect_);
+    CriticalSectionScoped lock(acm_crit_sect_.get());
     // Initialize receiver, if it is not initialized. Otherwise, initial delay
     // is reset upon initialization of the receiver.
     if (!receiver_initialized_)
@@ -925,7 +909,7 @@ const CodecInst* AudioCodingImpl::GetSenderCodecInst() {
 
 int AudioCodingImpl::Add10MsAudio(const AudioFrame& audio_frame) {
   acm2::AudioCodingModuleImpl::InputData input_data;
-  CriticalSectionScoped lock(acm_old_->acm_crit_sect_);
+  CriticalSectionScoped lock(acm_old_->acm_crit_sect_.get());
   if (acm_old_->Add10MsDataInternal(audio_frame, &input_data) != 0)
     return -1;
   return acm_old_->Encode(input_data);