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

Fixed BW measurement. Prevented from measuring between non-monotonic or rexmitted packets. #938

Merged

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Nov 4, 2019

The original bandwidth measurement states that 16th and 17th packets in order are sent without delay between one another so that the difference in arrival time between 17th and 16th packet in order can suggest the highest possible potential sender speed, which is the absolute maximum link bandwidth.

This measurement is completely blown, however, in cases when:

  • the probe 2 triggered (on the "17th packet"), but there was no 16th packet preceding it received yet
  • the 16th packet, which's arrival time has been recorded, is not the direct predecessor of the 17th packet that has triggered the probe 2 - the measurement is then taken between unrelated packets
  • the 17th packet has been retransmitted and therefore the distance measured in such a case is completely out of the blue

Even though the bandwidth measurement is prepared to eliminate those values as too much differing to the average, relying on that can be slick, especially if the original state is such that no measurement has been taken yet, or when momentarily the gap between 16th and 17th happens to be occasionally extremely big and therefore an "out of the blue" measurement doesn't seem to be so distant to the valid values.

This fix simply prevents taking any measurements that would not be conducted on exactly two consecutive 16th and 17th packets in order.

@ethouris ethouris added [core] Area: Changes in SRT library core Priority: Medium Status: Review Needed Type: Maintenance Work required to maintain or clean up the code labels Nov 4, 2019
@ethouris ethouris added this to the v1.4.1 milestone Nov 4, 2019
srtcore/core.cpp Outdated Show resolved Hide resolved
srtcore/window.h Outdated Show resolved Hide resolved
srtcore/window.h Outdated Show resolved Hide resolved
srtcore/window.h Outdated Show resolved Hide resolved
srtcore/window.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the code is perfect except for one condition might not be handled well.
Say packet A is (pkt.m_iSeqNo & PUMASK_SEQNO_PROBE) == 0
Packet B is (pkt.m_iSeqNo & PUMASK_SEQNO_PROBE) == 1

Process the following order of receiving:
Packet A - original
Packet A - re-transmitted
Packet B - original

In this case we can't consider packet B as a probing packet.

srtcore/window.h Outdated
Comment on lines 269 to 273
if ( // probe1Arrival wasn't called yet, no start point then
m_Probe1Sequence == -1
// not very next towards lately recorded 16th
|| CSeqNo::incseq(m_Probe1Sequence) != pkt.m_iSeqNo
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened with formatting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to somehow add comments by // to the particular condition being tested.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the comments out of the if() statement. It is not very hard to reword the comment so that it can correlate to relevant parts of the conditions within the if().

srtcore/window.h Outdated
Comment on lines 269 to 273
if ( // probe1Arrival wasn't called yet, no start point then
m_Probe1Sequence == -1
// not very next towards lately recorded 16th
|| CSeqNo::incseq(m_Probe1Sequence) != pkt.m_iSeqNo
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the comments out of the if() statement. It is not very hard to reword the comment so that it can correlate to relevant parts of the conditions within the if().

@rndi rndi merged commit db9dc76 into Haivision:master Nov 7, 2019
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 Priority: Medium Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants