Bug 1495446 - Fix timestamp of WebRTC RTCP stats. r=jib, a=RyanVM
authorNico Grunbaum <na-g@nostrum.com>
Fri, 14 Dec 2018 01:55:21 +0000
changeset 509043 97d55c66f8eeac0d8fa22e8338d4181b30dfccd3
parent 509042 309f7755407e1bed1b2b976659e7f5f4f91ce2ab
child 509044 d55ede8c39ff502e2e54cac78ef4be87256dd84c
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjib, RyanVM
bugs1495446
milestone65.0
Bug 1495446 - Fix timestamp of WebRTC RTCP stats. r=jib, a=RyanVM The RTCP timestamps have different timebases and reporting sources, this makes the source and timebase the same for all RTCP stats Differential Revision: https://phabricator.services.mozilla.com/D7354
dom/media/tests/mochitest/pc.js
dom/media/tests/mochitest/test_peerConnection_stats.html
media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
media/webrtc/signaling/src/media-conduit/AudioConduit.h
media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
media/webrtc/signaling/src/media-conduit/VideoConduit.h
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
--- a/dom/media/tests/mochitest/pc.js
+++ b/dom/media/tests/mochitest/pc.js
@@ -1775,42 +1775,36 @@ PeerConnectionWrapper.prototype = {
    * Checks that we are getting the media streams we expect.
    *
    * @param {object} stats
    *        The stats to check from this PeerConnectionWrapper
    */
   checkStats : function(stats, twoMachines) {
     // Allow for clock drift observed on Windows 7. (Bug 979649)
     const isWin7 = navigator.userAgent.includes("Windows NT 6.1");
-    const clockDriftAllowance = isWin7 ? 1000 : 0;
+    const clockDriftAllowanceMs = isWin7 ? 1000 : 250;
 
     // Use spec way of enumerating stats
     var counters = {};
     for (let [key, res] of stats) {
       info("Checking stats for " + key + " : " + res);
       // validate stats
       ok(res.id == key, "Coherent stats id");
-      var nowish = Date.now() + clockDriftAllowance;
-      var minimum = this.whenCreated;
+      // Bug 1430255: WebRTC uses a different timebase than JS ATM
+      // so there can be differences between timestamp and Date.now().
+      const nowish = Date.now() + clockDriftAllowanceMs;
+      const minimum = this.whenCreated - clockDriftAllowanceMs;
+      const type = res.isRemote ? "rtcp" : "rtp";
       if (!twoMachines) {
-        // Bug 1225729: On android, sometimes the first RTCP of the first
-        // test run gets this value, likely because no RTP has been sent yet.
-        if (false) {
-          ok(res.timestamp >= minimum,
-             "Valid " + (res.isRemote? "rtcp" : "rtp") + " timestamp " +
-                 res.timestamp + " >= " + minimum + " (" +
-                 (res.timestamp - minimum) + " ms)");
-        } else {
-          info("FIXME bug 1495446: uncomment the timestamp test case " +
-               "above after RTCP epoch bug 1495446 is fixed.");
-        }
+        ok(res.timestamp >= minimum,
+           `Valid ${type} timestamp ${res.timestamp} >= ${minimum} (
+              ${res.timestamp - minimum} ms)`);
         ok(res.timestamp <= nowish,
-           "Valid " + (res.isRemote? "rtcp" : "rtp") + " timestamp " +
-               res.timestamp + " <= " + nowish + " (" +
-               (res.timestamp - nowish) + " ms)");
+           `Valid ${type} timestamp ${res.timestamp} <= ${nowish} (
+              ${res.timestamp - nowish} ms)`);
       }
       if (res.isRemote) {
         continue;
       }
       counters[res.type] = (counters[res.type] || 0) + 1;
 
       switch (res.type) {
         case "inbound-rtp":
--- a/dom/media/tests/mochitest/test_peerConnection_stats.html
+++ b/dom/media/tests/mochitest/test_peerConnection_stats.html
@@ -141,17 +141,21 @@ var pedanticChecks = report => {
       return;
     }
 
     // All stats share the following attributes inherited from RTCStats
     is(stat.id, mapKey, stat.type + ".id is the same as the report key.");
 
     // timestamp
     ok(stat.timestamp >= 0, stat.type + ".timestamp is not less than 0");
-
+    // If the timebase for the timestamp is not properly set the timestamp will
+    // appear relative to the year 1970; Bug 1495446
+    const date = new Date(stat.timestamp);
+    ok(date.getFullYear() > 1970,
+       `${stat.type}.timestamp is relative to current time, date=${date}`);
     //
     // RTCStreamStats attributes with common behavior
     //
     // inbound-rtp and outbound-rtp inherit from RTCStreamStats
     if (["inbound-rtp", "outbound-rtp"].includes(stat.type)) {
       //
       // Common RTCStreamStats fields
       //
--- a/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
+++ b/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ -186,40 +186,31 @@ bool WebrtcAudioConduit::GetAVStats(int3
 bool WebrtcAudioConduit::GetRTPStats(unsigned int* jitterMs,
                                      unsigned int* cumulativeLost) {
   ASSERT_ON_THREAD(mStsThread);
   *jitterMs = 0;
   *cumulativeLost = 0;
   return !mSendChannelProxy->GetRTPStatistics(*jitterMs, *cumulativeLost);
 }
 
-DOMHighResTimeStamp NTPtoDOMHighResTimeStamp(uint32_t ntpHigh,
-                                             uint32_t ntpLow) {
-  return (uint32_t(ntpHigh - webrtc::kNtpJan1970) +
-          double(ntpLow) / webrtc::kMagicNtpFractionalUnit) *
-         1000;
-}
-
-bool WebrtcAudioConduit::GetRTCPReceiverReport(DOMHighResTimeStamp* timestamp,
-                                               uint32_t* jitterMs,
+bool WebrtcAudioConduit::GetRTCPReceiverReport(uint32_t* jitterMs,
                                                uint32_t* packetsReceived,
                                                uint64_t* bytesReceived,
                                                uint32_t* cumulativeLost,
                                                int32_t* rttMs) {
   ASSERT_ON_THREAD(mStsThread);
   double fractionLost = 0.0;
   int64_t timestampTmp = 0;
   int64_t rttMsTmp = 0;
   bool res = false;
   if (mSendChannelProxy) {
     res = mSendChannelProxy->GetRTCPReceiverStatistics(
         &timestampTmp, jitterMs, cumulativeLost, packetsReceived, bytesReceived,
         &fractionLost, &rttMsTmp);
   }
-  *timestamp = static_cast<double>(timestampTmp);
   auto stats = mCall->Call()->GetStats();
   int64_t rtt = stats.rtt_ms;
 #ifdef DEBUG
   if (rtt > INT32_MAX) {
     CSFLogError(LOGTAG,
                 "%s for VideoConduit:%p RTT is larger than the"
                 " maximum size of an RTCP RTT.",
                 __FUNCTION__, this);
@@ -229,26 +220,24 @@ bool WebrtcAudioConduit::GetRTCPReceiver
     *rttMs = rtt;
   } else {
     *rttMs = 0;
   }
 
   return res;
 }
 
-bool WebrtcAudioConduit::GetRTCPSenderReport(DOMHighResTimeStamp* timestamp,
-                                             unsigned int* packetsSent,
+bool WebrtcAudioConduit::GetRTCPSenderReport(unsigned int* packetsSent,
                                              uint64_t* bytesSent) {
   ASSERT_ON_THREAD(mStsThread);
   if (!mRecvChannelProxy) {
     return false;
   }
 
   webrtc::CallStatistics stats = mRecvChannelProxy->GetRTCPStatistics();
-  *timestamp = webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds();
   *packetsSent = stats.rtcp_sender_packets_sent;
   *bytesSent = stats.rtcp_sender_octets_sent;
   return *packetsSent > 0 && *bytesSent > 0;
 }
 
 bool WebrtcAudioConduit::SetDtmfPayloadType(unsigned char type, int freq) {
   CSFLogInfo(LOGTAG, "%s : setting dtmf payload %d", __FUNCTION__, (int)type);
   MOZ_ASSERT(NS_IsMainThread());
--- a/media/webrtc/signaling/src/media-conduit/AudioConduit.h
+++ b/media/webrtc/signaling/src/media-conduit/AudioConduit.h
@@ -237,21 +237,20 @@ class WebrtcAudioConduit : public AudioS
                             uint32_t* discardedPackets,
                             uint32_t* framesDecoded) override {
     return false;
   }
   bool GetAVStats(int32_t* jitterBufferDelayMs, int32_t* playoutBufferDelayMs,
                   int32_t* avSyncOffsetMs) override;
   bool GetRTPStats(unsigned int* jitterMs,
                    unsigned int* cumulativeLost) override;
-  bool GetRTCPReceiverReport(DOMHighResTimeStamp* timestamp, uint32_t* jitterMs,
-                             uint32_t* packetsReceived, uint64_t* bytesReceived,
-                             uint32_t* cumulativeLost, int32_t* rttMs) override;
-  bool GetRTCPSenderReport(DOMHighResTimeStamp* timestamp,
-                           unsigned int* packetsSent,
+  bool GetRTCPReceiverReport(uint32_t* jitterMs, uint32_t* packetsReceived,
+                             uint64_t* bytesReceived, uint32_t* cumulativeLost,
+                             int32_t* rttMs) override;
+  bool GetRTCPSenderReport(unsigned int* packetsSent,
                            uint64_t* bytesSent) override;
 
   bool SetDtmfPayloadType(unsigned char type, int freq) override;
 
   bool InsertDTMFTone(int channel, int eventCode, bool outOfBand, int lengthMs,
                       int attenuationDb) override;
 
   void GetRtpSources(const int64_t aTimeNow,
--- a/media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
+++ b/media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
@@ -237,24 +237,22 @@ class MediaSessionConduit {
                                     double* bitrateMean, double* bitrateStdDev,
                                     uint32_t* discardedPackets,
                                     uint32_t* framesDecoded) = 0;
   virtual bool GetAVStats(int32_t* jitterBufferDelayMs,
                           int32_t* playoutBufferDelayMs,
                           int32_t* avSyncOffsetMs) = 0;
   virtual bool GetRTPStats(unsigned int* jitterMs,
                            unsigned int* cumulativeLost) = 0;
-  virtual bool GetRTCPReceiverReport(DOMHighResTimeStamp* timestamp,
-                                     uint32_t* jitterMs,
+  virtual bool GetRTCPReceiverReport(uint32_t* jitterMs,
                                      uint32_t* packetsReceived,
                                      uint64_t* bytesReceived,
                                      uint32_t* cumulativeLost,
                                      int32_t* rttMs) = 0;
-  virtual bool GetRTCPSenderReport(DOMHighResTimeStamp* timestamp,
-                                   unsigned int* packetsSent,
+  virtual bool GetRTCPSenderReport(unsigned int* packetsSent,
                                    uint64_t* bytesSent) = 0;
 
   virtual uint64_t CodecPluginID() = 0;
 
   virtual void SetPCHandle(const std::string& aPCHandle) = 0;
 
   virtual MediaConduitErrorCode DeliverPacket(const void* data, int len) = 0;
 
--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ -1177,18 +1177,17 @@ bool WebrtcVideoConduit::GetRTPStats(uin
     return false;
   }
 
   *jitterMs = mRecvStreamStats.JitterMs();
   *packetsLost = mRecvStreamStats.PacketsLost();
   return true;
 }
 
-bool WebrtcVideoConduit::GetRTCPReceiverReport(DOMHighResTimeStamp* timestamp,
-                                               uint32_t* jitterMs,
+bool WebrtcVideoConduit::GetRTCPReceiverReport(uint32_t* jitterMs,
                                                uint32_t* packetsReceived,
                                                uint64_t* bytesReceived,
                                                uint32_t* cumulativeLost,
                                                int32_t* rttMs) {
   ASSERT_ON_THREAD(mStsThread);
 
   CSFLogVerbose(LOGTAG, "%s for VideoConduit:%p", __FUNCTION__, this);
   if (!mSendStreamStats.Active()) {
@@ -1197,35 +1196,29 @@ bool WebrtcVideoConduit::GetRTCPReceiver
   if (!mSendStreamStats.SsrcFound()) {
     return false;
   }
   *jitterMs = mSendStreamStats.JitterMs();
   *packetsReceived = mSendStreamStats.PacketsReceived();
   *bytesReceived = mSendStreamStats.BytesReceived();
   *cumulativeLost = mSendStreamStats.PacketsLost();
   *rttMs = mCallStats.RttMs();
-  // Note: timestamp is not correct per the spec... should be time the rtcp
-  // was received (remote) or sent (local)
-  *timestamp = webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds();
-
   return true;
 }
 
-bool WebrtcVideoConduit::GetRTCPSenderReport(DOMHighResTimeStamp* timestamp,
-                                             unsigned int* packetsSent,
+bool WebrtcVideoConduit::GetRTCPSenderReport(unsigned int* packetsSent,
                                              uint64_t* bytesSent) {
   ASSERT_ON_THREAD(mStsThread);
 
   CSFLogVerbose(LOGTAG, "%s for VideoConduit:%p", __FUNCTION__, this);
 
   if (!mRecvStreamStats.Active()) {
     return false;
   }
 
-  *timestamp = webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds();
   *packetsSent = mRecvStreamStats.PacketsSent();
   *bytesSent = mRecvStreamStats.BytesSent();
   return true;
 }
 
 MediaConduitErrorCode WebrtcVideoConduit::InitMain() {
   MOZ_ASSERT(NS_IsMainThread());
 
--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.h
+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ -261,21 +261,20 @@ class WebrtcVideoConduit
   bool GetVideoDecoderStats(double* framerateMean, double* framerateStdDev,
                             double* bitrateMean, double* bitrateStdDev,
                             uint32_t* discardedPackets,
                             uint32_t* framesDecoded) override;
   bool GetAVStats(int32_t* jitterBufferDelayMs, int32_t* playoutBufferDelayMs,
                   int32_t* avSyncOffsetMs) override;
   bool GetRTPStats(unsigned int* jitterMs,
                    unsigned int* cumulativeLost) override;
-  bool GetRTCPReceiverReport(DOMHighResTimeStamp* timestamp, uint32_t* jitterMs,
-                             uint32_t* packetsReceived, uint64_t* bytesReceived,
-                             uint32_t* cumulativeLost, int32_t* rttMs) override;
-  bool GetRTCPSenderReport(DOMHighResTimeStamp* timestamp,
-                           unsigned int* packetsSent,
+  bool GetRTCPReceiverReport(uint32_t* jitterMs, uint32_t* packetsReceived,
+                             uint64_t* bytesReceived, uint32_t* cumulativeLost,
+                             int32_t* rttMs) override;
+  bool GetRTCPSenderReport(unsigned int* packetsSent,
                            uint64_t* bytesSent) override;
   uint64_t MozVideoLatencyAvg();
 
   void DisableSsrcChanges() override {
     ASSERT_ON_THREAD(mStsThread);
     mAllowSsrcChange = false;
   }
 
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -2800,28 +2800,28 @@ RefPtr<RTCStatsQueryPromise> PeerConnect
         std::vector<unsigned int> ssrcvals = mp.Conduit()->GetLocalSSRCs();
         if (!ssrcvals.empty()) {
           ssrc = Some(ssrcvals[0]);
         }
         {
           // First, fill in remote stat with rtcp receiver data, if present.
           // ReceiverReports have less information than SenderReports,
           // so fill in what we can.
-          DOMHighResTimeStamp timestamp;
           uint32_t jitterMs;
           uint32_t packetsReceived;
           uint64_t bytesReceived;
           uint32_t packetsLost;
           int32_t rtt;
-          if (mp.Conduit()->GetRTCPReceiverReport(
-                  &timestamp, &jitterMs, &packetsReceived, &bytesReceived,
-                  &packetsLost, &rtt)) {
+          if (mp.Conduit()->GetRTCPReceiverReport(&jitterMs, &packetsReceived,
+                                                  &bytesReceived, &packetsLost,
+                                                  &rtt)) {
             remoteId = NS_LITERAL_STRING("outbound_rtcp_") + idstr;
             RTCInboundRTPStreamStats s;
-            s.mTimestamp.Construct(timestamp);
+            // TODO Bug 1496533 - use reception time not query time
+            s.mTimestamp.Construct(query->now);
             s.mId.Construct(remoteId);
             s.mType.Construct(RTCStatsType::Inbound_rtp);
             ssrc.apply([&s](uint32_t aSsrc) { s.mSsrc.Construct(aSsrc); });
             s.mMediaType.Construct(
                 kind);  // mediaType is the old name for kind.
             s.mKind.Construct(kind);
             s.mJitter.Construct(double(jitterMs) / 1000);
             s.mRemoteId.Construct(localId);
@@ -2834,16 +2834,17 @@ RefPtr<RTCStatsQueryPromise> PeerConnect
             }
             query->report->mInboundRTPStreamStats.Value().AppendElement(
                 s, fallible);
           }
         }
         // Then, fill in local side (with cross-link to remote only if present)
         {
           RTCOutboundRTPStreamStats s;
+          // TODO Bug 1496533 - use reception time not query time
           s.mTimestamp.Construct(query->now);
           s.mId.Construct(localId);
           s.mType.Construct(RTCStatsType::Outbound_rtp);
           ssrc.apply([&s](uint32_t aSsrc) { s.mSsrc.Construct(aSsrc); });
           s.mMediaType.Construct(kind);  // mediaType is the old name for kind.
           s.mKind.Construct(kind);
           s.mRemoteId.Construct(remoteId);
           s.mIsRemote = false;
@@ -2890,24 +2891,23 @@ RefPtr<RTCStatsQueryPromise> PeerConnect
         nsString remoteId;
         Maybe<uint32_t> ssrc;
         unsigned int ssrcval;
         if (mp.Conduit()->GetRemoteSSRC(&ssrcval)) {
           ssrc = Some(ssrcval);
         }
         {
           // First, fill in remote stat with rtcp sender data, if present.
-          DOMHighResTimeStamp timestamp;
           uint32_t packetsSent;
           uint64_t bytesSent;
-          if (mp.Conduit()->GetRTCPSenderReport(&timestamp, &packetsSent,
-                                                &bytesSent)) {
+          if (mp.Conduit()->GetRTCPSenderReport(&packetsSent, &bytesSent)) {
             remoteId = NS_LITERAL_STRING("inbound_rtcp_") + idstr;
             RTCOutboundRTPStreamStats s;
-            s.mTimestamp.Construct(timestamp);
+            // TODO Bug 1496533 - use reception time not query time
+            s.mTimestamp.Construct(query->now);
             s.mId.Construct(remoteId);
             s.mType.Construct(RTCStatsType::Outbound_rtp);
             ssrc.apply([&s](uint32_t aSsrc) { s.mSsrc.Construct(aSsrc); });
             s.mMediaType.Construct(
                 kind);  // mediaType is the old name for kind.
             s.mKind.Construct(kind);
             s.mRemoteId.Construct(localId);
             s.mIsRemote = true;