feat: robust RTP depacketizer — STAP-A/AP, CSRC/extension, loss handling (#10)

[compot-dev]

Fixes #10.

Changes in NalAssembler

Before	Now
The RTP header was always assumed to be 12 bytes, and video packets with CSRC or an extension were dropped entirely	The real header length is used: CSRC, extension and padding
STAP-A / AP were passed to the decoder as one malformed NAL	They are split into separate NAL units (some servers send SPS/PPS/VPS this way)
A lost fragment produced a corrupted NAL; a continuation without its start was appended to an empty buffer with no start code	RTP sequence numbers are tracked (with wrap-around through 65535). An incomplete NAL is dropped; the next complete one is assembled normally
A NAL larger than the buffer was silently truncated	It is dropped and logged
assemble() returned one Frame	assemble(buf, offset, length, h265, sink) hands every NAL to Sink (an aggregation packet yields several)

Side effects:

• RtspClient and QuadCell no longer copy each video packet into a Frame before parsing; the NAL is built straight from the receive buffer.
• The "CSRC/extension not supported" checks in both classes are removed.
• RtspSession.rtpPayloadEnd() gets a variant with an offset.

Diagnostics: droppedCount() counts dropped NAL units. The log records the first three and then every hundredth.

Tests

68 in total; 12 of them are new in NalAssemblerTest.

• STAP-A, AP, a truncated aggregation packet.
• A packet with CSRC + extension + padding.
• Malformed headers.
• A lost middle fragment, a continuation without its start, a start without its end.
• Loss between single NALs (harmless).
• Sequence number wrap 65535 → 0.
• An oversized NAL.
• A packet at an offset in the buffer.
• Checked that the tests catch errors: disabling the loss check fails lostMiddleFragmentDropsTheNal.

Notes

• Independent of feat: RTSP Digest authentication and keepalive (#8, #11) #46 at the code level. The #10 line in the README's "Known limitations" will be removed after feat: RTSP Digest authentication and keepalive (#8, #11) #46 is merged, to avoid a conflict in the same block.
• Not tested on a device. The decoder now gets fewer corrupted frames, but after a loss it waits for the next complete unit, so there may be short freezes instead of artefacts.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Handle RTP aggregation, variable headers, and fragmented NAL loss

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Parse RTP video packets with CSRC, extensions, and padding instead of rejecting them.
• Split H.264/H.265 aggregation packets and drop incomplete or oversized NAL units.
• Remove video packet copies and test depacketization, loss recovery, and sequence wrap-around.

Diagram

graph TD
  RTP["RTP packets"] --> Session["RTSP session"] --> Client["Main client"] --> Assembler["NAL assembler"] --> Queue[("NAL queues")] --> Decoder["Video decoders"]
  Session --> Quad["Quad-cell streams"] --> Assembler

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Normalize RTP payloads in RtspSession

• ➕ Centralizes header and padding validation for packet consumers.
• ➖ Requires a broader callback contract change; codec-specific aggregation and fragment state still belong in the depacketizer.

Recommendation: Keep the PR's localized depacketizer and sink contract. It supports multiple NAL outputs and avoids packet copies while reusing the session's RTP helpers; session-level payload normalization would broaden this change without removing codec-specific work.

Files changed (5) +332 / -149

Enhancement (4) +177 / -117

NalAssembler.javaDepacketize complete RTP packets into loss-aware NAL units +134/-75

Depacketize complete RTP packets into loss-aware NAL units

• Parses variable RTP headers and padding, emits individual H.264 STAP-A and H.265 AP units, and delivers complete Annex B NAL units through a sink. Tracks sequence gaps and fragment state to discard incomplete or oversized units, with a diagnostic drop count.

android/app/src/main/java/com/openipc/decoder/NalAssembler.java

QuadCell.javaFeed quad-view RTP buffers directly to the assembler +9/-12

Feed quad-view RTP buffers directly to the assembler

• Removes the CSRC/extension rejection and intermediate video packet copy. A sink queues each emitted NAL unit or recycles it when the queue is full.

android/app/src/main/java/com/openipc/decoder/QuadCell.java

RtspClient.javaProcess primary video without copying RTP packets +26/-28

Process primary video without copying RTP packets

• Passes video receive-buffer slices directly to the assembler and queues each emitted NAL unit through a sink. Retains copying for audio and reads jitter timestamps relative to the packet offset.

android/app/src/main/java/com/openipc/decoder/RtspClient.java

RtspSession.javaSupport offset-aware RTP padding calculation +8/-2

Support offset-aware RTP padding calculation

• Adds an offset-taking payload-end helper for packets stored within a larger receive buffer. The existing zero-offset helper delegates to it.

android/app/src/main/java/com/openipc/decoder/RtspSession.java

Tests (1) +155 / -32

NalAssemblerTest.javaCover aggregation, RTP headers, and fragment loss +155/-32

Cover aggregation, RTP headers, and fragment loss

• Adapts existing tests to the sink API and adds cases for STAP-A/AP, malformed or extended headers, padding, packet offsets, fragment loss, sequence wrap-around, and oversized units.

android/app/src/test/java/com/openipc/decoder/NalAssemblerTest.java

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Lost fragment starts go uncounted ✓ Resolved

Description

fragmentUnit() ignores a continuation when inFragment is false without recording a dropped NAL
unit. If the start packet is lost or playback begins mid-unit, the remaining fragments produce no
output while droppedCount() and its diagnostic warnings remain unchanged.

Code

android/app/src/main/java/com/openipc/decoder/NalAssembler.java[R123-125]

+        } else if (!inFragment) {
+            // The start was lost (or we joined mid-unit): nothing to attach this piece to
+            return;

Evidence

reset() leaves no active fragment, and gap handling calls drop() only when a fragment is active.
The orphan-continuation branch returns without calling drop(), which is the only method that
increments the diagnostic counter.

android/app/src/main/java/com/openipc/decoder/NalAssembler.java[49-56]
android/app/src/main/java/com/openipc/decoder/NalAssembler.java[76-81]
android/app/src/main/java/com/openipc/decoder/NalAssembler.java[123-126]
android/app/src/main/java/com/openipc/decoder/NalAssembler.java[169-175]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A continuation received without its FU start is discarded without updating the new dropped-NAL diagnostic.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/NalAssembler.java[123-126]
- android/app/src/main/java/com/openipc/decoder/NalAssembler.java[169-175]
- android/app/src/test/java/com/openipc/decoder/NalAssemblerTest.java[141-146]
## Recommended Fix
Record one drop when the first orphaned continuation is received, track subsequent pieces so the same NAL is not counted repeatedly, and test the reported count.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Interrupted fragments reach the decoder ✓ Resolved

Description

assemble() emits a standalone NAL or an aggregation without clearing an active inFragment or its
saved bytes. If a sender places a complete packet between an unfinished FU and a later FU
continuation with consecutive sequence numbers, that continuation attaches to the stale fragment and
the resulting NAL is submitted for decoding out of order.

Code

android/app/src/main/java/com/openipc/decoder/NalAssembler.java[R89-90]

+        } else if (codecH265 ? type == H265_AP : type == H264_STAP_A) {
+            aggregation(buf, start + nalHeader, end, sink);

Evidence

The dispatch emits non-FU payloads without changing fragment state, while the continuation path
accepts any later fragment whenever inFragment is still true. The sequence-gap check does not
intervene when those packets have consecutive sequence numbers.

android/app/src/main/java/com/openipc/decoder/NalAssembler.java[76-96]
android/app/src/main/java/com/openipc/decoder/NalAssembler.java[108-142]
android/app/src/main/java/com/openipc/decoder/NalAssembler.java[146-166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A non-FU video packet can pass through while an unfinished fragmented NAL remains active, allowing a later continuation to reuse stale bytes.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/NalAssembler.java[83-97]
- android/app/src/main/java/com/openipc/decoder/NalAssembler.java[123-142]
- android/app/src/test/java/com/openipc/decoder/NalAssemblerTest.java[127-156]
## Recommended Fix
Before processing any non-FU payload, drop an active unfinished fragment. Add a sequence test with FU start, standalone or aggregate packet, and FU continuation to verify that no stale NAL is emitted.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. One packet floods logs when queues fill ✓ Resolved

Description

nalSink writes a warning for every rejected NAL, while aggregation() invokes it separately for
every member of one RTP packet. When a video queue is full, a single aggregation packet containing
many members generates as many warnings on the session's packet-processing thread.

Code

android/app/src/main/java/com/openipc/decoder/RtspClient.java[R50-53]

+        this.nalSink = nal -> {
+            if (!nalQueue.offer(nal)) {
+                Log.w(TAG, "Video queue full, frame dropped");
+                framePool.recycle(nal);

Evidence

Each length-prefixed aggregate member causes a separate synchronous sink call. Both sinks log on
every failed offer; their queues hold only 32 and 30 frames, respectively, while the receive loop
invokes the packet callback synchronously.

android/app/src/main/java/com/openipc/decoder/NalAssembler.java[147-166]
android/app/src/main/java/com/openipc/decoder/RtspClient.java[50-54]
android/app/src/main/java/com/openipc/decoder/QuadCell.java[43-43]
android/app/src/main/java/com/openipc/decoder/QuadCell.java[65-69]
android/app/src/main/java/com/openipc/decoder/Decoder.java[67-67]
android/app/src/main/java/com/openipc/decoder/RtspSession.java[265-279]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Aggregation can turn one received RTP packet into many synchronous full-queue log warnings.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/NalAssembler.java[147-166]
- android/app/src/main/java/com/openipc/decoder/RtspClient.java[50-54]
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[65-69]
## Recommended Fix
Limit full-queue warnings to one per packet or a bounded time interval in both sinks, and avoid continuing to allocate and offer aggregate members once the packet's output cannot be queued.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View review recommended (1)

4. Quad playback churns more frame buffers ✓ Resolved

Description

aggregation() now creates a distinct pooled Frame for every NAL it extracts, but the quad
decoder worker does not recycle frames after consuming them. Cameras that repeatedly send
aggregation packets therefore multiply quad mode's existing unreused frame allocations, whereas the
main decoder returns consumed frames to the pool.

Code

android/app/src/main/java/com/openipc/decoder/NalAssembler.java[R152-153]

+            emit(buf, pos, size, sink);
+            pos += size;

Evidence

The new aggregation loop allocates and enqueues one frame per member. Quad mode recycles only frames
rejected by its queue, not frames polled by its decoder worker; FramePool.obtain() allocates a new
buffer when no recycled frame is available, while the main decoder explicitly recycles consumed
frames.

android/app/src/main/java/com/openipc/decoder/NalAssembler.java[147-166]
android/app/src/main/java/com/openipc/decoder/QuadCell.java[65-70]
android/app/src/main/java/com/openipc/decoder/QuadCell.java[129-140]
android/app/src/main/java/com/openipc/decoder/FramePool.java[20-31]
android/app/src/main/java/com/openipc/decoder/Decoder.java[684-690]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new per-member aggregation output increases allocations in quad mode because its decoder worker does not return consumed NAL frames to the pool.
## Fix Focus Areas
- android/app/src/main/java/com/openipc/decoder/NalAssembler.java[147-166]
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[65-70]
- android/app/src/main/java/com/openipc/decoder/QuadCell.java[129-140]
## Recommended Fix
Recycle each frame polled by the quad decoder worker in a finally block after decoding, including when decoding throws, while retaining the sink's existing recycling of rejected frames.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗