From 2f71b61a34c86c5a267e671d36ad5c74f1a0fb69 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= <hbos@webrtc.org>
Date: Tue, 23 Mar 2021 15:18:55 +0100
Subject: [PATCH] Make sure "remote-inbound-rtp.jitter" and "packetsLost" is
 exposed to JS
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In refactoring CL https://webrtc-review.googlesource.com/c/src/+/210340,
the RTCRemoteInboundRtpStreamStats hierarchy was updated to inherit from
RTCReceivedRtpStreamStats but we forgot to update the
WEBRTC_RTCSTATS_IMPL() macro to say that RTCReceivedRtpStreamStats is
the parent. As a consequence, RTCReceivedRtpStreamStats's members
(jitter and packetsLost) were not included when iterating over all
members of RTCRemoteInboundRtpStreamStats, which means these two merics
stopped being exposed to JavaScript in Chromium.

There is sadly no way to safe-guard against this, but the fix is simple.

TBR=hta@webrtc.org,meetwudi@gmail.com

Bug: webrtc:12532
Change-Id: I0179dad6eaa592ee36cfe48978f2fc22133b8f45
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/212866
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Alessio Bazzica <alessiob@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33543}
---
 pc/rtc_stats_integrationtest.cc | 36 +++++++++++++++++++++------------
 stats/rtcstats_objects.cc       |  3 ++-
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/pc/rtc_stats_integrationtest.cc b/pc/rtc_stats_integrationtest.cc
index 456ca72dcd..a285555b96 100644
--- a/pc/rtc_stats_integrationtest.cc
+++ b/pc/rtc_stats_integrationtest.cc
@@ -771,10 +771,18 @@ class RTCStatsReportVerifier {
   void VerifyRTCRTPStreamStats(const RTCRTPStreamStats& stream,
                                RTCStatsVerifier* verifier) {
     verifier->TestMemberIsDefined(stream.ssrc);
-    verifier->TestMemberIsDefined(stream.media_type);
     verifier->TestMemberIsDefined(stream.kind);
-    verifier->TestMemberIsIDReference(stream.track_id,
-                                      RTCMediaStreamTrackStats::kType);
+    // Some legacy metrics are only defined for some of the RTP types in the
+    // hierarcy.
+    if (stream.type() == RTCInboundRTPStreamStats::kType ||
+        stream.type() == RTCOutboundRTPStreamStats::kType) {
+      verifier->TestMemberIsDefined(stream.media_type);
+      verifier->TestMemberIsIDReference(stream.track_id,
+                                        RTCMediaStreamTrackStats::kType);
+    } else {
+      verifier->TestMemberIsUndefined(stream.media_type);
+      verifier->TestMemberIsUndefined(stream.track_id);
+    }
     verifier->TestMemberIsIDReference(stream.transport_id,
                                       RTCTransportStats::kType);
     verifier->TestMemberIsIDReference(stream.codec_id, RTCCodecStats::kType);
@@ -917,6 +925,9 @@ class RTCStatsReportVerifier {
   bool VerifyRTCOutboundRTPStreamStats(
       const RTCOutboundRTPStreamStats& outbound_stream) {
     RTCStatsVerifier verifier(report_, &outbound_stream);
+    // TODO(https://crbug.com/webrtc/12532): Invoke
+    // VerifyRTCReceivedRtpStreamStats() instead of VerifyRTCRTPStreamStats()
+    // because they have a shared hierarchy now!
     VerifyRTCRTPStreamStats(outbound_stream, &verifier);
     if (outbound_stream.media_type.is_defined() &&
         *outbound_stream.media_type == "video") {
@@ -1008,20 +1019,19 @@ class RTCStatsReportVerifier {
     return verifier.ExpectAllMembersSuccessfullyTested();
   }
 
+  void VerifyRTCReceivedRtpStreamStats(
+      const RTCReceivedRtpStreamStats& received_rtp,
+      RTCStatsVerifier* verifier) {
+    VerifyRTCRTPStreamStats(received_rtp, verifier);
+    verifier->TestMemberIsNonNegative<double>(received_rtp.jitter);
+    verifier->TestMemberIsDefined(received_rtp.packets_lost);
+  }
+
   bool VerifyRTCRemoteInboundRtpStreamStats(
       const RTCRemoteInboundRtpStreamStats& remote_inbound_stream) {
     RTCStatsVerifier verifier(report_, &remote_inbound_stream);
-    verifier.TestMemberIsDefined(remote_inbound_stream.ssrc);
-    verifier.TestMemberIsDefined(remote_inbound_stream.kind);
-    verifier.TestMemberIsIDReference(remote_inbound_stream.transport_id,
-                                     RTCTransportStats::kType);
-    verifier.TestMemberIsIDReference(remote_inbound_stream.codec_id,
-                                     RTCCodecStats::kType);
-    verifier.TestMemberIsDefined(remote_inbound_stream.packets_lost);
+    VerifyRTCReceivedRtpStreamStats(remote_inbound_stream, &verifier);
     verifier.TestMemberIsDefined(remote_inbound_stream.fraction_lost);
-    // Note that the existance of RTCCodecStats is needed for |codec_id| and
-    // |jitter| to be present.
-    verifier.TestMemberIsNonNegative<double>(remote_inbound_stream.jitter);
     verifier.TestMemberIsIDReference(remote_inbound_stream.local_id,
                                      RTCOutboundRTPStreamStats::kType);
     verifier.TestMemberIsNonNegative<double>(
diff --git a/stats/rtcstats_objects.cc b/stats/rtcstats_objects.cc
index 33492e784d..3a12eea1c0 100644
--- a/stats/rtcstats_objects.cc
+++ b/stats/rtcstats_objects.cc
@@ -874,7 +874,8 @@ RTCOutboundRTPStreamStats::~RTCOutboundRTPStreamStats() {}
 
 // clang-format off
 WEBRTC_RTCSTATS_IMPL(
-    RTCRemoteInboundRtpStreamStats, RTCStats, "remote-inbound-rtp",
+    RTCRemoteInboundRtpStreamStats, RTCReceivedRtpStreamStats,
+        "remote-inbound-rtp",
     &ssrc,
     &kind,
     &transport_id,