Get rid of the manual destructor in AudioCodingModuleImpl

webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.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.
  */
 
@@ skipping 121 matching lines @@
 
 AudioCodingModuleImpl::AudioCodingModuleImpl(
     const AudioCodingModule::Config& config)
     : acm_crit_sect_(CriticalSectionWrapper::CreateCriticalSection()),
       id_(config.id),
       expected_codec_ts_(0xD87F3F9F),
       expected_in_ts_(0xD87F3F9F),
       receiver_(config),
       bitrate_logger_("WebRTC.Audio.TargetBitrateInKbps"),
       previous_pltype_(255),
-      aux_rtp_header_(NULL),
       receiver_initialized_(false),
       first_10ms_data_(false),
       first_frame_(true),
       callback_crit_sect_(CriticalSectionWrapper::CreateCriticalSection()),
       packetization_callback_(NULL),
       vad_callback_(NULL) {
   if (InitializeReceiverSafe() < 0) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, id_,
                  "Cannot initialize receiver");
   }
   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;
   uint8_t previous_pltype;
 
   // Check if there is an encoder before.
   if (!HaveValidEncoder("Process"))
     return -1;
 
   AudioEncoder* audio_encoder = codec_manager_.CurrentEncoder();
@@ skipping 25 matching lines @@
   FrameType frame_type;
   if (encoded_info.encoded_bytes == 0 && encoded_info.send_even_if_empty) {
     frame_type = kFrameEmpty;
     encoded_info.payload_type = previous_pltype;
   } else {
     DCHECK_GT(encoded_info.encoded_bytes, 0u);
     frame_type = encoded_info.speech ? kAudioFrameSpeech : kAudioFrameCN;
   }
 
   {
-    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,
           stream, encoded_info.encoded_bytes,
           my_fragmentation.fragmentationVectorSize > 0 ? &my_fragmentation
                                                        : nullptr);
     }
 
     if (vad_callback_) {
       // Callback with VAD decision.
       vad_callback_->InFrameType(frame_type);
     }
   }
   previous_pltype_ = encoded_info.payload_type;
   return static_cast<int32_t>(encoded_info.encoded_bytes);
 }
 
 /////////////////////////////////////////
 //   Sender
 //
 
 // 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);
 }
 
 // Get current send frequency.
 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_,
                  "SendFrequency Failed, no codec is registered");
     return -1;
   }
 
   return codec_manager_.CurrentEncoder()->SampleRateHz();
 }
 
 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);
   }
 }
 
 // Register a transport callback which will be called to deliver
 // the encoded buffers.
 int AudioCodingModuleImpl::RegisterTransportCallback(
     AudioPacketizationCallback* transport) {
-  CriticalSectionScoped lock(callback_crit_sect_);
+  CriticalSectionScoped lock(callback_crit_sect_.get());
   packetization_callback_ = transport;
   return 0;
 }
 
 // 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);
 }
 
 int AudioCodingModuleImpl::Add10MsDataInternal(const AudioFrame& audio_frame,
                                                InputData* input_data) {
   if (audio_frame.samples_per_channel_ <= 0) {
     assert(false);
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, id_,
                  "Cannot Add 10 ms audio, payload length is negative or "
@@ skipping 152 matching lines @@
   expected_in_ts_ += static_cast<uint32_t>(in_frame.samples_per_channel_);
 
   return 0;
 }
 
 /////////////////////////////////////////
 //   (RED) Redundant Coding
 //
 
 bool AudioCodingModuleImpl::REDStatus() const {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return codec_manager_.red_enabled();
 }
 
 // Configure RED status i.e on/off.
 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 */) {
   WEBRTC_TRACE(webrtc::kTraceWarning, webrtc::kTraceAudioCoding, id_,
                "  WEBRTC_CODEC_RED is undefined");
   return -1;
 #endif
 }
 
 /////////////////////////////////////////
 //   (FEC) Forward Error Correction (codec internal)
 //
 
 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);
   }
   return 0;
 }
 
 /////////////////////////////////////////
 //   (VAD) Voice Activity Detection
 //
 int AudioCodingModuleImpl::SetVAD(bool enable_dtx,
                                   bool enable_vad,
                                   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;
 }
 
 /////////////////////////////////////////
 //   Receiver
 //
 
 int AudioCodingModuleImpl::InitializeReceiver() {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   return InitializeReceiverSafe();
 }
 
 // Initialize receiver, resets codec database etc.
 int AudioCodingModuleImpl::InitializeReceiverSafe() {
   // If the receiver is already initialized then we want to destroy any
   // existing decoders. After a call to this function, we should have a clean
   // start-up.
   if (receiver_initialized_) {
     if (receiver_.RemoveAllCodecs() < 0)
@@ skipping 19 matching lines @@
   }
   receiver_initialized_ = true;
   return 0;
 }
 
 // Get current receive frequency.
 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();
 
   return codec_id < 0 ? receiver_.current_sample_rate_hz() :
                         ACMCodecDB::database_[codec_id].plfreq;
 }
 
 // Get current playout frequency.
 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();
 }
 
 // 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;
     return -1;
   }
 
   int codec_id = ACMCodecDB::ReceiverCodecNumber(codec);
   if (codec_id < 0 || codec_id >= ACMCodecDB::kNumCodecs) {
     LOG_F(LS_ERROR) << "Wrong codec params to be registered as receive codec";
     return -1;
@@ skipping 11 matching lines @@
   return receiver_.AddCodec(codec_id, codec.pltype, codec.channels,
                             codec.plfreq,
                             codec_manager_.GetAudioDecoder(codec));
 }
 
 int AudioCodingModuleImpl::RegisterExternalReceiveCodec(
     int rtp_payload_type,
     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;
     return -1;
   }
 
   // Check if the payload-type is valid.
   if (!ACMCodecDB::ValidPayloadType(rtp_payload_type)) {
     LOG_F(LS_ERROR) << "Invalid payload-type " << rtp_payload_type
                     << " for external decoder.";
     return -1;
   }
 
   return receiver_.AddCodec(-1 /* external */, rtp_payload_type, num_channels,
                             sample_rate_hz, external_decoder);
 }
 
 // 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);
 }
 
 // Incoming packet from network parsed and ready for decode.
 int AudioCodingModuleImpl::IncomingPacket(const uint8_t* incoming_payload,
                                           const size_t payload_length,
                                           const WebRtcRTPHeader& rtp_header) {
   return receiver_.InsertPacket(rtp_header, incoming_payload, payload_length);
 }
 
@@ skipping 49 matching lines @@
 // TODO(turajs) change the return value to void. Also change the corresponding
 // NetEq function.
 int AudioCodingModuleImpl::GetNetworkStatistics(NetworkStatistics* statistics) {
   receiver_.GetNetworkStatistics(statistics);
   return 0;
 }
 
 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;
 }
 
 // TODO(tlegrand): Modify this function to work for stereo, and add tests.
 int AudioCodingModuleImpl::IncomingPayload(const uint8_t* incoming_payload,
                                            size_t payload_length,
                                            uint8_t payload_type,
                                            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;
     aux_rtp_header_->header.markerBit = false;
     // Start with random numbers.
     aux_rtp_header_->header.sequenceNumber = 0x1234;  // Arbitrary.
     aux_rtp_header_->type.Audio.channel = 1;
   }
 
   aux_rtp_header_->header.timestamp = timestamp;
   IncomingPacket(incoming_payload, payload_length, *aux_rtp_header_);
   // Get ready for the next payload.
   aux_rtp_header_->header.sequenceNumber++;
   return 0;
 }
 
 // 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;
   }
 
   codec_manager_.CurrentSpeechEncoder()->SetMaxRate(max_bit_per_sec);
   return 0;
 }
 
 // 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;
   }
 
   codec_manager_.CurrentSpeechEncoder()->SetMaxPayloadSize(max_size_bytes);
   return 0;
 }
 
 int AudioCodingModuleImpl::SetOpusApplication(OpusApplicationMode application) {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   if (!HaveValidEncoder("SetOpusApplication")) {
     return -1;
   }
   AudioEncoderMutable::Application app;
   switch (application) {
     case kVoip:
       app = AudioEncoderMutable::kApplicationSpeech;
       break;
     case kAudio:
       app = AudioEncoderMutable::kApplicationAudio;
       break;
     default:
       FATAL();
       return 0;
   }
   return codec_manager_.CurrentSpeechEncoder()->SetApplication(app) ? 0 : -1;
 }
 
 // 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;
   }
   return codec_manager_.CurrentSpeechEncoder()->SetMaxPlaybackRate(frequency_hz)
              ? 0
              : -1;
 }
 
 int AudioCodingModuleImpl::EnableOpusDtx() {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   if (!HaveValidEncoder("EnableOpusDtx")) {
     return -1;
   }
   return codec_manager_.CurrentSpeechEncoder()->SetDtx(true) ? 0 : -1;
 }
 
 int AudioCodingModuleImpl::DisableOpusDtx() {
-  CriticalSectionScoped lock(acm_crit_sect_);
+  CriticalSectionScoped lock(acm_crit_sect_.get());
   if (!HaveValidEncoder("DisableOpusDtx")) {
     return -1;
   }
   return codec_manager_.CurrentSpeechEncoder()->SetDtx(false) ? 0 : -1;
 }
 
 int AudioCodingModuleImpl::PlayoutTimestamp(uint32_t* timestamp) {
   return receiver_.GetPlayoutTimestamp(timestamp) ? 0 : -1;
 }
 
 bool AudioCodingModuleImpl::HaveValidEncoder(const char* caller_name) const {
   if (!codec_manager_.CurrentEncoder()) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, id_,
                  "%s failed: No send codec is registered.", caller_name);
     return false;
   }
   return true;
 }
 
 int AudioCodingModuleImpl::UnregisterReceiveCodec(uint8_t payload_type) {
   return receiver_.RemoveCodec(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_)
       InitializeReceiverSafe();
   }
   return receiver_.SetInitialDelay(delay_ms);
 }
 
 int AudioCodingModuleImpl::SetDtmfPlayoutStatus(bool enable) {
   return 0;
@@ skipping 73 matching lines @@
 
 const CodecInst* AudioCodingImpl::GetSenderCodecInst() {
   if (acm_old_->SendCodec(&current_send_codec_) != 0) {
     return NULL;
   }
   return &current_send_codec_;
 }
 
 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);
 }
 
 const ReceiverInfo* AudioCodingImpl::GetReceiverInfo() const {
   FATAL() << "Not implemented yet.";
   return reinterpret_cast<const ReceiverInfo*>(NULL);
 }
 
@@ skipping 219 matching lines @@
       *channels = 1;
       break;
 #endif
     default:
       FATAL() << "Codec type " << codec_type << " not supported.";
   }
   return true;
 }
 
 }  // namespace webrtc