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

[core] check timestamp carryover for keepalive packets #1895

Closed
wants to merge 1 commit into from

Conversation

gou4shi1
Copy link
Contributor

If client keepalive for more then 1h10m, then server will not know the timestamp has been carryover.

@gou4shi1 gou4shi1 closed this Mar 29, 2021
@gou4shi1 gou4shi1 deleted the gou4shi1_master branch March 29, 2021 05:41
@gou4shi1 gou4shi1 restored the gou4shi1_master branch March 29, 2021 05:44
@gou4shi1 gou4shi1 reopened this Mar 29, 2021
@maxsharabayko maxsharabayko added this to the v1.4.3 milestone Mar 29, 2021
@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior labels Mar 29, 2021
@maxsharabayko
Copy link
Collaborator

Good catch!
If a client is sending keepalive for more than 1h10m and does not receive any data, another problem would be to compensate clock drift once the transmission is resumed. 🤔

@gou4shi1
Copy link
Contributor Author

So we should also addRcvTsbPdDriftSample() for keepalive packets?

@ethouris
Copy link
Collaborator

No, this won't help. The problem of keepalive packets is that it's one direction only and unreliable as well, while in order to update the drift you need to have some request-response cycle.

I think that it might be needed that in the situation of stopped transmission and sending keepalives the transmission start time shall be reset to 0 and set again only at the first received packet. This time always serves as a time pattern of the expected arrival time, so such a time reset shall restore time conditions to "zero drift" again. The problem might be though that this method need not be reliable, and of course, this must be also treated exceptionally differently for backup groups.

@gou4shi1
Copy link
Contributor Author

gou4shi1 commented Apr 13, 2021

My fix will cause the "No room" issue...
Can you provide a hotfix without drift?
I don't care about drift since I disabled drift tracer (which will cause the "No room" issue).

@maxsharabayko
Copy link
Collaborator

My fix will cause the "No room" issue...

Do you mean that the fix you've provided does not help in your use case?

The "no room to store.." will likely be caused by clock drift, accumulated while the link is idle.
Note. The error message is to be improved in #1909 .

@maxsharabayko maxsharabayko modified the milestones: v1.4.3, v1.4.4 Apr 19, 2021
@gou4shi1
Copy link
Contributor Author

Do you mean that the fix you've provided does not help in your use case?

It should help, but with this fix, srt report "No room..." frequently

The "no room to store.." will likely be caused by clock drift, accumulated while the link is idle.

so I have disabled the drift tracer long time ago

Note. The error message is to be improved in #1909 .

Thanks!

@gou4shi1
Copy link
Contributor Author

Anyway, I have workaround this issue with a hack:
if the connection is connected for more than 1h and idle for more than 5s, reconnect it :)

@gou4shi1 gou4shi1 closed this May 31, 2021
@gou4shi1 gou4shi1 deleted the gou4shi1_master branch May 31, 2021 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants