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

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

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

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 3 • 7 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).

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 4 • 7 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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 6 • 7 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;
    }

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

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 9 • 7 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?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 10 • 7 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/

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

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

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 13 • 7 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.

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

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

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

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

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 7 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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 18 • 7 years ago

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

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 19 • 7 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.

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

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

	Pulsebot
	Comment 24 • 7 years ago

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

	Carsten Book [:Tomcat]
	Comment 25 • 7 years ago
bugherder

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

	Randell Jesup [:jesup] (needinfo me)
	Comment 27 • 7 years ago

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

	Gerry Chang [:gchang]
	Comment 28 • 7 years ago

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

	Kohei Yoshino [:kohei]
	Comment 29 • 7 years ago

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

	Eric Shepherd [:sheppy]
	Comment 30 • 7 years ago

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