1344970 - Rename mozRtt stat to roundTripTime and change behavior to match spec

Status: RESOLVED FIXED

(Core :: WebRTC, enhancement, P1)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Given bug 1241066 we can probably skip the backwards comp for mozRtt...

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1344970 - rename mozRtt to roundTripTime

Review commit: https://reviewboard.mozilla.org/r/118138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118138/

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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).

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118138/diff/1-2/

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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;
    }

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Reminder to self to un-bit rot this, and get webidl review.

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118138/diff/2-3/

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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/

Comment 11

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

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

Comment 12

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118138/diff/3-4/

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 14

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118138/diff/4-5/

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

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

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

wth is up with that try build? https://treeherder.mozilla.org/#/jobs?repo=try&revision=0139e8f6c433&selectedJob=87790033

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

(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.

Comment 19

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(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 20

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 21

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118138/diff/5-6/

Comment 22

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118138/diff/6-7/

Comment 23

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8844810 [details]
Bug 1344970 - rename mozRtt to roundTripTime

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118138/diff/7-8/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/485ce0191284
rename mozRtt to roundTripTime r=jib,smaug

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/485ce0191284

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Gerry - (re the affected -> --- change) wouldn't "WONTFIX" be more appropriate for changes of this sort?

Comment 28

[Gerry Chang [:gchang]]

Hi :jesup,
Yes. Mark 54 won't fix and let this ride the train.

Comment 29

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/rtcinboundrtpstreamstats-mozrtt-has-been-unprefixed/

Comment 30

[Eric Shepherd [:sheppy]]

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