Adding locking to webrtc::voe::Channel to fix race conditions

webrtc/voice_engine/channel.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 762 matching lines @@
     _mute(false),
     _panLeft(1.0f),
     _panRight(1.0f),
     _outputGain(1.0f),
     _playOutbandDtmfEvent(false),
     _playInbandDtmfEvent(false),
     _lastLocalTimeStamp(0),
     _lastPayloadType(0),
     _includeAudioLevelIndication(false),
     _outputSpeechType(AudioFrame::kNormalSpeech),
+    video_sync_lock_(CriticalSectionWrapper::CreateCriticalSection()),
     _average_jitter_buffer_delay_us(0),
-    least_required_delay_ms_(0),
     _previousTimestamp(0),
     _recPacketDelayMs(20),
     _RxVadDetection(false),
     _rxAgcIsEnabled(false),
     _rxNsIsEnabled(false),
     restored_packet_in_use_(false),
     rtcp_observer_(new VoERtcpObserver(this)),
     network_predictor_(new NetworkPredictor(Clock::GetRealTimeClock())),
     assoc_send_channel_lock_(CriticalSectionWrapper::CreateCriticalSection()),
     associate_send_channel_(ChannelOwner(nullptr))
@@ skipping 2759 matching lines @@
                  "Channel::GetNetworkStatistics()");
     return audio_coding_->GetNetworkStatistics(&stats);
 }
 
 void Channel::GetDecodingCallStatistics(AudioDecodingCallStats* stats) const {
   audio_coding_->GetDecodingCallStatistics(stats);
 }
 
 bool Channel::GetDelayEstimate(int* jitter_buffer_delay_ms,
                                int* playout_buffer_delay_ms) const {
+  CriticalSectionScoped cs(video_sync_lock_.get());
   if (_average_jitter_buffer_delay_us == 0) {
     WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId,_channelId),
                  "Channel::GetDelayEstimate() no valid estimate.");
     return false;
   }
   *jitter_buffer_delay_ms = (_average_jitter_buffer_delay_us + 500) / 1000 +
       _recPacketDelayMs;
   *playout_buffer_delay_ms = playout_delay_ms_;
   WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId,_channelId),
                "Channel::GetDelayEstimate()");
@@ skipping 39 matching lines @@
     if (audio_coding_->SetMinimumPlayoutDelay(delayMs) != 0)
     {
         _engineStatisticsPtr->SetLastError(
             VE_AUDIO_CODING_MODULE_ERROR, kTraceError,
             "SetMinimumPlayoutDelay() failed to set min playout delay");
         return -1;
     }
     return 0;
 }
 
-void Channel::UpdatePlayoutTimestamp(bool rtcp) {
-  uint32_t playout_timestamp = 0;
-
-  if (audio_coding_->PlayoutTimestamp(&playout_timestamp) == -1)  {
-    // This can happen if this channel has not been received any RTP packet. In
-    // this case, NetEq is not capable of computing playout timestamp.
-    return;
-  }
-
-  uint16_t delay_ms = 0;
-  if (_audioDeviceModulePtr->PlayoutDelay(&delay_ms) == -1) {
-    WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId,_channelId),
-                 "Channel::UpdatePlayoutTimestamp() failed to read playout"
-                 " delay from the ADM");
-    _engineStatisticsPtr->SetLastError(
-        VE_CANNOT_RETRIEVE_VALUE, kTraceError,
-        "UpdatePlayoutTimestamp() failed to retrieve playout delay");
-    return;
-  }
-
-  jitter_buffer_playout_timestamp_ = playout_timestamp;
-
-  // Remove the playout delay.
-  playout_timestamp -= (delay_ms * (GetPlayoutFrequency() / 1000));
-
-  WEBRTC_TRACE(kTraceStream, kTraceVoice, VoEId(_instanceId,_channelId),
-               "Channel::UpdatePlayoutTimestamp() => playoutTimestamp = %lu",
-               playout_timestamp);
-
-  if (rtcp) {
-    playout_timestamp_rtcp_ = playout_timestamp;
-  } else {
-    playout_timestamp_rtp_ = playout_timestamp;
-  }
-  playout_delay_ms_ = delay_ms;
-}
-
 int Channel::GetPlayoutTimestamp(unsigned int& timestamp) {
   WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId,_channelId),
                "Channel::GetPlayoutTimestamp()");
-  if (playout_timestamp_rtp_ == 0)  {
+  uint32_t playout_timestamp_rtp = 0;
+  {
+    CriticalSectionScoped cs(video_sync_lock_.get());
+    playout_timestamp_rtp = playout_timestamp_rtp_;
+  }
+  if (playout_timestamp_rtp == 0)  {
     _engineStatisticsPtr->SetLastError(
         VE_CANNOT_RETRIEVE_VALUE, kTraceError,
         "GetPlayoutTimestamp() failed to retrieve timestamp");
     return -1;
   }
-  timestamp = playout_timestamp_rtp_;
+  timestamp = playout_timestamp_rtp;
   WEBRTC_TRACE(kTraceStateInfo, kTraceVoice,
                VoEId(_instanceId,_channelId),
                "GetPlayoutTimestamp() => timestamp=%u", timestamp);
   return 0;
 }
 
 int Channel::SetInitTimestamp(unsigned int timestamp) {
   WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId, _channelId),
                "Channel::SetInitTimestamp()");
   if (channel_state_.Get().sending) {
@@ skipping 235 matching lines @@
     if (!RTCP)
     {
         return _transportPtr->SendPacket(_channelId, data, len);
     }
     else
     {
         return _transportPtr->SendRTCPPacket(_channelId, data, len);
     }
 }
 
+void Channel::UpdatePlayoutTimestamp(bool rtcp) {
+  uint32_t playout_timestamp = 0;
+
+  if (audio_coding_->PlayoutTimestamp(&playout_timestamp) == -1)  {
+    // This can happen if this channel has not been received any RTP packet. In
+    // this case, NetEq is not capable of computing playout timestamp.
+    return;
+  }
+
+  uint16_t delay_ms = 0;
+  if (_audioDeviceModulePtr->PlayoutDelay(&delay_ms) == -1) {
+    WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId,_channelId),
+                 "Channel::UpdatePlayoutTimestamp() failed to read playout"
+                 " delay from the ADM");
+    _engineStatisticsPtr->SetLastError(
+        VE_CANNOT_RETRIEVE_VALUE, kTraceError,
+        "UpdatePlayoutTimestamp() failed to retrieve playout delay");
+    return;
+  }
+
+  jitter_buffer_playout_timestamp_ = playout_timestamp;
+
+  // Remove the playout delay.
+  playout_timestamp -= (delay_ms * (GetPlayoutFrequency() / 1000));
+
+  WEBRTC_TRACE(kTraceStream, kTraceVoice, VoEId(_instanceId,_channelId),
+               "Channel::UpdatePlayoutTimestamp() => playoutTimestamp = %lu",
+               playout_timestamp);
+
+  {
+    CriticalSectionScoped cs(video_sync_lock_.get());

[Henrik Grunell WebRTC, 2015/08/12 15:03:22]

A bit hard to see the diff when moved :-/ but it afaict the lock is the only
change. Should playout_timestamp_rtcp_ also be protected everywhere as here?

[Taylor Brandstetter, 2015/08/12 19:49:04]

Yes, the locking is the only change. As I said in my other comment, I don't
think playout_timestamp_rtcp_ needs to be protected; this just seemed like the
simplest way to protect both playout_timestamp_rtp_ and playout_delay_ms_ within
one lock acquisition.

[Henrik Grunell WebRTC, 2015/08/13 08:25:02]

Acknowledged.

+    if (rtcp) {
+      playout_timestamp_rtcp_ = playout_timestamp;
+    } else {
+      playout_timestamp_rtp_ = playout_timestamp;
+    }
+    playout_delay_ms_ = delay_ms;
+  }
+}
+
 // Called for incoming RTP packets after successful RTP header parsing.
 void Channel::UpdatePacketDelay(uint32_t rtp_timestamp,
                                 uint16_t sequence_number) {
   WEBRTC_TRACE(kTraceStream, kTraceVoice, VoEId(_instanceId,_channelId),
                "Channel::UpdatePacketDelay(timestamp=%lu, sequenceNumber=%u)",
                rtp_timestamp, sequence_number);
 
   // Get frequency of last received payload
   int rtp_receive_frequency = GetPlayoutFrequency();
 
-  // Update the least required delay.
-  least_required_delay_ms_ = audio_coding_->LeastRequiredDelayMs();
-
   // |jitter_buffer_playout_timestamp_| updated in UpdatePlayoutTimestamp for
   // every incoming packet.
   uint32_t timestamp_diff_ms = (rtp_timestamp -
       jitter_buffer_playout_timestamp_) / (rtp_receive_frequency / 1000);
   if (!IsNewerTimestamp(rtp_timestamp, jitter_buffer_playout_timestamp_) ||
       timestamp_diff_ms > (2 * kVoiceEngineMaxMinPlayoutDelayMs)) {
     // If |jitter_buffer_playout_timestamp_| is newer than the incoming RTP
     // timestamp, the resulting difference is negative, but is set to zero.
     // This can happen when a network glitch causes a packet to arrive late,
     // and during long comfort noise periods with clock drift.
     timestamp_diff_ms = 0;
   }
 
   uint16_t packet_delay_ms = (rtp_timestamp - _previousTimestamp) /
       (rtp_receive_frequency / 1000);
 
   _previousTimestamp = rtp_timestamp;
 
   if (timestamp_diff_ms == 0) return;
 
-  if (packet_delay_ms >= 10 && packet_delay_ms <= 60) {
-    _recPacketDelayMs = packet_delay_ms;
+  {
+    CriticalSectionScoped cs(video_sync_lock_.get());
+
+    if (packet_delay_ms >= 10 && packet_delay_ms <= 60) {
+      _recPacketDelayMs = packet_delay_ms;
+    }
+
+    if (_average_jitter_buffer_delay_us == 0) {
+      _average_jitter_buffer_delay_us = timestamp_diff_ms * 1000;
+      return;
+    }
+
+    // Filter average delay value using exponential filter (alpha is
+    // 7/8). We derive 1000 *_average_jitter_buffer_delay_us here (reduces
+    // risk of rounding error) and compensate for it in GetDelayEstimate()
+    // later.
+    _average_jitter_buffer_delay_us = (_average_jitter_buffer_delay_us * 7 +
+        1000 * timestamp_diff_ms + 500) / 8;
   }
-
-  if (_average_jitter_buffer_delay_us == 0) {
-    _average_jitter_buffer_delay_us = timestamp_diff_ms * 1000;
-    return;
-  }
-
-  // Filter average delay value using exponential filter (alpha is
-  // 7/8). We derive 1000 *_average_jitter_buffer_delay_us here (reduces
-  // risk of rounding error) and compensate for it in GetDelayEstimate()
-  // later.
-  _average_jitter_buffer_delay_us = (_average_jitter_buffer_delay_us * 7 +
-      1000 * timestamp_diff_ms + 500) / 8;
 }
 
 void
 Channel::RegisterReceiveCodecsToRTPModule()
 {
     WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId,_channelId),
                  "Channel::RegisterReceiveCodecsToRTPModule()");
 
 
     CodecInst codec;
@@ skipping 148 matching lines @@
   int64_t min_rtt = 0;
   if (_rtpRtcpModule->RTT(remoteSSRC, &rtt, &avg_rtt, &min_rtt, &max_rtt)
       != 0) {
     return 0;
   }
   return rtt;
 }
 
 }  // namespace voe
 }  // namespace webrtc