Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RoundTripTime not defined when the underlying stream can't calculate it #180

Closed
jesup opened this issue Mar 6, 2017 · 10 comments · Fixed by #188
Closed

RoundTripTime not defined when the underlying stream can't calculate it #180

jesup opened this issue Mar 6, 2017 · 10 comments · Fixed by #188
Assignees

Comments

@jesup
Copy link

jesup commented Mar 6, 2017

RoundTripTime can only be calculated after an exchange of RTCP packets with the relevant information. Until then, it's not possible to calculate. What should be RoundTripTime have before we know the time? 0? undefined? null? And what will be the impact on existing applications if we choose something like undefined or null?

@vr000m
Copy link
Contributor

vr000m commented Mar 6, 2017

@jesup: I think at the TPAC the decision was to use undefined if it is not measured. and 0 or any good starting value, if it is measuring but doe not have a measurement.

I think -1 would make sense in this case.

@vr000m
Copy link
Contributor

vr000m commented Mar 6, 2017

for deployments when no RTCP RR is sent/received (some legacy middleboxes do not send RTCP RRs, although send RTCP SRs), I think it will remain -1.

@jan-ivar
Copy link
Member

jan-ivar commented Mar 7, 2017

We just moved roundTripTime to the isRemote == true dictionary which doesn't exist until RTCP packlets come in.

@henbos
Copy link
Collaborator

henbos commented Mar 7, 2017

If we don't have a value for it (because we haven't exchanged packets) then it should be undefined, not -1.

@jan-ivar
Copy link
Member

jan-ivar commented Mar 7, 2017

Again, there should never be a need for either, since the whole dictionary will be absent in that case, and the associate stats dictionary's associateStatsId (remoteId in Firefox atm) will be undefined.

That's my best reading of the spec, and is how Firefox implements it.

@henbos
Copy link
Collaborator

henbos commented Mar 7, 2017

Oh right, that makes sense.

@jan-ivar
Copy link
Member

jan-ivar commented Mar 7, 2017

@jesup schooled me on the round-trip time calculation, and it turns out it can't be calculated until an RR comes in with data based off a remotely-receivecd SR. In other words, not necessarily the first RR.

So based on the same reading of the stats spec I mentioned above I think it makes sense for the roundTripTime member to be missing (effectively undefined) until a proper value is available. Does that make sense?

@henbos
Copy link
Collaborator

henbos commented Mar 8, 2017

Yes.

@jesup
Copy link
Author

jesup commented Mar 8, 2017

I agree - missing/undefined

@vr000m
Copy link
Contributor

vr000m commented Mar 14, 2017

I will fix it.

@vr000m vr000m self-assigned this Mar 14, 2017
vr000m pushed a commit that referenced this issue Mar 22, 2017
alvestrand pushed a commit that referenced this issue Mar 30, 2017
* fixes #180, rtt undefined

* addressing @jesup's comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants