Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Description • 7 years ago

The isRemote from has been removed RTCInboundRTPStreamStats, RTCOutboundRTPStreamStats. Instead new dictionaries exist for the local and remote, inbound and outbound stats.

See the following spec PR: https://github.com/w3c/webrtc-stats/pull/191

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 1 • 7 years ago

Setting P1 since you're working on it.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8892161 [details]
Bug 1380555 - refactor out isRemote;

https://reviewboard.mozilla.org/r/163130/#review168534

Looks good! Some problems with ReadParam() and a general increase in conditional code-paths, which I wonder if we need, + enough minor issues that I'd like to see it again.

::: dom/media/tests/mochitest/pc.js:1665
(Diff revision 2)
>        if (res.isRemote) {
>          continue;
>        }

We can remove this now.

::: dom/media/tests/mochitest/pc.js:1689
(Diff revision 2)
>              ok(res.packetsReceived !== undefined, "Rtp packetsReceived");
>              ok(res.bytesReceived >= res.packetsReceived, "Rtp bytesReceived");
>            }
>            if (res.remoteId) {
>              var rem = stats.get(res.remoteId);
> -            ok(rem.isRemote, "Remote is rtcp");
> +            ok(rem.type, "Remote is rtcp");

seems redundant.

::: dom/media/tests/mochitest/pc.js:1696
(Diff revision 2)
> -               if (false) { // Bug 1325430 if (!this.disableRtpCountChecking) {
> +              if (false) { // Bug 1325430 if (!this.disableRtpCountChecking) {
> -               // no guarantee which one is newer!
> +         // no guarantee which one is newer!
> -               // Note: this must change when we add a timestamp field to remote RTCP reports
> +         // Note: this must change when we add a timestamp field to remote RTCP reports
> -               // and make rem.timestamp be the reception time
> +         // and make rem.timestamp be the reception time
> -                if (res.timestamp >= rem.timestamp) {
> +    if (res.timestamp >= rem.timestamp) {
>                   ok(rem.packetsReceived <= res.packetsSent, "No more than sent packets");
> -                 } else {
> +     } else {
>                    info("REVERSED timestamps: rec:" +
> -                     rem.packetsReceived + " time:" + rem.timestamp + " sent:" + res.packetsSent + " time:" + res.timestamp);
> +         rem.packetsReceived + " time:" + rem.timestamp + " sent:" + res.packetsSent + " time:" + res.timestamp);
> -                 }
> +     }
> -                // Else we may have received more than outdated Rtcp packetsSent
> +    // Else we may have received more than outdated Rtcp packetsSent

Odd indent

::: dom/media/tests/mochitest/test_peerConnection_stats.html:26
(Diff revision 2)
> -      "burstPacketsLost", "burstLossCount", "burstDiscardCount",
> +      return this.types.includes(type);
> -      "gapDiscardRate", "gapLossRate",],
> -    deprecated: ["mozRtt"],
> -  },
> +    },
> -  "outbound-rtp": {
> -    expected: ["id", "timestamp", "type", "ssrc", "isRemote", "mediaType",
> +  };
> +  if (superTypeDefinition.types) {

When is this ever falsy?

::: dom/media/tests/mochitest/test_peerConnection_stats.html:29
(Diff revision 2)
> -    localVideoOnly: ["droppedFrames", "bitrateMean", "bitrateStdDev",
> -      "framerateMean", "framerateStdDev", "framesEncoded", "firCount",
> +  for (let i of ["expected", "optional", "unimplemented", "localVideoOnly",
> +            "deprecated"]) {

odd indent

::: dom/media/tests/mochitest/test_peerConnection_stats.html:54
(Diff revision 2)
> +                     "mediaType",],
> +  optional:         ["nackCount",],
> +  unimplemented:    ["mediaTrackId",
> +                     "transportId",
> +                     "codecId",
> +                     "sliCount",
> +                     "qpSum"],

inconsistent tail-commas

::: dom/media/tests/mochitest/test_peerConnection_stats.html:95
(Diff revision 2)
> +});
> +
> +var statsExpectedByType = {
> +  "inbound-rtp": extendTypeWith(rtcReceivedRtpStreamStats, {
> +    name:           "inbound-rtp",
> +    optional:       ["remoteId",],

expected

::: dom/media/tests/mochitest/test_peerConnection_stats.html:114
(Diff revision 2)
> +  }),
> +  "outbound-rtp": extendTypeWith(rtcSentRtpStreamStats, {
> +    name:           "outbound-rtp",
> +    expected:       ["packetsSent",
> +                     "bytesSent",],
> +    optional:       ["remoteId",

expected

::: dom/media/tests/mochitest/test_peerConnection_stats.html:263
(Diff revision 2)
>        ok(["audio", "video"].includes(stat.mediaType),
>          stat.type + ".mediaType is 'audio' or 'video'");
>  
>        // remote id
> -      if (stat.remoteId) {
> -        ok(report.has(stat.remoteId), "remoteId exists in report.");
> +      if (stat.inner.remoteId) {
> +        ok(report.has(stat.remoteId), "localId exists in report.");

s/localId/remoteId/

::: dom/media/tests/mochitest/test_peerConnection_stats.html:276
(Diff revision 2)
> +      if (stat.inner.localId) {
> +        ok(report.has(stat.localId), "localId exists in report.");
> +        is(report.get(stat.localId).ssrc, stat.ssrc,
> +          "remote ssrc and local ssrc match.");
> +        is(report.get(stat.localId).remoteId, stat.id,
>            "remote object has local object as it's own remote object.");

"local object has remote object as it's own remote object." - I think.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:574
(Diff revision 2)
>  var waitForSyncedRtcp = async pc => {
>    // Ensures that RTCP is present
>    let ensureSyncedRtcp = async () => {
>      let stats = await pc.getStats();
>      for (let [k, v] of stats) {
> -      if (v.type.endsWith("bound-rtp") && !v.remoteId) {
> +      if (v.type.endsWith("bound-rtp") && (!v.remoteId && !v.localId)) {

Redundant parens.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:574
(Diff revision 2)
> -      if (v.type.endsWith("bound-rtp") && !v.remoteId) {
> +      if (v.type.endsWith("bound-rtp") && (!v.remoteId && !v.localId)) {
>          throw new Error(v.id + " is missing remoteId: "

"is missing remoteId or localId" - actually, maybe break this test up with specific log messages?

::: dom/media/webrtc/WebrtcGlobal.h:35
(Diff revision 2)
>      }
>  
>      WriteParam(aMsg, false);
>    }
>  
> -  static bool Read(const Message* aMsg, PickleIterator* aIter, paramType* aResult)
> +  static bool Read(const Message* aMsg, PickleIterator* aItr, paramType* aReslt)

s/aReslt/aResult/

::: dom/media/webrtc/WebrtcGlobal.h:65
(Diff revision 2)
>    static void Write(Message* aMsg, const paramType& aParam)
>    {
>      WriteParam(aMsg, static_cast<const FallibleTArray<T>&>(aParam));
>    }
>  
> -  static bool Read(const Message* aMsg, PickleIterator* aIter, paramType* aResult)
> +  static bool Read(const Message* aMsg, PickleIterator* aItr, paramType* aReslt)

s/aReslt/aResult/

::: dom/media/webrtc/WebrtcGlobal.h:165
(Diff revision 2)
> -static bool ReadRTCStats(const Message* aMsg, PickleIterator* aIter, RTCStats* aResult)
> +  static bool Read(const Message* aMsg, PickleIterator* aItr, RTCStats* aReslt)
> -{
> +  {
> -  // RTCStats base class
> +    // RTCStats base class
> -  if (!ReadParam(aMsg, aIter, &(aResult->mId)) ||
> +    if (!aMsg || !aItr || !aReslt ||

There seem to be a lot more conditionals here now. How come, and can they be avoided?

::: dom/media/webrtc/WebrtcGlobal.h:272
(Diff revision 2)
>      WriteParam(aMsg, aParam.mPortNumber);
>      WriteParam(aMsg, aParam.mTransport);
> -    WriteRTCStats(aMsg, aParam);
> +    ParamTraits<mozilla::dom::RTCStats>::Write(aMsg, aParam);
>    }
>  
> -  static bool Read(const Message* aMsg, PickleIterator* aIter, paramType* aResult)
> +  static bool Read(const Message* aMsg, PickleIterator* aItr, paramType* aReslt)

s/aReslt/aResult/

::: dom/media/webrtc/WebrtcGlobal.h:306
(Diff revision 2)
> -    WriteRTCStats(aMsg, aParam);
> +    ParamTraits<mozilla::dom::RTCStats>::Write(aMsg, aParam);
>    }
>  
> -  static bool Read(const Message* aMsg, PickleIterator* aIter, paramType* aResult)
> +  static bool Read(const Message* aMsg, PickleIterator* aItr, paramType* aReslt)
>    {
> -    if (!ReadParam(aMsg, aIter, &(aResult->mActiveConnection)) ||
> +    if (!ReadParam(aMsg, aItr, &(aReslt->mActiveConnection)) ||

Here you don't check for !aMsg || !aItr, how come you had to elsewhere?

Fewer if's == better.

::: dom/media/webrtc/WebrtcGlobal.h:323
(Diff revision 2)
> -static void WriteRTCRTPStreamStats(
> -              Message* aMsg,
> +template<>
> +struct ParamTraits<mozilla::dom::RTCRTPStreamStats>
> -              const mozilla::dom::RTCRTPStreamStats& aParam)
>  {
> -    WriteParam(aMsg, aParam.mBitrateMean);
> -    WriteParam(aMsg, aParam.mBitrateStdDev);
> +  typedef mozilla::dom::RTCRTPStreamStats paramType;
> +  typedef mozilla::dom::RTCStats superType;

Did you try regular inheritance here? Just curious.

::: dom/media/webrtc/WebrtcGlobal.h:342
(Diff revision 2)
> -              mozilla::dom::RTCRTPStreamStats* aResult)
> -{
> -  if (!ReadParam(aMsg, aIter, &(aResult->mBitrateMean)) ||
> -      !ReadParam(aMsg, aIter, &(aResult->mBitrateStdDev)) ||
> -      !ReadParam(aMsg, aIter, &(aResult->mCodecId)) ||
> -      !ReadParam(aMsg, aIter, &(aResult->mFramerateMean)) ||
> -      !ReadParam(aMsg, aIter, &(aResult->mFramerateStdDev)) ||
> -      !ReadParam(aMsg, aIter, &(aResult->mIsRemote)) ||
> -      !ReadParam(aMsg, aIter, &(aResult->mMediaTrackId)) ||
> -      !ReadParam(aMsg, aIter, &(aResult->mMediaType)) ||
> +    if (!aMsg || !aItr || !aReslt ||
> +        !ReadParam(aMsg, aItr, &(aReslt->mSsrc)) ||
> +        !ReadParam(aMsg, aItr, &(aReslt->mMediaType)) ||
> +        !ReadParam(aMsg, aItr, &(aReslt->mMediaTrackId)) ||
> +        !ReadParam(aMsg, aItr, &(aReslt->mTransportId)) ||
> +        !ReadParam(aMsg, aItr, &(aReslt->mCodecId)) ||
> +        !ReadParam(aMsg, aItr, &(aReslt->mFirCount)) ||
> +        !ReadParam(aMsg, aItr, &(aReslt->mNackCount)) ||
> +        !ReadParam(aMsg, aItr, &(aReslt->mPliCount))) {
> +      return ParamTraits<superType>::Read(aMsg, aItr, aReslt);

This only calls superType::Read if reading fails. That seems wrong. Applies throughout.

Have we considered reversing this pattern to:

  return ReadParam(aMsg, aItr, &(aReslt->mSsrc)) &&
         ReadParam(aMsg, aItr, &(aReslt->mMediaType)) &&
         ...,
         ParamTraits<superType>::Read(aMsg, aItr, aReslt));

?

::: dom/media/webrtc/WebrtcGlobal.h:442
(Diff revision 2)
> -  static bool Read(const Message* aMsg, PickleIterator* aIter, paramType* aResult)
> +  static bool Read(const Message* aMsg, PickleIterator* aItr, paramType* aReslt)
>    {
> -    if (!ReadParam(aMsg, aIter, &(aResult->mBytesReceived)) ||
> -        !ReadParam(aMsg, aIter, &(aResult->mDiscardedPackets)) ||
> -        !ReadParam(aMsg, aIter, &(aResult->mFramesDecoded)) ||
> -        !ReadParam(aMsg, aIter, &(aResult->mJitter)) ||
> +    if (!aMsg || !aItr || !aReslt ||
> +        !ReadParam(aMsg, aItr, &(aReslt->mLocalId)) ||
> +        !ReadParam(aMsg, aItr, &(aReslt->mRoundTripTime))) {
> +      return ParamTraits<superType>::Read(aMsg, aItr, aReslt);

s/aReslt/aResult/

::: dom/webidl/RTCStatsReport.webidl
(Diff revision 2)
> -  // Local only measurements, RTCP related but not communicated via RTCP. Not
> -  // present in RTCP case.
>    unsigned long firCount;
>    unsigned long pliCount;
>    unsigned long nackCount;

Hmm, reading this comment makes me wonder: Should these be moved in the spec instead?

Maybe retain the comment if it is still valid, or add a bug # if this is outstanding work?

::: dom/webidl/RTCStatsReport.webidl:42
(Diff revision 2)
> -dictionary RTCInboundRTPStreamStats : RTCRTPStreamStats {
> -  unsigned long packetsReceived;
> +dictionary RTCReceivedRTPStreamStats : RTCRTPStreamStats {
> +    unsigned long      packetsReceived;
> -  unsigned long long bytesReceived;
> +    unsigned long long bytesReceived;
> -  double jitter;
> -  unsigned long packetsLost;
> -  long mozAvSyncDelay;
> +    unsigned long      packetsLost;
> +    double             jitter;
> +    double             fractionLost;

We seem to be using an indent of 2 in this file. Let's keep it consistent whatever it is.

::: dom/webidl/RTCStatsReport.webidl:48
(Diff revision 2)
> -  unsigned long packetsReceived;
> +    unsigned long      packetsReceived;
> -  unsigned long long bytesReceived;
> +    unsigned long long bytesReceived;
> -  double jitter;
> -  unsigned long packetsLost;
> -  long mozAvSyncDelay;
> -  long mozJitterBufferDelay;
> +    unsigned long      packetsLost;
> +    double             jitter;
> +    double             fractionLost;
> +    // unsigned long discardedPackets; TODO @@NG handle backwards compat in JS

You have this implemented in this patch AFAICT, so maybe just a comment rather than a TODO?

::: dom/webidl/RTCStatsReport.webidl:49
(Diff revision 2)
> -  unsigned long long bytesReceived;
> +    unsigned long long bytesReceived;
> -  double jitter;
> -  unsigned long packetsLost;
> -  long mozAvSyncDelay;
> -  long mozJitterBufferDelay;
> -  long roundTripTime;
> +    unsigned long      packetsLost;
> +    double             jitter;
> +    double             fractionLost;
> +    // unsigned long discardedPackets; TODO @@NG handle backwards compat in JS
> +    // packetsDiscarded not yet supported for audio

bug #?

::: dom/webidl/RTCStatsReport.webidl:55
(Diff revision 2)
> +  long          mozAvSyncDelay;       // TODO @@NG remove - create bug
> +  long          mozJitterBufferDelay; // TODO @@NG remove - create bug

bug #

::: dom/webidl/RTCStatsReport.webidl:70
(Diff revision 2)
> -  unsigned long packetsSent;
> +    unsigned long      packetsSent;
> -  unsigned long long bytesSent;
> +    unsigned long long bytesSent;

2 indent

::: dom/webidl/RTCStatsReport.webidl:76
(Diff revision 2)
> -  unsigned long long bytesSent;
> +    unsigned long long bytesSent;
> -  double targetBitrate;  // config encoder bitrate target of this SSRC in bits/s
> +};
> +
> +dictionary RTCOutboundRTPStreamStats : RTCSentRTPStreamStats {
> +  DOMString         remoteId;
> +  double            targetBitrate;  // config encoder bitrate target of this SSRC in bits/s

lose the comment

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:248
(Diff revision 2)
> -          if (s.mIsRemote) {
> -            id = isAudio ? WEBRTC_AUDIO_QUALITY_OUTBOUND_PACKETLOSS_RATE :
> -                           WEBRTC_VIDEO_QUALITY_OUTBOUND_PACKETLOSS_RATE;
> -          } else {
> +          // TODO @@NG
> +          // if (s.mIsRemote) {
> +          //   id = isAudio ? WEBRTC_AUDIO_QUALITY_OUTBOUND_PACKETLOSS_RATE :
> +          //                  WEBRTC_VIDEO_QUALITY_OUTBOUND_PACKETLOSS_RATE;
> +          // } else {

You have some work left moving some telemetry code. I understand it's coming in a separate patch, but I'm opening an issue still so we don't forget.

Note that it's generally frowned on to check in commented out code.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3817
(Diff revision 2)
> -            RTCOutboundRTPStreamStats s;
> +            // TODO @@NG make sure that all fields are being populated
> +            // TODO @@NG change "remoteIds"

Todo

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:1119
(Diff revision 2)
>      for (decltype(array.Length()) i = 0; i < array.Length(); i++) {
>        auto& s = array[i];

I wonder: can we do a

    for (auto& s : query->report->mOutboundRTPStreamStats.Value())
    
here now? I may be wrong. Worth a try.

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 5 • 7 years ago
mozreview-review-reply

Comment on attachment 8892161 [details]
Bug 1380555 - refactor out isRemote;

https://reviewboard.mozilla.org/r/163130/#review168534

> When is this ever falsy?

Yes "var rtcStats = extendTypeWith({}, ..." 
This may be more concise though
newType.types = [...newType.types, ...superTypeDefinition.types || []];

> expected

localId is alway expected, remoteId is only expected if RTCP has arrived

> expected

see above

> There seem to be a lot more conditionals here now. How come, and can they be avoided?

Are you referring to checking the naked pointers before using them?

> Did you try regular inheritance here? Just curious.

There is no way around explicitly specifying the super type that I can think of, and this seems pretty readable.

> This only calls superType::Read if reading fails. That seems wrong. Applies throughout.
> 
> Have we considered reversing this pattern to:
> 
>   return ReadParam(aMsg, aItr, &(aReslt->mSsrc)) &&
>          ReadParam(aMsg, aItr, &(aReslt->mMediaType)) &&
>          ...,
>          ParamTraits<superType>::Read(aMsg, aItr, aReslt));
> 
> ?

I like it. I will fix the superType::read, turning the whole file upside down seems like a job for another patch though, which is fine because I also want to check the raw pointers in the methods I didn't need to touch.

> You have some work left moving some telemetry code. I understand it's coming in a separate patch, but I'm opening an issue still so we don't forget.
> 
> Note that it's generally frowned on to check in commented out code.

I am leaving the issue open for the reason you mentioned, but I have removed the commented code.

> I wonder: can we do a
> 
>     for (auto& s : query->report->mOutboundRTPStreamStats.Value())
>     
> here now? I may be wrong. Worth a try.

Yes, it does!

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8892161 [details]
Bug 1380555 - refactor out isRemote;

https://reviewboard.mozilla.org/r/163130/#review171036

Nice! Missed flipping a few ReadParam's that you're touching anyway due to renaming. Also, don't forget DOM review.

::: dom/media/webrtc/WebrtcGlobal.h:223
(Diff revisions 2 - 3)
> -    if (!ReadParam(aMsg, aItr, &(aReslt->mTransportId)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mLocalCandidateId)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mPriority)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mNominated)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mWritable)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mReadable)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mRemoteCandidateId)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mSelected)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mState)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mBytesSent)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mBytesReceived)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mLastPacketSentTimestamp)) ||
> -        !ReadParam(aMsg, aItr, &(aReslt->mLastPacketReceivedTimestamp)) ||
> -        !ParamTraits<mozilla::dom::RTCStats>::Read(aMsg, aItr, aReslt)) {
> +    if (!ReadParam(aMsg, aItr, &(aResult->mTransportId)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mLocalCandidateId)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mPriority)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mNominated)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mWritable)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mReadable)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mRemoteCandidateId)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mSelected)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mState)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mBytesSent)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mBytesReceived)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mLastPacketSentTimestamp)) ||
> +        !ReadParam(aMsg, aItr, &(aResult->mLastPacketReceivedTimestamp)) ||
> +        !ParamTraits<mozilla::dom::RTCStats>::Read(aMsg, aItr, aResult)) {
>        return false;
>      }
>  
>      return true;

Might as well flip this one as well, since you touched it?

x4

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:1017
(Diff revisions 2 - 3)
>    // the ICE component id for each candidate pair and candidate)
>  
>    std::map<std::string, StreamResult> streamResults;
>  
>    // Build list of streams, and whether or not they failed.
> -  for (size_t i = 0;
> +  for (const auto& pair: query->report->mIceCandidatePairStats.Value()) {

space before :

x4

	Michael Froman [:mjf]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8895467 [details]
Bug 1380555 - P2 - Refactor isRemote from telemetry;

https://reviewboard.mozilla.org/r/166662/#review171838

w/ nits.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:255
(Diff revision 1)
> -        bool isAudio = (s.mId.Value().Find("audio") != -1);
> +        if (s.mPacketsLost.WasPassed() && s.mPacketsReceived.WasPassed() &&
> +            (s.mPacketsLost.Value() + s.mPacketsReceived.Value()) != 0) {
> +              id = isAudio ? WEBRTC_AUDIO_QUALITY_OUTBOUND_PACKETLOSS_RATE :
> +                             WEBRTC_VIDEO_QUALITY_OUTBOUND_PACKETLOSS_RATE;
> +              Accumulate(id,
> +                        (s.mPacketsLost.Value() * 1000) /
> +                        (s.mPacketsLost.Value() + s.mPacketsReceived.Value()));
> +
> +        }

Is there any way to abstract this since it is mostly duplicated for the Inbound stats below?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:269
(Diff revision 1)
> +        if (lastRemoteInboundStats && s.mBytesReceived.WasPassed()) {
> +          auto& laststats = *lastRemoteInboundStats;
> +          auto i = FindId(laststats, s.mId.Value());
> +          if (i != laststats.NoIndex) {
> +            auto& lasts = laststats[i];
> +            if (lasts.mBytesReceived.WasPassed()) {
> +              auto delta_ms = int32_t(s.mTimestamp.Value() -
> +                                      lasts.mTimestamp.Value());
> +              // In theory we're called every second, so delta *should* be in that range.
> +              // Small deltas could cause errors due to division
> +              if (delta_ms > 500 && delta_ms < 60000) {
> +                id = isAudio ? WEBRTC_AUDIO_QUALITY_OUTBOUND_BANDWIDTH_KBITS :
> +                                WEBRTC_VIDEO_QUALITY_OUTBOUND_BANDWIDTH_KBITS;
> +                Accumulate(id, ((s.mBytesReceived.Value() -
> +                                 lasts.mBytesReceived.Value()) * 8) / delta_ms);
> +              }
> +              // We could accumulate values until enough time has passed
> +              // and then Accumulate() but this isn't that important.
> +            }
> +          }
> +        }

Also, mostly duplicated below in the inbound stats.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:319
(Diff revision 1)
>                auto delta_ms = int32_t(s.mTimestamp.Value() -
>                                        lasts.mTimestamp.Value());
>                // In theory we're called every second, so delta *should* be in that range.
>                // Small deltas could cause errors due to division
>                if (delta_ms > 500 && delta_ms < 60000) {
>                  HistogramID id;

I think you missed removing this occurance of HistogramId.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8895497 [details]
Bug 1380555 - refactor out isRemote;

https://reviewboard.mozilla.org/r/166698/#review171922

	Michael Froman [:mjf]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8897182 [details]
Bug 1380555 - P2 - Refactor isRemote from telemetry;

https://reviewboard.mozilla.org/r/168468/#review174086

Looks good to me after adding the comment below!

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:279
(Diff revision 1)
> -        auto& s = array[i];
> -        bool isAudio = (s.mId.Value().Find("audio") != -1);
> +        bool isAudio = mediaIsAudio(s);
> +        if (s.mPacketsLost.WasPassed() && s.mPacketsReceived.WasPassed() &&
> +            (s.mPacketsLost.Value() + s.mPacketsReceived.Value()) != 0) {
> +              id = isAudio ? WEBRTC_AUDIO_QUALITY_OUTBOUND_PACKETLOSS_RATE :
> +                             WEBRTC_VIDEO_QUALITY_OUTBOUND_PACKETLOSS_RATE;
> +              Accumulate(id,

I would add the same comment from the packets lost section for recording inbound stats here (or abstract this code like you did for recordAudioBandwidth).

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 14 • 7 years ago

Spec Document: http://rawgit.com/w3c/webrtc-stats/master/webrtc-stats.html

Webidl review notes:

discaredPackets is cloned into packetsDiscarded for now, a warning will be coming, and eventually discaredPackets be removed

When called with the legacy getStats call back API:
 * the old type names are still used
 * isRemote is inserted
 * if localId is present it is duplicated into remoteId

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8898951 [details]
Bug 1380555 - refactor out isRemote;

https://reviewboard.mozilla.org/r/170304/#review175522

	Bulk Bug Changes for mreavy's org
	Comment 18 • 7 years ago

Mass change P1->P2 to align with new Mozilla triage process

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 19 • 6 years ago

Target for this is 65 (bug 1393306 added warnings that say so).

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 21 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e04bc2a495062e0d9edeb55d82ec261fe74cbe09

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 22 • 5 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d8628f026a76fd10dd7f2b9c3066eeacd1b00cb

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 23 • 5 years ago

Removes RTP stat field isRemote and adds the new types remote-inbound-rtp, and remote-outbound-rtp

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 24 • 5 years ago

Spec Review Notes:
This changeset removes the isRemote field of the WebRTC RTP Stats [0], and instead uses the new method of indicating remoteness with the type[1][2] field. This also necessitates the addition of the localId [3][4] field. Bug 1515716 has been filed as a follow up to more fully refactor the types and move remoteId and localId into their respective new dictionaries.

[0] https://w3c.github.io/webrtc-stats/#streamstats-dict*
[1] https://www.w3.org/TR/webrtc/#dom-rtcstats-type
[2] https://w3c.github.io/webrtc-stats/#rtcstatstype-str*
[3] https://w3c.github.io/webrtc-stats/#remoteoutboundrtpstats-dict*
[4] https://w3c.github.io/webrtc-stats/#remoteinboundrtpstats-dict*

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 25 • 5 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c25b008c824e74fd2f6be4e87eb74f4e30d9a6f3

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 26 • 5 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6248d649b9a132406b31fcab083776e18bafad66

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 27 • 5 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ed16c2976920a000394f52fe0383a7589ffc5aa

	Pulsebot
	Comment 28 • 5 years ago

Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/ccb218cd2d87
remove deprecated WebRTC RTP stat isRemote in favor of new stat types r=jib,smaug

	Narcis Beleuzu [:NarcisB]
	Comment 29 • 5 years ago

Backed out for mozlint failure on PeerConnection.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/3d48e9c1530eb7446e01c1b68121e733fa848173
Push link: https://hg.mozilla.org/integration/autoland/rev/ccb218cd2d8781509264669b290fbdee586ff513
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218267860&repo=autoland&lineNumber=272

	Pulsebot
	Comment 30 • 5 years ago

Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/ccca038b9272
remove deprecated WebRTC RTP stat isRemote in favor of new stat types r=jib,smaug

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 31 • 5 years ago

Fixed the lint issue, re-opened the phabricator revision, and re-auto-landing.

	Eliza Balazs [:ebalazs_]
	Comment 32 • 5 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ccca038b9272

	Kohei Yoshino [:kohei]
	Comment 33 • 5 years ago

Posted site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/legacy-peerconnection-getstats-support-has-been-removed/