Skip to content

Fix additive increase step at packet boundaries - #440

Open
jayli-nuro wants to merge 1 commit into
pion:mainfrom
jayli-nuro:fix/additive-increase-packet-size-quantization
Open

Fix additive increase step at packet boundaries#440
jayli-nuro wants to merge 1 commit into
pion:mainfrom
jayli-nuro:fix/additive-increase-packet-size-quantization

Conversation

@jayli-nuro

@jayli-nuro jayli-nuro commented Aug 26, 2026

Copy link
Copy Markdown

The additive increase is documented as raising the estimate by "at most half a packet per response_time". It doesn't, because of how the packet size is derived.

A packet holds at most 1200 bytes, or 9600 bits. The code derives the size by averaging a frame over the packets it needs:

bitsPerFrame := float64(c.target) / 30.0
packetsPerFrame := math.Ceil(bitsPerFrame / (1200 * 8))
expectedPacketSizeBits := bitsPerFrame / packetsPerFrame

At a 288 kbps target a frame is exactly 9600 bits, so it fits one packet and the step is half of a full packet:

frame 9600 bits  ->  [9600]         average 9600  ->  step 4800

At 290 kbps the frame is 9667 bits. That is 67 bits too big for one packet, so it takes two: one full, one nearly empty.

frame 9667 bits  ->  [9600][67]     average 4833  ->  step 2416

The target rose by 0.7% and the step halved. Averaging a full packet together with a nearly empty trailing one answers "what is the mean fill of this frame's packets", not "how big is a packet". It recurs at every packet boundary:

target frame packets step fraction of a packet
288 kbps 9600 [9600] 4800 0.50
290 kbps 9667 [9600][67] 2416 0.25
580 kbps 19333 [9600][9600][133] 3222 0.34
1.5 Mbps 50000 six packets 4166 0.43

So the estimate grows by a quarter of a packet where the text says half, and the controller ramps most slowly exactly while climbing through a boundary.

The fix: a packet holds at most one MTU, and a frame smaller than that travels in a single packet.

expectedPacketSizeBits := math.Min(bitsPerFrame, maxPacketSizeBits)

Every row above becomes 0.50. Targets below one MTU per frame are unchanged — 240 kbps still steps 4000 — so the "slightly slower slope for the additive increase at lower bitrates" is preserved. Only the boundary jumps go away. The inlined 1200 * 8 becomes a named constant.

On provenance, since the three replaced lines will look familiar: they are from draft-ietf-rmcat-gcc-02 section 5.5. That section introduces them with "it can for instance be computed", and it names the input to the increase expected_packet_size_bits while the example computes avg_packet_size_bits. The normative statement is the "at most half a packet" sentence, and this change is what makes it true.

The added test asserts the exact step across the boundaries above. On main it fails with target 290000: additive step was 2416, want 4800. go test ./... is green across the repo.

@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.91%. Comparing base (4680cd3) to head (f08a9d9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #440      +/-   ##
==========================================
+ Coverage   80.14%   80.91%   +0.77%     
==========================================
  Files          88       88              
  Lines        4624     4664      +40     
==========================================
+ Hits         3706     3774      +68     
+ Misses        735      710      -25     
+ Partials      183      180       -3     
Flag Coverage Δ
go 80.91% <100.00%> (+0.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The additive increase is documented as raising the estimate by at most
half a packet per response_time. It does not, because of how the packet
size is derived.

A packet holds at most 1200 bytes, or 9600 bits. The size is derived by
averaging a frame over the packets it needs. At a 288 kbps target a
frame is exactly 9600 bits, fits one packet, and the step is half of a
full packet:

    frame 9600 bits -> [9600]        average 9600 -> step 4800

At 290 kbps the frame is 9667 bits, 67 bits too large for one packet, so
it takes two: one full, one nearly empty.

    frame 9667 bits -> [9600][67]    average 4833 -> step 2416

The target rose by 0.7% and the step halved. Averaging a full packet
with a nearly empty trailing one answers what the mean fill of a frame's
packets is, not how big a packet is. It recurs at every boundary: 580
kbps steps 3222, a third of a packet rather than half.

A packet holds at most one MTU, and a frame smaller than that travels in
a single packet, so bound the expected size by the MTU. Targets below
one MTU per frame are unchanged, so the slightly slower slope at lower
bitrates is preserved; only the boundary jumps go away.
@jayli-nuro
jayli-nuro force-pushed the fix/additive-increase-packet-size-quantization branch from cfcd5ea to f08a9d9 Compare August 27, 2026 21:31
@jayli-nuro jayli-nuro changed the title gcc: fix additive-increase step shrinking as the target grows Fix additive increase step at packet boundaries Aug 27, 2026
@jayli-nuro

Copy link
Copy Markdown
Author

Closing this for now — we are carrying the change in an internal fork while we ship, so I do not want to leave a PR pending review on your side.

The analysis above stands on its own if anyone wants to pick it up: the additive-increase step is derived from bitsPerFrame / ceil(bitsPerFrame/MTU), which is the mean packet size rather than a packet size, so it drops discontinuously at each packet-count boundary — a target rising 288 → 290 kbps halves the step, and the increase becomes a quarter of a packet where the text says half. Happy to reopen if there is interest.

@jayli-nuro jayli-nuro closed this Aug 31, 2026
@JoTurk

JoTurk commented Aug 31, 2026

Copy link
Copy Markdown
Member

Sorry, our todo list is huge, we don't have many people and I have been super busy with dtls I'll open the pr and try to get it reviewed this week

@JoTurk JoTurk reopened this Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants