Bug 1456115 - DO NOT LAND. Use finer grained locking in Neteq::GetAudioInternal. draft
authorPaul Adenot <paul@paul.cx>
Fri, 20 Apr 2018 15:38:25 +0200
changeset 786559 fcd101b666349cc20a584b26c1efb6443599f355
parent 786558 2c7df2f6c5fe0ce219db94ecb247317227f0a9d9
push id107512
push userpaul@paul.cx
push dateMon, 23 Apr 2018 15:14:13 +0000
bugs1456115
milestone61.0a1
Bug 1456115 - DO NOT LAND. Use finer grained locking in Neteq::GetAudioInternal. MozReview-Commit-ID: 91EsrNP6uL3
media/webrtc/trunk/webrtc/modules/audio_coding/neteq/neteq_impl.cc
--- a/media/webrtc/trunk/webrtc/modules/audio_coding/neteq/neteq_impl.cc
+++ b/media/webrtc/trunk/webrtc/modules/audio_coding/neteq/neteq_impl.cc
@@ -850,236 +850,265 @@ int NetEqImpl::InsertPacketInternal(cons
 }
 
 int NetEqImpl::GetAudioInternal(AudioFrame* audio_frame, bool* muted) {
   PacketList packet_list;
   DtmfEvent dtmf_event;
   Operations operation;
   bool play_dtmf;
   *muted = false;
-  tick_timer_->Increment();
-  stats_.IncreaseCounter(output_size_samples_, fs_hz_);
+  {
+    rtc::CritScope lock(&crit_sect_);
+    tick_timer_->Increment();
+    stats_.IncreaseCounter(output_size_samples_, fs_hz_);
+  }
 
+  bool ismuted = false;
+  {
+    rtc::CritScope lock(&crit_sect_);
+    ismuted = enable_muted_state_ && expand_->Muted() && packet_buffer_->Empty();
+  }
   // Check for muted state.
-  if (enable_muted_state_ && expand_->Muted() && packet_buffer_->Empty()) {
+  if (ismuted) {
     RTC_DCHECK_EQ(last_mode_, kModeExpand);
     playout_timestamp_ += static_cast<uint32_t>(output_size_samples_);
     audio_frame->sample_rate_hz_ = fs_hz_;
     audio_frame->samples_per_channel_ = output_size_samples_;
     audio_frame->timestamp_ =
         first_packet_
             ? 0
             : timestamp_scaler_->ToExternal(playout_timestamp_) -
                   static_cast<uint32_t>(audio_frame->samples_per_channel_);
     audio_frame->num_channels_ = sync_buffer_->Channels();
     stats_.ExpandedNoiseSamples(output_size_samples_);
     *muted = true;
     return 0;
   }
 
-  int return_value = GetDecision(&operation, &packet_list, &dtmf_event,
-                                 &play_dtmf);
+  int return_value;
+  {
+    rtc::CritScope lock(&crit_sect_);
+    return_value =
+      GetDecision(&operation, &packet_list, &dtmf_event, &play_dtmf);
+  }
   if (return_value != 0) {
     last_mode_ = kModeError;
     return return_value;
   }
 
   AudioDecoder::SpeechType speech_type;
   int length = 0;
-  int decode_return_value = Decode(&packet_list, &operation,
-                                   &length, &speech_type);
+  int decode_return_value ;
+  {
+    rtc::CritScope lock(&crit_sect_);
+    decode_return_value = Decode(&packet_list, &operation,
+                                 &length, &speech_type);
+  }
 
   assert(vad_.get());
   bool sid_frame_available =
       (operation == kRfc3389Cng && !packet_list.empty());
+{
+  rtc::CritScope lock(&crit_sect_);
   vad_->Update(decoded_buffer_.get(), static_cast<size_t>(length), speech_type,
                sid_frame_available, fs_hz_);
+}
 
   if (sid_frame_available || speech_type == AudioDecoder::kComfortNoise) {
     // Start a new stopwatch since we are decoding a new CNG packet.
-    generated_noise_stopwatch_ = tick_timer_->GetNewStopwatch();
-  }
-
-  algorithm_buffer_->Clear();
-  switch (operation) {
-    case kNormal: {
-      DoNormal(decoded_buffer_.get(), length, speech_type, play_dtmf);
-      break;
-    }
-    case kMerge: {
-      DoMerge(decoded_buffer_.get(), length, speech_type, play_dtmf);
-      break;
-    }
-    case kExpand: {
-      return_value = DoExpand(play_dtmf);
-      break;
-    }
-    case kAccelerate:
-    case kFastAccelerate: {
-      const bool fast_accelerate =
-          enable_fast_accelerate_ && (operation == kFastAccelerate);
-      return_value = DoAccelerate(decoded_buffer_.get(), length, speech_type,
-                                  play_dtmf, fast_accelerate);
-      break;
-    }
-    case kPreemptiveExpand: {
-      return_value = DoPreemptiveExpand(decoded_buffer_.get(), length,
-                                        speech_type, play_dtmf);
-      break;
-    }
-    case kRfc3389Cng:
-    case kRfc3389CngNoPacket: {
-      return_value = DoRfc3389Cng(&packet_list, play_dtmf);
-      break;
-    }
-    case kCodecInternalCng: {
-      // This handles the case when there is no transmission and the decoder
-      // should produce internal comfort noise.
-      // TODO(hlundin): Write test for codec-internal CNG.
-      DoCodecInternalCng(decoded_buffer_.get(), length);
-      break;
+    {
+      rtc::CritScope lock(&crit_sect_);
+      generated_noise_stopwatch_ = tick_timer_->GetNewStopwatch();
     }
-    case kDtmf: {
-      // TODO(hlundin): Write test for this.
-      return_value = DoDtmf(dtmf_event, &play_dtmf);
-      break;
-    }
-    case kAlternativePlc: {
-      // TODO(hlundin): Write test for this.
-      DoAlternativePlc(false);
-      break;
-    }
-    case kAlternativePlcIncreaseTimestamp: {
-      // TODO(hlundin): Write test for this.
-      DoAlternativePlc(true);
-      break;
-    }
-    case kAudioRepetitionIncreaseTimestamp: {
-      // TODO(hlundin): Write test for this.
-      sync_buffer_->IncreaseEndTimestamp(
-          static_cast<uint32_t>(output_size_samples_));
-      // Skipping break on purpose. Execution should move on into the
-      // next case.
-      FALLTHROUGH();
-    }
-    case kAudioRepetition: {
-      // TODO(hlundin): Write test for this.
-      // Copy last |output_size_samples_| from |sync_buffer_| to
-      // |algorithm_buffer|.
-      algorithm_buffer_->PushBackFromIndex(
-          *sync_buffer_, sync_buffer_->Size() - output_size_samples_);
-      expand_->Reset();
-      break;
-    }
-    case kUndefined: {
-      LOG(LS_ERROR) << "Invalid operation kUndefined.";
-      assert(false);  // This should not happen.
-      last_mode_ = kModeError;
-      return kInvalidOperation;
-    }
-  }  // End of switch.
-  last_operation_ = operation;
-  if (return_value < 0) {
-    return return_value;
-  }
-
-  if (last_mode_ != kModeRfc3389Cng) {
-    comfort_noise_->Reset();
   }
 
-  // Copy from |algorithm_buffer| to |sync_buffer_|.
-  sync_buffer_->PushBack(*algorithm_buffer_);
+  {
+    rtc::CritScope lock(&crit_sect_);
+    algorithm_buffer_->Clear();
+    switch (operation) {
+      case kNormal: {
+        DoNormal(decoded_buffer_.get(), length, speech_type, play_dtmf);
+        break;
+      }
+      case kMerge: {
+        DoMerge(decoded_buffer_.get(), length, speech_type, play_dtmf);
+        break;
+      }
+      case kExpand: {
+        return_value = DoExpand(play_dtmf);
+        break;
+      }
+      case kAccelerate:
+      case kFastAccelerate: {
+        const bool fast_accelerate =
+            enable_fast_accelerate_ && (operation == kFastAccelerate);
+        return_value = DoAccelerate(decoded_buffer_.get(), length, speech_type,
+                                    play_dtmf, fast_accelerate);
+        break;
+      }
+      case kPreemptiveExpand: {
+        return_value = DoPreemptiveExpand(decoded_buffer_.get(), length,
+                                          speech_type, play_dtmf);
+        break;
+      }
+      case kRfc3389Cng:
+      case kRfc3389CngNoPacket: {
+        return_value = DoRfc3389Cng(&packet_list, play_dtmf);
+        break;
+      }
+      case kCodecInternalCng: {
+        // This handles the case when there is no transmission and the decoder
+        // should produce internal comfort noise.
+        // TODO(hlundin): Write test for codec-internal CNG.
+        DoCodecInternalCng(decoded_buffer_.get(), length);
+        break;
+      }
+      case kDtmf: {
+        // TODO(hlundin): Write test for this.
+        return_value = DoDtmf(dtmf_event, &play_dtmf);
+        break;
+      }
+      case kAlternativePlc: {
+        // TODO(hlundin): Write test for this.
+        DoAlternativePlc(false);
+        break;
+      }
+      case kAlternativePlcIncreaseTimestamp: {
+        // TODO(hlundin): Write test for this.
+        DoAlternativePlc(true);
+        break;
+      }
+      case kAudioRepetitionIncreaseTimestamp: {
+        // TODO(hlundin): Write test for this.
+        sync_buffer_->IncreaseEndTimestamp(
+            static_cast<uint32_t>(output_size_samples_));
+        // Skipping break on purpose. Execution should move on into the
+        // next case.
+        FALLTHROUGH();
+      }
+      case kAudioRepetition: {
+        // TODO(hlundin): Write test for this.
+        // Copy last |output_size_samples_| from |sync_buffer_| to
+        // |algorithm_buffer|.
+        algorithm_buffer_->PushBackFromIndex(
+            *sync_buffer_, sync_buffer_->Size() - output_size_samples_);
+        expand_->Reset();
+        break;
+      }
+      case kUndefined: {
+        LOG(LS_ERROR) << "Invalid operation kUndefined.";
+        assert(false);  // This should not happen.
+        last_mode_ = kModeError;
+        return kInvalidOperation;
+      }
+    }  // End of switch.
+    last_operation_ = operation;
+    if (return_value < 0) {
+      return return_value;
+    }
+    
 
-  // Extract data from |sync_buffer_| to |output|.
-  size_t num_output_samples_per_channel = output_size_samples_;
-  size_t num_output_samples = output_size_samples_ * sync_buffer_->Channels();
-  if (num_output_samples > AudioFrame::kMaxDataSizeSamples) {
-    LOG(LS_WARNING) << "Output array is too short. "
-                    << AudioFrame::kMaxDataSizeSamples << " < "
-                    << output_size_samples_ << " * "
-                    << sync_buffer_->Channels();
-    num_output_samples = AudioFrame::kMaxDataSizeSamples;
-    num_output_samples_per_channel =
-        AudioFrame::kMaxDataSizeSamples / sync_buffer_->Channels();
-  }
-  sync_buffer_->GetNextAudioInterleaved(num_output_samples_per_channel,
-                                        audio_frame);
-  audio_frame->sample_rate_hz_ = fs_hz_;
-  if (sync_buffer_->FutureLength() < expand_->overlap_length()) {
-    // The sync buffer should always contain |overlap_length| samples, but now
-    // too many samples have been extracted. Reinstall the |overlap_length|
-    // lookahead by moving the index.
-    const size_t missing_lookahead_samples =
-        expand_->overlap_length() - sync_buffer_->FutureLength();
-    RTC_DCHECK_GE(sync_buffer_->next_index(), missing_lookahead_samples);
-    sync_buffer_->set_next_index(sync_buffer_->next_index() -
-                                 missing_lookahead_samples);
-  }
-  if (audio_frame->samples_per_channel_ != output_size_samples_) {
-    LOG(LS_ERROR) << "audio_frame->samples_per_channel_ ("
-                  << audio_frame->samples_per_channel_
-                  << ") != output_size_samples_ (" << output_size_samples_
-                  << ")";
-    // TODO(minyue): treatment of under-run, filling zeros
-    memset(audio_frame->data_, 0, num_output_samples * sizeof(int16_t));
-    return kSampleUnderrun;
+    if (last_mode_ != kModeRfc3389Cng) {
+      comfort_noise_->Reset();
+    }
   }
 
-  // Should always have overlap samples left in the |sync_buffer_|.
-  RTC_DCHECK_GE(sync_buffer_->FutureLength(), expand_->overlap_length());
-
-  if (play_dtmf) {
-    return_value =
-        DtmfOverdub(dtmf_event, sync_buffer_->Channels(), audio_frame->data_);
-  }
+  {
+    rtc::CritScope lock(&crit_sect_);
+    // Copy from |algorithm_buffer| to |sync_buffer_|.
+    sync_buffer_->PushBack(*algorithm_buffer_);
 
-  // Update the background noise parameters if last operation wrote data
-  // straight from the decoder to the |sync_buffer_|. That is, none of the
-  // operations that modify the signal can be followed by a parameter update.
-  if ((last_mode_ == kModeNormal) ||
-      (last_mode_ == kModeAccelerateFail) ||
-      (last_mode_ == kModePreemptiveExpandFail) ||
-      (last_mode_ == kModeRfc3389Cng) ||
-      (last_mode_ == kModeCodecInternalCng)) {
-    background_noise_->Update(*sync_buffer_, *vad_.get());
-  }
+    // Extract data from |sync_buffer_| to |output|.
+    size_t num_output_samples_per_channel = output_size_samples_;
+    size_t num_output_samples = output_size_samples_ * sync_buffer_->Channels();
+    if (num_output_samples > AudioFrame::kMaxDataSizeSamples) {
+      LOG(LS_WARNING) << "Output array is too short. "
+                      << AudioFrame::kMaxDataSizeSamples << " < "
+                      << output_size_samples_ << " * "
+                      << sync_buffer_->Channels();
+      num_output_samples = AudioFrame::kMaxDataSizeSamples;
+      num_output_samples_per_channel =
+          AudioFrame::kMaxDataSizeSamples / sync_buffer_->Channels();
+    }
+    sync_buffer_->GetNextAudioInterleaved(num_output_samples_per_channel,
+                                          audio_frame);
+    audio_frame->sample_rate_hz_ = fs_hz_;
+    if (sync_buffer_->FutureLength() < expand_->overlap_length()) {
+      // The sync buffer should always contain |overlap_length| samples, but now
+      // too many samples have been extracted. Reinstall the |overlap_length|
+      // lookahead by moving the index.
+      const size_t missing_lookahead_samples =
+          expand_->overlap_length() - sync_buffer_->FutureLength();
+      RTC_DCHECK_GE(sync_buffer_->next_index(), missing_lookahead_samples);
+      sync_buffer_->set_next_index(sync_buffer_->next_index() -
+                                   missing_lookahead_samples);
+    }
+    if (audio_frame->samples_per_channel_ != output_size_samples_) {
+      LOG(LS_ERROR) << "audio_frame->samples_per_channel_ ("
+                    << audio_frame->samples_per_channel_
+                    << ") != output_size_samples_ (" << output_size_samples_
+                    << ")";
+      // TODO(minyue): treatment of under-run, filling zeros
+      memset(audio_frame->data_, 0, num_output_samples * sizeof(int16_t));
+      return kSampleUnderrun;
+    }
 
-  if (operation == kDtmf) {
-    // DTMF data was written the end of |sync_buffer_|.
-    // Update index to end of DTMF data in |sync_buffer_|.
-    sync_buffer_->set_dtmf_index(sync_buffer_->Size());
-  }
+    // Should always have overlap samples left in the |sync_buffer_|.
+    RTC_DCHECK_GE(sync_buffer_->FutureLength(), expand_->overlap_length());
 
-  if (last_mode_ != kModeExpand) {
-    // If last operation was not expand, calculate the |playout_timestamp_| from
-    // the |sync_buffer_|. However, do not update the |playout_timestamp_| if it
-    // would be moved "backwards".
-    uint32_t temp_timestamp = sync_buffer_->end_timestamp() -
-        static_cast<uint32_t>(sync_buffer_->FutureLength());
-    if (static_cast<int32_t>(temp_timestamp - playout_timestamp_) > 0) {
-      playout_timestamp_ = temp_timestamp;
+    if (play_dtmf) {
+      return_value =
+          DtmfOverdub(dtmf_event, sync_buffer_->Channels(), audio_frame->data_);
+    }
+
+    // Update the background noise parameters if last operation wrote data
+    // straight from the decoder to the |sync_buffer_|. That is, none of the
+    // operations that modify the signal can be followed by a parameter update.
+    if ((last_mode_ == kModeNormal) ||
+        (last_mode_ == kModeAccelerateFail) ||
+        (last_mode_ == kModePreemptiveExpandFail) ||
+        (last_mode_ == kModeRfc3389Cng) ||
+        (last_mode_ == kModeCodecInternalCng)) {
+      background_noise_->Update(*sync_buffer_, *vad_.get());
+    }
+
+    if (operation == kDtmf) {
+      // DTMF data was written the end of |sync_buffer_|.
+      // Update index to end of DTMF data in |sync_buffer_|.
+      sync_buffer_->set_dtmf_index(sync_buffer_->Size());
     }
-  } else {
-    // Use dead reckoning to estimate the |playout_timestamp_|.
-    playout_timestamp_ += static_cast<uint32_t>(output_size_samples_);
-  }
-  // Set the timestamp in the audio frame to zero before the first packet has
-  // been inserted. Otherwise, subtract the frame size in samples to get the
-  // timestamp of the first sample in the frame (playout_timestamp_ is the
-  // last + 1).
-  audio_frame->timestamp_ =
-      first_packet_
-          ? 0
-          : timestamp_scaler_->ToExternal(playout_timestamp_) -
-                static_cast<uint32_t>(audio_frame->samples_per_channel_);
 
-  if (!(last_mode_ == kModeRfc3389Cng ||
-      last_mode_ == kModeCodecInternalCng ||
-      last_mode_ == kModeExpand)) {
-    generated_noise_stopwatch_.reset();
+    if (last_mode_ != kModeExpand) {
+      // If last operation was not expand, calculate the |playout_timestamp_| from
+      // the |sync_buffer_|. However, do not update the |playout_timestamp_| if it
+      // would be moved "backwards".
+      uint32_t temp_timestamp = sync_buffer_->end_timestamp() -
+          static_cast<uint32_t>(sync_buffer_->FutureLength());
+      if (static_cast<int32_t>(temp_timestamp - playout_timestamp_) > 0) {
+        playout_timestamp_ = temp_timestamp;
+      }
+    } else {
+      // Use dead reckoning to estimate the |playout_timestamp_|.
+      playout_timestamp_ += static_cast<uint32_t>(output_size_samples_);
+    }
+    // Set the timestamp in the audio frame to zero before the first packet has
+    // been inserted. Otherwise, subtract the frame size in samples to get the
+    // timestamp of the first sample in the frame (playout_timestamp_ is the
+    // last + 1).
+    audio_frame->timestamp_ =
+        first_packet_
+            ? 0
+            : timestamp_scaler_->ToExternal(playout_timestamp_) -
+                  static_cast<uint32_t>(audio_frame->samples_per_channel_);
+
+    if (!(last_mode_ == kModeRfc3389Cng ||
+        last_mode_ == kModeCodecInternalCng ||
+        last_mode_ == kModeExpand)) {
+      generated_noise_stopwatch_.reset();
+    }
   }
 
   if (decode_return_value) return decode_return_value;
   return return_value;
 }
 
 int NetEqImpl::GetDecision(Operations* operation,
                            PacketList* packet_list,