From fd1e9d1af4620a753008e9cf2281da25dc7add34 Mon Sep 17 00:00:00 2001 From: Di Wu Date: Tue, 9 Mar 2021 09:25:28 -0800 Subject: [PATCH] [Stats] Add minimum RTCReceivedRtpStreamStats with jitter and packetsLost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec: https://www.w3.org/TR/webrtc-stats/#receivedrtpstats-dict* According to the spec, |RTCReceivedRtpStreamStats| is the base class for |RTCInboundRtpStreamStats| and |RTCRemoteInboundRtpStreamStats|. This structure isn't visible in JavaScript but it's important to bring it up to spec for the C++ part. This CL adds the barebone |RTCReceivedRtpStreamStats| with a bunch of TODOs for later migrations. This commit makes the minimum |RTCReceivedRtpStreamStats| and rebase |RTCInboundRtpStreamStats| and |RTCRemoteInboundRtpStreamStats| to use the new class as the parent class. This commit also moves |jitter| and |packets_lost| to |RTCReceivedRtpStreamStats|, from |RTCInboundRtpStreamStats| and |RTCRemoteInboundRtpStreamStats|. Moving these two first because they are the two that exist in both subclasses for now. Bug: webrtc:12532 Change-Id: I0ec74fd241f16c1e1a6498b6baa621ca0489f279 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/210340 Commit-Queue: Henrik Boström Reviewed-by: Henrik Boström Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#33435} --- api/stats/rtcstats_objects.h | 86 +++++++++++++---------- pc/rtc_stats_integrationtest.cc | 25 ++++--- stats/rtcstats_objects.cc | 121 ++++++++++++++++++-------------- 3 files changed, 132 insertions(+), 100 deletions(-) diff --git a/api/stats/rtcstats_objects.h b/api/stats/rtcstats_objects.h index 7590a620c2..a4e8ead6b7 100644 --- a/api/stats/rtcstats_objects.h +++ b/api/stats/rtcstats_objects.h @@ -374,34 +374,46 @@ class RTC_EXPORT RTCRTPStreamStats : public RTCStats { ~RTCRTPStreamStats() override; RTCStatsMember ssrc; - // TODO(hbos): Remote case not supported by |RTCStatsCollector|. - // crbug.com/657855, 657856 - RTCStatsMember is_remote; // = false - RTCStatsMember media_type; // renamed to kind. RTCStatsMember kind; RTCStatsMember track_id; RTCStatsMember transport_id; RTCStatsMember codec_id; - // 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; - // TODO(hbos): Not collected by |RTCStatsCollector|. crbug.com/657854 - // SLI count is only defined for |media_type == "video"|. - RTCStatsMember sli_count; - RTCStatsMember qp_sum; + + // Obsolete + RTCStatsMember media_type; // renamed to kind. protected: RTCRTPStreamStats(const std::string& id, int64_t timestamp_us); RTCRTPStreamStats(std::string&& id, int64_t timestamp_us); }; +class RTC_EXPORT RTCReceivedRtpStreamStats : public RTCRTPStreamStats { + public: + WEBRTC_RTCSTATS_DECL(); + + RTCReceivedRtpStreamStats(const RTCReceivedRtpStreamStats& other); + ~RTCReceivedRtpStreamStats() override; + + // TODO(hbos) The following fields need to be added and migrated + // both from RTCInboundRtpStreamStats and RTCRemoteInboundRtpStreamStats: + // packetsReceived, packetsDiscarded, packetsRepaired, burstPacketsLost, + // burstPacketDiscarded, burstLossCount, burstDiscardCount, burstLossRate, + // burstDiscardRate, gapLossRate, gapDiscardRate, framesDropped, + // partialFramesLost, fullFramesLost + // crbug.com/webrtc/12532 + RTCStatsMember jitter; + RTCStatsMember packets_lost; // Signed per RFC 3550 + + protected: + RTCReceivedRtpStreamStats(const std::string&& id, int64_t timestamp_us); + RTCReceivedRtpStreamStats(std::string&& id, int64_t timestamp_us); +}; + // https://w3c.github.io/webrtc-stats/#inboundrtpstats-dict* // TODO(hbos): Support the remote case |is_remote = true|. // https://bugs.webrtc.org/7065 -class RTC_EXPORT RTCInboundRTPStreamStats final : public RTCRTPStreamStats { +class RTC_EXPORT RTCInboundRTPStreamStats final + : public RTCReceivedRtpStreamStats { public: WEBRTC_RTCSTATS_DECL(); @@ -415,9 +427,7 @@ class RTC_EXPORT RTCInboundRTPStreamStats final : public RTCRTPStreamStats { RTCStatsMember fec_packets_discarded; RTCStatsMember bytes_received; RTCStatsMember header_bytes_received; - RTCStatsMember packets_lost; // Signed per RFC 3550 RTCStatsMember last_packet_received_timestamp; - RTCStatsMember jitter; RTCStatsMember jitter_buffer_delay; RTCStatsMember jitter_buffer_emitted_count; RTCStatsMember total_samples_received; @@ -469,6 +479,16 @@ class RTC_EXPORT RTCInboundRTPStreamStats final : public RTCRTPStreamStats { // TODO(hbos): This is only implemented for video; implement it for audio as // well. RTCStatsMember decoder_implementation; + // 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; + + // Obsolete + RTCStatsMember is_remote; // = false }; // https://w3c.github.io/webrtc-stats/#outboundrtpstats-dict* @@ -517,18 +537,21 @@ class RTC_EXPORT RTCOutboundRTPStreamStats final : public RTCRTPStreamStats { // TODO(hbos): This is only implemented for video; implement it for audio as // well. RTCStatsMember encoder_implementation; + // 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; + + // Obsolete + RTCStatsMember is_remote; // = false }; -// TODO(https://crbug.com/webrtc/10671): Refactor the stats dictionaries to have -// the same hierarchy as in the spec; implement RTCReceivedRtpStreamStats. -// Several metrics are shared between "outbound-rtp", "remote-inbound-rtp", -// "inbound-rtp" and "remote-outbound-rtp". In the spec there is a hierarchy of -// dictionaries that minimizes defining the same metrics in multiple places. -// From JavaScript this hierarchy is not observable and the spec's hierarchy is -// purely editorial. In C++ non-final classes in the hierarchy could be used to -// refer to different stats objects within the hierarchy. // https://w3c.github.io/webrtc-stats/#remoteinboundrtpstats-dict* -class RTC_EXPORT RTCRemoteInboundRtpStreamStats final : public RTCStats { +class RTC_EXPORT RTCRemoteInboundRtpStreamStats final + : public RTCReceivedRtpStreamStats { public: WEBRTC_RTCSTATS_DECL(); @@ -537,17 +560,6 @@ class RTC_EXPORT RTCRemoteInboundRtpStreamStats final : public RTCStats { RTCRemoteInboundRtpStreamStats(const RTCRemoteInboundRtpStreamStats& other); ~RTCRemoteInboundRtpStreamStats() override; - // In the spec RTCRemoteInboundRtpStreamStats inherits from RTCRtpStreamStats - // and RTCReceivedRtpStreamStats. The members here are listed based on where - // they are defined in the spec. - // RTCRtpStreamStats - RTCStatsMember ssrc; - RTCStatsMember kind; - RTCStatsMember transport_id; - RTCStatsMember codec_id; - // RTCReceivedRtpStreamStats - RTCStatsMember packets_lost; - RTCStatsMember jitter; // TODO(hbos): The following RTCReceivedRtpStreamStats metrics should also be // implemented: packetsReceived, packetsDiscarded, packetsRepaired, // burstPacketsLost, burstPacketsDiscarded, burstLossCount, burstDiscardCount, diff --git a/pc/rtc_stats_integrationtest.cc b/pc/rtc_stats_integrationtest.cc index a6044ba975..68dd17b216 100644 --- a/pc/rtc_stats_integrationtest.cc +++ b/pc/rtc_stats_integrationtest.cc @@ -770,7 +770,6 @@ class RTCStatsReportVerifier { void VerifyRTCRTPStreamStats(const RTCRTPStreamStats& stream, RTCStatsVerifier* verifier) { verifier->TestMemberIsDefined(stream.ssrc); - verifier->TestMemberIsDefined(stream.is_remote); verifier->TestMemberIsDefined(stream.media_type); verifier->TestMemberIsDefined(stream.kind); verifier->TestMemberIsIDReference(stream.track_id, @@ -778,16 +777,6 @@ class RTCStatsReportVerifier { verifier->TestMemberIsIDReference(stream.transport_id, RTCTransportStats::kType); verifier->TestMemberIsIDReference(stream.codec_id, RTCCodecStats::kType); - if (stream.media_type.is_defined() && *stream.media_type == "video") { - verifier->TestMemberIsNonNegative(stream.fir_count); - verifier->TestMemberIsNonNegative(stream.pli_count); - verifier->TestMemberIsNonNegative(stream.nack_count); - } else { - verifier->TestMemberIsUndefined(stream.fir_count); - verifier->TestMemberIsUndefined(stream.pli_count); - verifier->TestMemberIsUndefined(stream.nack_count); - } - verifier->TestMemberIsUndefined(stream.sli_count); } bool VerifyRTCInboundRTPStreamStats( @@ -820,6 +809,7 @@ class RTCStatsReportVerifier { // this test. See RFC 3550. verifier.TestMemberIsNonNegative(inbound_stream.packets_lost); verifier.TestMemberIsDefined(inbound_stream.last_packet_received_timestamp); + verifier.TestMemberIsDefined(inbound_stream.is_remote); if (inbound_stream.frames_received.ValueOrDefault(0) > 0) { verifier.TestMemberIsNonNegative(inbound_stream.frame_width); verifier.TestMemberIsNonNegative(inbound_stream.frame_height); @@ -852,7 +842,13 @@ class RTCStatsReportVerifier { verifier.TestMemberIsUndefined(inbound_stream.total_audio_energy); verifier.TestMemberIsUndefined(inbound_stream.total_samples_duration); verifier.TestMemberIsNonNegative(inbound_stream.frames_received); + verifier.TestMemberIsNonNegative(inbound_stream.fir_count); + verifier.TestMemberIsNonNegative(inbound_stream.pli_count); + verifier.TestMemberIsNonNegative(inbound_stream.nack_count); } else { + verifier.TestMemberIsUndefined(inbound_stream.fir_count); + verifier.TestMemberIsUndefined(inbound_stream.pli_count); + verifier.TestMemberIsUndefined(inbound_stream.nack_count); verifier.TestMemberIsNonNegative(inbound_stream.jitter); verifier.TestMemberIsNonNegative( inbound_stream.jitter_buffer_delay); @@ -925,16 +921,23 @@ class RTCStatsReportVerifier { *outbound_stream.media_type == "video") { verifier.TestMemberIsIDReference(outbound_stream.media_source_id, RTCVideoSourceStats::kType); + verifier.TestMemberIsNonNegative(outbound_stream.fir_count); + verifier.TestMemberIsNonNegative(outbound_stream.pli_count); + verifier.TestMemberIsNonNegative(outbound_stream.nack_count); if (*outbound_stream.frames_encoded > 0) { verifier.TestMemberIsNonNegative(outbound_stream.qp_sum); } else { verifier.TestMemberIsUndefined(outbound_stream.qp_sum); } } else { + verifier.TestMemberIsUndefined(outbound_stream.fir_count); + verifier.TestMemberIsUndefined(outbound_stream.pli_count); + verifier.TestMemberIsUndefined(outbound_stream.nack_count); verifier.TestMemberIsIDReference(outbound_stream.media_source_id, RTCAudioSourceStats::kType); verifier.TestMemberIsUndefined(outbound_stream.qp_sum); } + verifier.TestMemberIsDefined(outbound_stream.is_remote); verifier.TestMemberIsOptionalIDReference( outbound_stream.remote_id, RTCRemoteInboundRtpStreamStats::kType); verifier.TestMemberIsNonNegative(outbound_stream.packets_sent); diff --git a/stats/rtcstats_objects.cc b/stats/rtcstats_objects.cc index e234705767..8e9f047856 100644 --- a/stats/rtcstats_objects.cc +++ b/stats/rtcstats_objects.cc @@ -547,17 +547,11 @@ RTCPeerConnectionStats::~RTCPeerConnectionStats() {} // clang-format off WEBRTC_RTCSTATS_IMPL(RTCRTPStreamStats, RTCStats, "rtp", &ssrc, - &is_remote, - &media_type, &kind, &track_id, &transport_id, &codec_id, - &fir_count, - &pli_count, - &nack_count, - &sli_count, - &qp_sum) + &media_type) // clang-format on RTCRTPStreamStats::RTCRTPStreamStats(const std::string& id, @@ -567,46 +561,57 @@ RTCRTPStreamStats::RTCRTPStreamStats(const std::string& id, RTCRTPStreamStats::RTCRTPStreamStats(std::string&& id, int64_t timestamp_us) : RTCStats(std::move(id), timestamp_us), ssrc("ssrc"), - is_remote("isRemote", false), - media_type("mediaType"), kind("kind"), track_id("trackId"), transport_id("transportId"), codec_id("codecId"), - fir_count("firCount"), - pli_count("pliCount"), - nack_count("nackCount"), - sli_count("sliCount"), - qp_sum("qpSum") {} + media_type("mediaType") {} RTCRTPStreamStats::RTCRTPStreamStats(const RTCRTPStreamStats& other) : RTCStats(other.id(), other.timestamp_us()), ssrc(other.ssrc), - is_remote(other.is_remote), - media_type(other.media_type), kind(other.kind), track_id(other.track_id), transport_id(other.transport_id), codec_id(other.codec_id), - fir_count(other.fir_count), - pli_count(other.pli_count), - nack_count(other.nack_count), - sli_count(other.sli_count), - qp_sum(other.qp_sum) {} + media_type(other.media_type) {} RTCRTPStreamStats::~RTCRTPStreamStats() {} // clang-format off WEBRTC_RTCSTATS_IMPL( - RTCInboundRTPStreamStats, RTCRTPStreamStats, "inbound-rtp", + RTCReceivedRtpStreamStats, RTCRTPStreamStats, "received-rtp", + &jitter, + &packets_lost) +// clang-format on + +RTCReceivedRtpStreamStats::RTCReceivedRtpStreamStats(const std::string&& id, + int64_t timestamp_us) + : RTCReceivedRtpStreamStats(std::string(id), timestamp_us) {} + +RTCReceivedRtpStreamStats::RTCReceivedRtpStreamStats(std::string&& id, + int64_t timestamp_us) + : RTCRTPStreamStats(std::move(id), timestamp_us), + jitter("jitter"), + packets_lost("packetsLost") {} + +RTCReceivedRtpStreamStats::RTCReceivedRtpStreamStats( + const RTCReceivedRtpStreamStats& other) + : RTCRTPStreamStats(other), + jitter(other.jitter), + packets_lost(other.packets_lost) {} + +RTCReceivedRtpStreamStats::~RTCReceivedRtpStreamStats() {} + +// clang-format off +WEBRTC_RTCSTATS_IMPL( + RTCInboundRTPStreamStats, RTCReceivedRtpStreamStats, "inbound-rtp", &packets_received, &fec_packets_received, &fec_packets_discarded, &bytes_received, &header_bytes_received, - &packets_lost, &last_packet_received_timestamp, - &jitter, &jitter_buffer_delay, &jitter_buffer_emitted_count, &total_samples_received, @@ -642,7 +647,12 @@ WEBRTC_RTCSTATS_IMPL( &total_squared_inter_frame_delay, &content_type, &estimated_playout_timestamp, - &decoder_implementation) + &decoder_implementation, + &fir_count, + &pli_count, + &nack_count, + &qp_sum, + &is_remote) // clang-format on RTCInboundRTPStreamStats::RTCInboundRTPStreamStats(const std::string& id, @@ -651,15 +661,13 @@ RTCInboundRTPStreamStats::RTCInboundRTPStreamStats(const std::string& id, RTCInboundRTPStreamStats::RTCInboundRTPStreamStats(std::string&& id, int64_t timestamp_us) - : RTCRTPStreamStats(std::move(id), timestamp_us), + : RTCReceivedRtpStreamStats(std::move(id), timestamp_us), packets_received("packetsReceived"), fec_packets_received("fecPacketsReceived"), fec_packets_discarded("fecPacketsDiscarded"), bytes_received("bytesReceived"), header_bytes_received("headerBytesReceived"), - packets_lost("packetsLost"), last_packet_received_timestamp("lastPacketReceivedTimestamp"), - jitter("jitter"), jitter_buffer_delay("jitterBufferDelay"), jitter_buffer_emitted_count("jitterBufferEmittedCount"), total_samples_received("totalSamplesReceived"), @@ -695,19 +703,22 @@ RTCInboundRTPStreamStats::RTCInboundRTPStreamStats(std::string&& id, total_squared_inter_frame_delay("totalSquaredInterFrameDelay"), content_type("contentType"), estimated_playout_timestamp("estimatedPlayoutTimestamp"), - decoder_implementation("decoderImplementation") {} + decoder_implementation("decoderImplementation"), + fir_count("firCount"), + pli_count("pliCount"), + nack_count("nackCount"), + qp_sum("qpSum"), + is_remote("isRemote") {} RTCInboundRTPStreamStats::RTCInboundRTPStreamStats( const RTCInboundRTPStreamStats& other) - : RTCRTPStreamStats(other), + : RTCReceivedRtpStreamStats(other), packets_received(other.packets_received), fec_packets_received(other.fec_packets_received), fec_packets_discarded(other.fec_packets_discarded), bytes_received(other.bytes_received), header_bytes_received(other.header_bytes_received), - packets_lost(other.packets_lost), last_packet_received_timestamp(other.last_packet_received_timestamp), - jitter(other.jitter), jitter_buffer_delay(other.jitter_buffer_delay), jitter_buffer_emitted_count(other.jitter_buffer_emitted_count), total_samples_received(other.total_samples_received), @@ -744,7 +755,12 @@ RTCInboundRTPStreamStats::RTCInboundRTPStreamStats( total_squared_inter_frame_delay(other.total_squared_inter_frame_delay), content_type(other.content_type), estimated_playout_timestamp(other.estimated_playout_timestamp), - decoder_implementation(other.decoder_implementation) {} + decoder_implementation(other.decoder_implementation), + fir_count(other.fir_count), + pli_count(other.pli_count), + nack_count(other.nack_count), + qp_sum(other.qp_sum), + is_remote(other.is_remote) {} RTCInboundRTPStreamStats::~RTCInboundRTPStreamStats() {} @@ -773,7 +789,12 @@ WEBRTC_RTCSTATS_IMPL( &quality_limitation_reason, &quality_limitation_resolution_changes, &content_type, - &encoder_implementation) + &encoder_implementation, + &fir_count, + &pli_count, + &nack_count, + &qp_sum, + &is_remote) // clang-format on RTCOutboundRTPStreamStats::RTCOutboundRTPStreamStats(const std::string& id, @@ -806,7 +827,12 @@ RTCOutboundRTPStreamStats::RTCOutboundRTPStreamStats(std::string&& id, quality_limitation_resolution_changes( "qualityLimitationResolutionChanges"), content_type("contentType"), - encoder_implementation("encoderImplementation") {} + encoder_implementation("encoderImplementation"), + fir_count("firCount"), + pli_count("pliCount"), + nack_count("nackCount"), + qp_sum("qpSum"), + is_remote("isRemote") {} RTCOutboundRTPStreamStats::RTCOutboundRTPStreamStats( const RTCOutboundRTPStreamStats& other) @@ -834,7 +860,12 @@ RTCOutboundRTPStreamStats::RTCOutboundRTPStreamStats( quality_limitation_resolution_changes( other.quality_limitation_resolution_changes), content_type(other.content_type), - encoder_implementation(other.encoder_implementation) {} + encoder_implementation(other.encoder_implementation), + fir_count(other.fir_count), + pli_count(other.pli_count), + nack_count(other.nack_count), + qp_sum(other.qp_sum), + is_remote(other.is_remote) {} RTCOutboundRTPStreamStats::~RTCOutboundRTPStreamStats() {} @@ -845,8 +876,6 @@ WEBRTC_RTCSTATS_IMPL( &kind, &transport_id, &codec_id, - &packets_lost, - &jitter, &local_id, &round_trip_time, &fraction_lost, @@ -862,13 +891,7 @@ RTCRemoteInboundRtpStreamStats::RTCRemoteInboundRtpStreamStats( RTCRemoteInboundRtpStreamStats::RTCRemoteInboundRtpStreamStats( std::string&& id, int64_t timestamp_us) - : RTCStats(std::move(id), timestamp_us), - ssrc("ssrc"), - kind("kind"), - transport_id("transportId"), - codec_id("codecId"), - packets_lost("packetsLost"), - jitter("jitter"), + : RTCReceivedRtpStreamStats(std::move(id), timestamp_us), local_id("localId"), round_trip_time("roundTripTime"), fraction_lost("fractionLost"), @@ -877,13 +900,7 @@ RTCRemoteInboundRtpStreamStats::RTCRemoteInboundRtpStreamStats( RTCRemoteInboundRtpStreamStats::RTCRemoteInboundRtpStreamStats( const RTCRemoteInboundRtpStreamStats& other) - : RTCStats(other), - ssrc(other.ssrc), - kind(other.kind), - transport_id(other.transport_id), - codec_id(other.codec_id), - packets_lost(other.packets_lost), - jitter(other.jitter), + : RTCReceivedRtpStreamStats(other), local_id(other.local_id), round_trip_time(other.round_trip_time), fraction_lost(other.fraction_lost),