Bound how long the packet queue holds a frame back - #120
Conversation
PR Summary by QodoBound BufferedPacketQueue latency on unrecoverable RTP gaps
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Wraparound flush misorders packets
|
| mPackets.size(), | ||
| (long long) MAX_BUFFER_AGE.count(), | ||
| static_cast<unsigned>(static_cast<SeqType>(mLastPacketIdx + 1))); | ||
| mLastPacketIdx = drainBufferInOrder(callback); |
There was a problem hiding this comment.
1. Wraparound flush misorders packets 🐞 Bug ≡ Correctness
The timeout path calls drainBufferInOrder, which numerically sorts buffered uint16_t sequence numbers, so a block spanning 65535→0 is delivered as 0 then 65535. This violates the queue's ordering guarantee and can advance mLastPacketIdx incorrectly.
Agent Prompt
## Issue description
Timeout flushing numerically sorts RTP sequence numbers, which misorders buffered packets across the uint16 wraparound boundary.
## Issue Context
Use wraparound-aware sequence distance relative to the last delivered sequence when ordering the remaining buffered packets and selecting the new last sequence.
## Fix Focus Areas
- app/videonative/src/main/cpp/BufferedPacketQueue.h[316-359]
- app/videonative/src/main/cpp/tests/BufferedPacketQueue_test.cpp[41-54]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| mPackets.size(), | ||
| (long long) MAX_BUFFER_AGE.count(), | ||
| static_cast<unsigned>(static_cast<SeqType>(mLastPacketIdx + 1))); | ||
| mLastPacketIdx = drainBufferInOrder(callback); |
There was a problem hiding this comment.
2. Late packets replay out-of-order 🐞 Bug ≡ Correctness
After a timeout or monotonic flush advances mLastPacketIdx, a subsequently arriving missing packet is still buffered even though it is now behind the delivered position. A later timeout drains that stale packet through the callback after newer packets, corrupting parser input order.
Agent Prompt
## Issue description
Packets arriving behind `mLastPacketIdx` after a gap flush are buffered and eventually replayed after newer RTP data.
## Issue Context
Distinguish stale packets behind the delivery point from packets ahead across wraparound. Drop stale packets rather than inserting them into the reorder buffer, while preserving valid wraparound handling.
## Fix Focus Areas
- app/videonative/src/main/cpp/BufferedPacketQueue.h[80-113]
- app/videonative/src/main/cpp/BufferedPacketQueue.h[216-268]
- app/videonative/src/main/cpp/tests/BufferedPacketQueue_test.cpp[91-116]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| auto dist = calculateDistance(mLastPacketIdx, currPacketIdx); | ||
| if (static_cast<size_t>(std::abs(dist)) < MONOTONIC_MAX_DISTANCE) |
There was a problem hiding this comment.
3. Duplicates trigger monotonic flush 🐞 Bug ≡ Correctness
The activated monotonic counter tests each packet only against mLastPacketIdx, not against the previous buffered arrival, so duplicates or a non-monotonic reorder increment it repeatedly. Five copies of the same ahead-of-gap sequence therefore force an unwarranted flush.
Agent Prompt
## Issue description
The monotonic escape counter counts any packet ahead of the last delivered sequence, including duplicates and decreasing reordered arrivals.
## Issue Context
Track the previous out-of-order arrival and increment only for a genuine forward sequence progression. Duplicates and backward movement should reset or leave the run unchanged as appropriate.
## Fix Focus Areas
- app/videonative/src/main/cpp/BufferedPacketQueue.h[126-129]
- app/videonative/src/main/cpp/BufferedPacketQueue.h[226-259]
- app/videonative/src/main/cpp/tests/BufferedPacketQueue_test.cpp[71-86]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@iflyhere thank you for the PR! I checked Qodo's comments they seem to be correct. could you please fix and I'll merge the PR |
|
@iflyhere additional findings:
|
BufferedPacketQueue reorders the RTP stream before it reaches the parser. When a packet is missing it holds everything behind the gap, which is right for a reorder and wrong for a loss - and past wfb-ng, on a loopback socket, a gap is almost always a loss that FEC could not recover. Three things made that wait longer than it needs to be. The monotonic-increase escape hatch, which exists precisely to notice "the sequence numbers keep climbing but the gap is not filling", never fires. calculateDistance(a, b) is how far b is ahead of a - seqLessThan reads it that way - but the call site asks for the distance from the incoming packet to the last delivered one. For a gap ahead of us that is negative, so the else branch clears the counter on every single packet. Fixing the argument order alone is not enough: the counter is only incremented while abs(dist) < MONOTONIC_THRESHOLD, and dist grows by one with every packet held back. Compared against the same constant that gates it, the counter tops out at three and can never reach five. The gate is now its own constant, so a run past a gap releases the buffer after five packets instead of never. That leaves MAX_BUFFER_SIZE as the only bound, and how much latency fifteen packets is depends entirely on the packet rate: about 20ms on a 1080p video stream, but roughly 300ms on the audio stream, which runs at a fraction of it. The buffer is now also bounded in time, so the worst case is the same on both. Twenty milliseconds is a little over one frame at 60fps and several orders of magnitude more than a loopback socket needs to reorder anything. Nothing is dropped by any of this - the buffered packets are still delivered, just without waiting on one that is not coming. The test target could not be built on a case-sensitive filesystem, since CMakeLists.txt spelled the source BufferedPacketqueue_test.cpp. The existing two cases both turn out to feed a strictly in-order stream, so neither reached the buffer at all; the new ones cover a permanent gap, the age bound, a reorder that resolves inside it, and a jump too large to be one. They drive the clock themselves rather than reading it, so the age bound cannot make them flaky on a loaded machine.
Two problems with the flush, both found in review. restartBuffering() set mLastPacketIdx to the packet that triggered the flush rather than the newest one delivered. With 1..5 delivered, 6 lost, then 7 8 9 12 10, the flush hands over 7 8 9 10 12 but leaves the pointer on 10 - so 11 would be delivered after 12, and 13..16 are held until 17 arrives. The queue re-created the stall it exists to prevent. bufferPacket() always runs before the flush, so the triggering packet is in the buffer either way and mLastPacketIdx = drainBufferInOrder() is all that was needed; restartBuffering() is gone. That alone would have broken the resync on a large jump, because drainBufferInOrder() seeded `highest` from mLastPacketIdx and compared with calculateDistance(), which reads as negative past half the sequence space. RTP starts at a random sequence number, so a VTX that reboots mid-session lands exactly there: fed 1, 2, then 40000, 40001, ..., neither the seed nor the comparison ever moves and every packet comes out one flush late, forever. On video the fifteen-packet cap used to paper over it by rewinding to currPacketIdx; on audio nothing did. `highest` is now seeded from a packet that is actually in the buffer. While sorting the flush: by raw value a block straddling the wrap point (65534, 65535, 0, 1) sorts to 0, 1, 65534, 65535 and was handed to the parser in that order. Sorting by distance from the last delivered packet fixes the order and makes the last element the newest. Three tests added, one per problem. The large-jump case uses 40000 - the existing LargeJumpFallsBackToTheBufferCap uses 30000, which is still positive as an int16 and does not reach any of this.
faed15f to
ca01d43
Compare
|
Both correct, and the second one would have bitten anyone whose VTX reboots mid-flight. Fixed 1. 2. 3. While in there — the flush sorted by raw value, so a block straddling the wrap point Three tests added, one per problem. Against the previous commit: and with the fix, 9/9. Compile tested for arm64-v8a + armeabi-v7a. |
BufferedPacketQueuereorders the RTP stream before it reaches the parser. When a packet ismissing it holds back everything behind the gap — correct for a reorder, wrong for a loss. And
by the time packets reach it they have already been through wfb-ng's FEC and a loopback
socket, so a gap here is almost always a packet FEC could not recover, not one that is about
to turn up. Three separate things made that wait longer than it needs to be.
The monotonic-increase escape hatch never fires
It exists precisely to notice "the sequence numbers keep climbing but the gap is not filling":
calculateDistance(a, b)returns how farbis ahead ofa— that is howseqLessThanjustbelow it reads the same function. So asking for the distance from the incoming packet to the
last delivered one gives a negative number for a gap ahead of us, which is the only case
this heuristic exists for. The
elsebranch runs instead, and clears the counter on everysingle packet.
Swapping the arguments back is not enough on its own. The counter is only incremented while
abs(dist) < MONOTONIC_THRESHOLD, anddistgrows by one with every packet held back — sogated on the same constant it is compared against, the counter tops out at three and can never
reach five. The gate now has its own constant.
Fifteen packets is a different amount of time on each stream
That left
MAX_BUFFER_SIZEas the only bound, and what it costs depends entirely on thepacket rate: roughly 20 ms on a 1080p video stream, but around 300 ms on the audio stream,
which runs at a fraction of it. The buffer is now bounded in time as well, so the worst case
is the same on both. 20 ms is a little over one frame at 60 fps, and several orders of
magnitude more than a loopback socket needs to reorder anything.
Nothing is dropped by any of this — the buffered packets are still delivered, just without
waiting on one that is not coming.
The tests
CMakeLists.txtspelled the sourceBufferedPacketqueue_test.cpp, so the target could not beconfigured on a case-sensitive filesystem. With that fixed, both existing cases turn out to
feed a strictly in-order stream, so neither ever reached the buffer they were meant to cover.
The new cases cover a permanent gap, the age bound, a reorder that resolves inside it, and a
jump too large to be one. They drive the clock themselves instead of reading it, so the age
bound cannot make them flaky on a loaded runner.
The permanent-gap case is a straight regression test. Against unmodified
master— one packetlost, then six more arriving in order:
Nothing behind the gap comes out at all. With this change all six pass:
The suite is a plain host gtest target — no NDK, no submodules, about a minute:
Scope
This is a jitter fix, not a throughput one. On a clean link the queue delivers every packet
straight through and none of this runs; it is the behaviour on loss that changes. For
reference, the decoder's own averages over a 138k-frame session on a Quest 3 are
Parsing: 11.9 ms | WaitInputBuffer: 0.55 ms | Decoding: 6.4 ms, so the decode path itself isnot where time goes — but the queue sits in front of all three and its holding time is not
counted in any of them.
Compile tested for arm64-v8a + armeabi-v7a.
Part of a series of independent fixes found while building an immersive (OpenXR) mode on a
Quest 3, each standalone and mergeable in any order:
wirelessInfo()safeVideoPlayer/WfbNgLinktake aContext#113 and #116 are now confirmed on hardware (Quest 3, Horizon OS, Android 14).