Rename mozRtt stat to roundTripTime and change behavior to match spec

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
Rank:
19
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jib, Assigned: ng)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

()

Attachments

(1 attachment)

Now that the roundTripTime has moved to the right dictionary [1] (matching our implementation) we should rename mozRtt to it.

We might leave mozRtt as a (non-enumerable in the legacy API) deprecation-warning-spouting property for a release or two.

[1] https://github.com/w3c/webrtc-stats/pull/167
Rank: 19
Priority: -- → P1
Given bug 1241066 we can probably skip the backwards comp for mozRtt...
Comment hidden (mozreview-request)
Reporter

Comment 3

2 years ago
mozreview-review
Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

https://reviewboard.mozilla.org/r/118138/#review120070

This goes a tiny bit further than renaming, fixing a bug, which I think is the right thing to if we're going to match the spec.

But in that vain I think we should also make sure to not construct the roundTripTime member if rtt is absent. Also, we should distinguish absence (-1) from an actual zero reported, which while I suppose an rtt of 0 is theoretically impossible, for debugging we want to represent truthfully what packets are reporting. 

I also found a nit in related code that already landed. Will revert to "r+ with nits" mode-of-operation once that is taken care of. ;)

Otherwise looks good!

Note you'll need a DOM reviewer (smaug?) for the webidl change. Be sure to point him to githhub tip [1] or he won't find the member in the right place.

[1] http://rawgit.com/w3c/webrtc-stats/master/webrtc-stats.html

::: commit-message-5891f:1
(Diff revision 1)
> +Bug 1344970 - rename mozRtt is roundTripTime;r?jib

s/is/to/

::: dom/media/tests/mochitest/test_peerConnection_stats.html:354
(Diff revision 1)
>    let ensureRtcp = async () => pc.getStats().then(stats => {
>      for (let [k, v] of stats) {

After the r+ with nits I gave in bug 1337525 comment 37, it looks like I missed that you went with `async` but forgot to use `await` here. We should pick a paradigm, i.e. and change to:

    let ensureRtcp = async () => {
      let stats = await pc.getStats();

::: dom/media/tests/mochitest/test_peerConnection_stats.html:360
(Diff revision 1)
> +      if (v.type == "inbound-rtp" && v.isRemote == true
> +          && v.roundTripTime === undefined) {
> +        throw new Error(v.id + " is missing roundTripTime: "
> +          + JSON.stringify(v));
> +      }

"Do not compare x == true or x == false."

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices

Also, maybe s/waitForRtcp/waitForSyncedRtcp/ and s/ensureRtcp/ensureSyncedRtcp/ ?

::: dom/media/tests/mochitest/test_peerConnection_stats.html:379
(Diff revision 1)
>      } catch (e) {
>        info(e);
>        await wait(waitPeriod);
>      }
>    }
> -  throw new Error("Waiting for RTCP timed out after at least " + totalTime
> +  throw new Error("Waiting for RTCP timed out after at least " + maxTime

Maybe "Waiting for synced RTCP ..." ?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:828
(Diff revision 1)
> +    *rttMs = (rtt > 0) ? // -1 indicates that RTT is unavailable
> +      static_cast<int32_t>(std::min<int64_t>(rtt, INT32_MAX)) : 0;

A potential problem here is this fails to distinguish a report of 0 rtt (can it ever happen??) from absence. In the interest of proper debugging, I think we want to be strict here.

Maybe either have GetRTCPReceiverReport return rtt == -1 to indicates absence, or refactor into a separate GetRoundtripTime function?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3654
(Diff revision 1)
> -            s.mMozRtt.Construct(rtt);
> +            // RTT is 0 when unavailable
> +            s.mRoundTripTime.Construct(rtt);

We should not construct this member if rtt is zero.

That seems to be doing right by the spec, and also prevents zeros from polluting telemetry (which uses .WasPassed() elsewhere in this patch).
Attachment #8844810 - Flags: review?(jib) → review-
Assignee

Comment 4

2 years ago
mozreview-review-reply
Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

https://reviewboard.mozilla.org/r/118138/#review120070

> A potential problem here is this fails to distinguish a report of 0 rtt (can it ever happen??) from absence. In the interest of proper debugging, I think we want to be strict here.
> 
> Maybe either have GetRTCPReceiverReport return rtt == -1 to indicates absence, or refactor into a separate GetRoundtripTime function?

The audio side returns zero to indicate absence because that is what webrtc.org uses internally. I don't think it can ever return 0 as a valid number RTT.
Comment hidden (mozreview-request)
Reporter

Comment 6

2 years ago
mozreview-review
Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

https://reviewboard.mozilla.org/r/118138/#review120142

Looks good, except the await is still mixed up.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:354
(Diff revisions 1 - 2)
> -  let ensureRtcp = async () => pc.getStats().then(stats => {
> +  let ensureSyncedRtcp = async () => {
> +      return await pc.getStats().then(stats => {
> -    for (let [k, v] of stats) {
> +      for (let [k, v] of stats) {

This should be:

    let ensureSyncedRtcp = async () => {
      let stats = await pc.getStats();
      for (let [k, v] of stats) {
        ...
      }
      return stats;
    }
Attachment #8844810 - Flags: review?(jib) → review-
Assignee

Updated

2 years ago
Summary: Rename mozRtt stat to roundTripTime to match spec. → Rename mozRtt stat to roundTripTime and change behavior to match spec
Assignee

Comment 7

2 years ago
Reminder to self to un-bit rot this, and get webidl review.
Flags: needinfo?(na-g)
Comment hidden (mozreview-request)
Reporter

Comment 9

2 years ago
mozreview-review
Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

https://reviewboard.mozilla.org/r/118138/#review128074

Bah, rebases with changes are a paint to review in mozReview, but this code looks confused now about whether -1 or 0 is returned when rtt is absent.

::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h:269
(Diff revision 3)
> +  // Upon return rttMs must be -1 if RTT is unavailable.
>    virtual bool GetRTCPReceiverReport(DOMHighResTimeStamp* timestamp,

I think this is true anymore, as you've removed all code for it, so let's remove the comment.

Also, if you change the comment to s/-1/0/, let's not imply secondary results are valid on failure, as that's a bug-prone pattern IMHO.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3643
(Diff revision 3)
> -            s.mMozRtt.Construct(rtt);
> +            // RTT is -1 when unavailable
> +            if (rtt != -1) {

Where's the code that ensures this now?
Attachment #8844810 - Flags: review?(jib) → review-
Reporter

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

https://reviewboard.mozilla.org/r/118138/#review128074

> I think this is true anymore, as you've removed all code for it, so let's remove the comment.
> 
> Also, if you change the comment to s/-1/0/, let's not imply secondary results are valid on failure, as that's a bug-prone pattern IMHO.

s/think/don't think/
Assignee

Comment 11

2 years ago
mozreview-review
Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

https://reviewboard.mozilla.org/r/118138/#review128170
Comment hidden (mozreview-request)
Reporter

Comment 13

2 years ago
mozreview-review
Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

https://reviewboard.mozilla.org/r/118138/#review128176

Almost there, just one question.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:201
(Diff revision 4)
>    bool result = !mPtrRTP->GetRemoteRTCPReceiverInfo(mChannel, ntpHigh, ntpLow,
>                                                      *packetsReceived,
>                                                      *bytesReceived,
>                                                      *jitterMs,
>                                                      fractionLost,
>                                                      *cumulativeLost,
>                                                      *rttMs);
>    if (!result) {
>      return false;
>    }
> +  if (*rttMs < 0) {
> +     *rttMs = 0;
> +  }

Why is this needed?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:843
(Diff revision 4)
> -    int64_t rtt = mSendStream->GetRtt(); // TODO: BUG 1241066, mozRtt is 0 or 1
> -    if (rtt >= 0) {
> +    int64_t rtt = mSendStream->GetRtt();
> +    if (rtt > 0) {
>        *rttMs = rtt;
>      } else {
>        *rttMs = 0;
>      }
>  #ifdef DEBUG
>      if (rtt > INT32_MAX) {
>        CSFLogError(logTag,
>          "%s for VideoConduit:%p mRecvStream->GetRtt() is larger than the"
>          " maximum size of an RTCP RTT.", __FUNCTION__, this);
>      }
>  #endif

Maybe move this logging above the clamp-and-transfer? Then using `rtt` won't feel wrong.
Attachment #8844810 - Flags: review?(jib) → review-
Comment hidden (mozreview-request)
Reporter

Comment 15

2 years ago
mozreview-review
Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

https://reviewboard.mozilla.org/r/118138/#review128208
Attachment #8844810 - Flags: review?(jib) → review+

Comment 17

2 years ago
mozreview-review
Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

https://reviewboard.mozilla.org/r/118138/#review128222

r+ for the .webidl. Seems like stuff coming from the spec.
Attachment #8844810 - Flags: review+
Assignee

Updated

2 years ago
Flags: needinfo?(na-g)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #16)
> wth is up with that try build?
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0139e8f6c433&selectedJob=87790033

Looks like a bad base for the try.
Assignee

Comment 19

2 years ago
(In reply to Randell Jesup [:jesup] from comment #18)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #16)
> > wth is up with that try build?
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=0139e8f6c433&selectedJob=87790033
> 
> Looks like a bad base for the try.

Yeah, I am rebasing and rebuilding locally to see if I can't find a newer working rev to rebase onto. The only delay is the long Windows build times.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/485ce0191284
rename mozRtt to roundTripTime r=jib,smaug

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/485ce0191284
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee

Updated

2 years ago
Duplicate of this bug: 1344946
Gerry - (re the affected -> --- change) wouldn't "WONTFIX" be more appropriate for changes of this sort?
Flags: needinfo?(gchang)
Hi :jesup,
Yes. Mark 54 won't fix and let this ride the train.
Flags: needinfo?(gchang)
Added a note to Firefox 55 for developers about this. I also mention bug 1380555, which will rename this.

https://developer.mozilla.org/en-US/Firefox/Releases/55#WebRTC
You need to log in before you can comment on or make changes to this bug.