Move AudioSendStream lifetime reporting into destructor

This avoids a data race in which the lifetime TimeInterval is accessed
by the owning Call objects concurrently with SendRtp calls on the
underlying Channel object.

Bug: webrtc:8794
Change-Id: If53d5680095c0177656b659162457287cb8e45dd
Reviewed-on: https://webrtc-review.googlesource.com/46525
Commit-Queue: Sam Zackrisson <saza@webrtc.org>
Reviewed-by: Fredrik Solenberg <solenberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21853}

3 files changed, 58 insertions, 19 deletions

diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc
index b39f9cd600..0a5fc58581 100644
--- a/audio/audio_send_stream.cc
+++ b/audio/audio_send_stream.cc
@@ -61,8 +61,7 @@ std::unique_ptr<voe::ChannelProxy> CreateChannelAndProxy(
 }
 }  // namespace
 
-// TODO(saza): Move this declaration further down when we can use
-// std::make_unique.
+// Helper class to track the actively sending lifetime of this stream.
 class AudioSendStream::TimedTransport : public Transport {
  public:
   TimedTransport(Transport* transport, TimeInterval* time_interval)
@@ -94,7 +93,8 @@ AudioSendStream::AudioSendStream(
     BitrateAllocator* bitrate_allocator,
     RtcEventLog* event_log,
     RtcpRttStats* rtcp_rtt_stats,
-    const rtc::Optional<RtpState>& suspended_rtp_state)
+    const rtc::Optional<RtpState>& suspended_rtp_state,
+    TimeInterval* overall_call_lifetime)
     : AudioSendStream(config,
                       audio_state,
                       worker_queue,
@@ -103,6 +103,7 @@ AudioSendStream::AudioSendStream(
                       event_log,
                       rtcp_rtt_stats,
                       suspended_rtp_state,
+                      overall_call_lifetime,
                       CreateChannelAndProxy(audio_state.get(),
                                             worker_queue,
                                             module_process_thread)) {}
@@ -116,6 +117,7 @@ AudioSendStream::AudioSendStream(
     RtcEventLog* event_log,
     RtcpRttStats* rtcp_rtt_stats,
     const rtc::Optional<RtpState>& suspended_rtp_state,
+    TimeInterval* overall_call_lifetime,
     std::unique_ptr<voe::ChannelProxy> channel_proxy)
     : worker_queue_(worker_queue),
       config_(Config(nullptr)),
@@ -128,7 +130,8 @@ AudioSendStream::AudioSendStream(
                            kPacketLossRateMinNumAckedPackets,
                            kRecoverablePacketLossRateMinNumAckedPairs),
       rtp_rtcp_module_(nullptr),
-      suspended_rtp_state_(suspended_rtp_state) {
+      suspended_rtp_state_(suspended_rtp_state),
+      overall_call_lifetime_(overall_call_lifetime) {
   RTC_LOG(LS_INFO) << "AudioSendStream: " << config.rtp.ssrc;
   RTC_DCHECK(worker_queue_);
   RTC_DCHECK(audio_state_);
@@ -136,6 +139,7 @@ AudioSendStream::AudioSendStream(
   RTC_DCHECK(bitrate_allocator_);
   RTC_DCHECK(transport);
   RTC_DCHECK(transport->send_side_cc());
+  RTC_DCHECK(overall_call_lifetime_);
 
   channel_proxy_->SetRtcEventLog(event_log_);
   channel_proxy_->SetRtcpRttStats(rtcp_rtt_stats);
@@ -160,6 +164,10 @@ AudioSendStream::~AudioSendStream() {
   channel_proxy_->ResetSenderCongestionControlObjects();
   channel_proxy_->SetRtcEventLog(nullptr);
   channel_proxy_->SetRtcpRttStats(nullptr);
+  // Lifetime can only be updated after deregistering
+  // |timed_send_transport_adapter_| in the underlying channel object to avoid
+  // data races in |active_lifetime_|.
+  overall_call_lifetime_->Extend(active_lifetime_);
 }
 
 const webrtc::AudioSendStream::Config& AudioSendStream::GetConfig() const {
@@ -456,10 +464,6 @@ RtpState AudioSendStream::GetRtpState() const {
   return rtp_rtcp_module_->GetRtpState();
 }
 
-const TimeInterval& AudioSendStream::GetActiveLifetime() const {
-  return active_lifetime_;
-}
-
 const voe::ChannelProxy& AudioSendStream::GetChannelProxy() const {
   RTC_DCHECK(channel_proxy_.get());
   return *channel_proxy_.get();
diff --git a/audio/audio_send_stream.h b/audio/audio_send_stream.h
index 093ca4621a..1cda778940 100644
--- a/audio/audio_send_stream.h
+++ b/audio/audio_send_stream.h
@@ -49,7 +49,8 @@ class AudioSendStream final : public webrtc::AudioSendStream,
                   BitrateAllocator* bitrate_allocator,
                   RtcEventLog* event_log,
                   RtcpRttStats* rtcp_rtt_stats,
-                  const rtc::Optional<RtpState>& suspended_rtp_state);
+                  const rtc::Optional<RtpState>& suspended_rtp_state,
+                  TimeInterval* overall_call_lifetime);
   // For unit tests, which need to supply a mock channel proxy.
   AudioSendStream(const webrtc::AudioSendStream::Config& config,
                   const rtc::scoped_refptr<webrtc::AudioState>& audio_state,
@@ -59,6 +60,7 @@ class AudioSendStream final : public webrtc::AudioSendStream,
                   RtcEventLog* event_log,
                   RtcpRttStats* rtcp_rtt_stats,
                   const rtc::Optional<RtpState>& suspended_rtp_state,
+                  TimeInterval* overall_call_lifetime,
                   std::unique_ptr<voe::ChannelProxy> channel_proxy);
   ~AudioSendStream() override;
 
@@ -92,7 +94,6 @@ class AudioSendStream final : public webrtc::AudioSendStream,
   void SetTransportOverhead(int transport_overhead_per_packet);
 
   RtpState GetRtpState() const;
-  const TimeInterval& GetActiveLifetime() const;
   const voe::ChannelProxy& GetChannelProxy() const;
 
  private:
@@ -148,6 +149,7 @@ class AudioSendStream final : public webrtc::AudioSendStream,
 
   std::unique_ptr<TimedTransport> timed_send_transport_adapter_;
   TimeInterval active_lifetime_;
+  TimeInterval* overall_call_lifetime_ = nullptr;
 
   // RFC 5285: Each distinct extension MUST have a unique ID. The value 0 is
   // reserved for padding and MUST NOT be used as a local identifier.
diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc
index 925f93e3cf..27b49a2849 100644
--- a/audio/audio_send_stream_unittest.cc
+++ b/audio/audio_send_stream_unittest.cc
@@ -28,11 +28,14 @@
 #include "modules/pacing/mock/mock_paced_sender.h"
 #include "modules/rtp_rtcp/mocks/mock_rtcp_rtt_stats.h"
 #include "modules/rtp_rtcp/mocks/mock_rtp_rtcp.h"
+#include "rtc_base/fakeclock.h"
 #include "rtc_base/ptr_util.h"
 #include "rtc_base/task_queue.h"
+#include "rtc_base/timedelta.h"
 #include "test/gtest.h"
 #include "test/mock_audio_encoder.h"
 #include "test/mock_audio_encoder_factory.h"
+#include "test/mock_transport.h"
 
 namespace webrtc {
 namespace test {
@@ -168,14 +171,9 @@ struct ConfigHelper {
   std::unique_ptr<internal::AudioSendStream> CreateAudioSendStream() {
     return std::unique_ptr<internal::AudioSendStream>(
         new internal::AudioSendStream(
-            stream_config_,
-            audio_state_,
-            &worker_queue_,
-            &fake_transport_,
-            &bitrate_allocator_,
-            &event_log_,
-            &rtcp_rtt_stats_,
-            rtc::nullopt,
+            stream_config_, audio_state_, &worker_queue_, &fake_transport_,
+            &bitrate_allocator_, &event_log_, &rtcp_rtt_stats_, rtc::nullopt,
+            &active_lifetime_,
             std::unique_ptr<voe::ChannelProxy>(channel_proxy_)));
   }
 
@@ -186,6 +184,7 @@ struct ConfigHelper {
   }
   MockVoEChannelProxy* channel_proxy() { return channel_proxy_; }
   RtpTransportControllerSendInterface* transport() { return &fake_transport_; }
+  TimeInterval* active_lifetime() { return &active_lifetime_; }
 
   static void AddBweToConfig(AudioSendStream::Config* config) {
     config->rtp.extensions.push_back(
@@ -223,7 +222,11 @@ struct ConfigHelper {
     }
     EXPECT_CALL(*channel_proxy_, ResetSenderCongestionControlObjects())
         .Times(1);
-    EXPECT_CALL(*channel_proxy_, RegisterTransport(nullptr)).Times(2);
+    {
+      ::testing::InSequence unregister_on_destruction;
+      EXPECT_CALL(*channel_proxy_, RegisterTransport(_)).Times(1);
+      EXPECT_CALL(*channel_proxy_, RegisterTransport(nullptr)).Times(1);
+    }
     EXPECT_CALL(*channel_proxy_, SetRtcEventLog(testing::NotNull())).Times(1);
     EXPECT_CALL(*channel_proxy_, SetRtcEventLog(testing::IsNull()))
         .Times(1);  // Destructor resets the event log
@@ -308,6 +311,7 @@ struct ConfigHelper {
   rtc::scoped_refptr<MockAudioProcessing> audio_processing_;
   AudioProcessingStats audio_processing_stats_;
   SimulatedClock simulated_clock_;
+  TimeInterval active_lifetime_;
   PacketRouter packet_router_;
   testing::NiceMock<MockPacedSender> pacer_;
   std::unique_ptr<SendSideCongestionController> send_side_cc_;
@@ -526,5 +530,34 @@ TEST(AudioSendStreamTest, ReconfigureTransportCcResetsFirst) {
   }
   send_stream->Reconfigure(new_config);
 }
+
+// Checks that AudioSendStream logs the times at which RTP packets are sent
+// through its interface.
+TEST(AudioSendStreamTest, UpdateLifetime) {
+  ConfigHelper helper(false, true);
+
+  MockTransport mock_transport;
+  helper.config().send_transport = &mock_transport;
+
+  Transport* registered_transport;
+  ON_CALL(*helper.channel_proxy(), RegisterTransport(_))
+      .WillByDefault(Invoke([&registered_transport](Transport* transport) {
+        registered_transport = transport;
+      }));
+
+  rtc::ScopedFakeClock fake_clock;
+  constexpr int64_t kTimeBetweenSendRtpCallsMs = 100;
+  {
+    auto send_stream = helper.CreateAudioSendStream();
+    EXPECT_CALL(mock_transport, SendRtp(_, _, _)).Times(2);
+    const PacketOptions options;
+    registered_transport->SendRtp(nullptr, 0, options);
+    fake_clock.AdvanceTime(
+        rtc::TimeDelta::FromMilliseconds(kTimeBetweenSendRtpCallsMs));
+    registered_transport->SendRtp(nullptr, 0, options);
+  }
+  EXPECT_TRUE(!helper.active_lifetime()->Empty());
+  EXPECT_EQ(helper.active_lifetime()->Length(), kTimeBetweenSendRtpCallsMs);
+}
 }  // namespace test
 }  // namespace webrtc