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

Higher googRTT values between 0.2.2 and 0.2.3 #982

Closed
whitemonkey opened this Issue Aug 24, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@whitemonkey
Copy link
Contributor

whitemonkey commented Aug 24, 2017

Upgrading to version 0.2.5 from 0.2.1 we noticed that googRTT times became noticeably higher. We tested other versions and found the change happened between 0.2.2 and 0.2.3. Using 0.2.2 our RTT times were pretty consistent at about 4ms; switching to 0.2.3 brings the times up considerably, with a lot more spikes.

rtt0 2 3 2
rttok

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Aug 25, 2017

Possibly some RTCP related thing, but not sure what. If you can narrow down the exact commit that breaks your googRTT evaluation it might help.

@whitemonkey

This comment has been minimized.

Copy link
Contributor

whitemonkey commented Sep 1, 2017

git bisect shows that this is the commit: edbdcdd

this change fixes it:

diff --git a/rtcp.c b/rtcp.c
index 5dd445a..07bf939 100644
--- a/rtcp.c
+++ b/rtcp.c
@@ -541,7 +541,7 @@ int janus_rtcp_report_block(rtcp_context *ctx, report_block *rb) {
        rb->flcnpl = htonl(lost | fraction);
        if(ctx->lsr > 0) {
                rb->lsr = htonl(ctx->lsr);
-               rb->delay = htonl(((now - ctx->lsr_ts) / 1000000) << 16);
+               rb->delay = htonl(((now - ctx->lsr_ts) << 16) / 1000000);
        } else {
                rb->lsr = 0;
                rb->delay = 0;```
@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Sep 1, 2017

Thanks for spotting that!

Mh, can't remember exactly the rationale behind that change, but I think it was because this seemed to be more accurate when computing the delay value, which may or may not have been indeed true. I'll have to make some tests printing both values, to see if one of the two is more "raw" than the other for some reason.

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Sep 1, 2017

PS: have you checked if the higher googRTT is indeed closer to the reality of the RTT?

@furqanrydhan

This comment has been minimized.

Copy link

furqanrydhan commented Sep 1, 2017

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Sep 1, 2017

Ack, will merge the change then, thanks for the feedback!

@lminiero lminiero closed this in #989 Sep 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment