Implement nack_count metric for inbound audio rtp streams.

Bug: webrtc:12925
Change-Id: I4542ca0f14a7dd7485ad5a2b6f2bd7051076f71f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/224085
Commit-Queue: Jakob Ivarsson <jakobi@webrtc.org>
Reviewed-by: Sam Zackrisson <saza@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34401}

api/stats/rtcstats_objects.h

@@ -502,8 +502,6 @@ class RTC_EXPORT RTCInboundRTPStreamStats final
   // FIR and PLI counts are only defined for |media_type == "video"|.
   RTCStatsMember<uint32_t> fir_count;
   RTCStatsMember<uint32_t> pli_count;
-  // TODO(hbos): NACK count should be collected by |RTCStatsCollector| for both
-  // audio and video but is only defined in the "video" case. crbug.com/657856
   RTCStatsMember<uint32_t> nack_count;
   RTCStatsMember<uint64_t> qp_sum;
 };

audio/BUILD.gn

@@ -140,6 +140,7 @@ if (rtc_include_tests) {
       "mock_voe_channel_proxy.h",
       "remix_resample_unittest.cc",
       "test/audio_stats_test.cc",
+      "test/nack_test.cc",
     ]
     deps = [
       ":audio",

audio/audio_receive_stream.cc

@@ -278,6 +278,7 @@ webrtc::AudioReceiveStream::Stats AudioReceiveStream::GetStats(
       call_stats.header_and_padding_bytes_rcvd;
   stats.packets_rcvd = call_stats.packetsReceived;
   stats.packets_lost = call_stats.cumulativeLost;
+  stats.nacks_sent = call_stats.nacks_sent;
   stats.capture_start_ntp_time_ms = call_stats.capture_start_ntp_time_ms_;
   stats.last_packet_received_timestamp_ms =
       call_stats.last_packet_received_timestamp_ms;

audio/channel_receive.cc

@@ -81,7 +81,8 @@ AudioCodingModule::Config AcmConfig(
   return acm_config;
 }
 
-class ChannelReceive : public ChannelReceiveInterface {
+class ChannelReceive : public ChannelReceiveInterface,
+                       public RtcpPacketTypeCounterObserver {
  public:
   // Used for receive streams.
   ChannelReceive(
@@ -182,6 +183,10 @@ class ChannelReceive : public ChannelReceiveInterface {
   void OnLocalSsrcChange(uint32_t local_ssrc) override;
   uint32_t GetLocalSsrc() const override;
 
+  void RtcpPacketTypesCounterUpdated(
+      uint32_t ssrc,
+      const RtcpPacketTypeCounter& packet_counter) override;
+
  private:
   void ReceivePacket(const uint8_t* packet,
                      size_t packet_length,
@@ -299,6 +304,10 @@ class ChannelReceive : public ChannelReceiveInterface {
   // A value of 100 means 100 callbacks, each one of which represents 10ms worth
   // of data, so the stats reporting frequency will be 1Hz (modulo failures).
   constexpr static int kHistogramReportingInterval = 100;
+
+  mutable Mutex rtcp_counter_mutex_;
+  RtcpPacketTypeCounter rtcp_packet_type_counter_
+      RTC_GUARDED_BY(rtcp_counter_mutex_);
 };
 
 void ChannelReceive::OnReceivedPayloadData(
@@ -563,6 +572,7 @@ ChannelReceive::ChannelReceive(
   configuration.receive_statistics = rtp_receive_statistics_.get();
   configuration.event_log = event_log_;
   configuration.local_media_ssrc = local_ssrc;
+  configuration.rtcp_packet_type_counter_observer = this;
 
   if (frame_transformer)
     InitFrameTransformerDelegate(std::move(frame_transformer));
@@ -819,6 +829,11 @@ CallReceiveStatistics ChannelReceive::GetRTCPStatistics() const {
     stats.last_packet_received_timestamp_ms = absl::nullopt;
   }
 
+  {
+    MutexLock lock(&rtcp_counter_mutex_);
+    stats.nacks_sent = rtcp_packet_type_counter_.nack_packets;
+  }
+
   // Timestamps.
   {
     MutexLock lock(&ts_stats_lock_);
@@ -863,6 +878,16 @@ int ChannelReceive::ResendPackets(const uint16_t* sequence_numbers,
   return rtp_rtcp_->SendNACK(sequence_numbers, length);
 }
 
+void ChannelReceive::RtcpPacketTypesCounterUpdated(
+    uint32_t ssrc,
+    const RtcpPacketTypeCounter& packet_counter) {
+  if (ssrc != remote_ssrc_) {
+    return;
+  }
+  MutexLock lock(&rtcp_counter_mutex_);
+  rtcp_packet_type_counter_ = packet_counter;
+}
+
 void ChannelReceive::SetAssociatedSendChannel(
     const ChannelSendInterface* channel) {
   RTC_DCHECK_RUN_ON(&network_thread_checker_);

audio/channel_receive.h

@@ -57,6 +57,7 @@ struct CallReceiveStatistics {
   int64_t payload_bytes_rcvd = 0;
   int64_t header_and_padding_bytes_rcvd = 0;
   int packetsReceived;
+  uint32_t nacks_sent = 0;
   // The capture NTP time (in local timebase) of the first played out audio
   // frame.
   int64_t capture_start_ntp_time_ms_;

audio/test/audio_end_to_end_test.cc

@@ -92,6 +92,8 @@ void AudioEndToEndTest::ModifyAudioConfigs(
                                               {{"stereo", "1"}});
   send_config->send_codec_spec = AudioSendStream::Config::SendCodecSpec(
       test::CallTest::kAudioSendPayloadType, kDefaultFormat);
+  send_config->min_bitrate_bps = 32000;
+  send_config->max_bitrate_bps = 32000;
 }
 
 void AudioEndToEndTest::OnAudioStreamsCreated(

audio/test/nack_test.cc

@@ -0,0 +1,56 @@
+/*
+ *  Copyright (c) 2021 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.
+ */
+
+#include "audio/test/audio_end_to_end_test.h"
+#include "system_wrappers/include/sleep.h"
+#include "test/gtest.h"
+
+namespace webrtc {
+namespace test {
+
+using NackTest = CallTest;
+
+TEST_F(NackTest, ShouldNackInLossyNetwork) {
+  class NackTest : public AudioEndToEndTest {
+   public:
+    const int kTestDurationMs = 1000;
+    const int64_t kRttMs = 30;
+    const int64_t kLossPercent = 30;
+    const int kNackHistoryMs = 1000;
+
+    BuiltInNetworkBehaviorConfig GetNetworkPipeConfig() const override {
+      BuiltInNetworkBehaviorConfig pipe_config;
+      pipe_config.queue_delay_ms = kRttMs / 2;
+      pipe_config.loss_percent = kLossPercent;
+      return pipe_config;
+    }
+
+    void ModifyAudioConfigs(
+        AudioSendStream::Config* send_config,
+        std::vector<AudioReceiveStream::Config>* receive_configs) override {
+      ASSERT_EQ(receive_configs->size(), 1U);
+      (*receive_configs)[0].rtp.nack.rtp_history_ms = kNackHistoryMs;
+      AudioEndToEndTest::ModifyAudioConfigs(send_config, receive_configs);
+    }
+
+    void PerformTest() override { SleepMs(kTestDurationMs); }
+
+    void OnStreamsStopped() override {
+      AudioReceiveStream::Stats recv_stats =
+          receive_stream()->GetStats(/*get_and_clear_legacy_stats=*/true);
+      EXPECT_GT(recv_stats.nacks_sent, 0U);
+    }
+  } test;
+
+  RunBaseTest(&test);
+}
+
+}  // namespace test
+}  // namespace webrtc

call/audio_receive_stream.h

@@ -39,6 +39,7 @@ class AudioReceiveStream : public MediaReceiveStream {
     uint64_t fec_packets_received = 0;
     uint64_t fec_packets_discarded = 0;
     uint32_t packets_lost = 0;
+    uint32_t nacks_sent = 0;
     std::string codec_name;
     absl::optional<int> codec_payload_type;
     uint32_t jitter_ms = 0;

media/base/media_channel.h

@@ -426,6 +426,7 @@ struct MediaReceiverInfo {
   int64_t header_and_padding_bytes_rcvd = 0;
   int packets_rcvd = 0;
   int packets_lost = 0;
+  absl::optional<uint32_t> nacks_sent;
   // Jitter (network-related) latency (cumulative).
   // https://w3c.github.io/webrtc-stats/#dom-rtcinboundrtpstreamstats-jitterbufferdelay
   double jitter_buffer_delay_seconds = 0.0;
@@ -579,7 +580,6 @@ struct VideoReceiverInfo : public MediaReceiverInfo {
   int packets_concealed = 0;
   int firs_sent = 0;
   int plis_sent = 0;
-  int nacks_sent = 0;
   int frame_width = 0;
   int frame_height = 0;
   int framerate_rcvd = 0;

media/engine/webrtc_video_engine_unittest.cc

@@ -1918,7 +1918,7 @@ TEST_F(WebRtcVideoChannelBaseTest, GetStats) {
   // EXPECT_EQ(0, info.receivers[0].packets_concealed);
   EXPECT_EQ(0, info.receivers[0].firs_sent);
   EXPECT_EQ(0, info.receivers[0].plis_sent);
-  EXPECT_EQ(0, info.receivers[0].nacks_sent);
+  EXPECT_EQ(0U, info.receivers[0].nacks_sent);
   EXPECT_EQ(kVideoWidth, info.receivers[0].frame_width);
   EXPECT_EQ(kVideoHeight, info.receivers[0].frame_height);
   EXPECT_GT(info.receivers[0].framerate_rcvd, 0);
@@ -6148,7 +6148,7 @@ TEST_F(WebRtcVideoChannelTest,
   EXPECT_EQ(stats.rtcp_packet_type_counts.fir_packets,
             rtc::checked_cast<unsigned int>(info.receivers[0].firs_sent));
   EXPECT_EQ(stats.rtcp_packet_type_counts.nack_packets,
-            rtc::checked_cast<unsigned int>(info.receivers[0].nacks_sent));
+            info.receivers[0].nacks_sent);
   EXPECT_EQ(stats.rtcp_packet_type_counts.pli_packets,
             rtc::checked_cast<unsigned int>(info.receivers[0].plis_sent));
 }

media/engine/webrtc_voice_engine.cc

@@ -2419,6 +2419,10 @@ bool WebRtcVoiceMediaChannel::GetStats(VoiceMediaInfo* info,
     rinfo.sender_reports_bytes_sent = stats.sender_reports_bytes_sent;
     rinfo.sender_reports_reports_count = stats.sender_reports_reports_count;
 
+    if (recv_nack_enabled_) {
+      rinfo.nacks_sent = stats.nacks_sent;
+    }
+
     info->receivers.push_back(rinfo);
   }
 

pc/rtc_stats_collector.cc

@@ -337,6 +337,9 @@ void SetInboundRTPStreamStatsFromMediaReceiverInfo(
       media_receiver_info.jitter_buffer_delay_seconds;
   inbound_stats->jitter_buffer_emitted_count =
       media_receiver_info.jitter_buffer_emitted_count;
+  if (media_receiver_info.nacks_sent) {
+    inbound_stats->nack_count = *media_receiver_info.nacks_sent;
+  }
 }
 
 std::unique_ptr<RTCInboundRTPStreamStats> CreateInboundAudioStreamStats(
@@ -454,8 +457,6 @@ void SetInboundRTPStreamStatsFromVideoReceiverInfo(
       static_cast<uint32_t>(video_receiver_info.firs_sent);
   inbound_video->pli_count =
       static_cast<uint32_t>(video_receiver_info.plis_sent);
-  inbound_video->nack_count =
-      static_cast<uint32_t>(video_receiver_info.nacks_sent);
   inbound_video->frames_received = video_receiver_info.frames_received;
   inbound_video->frames_decoded = video_receiver_info.frames_decoded;
   inbound_video->frames_dropped = video_receiver_info.frames_dropped;

pc/rtc_stats_collector_unittest.cc

@@ -1954,6 +1954,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Audio) {
   voice_media_info.receivers[0].local_stats[0].ssrc = 1;
   voice_media_info.receivers[0].packets_lost = -1;  // Signed per RFC3550
   voice_media_info.receivers[0].packets_rcvd = 2;
+  voice_media_info.receivers[0].nacks_sent = 5;
   voice_media_info.receivers[0].fec_packets_discarded = 5566;
   voice_media_info.receivers[0].fec_packets_received = 6677;
   voice_media_info.receivers[0].payload_bytes_rcvd = 3;
@@ -2002,6 +2003,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Audio) {
   expected_audio.transport_id = "RTCTransport_TransportName_1";
   expected_audio.codec_id = "RTCCodec_AudioMid_Inbound_42";
   expected_audio.packets_received = 2;
+  expected_audio.nack_count = 5;
   expected_audio.fec_packets_discarded = 5566;
   expected_audio.fec_packets_received = 6677;
   expected_audio.bytes_received = 3;

pc/stats_collector.cc

@@ -319,6 +319,10 @@ void ExtractStats(const cricket::VideoReceiverInfo& info,
   if (info.qp_sum)
     report->AddInt64(StatsReport::kStatsValueNameQpSum, *info.qp_sum);
 
+  if (info.nacks_sent) {
+    report->AddInt(StatsReport::kStatsValueNameNacksSent, *info.nacks_sent);
+  }
+
   const IntForAdd ints[] = {
       {StatsReport::kStatsValueNameCurrentDelayMs, info.current_delay_ms},
       {StatsReport::kStatsValueNameDecodeMs, info.decode_ms},
@@ -332,7 +336,6 @@ void ExtractStats(const cricket::VideoReceiverInfo& info,
       {StatsReport::kStatsValueNameMaxDecodeMs, info.max_decode_ms},
       {StatsReport::kStatsValueNameMinPlayoutDelayMs,
        info.min_playout_delay_ms},
-      {StatsReport::kStatsValueNameNacksSent, info.nacks_sent},
       {StatsReport::kStatsValueNamePacketsLost, info.packets_lost},
       {StatsReport::kStatsValueNamePacketsReceived, info.packets_rcvd},
       {StatsReport::kStatsValueNamePlisSent, info.plis_sent},