diff --git a/api/stats/rtcstats_objects.h b/api/stats/rtcstats_objects.h index cb8e25a02e..ebc6affb8d 100644 --- a/api/stats/rtcstats_objects.h +++ b/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 fir_count; RTCStatsMember 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 nack_count; RTCStatsMember qp_sum; }; diff --git a/audio/BUILD.gn b/audio/BUILD.gn index 054e090ba6..200f9f4038 100644 --- a/audio/BUILD.gn +++ b/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", diff --git a/audio/audio_receive_stream.cc b/audio/audio_receive_stream.cc index d4e9c0ea77..f243fa67db 100644 --- a/audio/audio_receive_stream.cc +++ b/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; diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc index acab1467f4..57269cd193 100644 --- a/audio/channel_receive.cc +++ b/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_); diff --git a/audio/channel_receive.h b/audio/channel_receive.h index f5afe50f3b..deec49feaf 100644 --- a/audio/channel_receive.h +++ b/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_; diff --git a/audio/test/audio_end_to_end_test.cc b/audio/test/audio_end_to_end_test.cc index 896b0f2dae..0d8529a913 100644 --- a/audio/test/audio_end_to_end_test.cc +++ b/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( diff --git a/audio/test/nack_test.cc b/audio/test/nack_test.cc new file mode 100644 index 0000000000..714a8775d6 --- /dev/null +++ b/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* 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 diff --git a/call/audio_receive_stream.h b/call/audio_receive_stream.h index 54729d0dc0..8403e6bea0 100644 --- a/call/audio_receive_stream.h +++ b/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 codec_payload_type; uint32_t jitter_ms = 0; diff --git a/media/base/media_channel.h b/media/base/media_channel.h index e9d48701c4..979a5c1477 100644 --- a/media/base/media_channel.h +++ b/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 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; diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 89cc1f4d9f..e2d83127c4 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/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(info.receivers[0].firs_sent)); EXPECT_EQ(stats.rtcp_packet_type_counts.nack_packets, - rtc::checked_cast(info.receivers[0].nacks_sent)); + info.receivers[0].nacks_sent); EXPECT_EQ(stats.rtcp_packet_type_counts.pli_packets, rtc::checked_cast(info.receivers[0].plis_sent)); } diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index 578eab4f59..f2783f6f92 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/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); } diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index 1fa731b912..070400c58f 100644 --- a/pc/rtc_stats_collector.cc +++ b/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 CreateInboundAudioStreamStats( @@ -454,8 +457,6 @@ void SetInboundRTPStreamStatsFromVideoReceiverInfo( static_cast(video_receiver_info.firs_sent); inbound_video->pli_count = static_cast(video_receiver_info.plis_sent); - inbound_video->nack_count = - static_cast(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; diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 845454148e..ca4f48e913 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/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; diff --git a/pc/stats_collector.cc b/pc/stats_collector.cc index 7376e24c8b..eb2176ed38 100644 --- a/pc/stats_collector.cc +++ b/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},