From 2fb83072dbf79b4fc4ca1fdefda48f1960dc50e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Thu, 6 Oct 2022 13:37:11 +0200 Subject: [PATCH] Move more non-standard metrics to inbound-rtp. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They may be non-standard, but they shouldn't be on a stats dictionary that is deprecated (track is going away soon-ish). By moving them to inbound-rtp they can continue to exist beyond track deprecation and live in the right place in case we decide to standardize them later. To help downstream projects transitions, the metrics are temporarily available in both old and new locations. Delete of old location will happen in a follow-up CL. TODOs added. Bug: webrtc:14524 Change-Id: I2008060fa4ba76cde859d9144d2bb9648c7ff9af Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/278200 Reviewed-by: Harald Alvestrand Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#38315} --- api/stats/rtcstats_objects.h | 11 +++++++---- pc/rtc_stats_collector.cc | 17 ++++++++++++++++ pc/rtc_stats_collector_unittest.cc | 14 ++++++++++++++ pc/rtc_stats_integrationtest.cc | 24 +++++++++++++++++++++++ pc/test/integration_test_helpers.h | 31 ++++++++++++------------------ stats/rtcstats_objects.cc | 22 +++++++++++++++++++++ 6 files changed, 96 insertions(+), 23 deletions(-) diff --git a/api/stats/rtcstats_objects.h b/api/stats/rtcstats_objects.h index b3245443d6..43bb3faf1d 100644 --- a/api/stats/rtcstats_objects.h +++ b/api/stats/rtcstats_objects.h @@ -346,13 +346,10 @@ class RTC_EXPORT RTCMediaStreamTrackStats final : public RTCStats { RTCStatsMember concealment_events; RTCStatsMember inserted_samples_for_deceleration; RTCStatsMember removed_samples_for_acceleration; - // Non-standard audio-only member - // https://w3c.github.io/webrtc-provisional-stats/#dom-rtcaudioreceiverstats-jitterbufferflushes + // TODO(crbug.com/webrtc/14524): These metrics have been moved, delete them. RTCNonStandardStatsMember jitter_buffer_flushes; RTCNonStandardStatsMember delayed_packet_outage_samples; RTCNonStandardStatsMember relative_packet_arrival_delay; - // TODO(henrik.lundin): Add description of the interruption metrics at - // https://github.com/w3c/webrtc-provisional-stats/issues/17 RTCNonStandardStatsMember interruption_count; RTCNonStandardStatsMember total_interruption_duration; // Non-standard video-only members. @@ -503,6 +500,12 @@ class RTC_EXPORT RTCInboundRTPStreamStats final RTCStatsMember pli_count; RTCStatsMember nack_count; RTCStatsMember qp_sum; + // Non-standard audio metrics. + RTCNonStandardStatsMember jitter_buffer_flushes; + RTCNonStandardStatsMember delayed_packet_outage_samples; + RTCNonStandardStatsMember relative_packet_arrival_delay; + RTCNonStandardStatsMember interruption_count; + RTCNonStandardStatsMember total_interruption_duration; // The former googMinPlayoutDelayMs (in seconds). RTCNonStandardStatsMember min_playout_delay; diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index 891e518570..7ef00a90a4 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -496,6 +496,19 @@ std::unique_ptr CreateInboundAudioStreamStats( inbound_audio->fec_packets_discarded = voice_receiver_info.fec_packets_discarded; inbound_audio->packets_discarded = voice_receiver_info.packets_discarded; + inbound_audio->jitter_buffer_flushes = + voice_receiver_info.jitter_buffer_flushes; + inbound_audio->delayed_packet_outage_samples = + voice_receiver_info.delayed_packet_outage_samples; + inbound_audio->relative_packet_arrival_delay = + voice_receiver_info.relative_packet_arrival_delay_seconds; + inbound_audio->interruption_count = + voice_receiver_info.interruption_count >= 0 + ? voice_receiver_info.interruption_count + : 0; + inbound_audio->total_interruption_duration = + static_cast(voice_receiver_info.total_interruption_duration_ms) / + rtc::kNumMillisecsPerSec; return inbound_audio; } @@ -1034,6 +1047,9 @@ ProduceMediaStreamTrackStatsFromVoiceReceiverInfo( voice_receiver_info.silent_concealed_samples; audio_track_stats->concealment_events = voice_receiver_info.concealment_events; + + // TODO(crbug.com/webrtc/14524): These metrics have been moved from "track" + // stats, delete them. audio_track_stats->jitter_buffer_flushes = voice_receiver_info.jitter_buffer_flushes; audio_track_stats->delayed_packet_outage_samples = @@ -1047,6 +1063,7 @@ ProduceMediaStreamTrackStatsFromVoiceReceiverInfo( audio_track_stats->total_interruption_duration = static_cast(voice_receiver_info.total_interruption_duration_ms) / rtc::kNumMillisecsPerSec; + return audio_track_stats; } diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 3c521c82e7..31083db94e 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -2234,6 +2234,8 @@ TEST_F(RTCStatsCollectorTest, voice_receiver_info.silent_concealed_samples = 765; voice_receiver_info.jitter_buffer_delay_seconds = 3.456; voice_receiver_info.jitter_buffer_emitted_count = 13; + // TODO(crbug.com/webrtc/14524): These metrics have been moved from "track" + // stats, no need to test these here. voice_receiver_info.jitter_buffer_flushes = 7; voice_receiver_info.delayed_packet_outage_samples = 15; voice_receiver_info.relative_packet_arrival_delay_seconds = 16; @@ -2278,6 +2280,8 @@ TEST_F(RTCStatsCollectorTest, expected_remote_audio_track.silent_concealed_samples = 765; expected_remote_audio_track.jitter_buffer_delay = 3.456; expected_remote_audio_track.jitter_buffer_emitted_count = 13; + // TODO(crbug.com/webrtc/14524): These metrics have been moved from "track" + // stats, delete them. expected_remote_audio_track.jitter_buffer_flushes = 7; expected_remote_audio_track.delayed_packet_outage_samples = 15; expected_remote_audio_track.relative_packet_arrival_delay = 16; @@ -2471,6 +2475,11 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Audio) { voice_media_info.receivers[0].audio_level = 14442; // [0,32767] voice_media_info.receivers[0].total_output_energy = 10.0; voice_media_info.receivers[0].total_output_duration = 11.0; + voice_media_info.receivers[0].jitter_buffer_flushes = 7; + voice_media_info.receivers[0].delayed_packet_outage_samples = 15; + voice_media_info.receivers[0].relative_packet_arrival_delay_seconds = 16; + voice_media_info.receivers[0].interruption_count = 7788; + voice_media_info.receivers[0].total_interruption_duration_ms = 778899; voice_media_info.receivers[0].last_packet_received_timestamp_ms = absl::nullopt; @@ -2526,6 +2535,11 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Audio) { expected_audio.audio_level = 14442.0 / 32767.0; // [0,1] expected_audio.total_audio_energy = 10.0; expected_audio.total_samples_duration = 11.0; + expected_audio.jitter_buffer_flushes = 7; + expected_audio.delayed_packet_outage_samples = 15; + expected_audio.relative_packet_arrival_delay = 16; + expected_audio.interruption_count = 7788; + expected_audio.total_interruption_duration = 778.899; ASSERT_TRUE(report->Get(expected_audio.id())); EXPECT_EQ( diff --git a/pc/rtc_stats_integrationtest.cc b/pc/rtc_stats_integrationtest.cc index 4bb8473deb..0cecdeaf02 100644 --- a/pc/rtc_stats_integrationtest.cc +++ b/pc/rtc_stats_integrationtest.cc @@ -656,6 +656,8 @@ class RTCStatsReportVerifier { media_stream_track.inserted_samples_for_deceleration); verifier.TestMemberIsUndefined( media_stream_track.removed_samples_for_acceleration); + // TODO(crbug.com/webrtc/14524): These metrics have been moved from + // "track" stats, delete them. verifier.TestMemberIsUndefined(media_stream_track.jitter_buffer_flushes); verifier.TestMemberIsUndefined( media_stream_track.delayed_packet_outage_samples); @@ -691,6 +693,8 @@ class RTCStatsReportVerifier { media_stream_track.inserted_samples_for_deceleration); verifier.TestMemberIsNonNegative( media_stream_track.removed_samples_for_acceleration); + // TODO(crbug.com/webrtc/14524): These metrics have been moved from + // "track" stats, delete them. verifier.TestMemberIsNonNegative( media_stream_track.jitter_buffer_flushes); verifier.TestMemberIsNonNegative( @@ -722,6 +726,8 @@ class RTCStatsReportVerifier { media_stream_track.inserted_samples_for_deceleration); verifier.TestMemberIsUndefined( media_stream_track.removed_samples_for_acceleration); + // TODO(crbug.com/webrtc/14524): These metrics have been moved from + // "track" stats, delete them. verifier.TestMemberIsUndefined( media_stream_track.jitter_buffer_flushes); verifier.TestMemberIsUndefined( @@ -920,6 +926,14 @@ class RTCStatsReportVerifier { // The integration test is not set up to test screen share; don't require // this to be present. verifier.MarkMemberTested(inbound_stream.content_type, true); + verifier.TestMemberIsUndefined(inbound_stream.jitter_buffer_flushes); + verifier.TestMemberIsUndefined( + inbound_stream.delayed_packet_outage_samples); + verifier.TestMemberIsUndefined( + inbound_stream.relative_packet_arrival_delay); + verifier.TestMemberIsUndefined(inbound_stream.interruption_count); + verifier.TestMemberIsUndefined( + inbound_stream.total_interruption_duration); verifier.TestMemberIsNonNegative( inbound_stream.min_playout_delay); } else { @@ -939,6 +953,16 @@ class RTCStatsReportVerifier { verifier.TestMemberIsUndefined(inbound_stream.freeze_count); verifier.TestMemberIsUndefined(inbound_stream.total_freezes_duration); verifier.TestMemberIsUndefined(inbound_stream.content_type); + verifier.TestMemberIsNonNegative( + inbound_stream.jitter_buffer_flushes); + verifier.TestMemberIsNonNegative( + inbound_stream.delayed_packet_outage_samples); + verifier.TestMemberIsNonNegative( + inbound_stream.relative_packet_arrival_delay); + verifier.TestMemberIsNonNegative( + inbound_stream.interruption_count); + verifier.TestMemberIsNonNegative( + inbound_stream.total_interruption_duration); verifier.TestMemberIsUndefined(inbound_stream.min_playout_delay); } return verifier.ExpectAllMembersSuccessfullyTested(); diff --git a/pc/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h index 7e5cc74758..1d9882ed81 100644 --- a/pc/test/integration_test_helpers.h +++ b/pc/test/integration_test_helpers.h @@ -171,6 +171,8 @@ void RemoveSsrcsAndMsids(cricket::SessionDescription* desc); // endpoint that only signals a=msid lines to convey stream_ids. void RemoveSsrcsAndKeepMsids(cricket::SessionDescription* desc); +// TODO(https://crbug.com/webrtc/14175): Stop depending on "track" stats, the +// metrics we're interested in are already available in "inbound-rtp". int FindFirstMediaStatsIndexByKind( const std::string& kind, const std::vector& @@ -650,34 +652,26 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, void StartWatchingDelayStats() { // Get the baseline numbers for audio_packets and audio_delay. auto received_stats = NewGetStats(); - auto track_stats = - received_stats->GetStatsOfType()[0]; - ASSERT_TRUE(track_stats->relative_packet_arrival_delay.is_defined()); auto rtp_stats = received_stats->GetStatsOfType()[0]; + ASSERT_TRUE(rtp_stats->relative_packet_arrival_delay.is_defined()); ASSERT_TRUE(rtp_stats->packets_received.is_defined()); ASSERT_TRUE(rtp_stats->track_id.is_defined()); - audio_track_stats_id_ = track_stats->id(); - ASSERT_TRUE(received_stats->Get(audio_track_stats_id_)); rtp_stats_id_ = rtp_stats->id(); - ASSERT_EQ(audio_track_stats_id_, *rtp_stats->track_id); audio_packets_stat_ = *rtp_stats->packets_received; - audio_delay_stat_ = *track_stats->relative_packet_arrival_delay; - audio_samples_stat_ = *track_stats->total_samples_received; - audio_concealed_stat_ = *track_stats->concealed_samples; + audio_delay_stat_ = *rtp_stats->relative_packet_arrival_delay; + audio_samples_stat_ = *rtp_stats->total_samples_received; + audio_concealed_stat_ = *rtp_stats->concealed_samples; } void UpdateDelayStats(std::string tag, int desc_size) { auto report = NewGetStats(); - auto track_stats = - report->GetAs(audio_track_stats_id_); - ASSERT_TRUE(track_stats); auto rtp_stats = report->GetAs(rtp_stats_id_); ASSERT_TRUE(rtp_stats); auto delta_packets = *rtp_stats->packets_received - audio_packets_stat_; auto delta_rpad = - *track_stats->relative_packet_arrival_delay - audio_delay_stat_; + *rtp_stats->relative_packet_arrival_delay - audio_delay_stat_; auto recent_delay = delta_packets > 0 ? delta_rpad / delta_packets : -1; // The purpose of these checks is to sound the alarm early if we introduce // serious regressions. The numbers are not acceptable for production, but @@ -694,9 +688,9 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, EXPECT_GT(0.1, recent_delay) << tag << " size " << desc_size; #endif auto delta_samples = - *track_stats->total_samples_received - audio_samples_stat_; + *rtp_stats->total_samples_received - audio_samples_stat_; auto delta_concealed = - *track_stats->concealed_samples - audio_concealed_stat_; + *rtp_stats->concealed_samples - audio_concealed_stat_; // These limits should be adjusted down as we improve: // // Concealing more than 4000 samples during a renegotiation is unacceptable. @@ -731,9 +725,9 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, } // Increment trailing counters audio_packets_stat_ = *rtp_stats->packets_received; - audio_delay_stat_ = *track_stats->relative_packet_arrival_delay; - audio_samples_stat_ = *track_stats->total_samples_received; - audio_concealed_stat_ = *track_stats->concealed_samples; + audio_delay_stat_ = *rtp_stats->relative_packet_arrival_delay; + audio_samples_stat_ = *rtp_stats->total_samples_received; + audio_concealed_stat_ = *rtp_stats->concealed_samples; } // Sets number of candidates expected @@ -1240,7 +1234,6 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, uint64_t audio_samples_stat_ = 0; uint64_t audio_concealed_stat_ = 0; std::string rtp_stats_id_; - std::string audio_track_stats_id_; ScopedTaskSafety task_safety_; diff --git a/stats/rtcstats_objects.cc b/stats/rtcstats_objects.cc index 3e0ea47694..2b275ad9eb 100644 --- a/stats/rtcstats_objects.cc +++ b/stats/rtcstats_objects.cc @@ -696,6 +696,11 @@ WEBRTC_RTCSTATS_IMPL( &pli_count, &nack_count, &qp_sum, + &jitter_buffer_flushes, + &delayed_packet_outage_samples, + &relative_packet_arrival_delay, + &interruption_count, + &total_interruption_duration, &min_playout_delay) // clang-format on @@ -754,6 +759,18 @@ RTCInboundRTPStreamStats::RTCInboundRTPStreamStats(std::string&& id, pli_count("pliCount"), nack_count("nackCount"), qp_sum("qpSum"), + jitter_buffer_flushes( + "jitterBufferFlushes", + {NonStandardGroupId::kRtcAudioJitterBufferMaxPackets}), + delayed_packet_outage_samples( + "delayedPacketOutageSamples", + {NonStandardGroupId::kRtcAudioJitterBufferMaxPackets, + NonStandardGroupId::kRtcStatsRelativePacketArrivalDelay}), + relative_packet_arrival_delay( + "relativePacketArrivalDelay", + {NonStandardGroupId::kRtcStatsRelativePacketArrivalDelay}), + interruption_count("interruptionCount"), + total_interruption_duration("totalInterruptionDuration"), min_playout_delay("minPlayoutDelay") {} RTCInboundRTPStreamStats::RTCInboundRTPStreamStats( @@ -808,6 +825,11 @@ RTCInboundRTPStreamStats::RTCInboundRTPStreamStats( pli_count(other.pli_count), nack_count(other.nack_count), qp_sum(other.qp_sum), + jitter_buffer_flushes(other.jitter_buffer_flushes), + delayed_packet_outage_samples(other.delayed_packet_outage_samples), + relative_packet_arrival_delay(other.relative_packet_arrival_delay), + interruption_count(other.interruption_count), + total_interruption_duration(other.total_interruption_duration), min_playout_delay(other.min_playout_delay) {} RTCInboundRTPStreamStats::~RTCInboundRTPStreamStats() {}