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] Fixed the issue with RTT in case of bidirectional transmission #2210

Conversation

mbakholdina
Copy link
Collaborator

Fixed the issue with RTT in case of bidirectional transmission introduced when adding atomic types in PR #1863.
I've also checked all the atomic related changes. They are not affecting RTT logic anywhere else luckily.

Replaces PR #2208.

@mbakholdina mbakholdina added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Dec 10, 2021
@mbakholdina mbakholdina added this to the v1.4.5 milestone Dec 10, 2021
@mbakholdina mbakholdina self-assigned this Dec 10, 2021
@maxsharabayko maxsharabayko merged commit 26678fe into Haivision:master Dec 13, 2021
@gou4shi1
Copy link
Contributor

If the received rtt happens to equal to INITIAL_RTT, it will be ignored?

@mbakholdina
Copy link
Collaborator Author

Hi @gou4shi1,

In the case of bidirectional transmission smoothed RTT m_iSRTT and RTT variance m_iRTTVar are updated by both functions:

  • processCtrlAck upon ACK packet reception
  • processCtrlAckAck upon ACKACK packet reception (at the same time the new RTT estimate is calculated from the ACK/ACKACK pair)

as in this case the same SRT peer functions as both receiver and sender.

That's is why the initial values of 100 and 50 ms for m_iSRTT and m_iRTTVar should be ignored in this piece of code so that they are not taken into account. Imagine, your real RTT is far from 100 ms, let's say 20 ms.

Screenshot 2021-12-25 at 15 20 36

The probability of RTT estimate being exactly 100 ms at the receiver side, or smoothed RTT extracted from the ACK and being exactly 100 ms at the sender side, is equal to 0. That's why if we receive such a value, we can assume that this is an initial value set at the beginning of the transmission INITIAL_RTT and it should be ignored. The same is true for RTTVar.

If your real RTT of a path is close to 100 ms, there is a really small chance (almost close to 0) that you will get such a value and it wouldn't be INITIAL_RTT. Fine, I guess we can live with skipping the only one value in this case to not overcomplicate the code :)

@gou4shi1
Copy link
Contributor

Hi @mbakholdina ,
Thanks for your detailed explanation, would if (rtt != INITIAL_RTT || rttvar != INITIAL_RTTVAR) is better?

@mbakholdina
Copy link
Collaborator Author

mbakholdina commented Jan 17, 2022

Hi @gou4shi1,

I guess it doesn't make sense to change && to || in this if-condition, because our main goal is to catch the initial values INITIAL_RTT and INITIAL_RTTVAR which are set to 100 and 50 ms at the beginning of a transmission and are sent from receiver to sender in ACK packets until there is no opportunity to calculate RTT samples on the receiver side (I mean from ACK/ACKACK pairs). The calculation of the very first RTT sample at the receiver side happens approximately in 1.5RTT + SYN ms (where SYN = 10 ms is the period of sending ACK packets) since the beginning of a transmission and is available at the sender side after 2RTT + SYN ms approximately.

Returning to your previous example, if it happens during transmission that RTT sample is exactly 100 ms (not equal to INITIAL_RTT, but calculated from ACK/ACKACK pair) while RTTVar isn't 50 ms as it's constantly recalculated, || will skip the smoothed RTT m_iSRTT recalculation in this case what shouldn't be done.

So, I am trying to catch exactly 100 ms (INITIAL_RTT) and 50 ms (INITIAL_RTTVAR) in this case to skip smoothed RTT recalculation only in the beginning of a transmission.

One more thing that's worth mentioning. If there is something stored in cache from the previous connection on the same IP address and port, the very first value of smoothed RTT m_iSRTT will be taken from cache (not set to INITIAL_RTT) which is the last smoothed RTT calculated during previous transmission. This is helpful when reconnection is happening, e.g. m_iRTTVar is simply taken as half m_iSRTT in this case.

@gou4shi1
Copy link
Contributor

So, I am trying to catch exactly 100 ms (INITIAL_RTT) and 50 ms (INITIAL_RTTVAR) in this case to skip smoothed RTT recalculation only in the beginning of a transmission.

Sorry, maybe I understand wrong, correct me if I'm wrong :)
So the intention is if (not at beginning of transmission) then recalculate smoothed RTT,
which is if (! (rtt== INITIAL_RTT && rttvar == INITIAL_RTTVAR)) recalculate_smoothed_rtt();.

While (! (rtt== INITIAL_RTT && rttvar == INITIAL_RTTVAR)) should be (rtt != INITIAL_RTT || rttvar != INITIAL_RTTVAR), instead of (rtt != INITIAL_RTT && rttvar != INITIAL_RTTVAR)

@mbakholdina
Copy link
Collaborator Author

Hi @gou4shi1,

Finally, I've got some time to look into this. I agree, we can safely change the condition to the one you are suggesting:
if (rtt != INITIAL_RTT || rttvar != INITIAL_RTTVAR) then recalculate smoothed_rtt;

I include here the table with possible combinations and notes for future reference.

rtt != INITIAL_RTT rttvar != INITIAL_RTTVAR Result Note
0 0 0 rtt is exactly 100 ms, rttvar is exactly 50 ms. Skip smoothed_rtt calculation.
0 1 1 The RTT estimate extracted from the ACK packet is exactly 100 ms while RTTVar isn't 50 ms. The probability of having this situation is close to 0, but we can recalculate smoothed_rtt in this case, I agree. Those values can't be from cache as the rttvar should be exactly 50 ms in this case.
1 0 1 The RTT estimate is something different from 100 ms (it's either calculated at the receiver side value, or the value extracted from cache). rttvar is exactly 50 ms. The probability of having this situation is close to 0, normally there should be some decimal places in rttvar. Those values can't be values from cache, in this case there should be decimal places in rttvar. But if it happens, we can recalculate smoothed_rtt, I agree.
1 1 1 Both values are different from their initial values. The case I was catching initially. Recalculate smoothed_rtt.

I'll prepare a PR to make the change. Thanks for catching this!

Best regards,
Maria

@gou4shi1
Copy link
Contributor

gou4shi1 commented Feb 1, 2022

I include here the table with possible combinations and notes for future reference.

Great analysis!

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