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] WIP: Extending ACK with RCV drop total #1889

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Mar 24, 2021

Problem: Sender does not know if and how many packets are being dropped by the receiver. SRT receiver, dropping a packet, just sends a regular ACK control packet, as if it had received them.

Proposed Solution: Extend ACK control packet to contain information about the number of packets dropped by the receiver: "Total Dropped Packets Count" field.

    0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+- SRT Header +-+-+-+-+-+-+-+-+-+-+-+-+-+
0  |1|        Control Type         |           Reserved            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
1  |                    Acknowledgement Number                     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
2  |                           Timestamp                           |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
3  |                     Destination Socket ID                     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+- CIF -+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
4  |            Last Acknowledged Packet Sequence Number           |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
5  |                              RTT                              |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
6  |                          RTT Variance                         |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
7  |                     Available Buffer Size                     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
8  |                     Packets Receiving Rate                    |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
9  |                     Estimated Link Capacity                   |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
10 |                         Receiving Rate                        |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
11 |                   Total Dropped Packets Count                 | <-- PROPOSED FIELD!!!
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Total Dropped Packets Count - the total number of packets dropped by the receiver
from the start of receiving. Only sent if differs from the previous value,
sent in previous ACK packets, that were acknowledged by the sender (ACKACK).
In other words, included in ACK packet until ACKACK is received acknowledging that the sender has received this number of dropped packets.

Older SRT versions will ignore this field (maximum size is not restricted). Also receiver knows sender’s SRT version, and can only send an extended ACK packet for newer versions.
ACKACK control packet: no changes are required. The structure remains the same.

Advantages

  • Easy to implement. Very straightforward improvement of the protocol.
  • Allows sender to know the number of packets dropped by the receiver, persistent to a loss of ACK packet.
  • Works with prior SRT versions by checking peer SRT version and not extending the ACK. However, older SRT will ignore the additional field (except for v1.0.2 which expects ACK extended with XMRATE).

Disadvantages

  • Does not let the sender know which exactly packets were dropped. Meaning that the group sender will not have a chance to estimate the number of packets dropped by the group receiver.

TODO

  • Add API statistics (note that extending statistics API breaks ABI compatibility between versions).
  • Do not include the Total Dropped Packets Count field if the value hasn't changed and the previous value was already received by sender (receiver got ACACK for ACK with that value).
  • Handle possible uint32_t overflow on the sender side.

@maxsharabayko maxsharabayko added Type: Enhancement Indicates new feature requests [core] Area: Changes in SRT library core labels Mar 24, 2021
@maxsharabayko maxsharabayko added this to the v1.4.4 milestone Mar 24, 2021
@maxsharabayko maxsharabayko self-assigned this Mar 24, 2021
@ethouris
Copy link
Collaborator

Why can't this be simply the number of dropped packets among those being ACK-ed with this control packet?

@maxsharabayko
Copy link
Collaborator Author

Why can't this be simply the number of dropped packets among those being ACK-ed with this control packet?

To handle the cases when ACK packet is lost.
Sending the total number allows recovering the actual loss value of the receiver with the first arrived ACK.

@mbakholdina
Copy link
Collaborator

@maxsharabayko Please take into consideration Issue #1001 when proceeding with the current PR.

@maxsharabayko maxsharabayko force-pushed the develop/ack-rcvdrop branch from 41cb53a to a382d61 Compare June 1, 2023 06:26
@codecov-commenter
Copy link

Codecov Report

Merging #1889 (a382d61) into master (3cefede) will decrease coverage by 0.60%.
The diff coverage is 94.11%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #1889      +/-   ##
==========================================
- Coverage   67.51%   66.92%   -0.60%     
==========================================
  Files          99       99              
  Lines       20166    20182      +16     
==========================================
- Hits        13616    13506     -110     
- Misses       6550     6676     +126     
Impacted Files Coverage Δ
srtcore/core.h 72.15% <ø> (-6.33%) ⬇️
srtcore/stats.h 72.94% <50.00%> (-0.56%) ⬇️
srtcore/core.cpp 60.91% <100.00%> (-1.32%) ⬇️

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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: Enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants