Skip to content

h264: wait for a partition head before depayloading - #2380

Open
ajplotkin wants to merge 1 commit into
AlexxIT:masterfrom
ajplotkin:fix/h264-depay-sync-to-partition-head
Open

h264: wait for a partition head before depayloading#2380
ajplotkin wants to merge 1 commit into
AlexxIT:masterfrom
ajplotkin:fix/h264-depay-sync-to-partition-head

Conversation

@ajplotkin

Copy link
Copy Markdown

Summary

RTPDepay (pkg/h264/rtp.go) creates a fresh codecs.H264Packet and begins feeding it whatever RTP packet arrives first. A consumer that attaches to a live stream partway through a fragmented NAL unit therefore starts with FU-A fragments whose start fragment was never received.

codecs.H264Packet appends FU-A fragments without checking the start bit, and on the end bit it synthesizes a NAL header from the fragment type. The result is a NAL unit that never existed on the wire: correctly shaped, but missing its head — including the slice header. When the fragmented type is 5, it also passes IsKeyframe, so callers treat it as a valid keyframe and hand it to a decoder that cannot decode it.

Reproduction

The added test feeds only the tail fragments of a fragmented IDR. On current main it emits:

[0 0 0 5 101 170 187 204 221]

101 is 0x65nal_ref_idc=3, nal_unit_type=5 (IDR) — followed by nothing but the orphan fragment bytes. A synthesized keyframe assembled from a NAL whose beginning was never seen.

Fix

Skip packets until IsPartitionHead reports one, then depayload exactly as before. Packets dropped by this gate could only ever have produced a malformed NAL.

This matches what pion already does elsewhere: IsPartitionHead is part of the Depacketizer interface, and pion's own SampleBuilder refuses to begin a sample that is not a partition head. RTPDepay bypasses SampleBuilder, so it never gets that protection. RFC 6184 §5.8 likewise requires discarding fragments that follow a lost start.

Where this shows up

Anything that builds a depayloader per consumer and attaches mid-stream. The snapshot path is the most visible: /api/frame.jpeg creates a new keyframe consumer per request, so a request landing mid-IDR gets a head-truncated "keyframe" and ffmpeg fails on it — intermittently, without any error that identifies the cause. On one long-running deployment this produced a few percent of snapshot requests failing with Invalid data found when processing input or a 100% decode-error rate, plus occasional solid-grey JPEGs (a decoder concealing a keyframe with no slice header). Live restream and MP4 consumers attach the same way.

I am deliberately not claiming this is the sole cause of those production failures — mid-stream packet loss can corrupt an access unit too, and I have not instrumented enough to separate them. The defect above stands on its own at the code level regardless, which is what the test demonstrates.

Notes

  • No behavior change once synced — the gate is one bool check per packet until the first partition head.
  • Does not address corruption from loss after attach (a sequence-continuity check would be a separate change).

RTPDepay creates a fresh codecs.H264Packet and starts feeding it whatever
packet arrives first. When a consumer attaches to a live stream that is
mid-way through a fragmented NAL unit, the first packets it sees are FU-A
fragments whose start fragment was never received.

codecs.H264Packet appends FU-A fragments without checking the start bit,
and on the end bit it synthesizes a NAL header from the fragment type. The
result is a NAL unit that never existed on the wire: correctly shaped, but
missing its head, including the slice header. When the fragmented type is
5 it also passes IsKeyframe, so callers treat it as a valid keyframe and
hand it to a decoder, which cannot decode it.

The added test demonstrates it: feeding only the tail fragments of a
fragmented IDR currently emits [0 0 0 5 101 aa bb cc dd] - a synthesized
type-5 NAL built purely from orphan fragments.

RFC 6184 section 5.8 requires that fragments following a lost start be
discarded, and pion's own SampleBuilder refuses to begin a sample that is
not a partition head. Do the same here: skip packets until
IsPartitionHead reports one, then depayload as before. Packets dropped by
this gate could only ever have produced a malformed NAL.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ajplotkin added a commit to ajplotkin/go2rtc that referenced this pull request Jul 27, 2026
…ilure

Three changes, all deployed as go2rtc-nestfix:v9-h264fix.

pkg/h264/rtp.go — RTPDepay fed pion whatever packet arrived first, so a
consumer attaching mid-fragmented-NAL had its orphan tail fragments
assembled under a synthesized NAL header: a head-truncated "keyframe" that
passes IsKeyframe but cannot decode. /api/frame.jpeg builds a fresh
depayloader per request, so it hit this constantly - measured 579 failures
with input sizes spread 1.5KB-190KB (a uniformly random truncation point),
330 of them ffmpeg exit 183 and 245 exit 69, plus intermittent solid-grey
JPEGs. Gate on IsPartitionHead until synced, which is what pion's own
SampleBuilder does and what RFC 6184 section 5.8 requires. Filed upstream
as AlexxIT#2380.

internal/ffmpeg/jpeg.go — transcode() discarded ExitError.Stderr, so a
failed snapshot surfaced only as "exit status N" with no diagnosis. Attach
the captured stderr (tail-bounded, since it reaches an HTTP body).

internal/mjpeg/mjpeg.go — log the transcode failure. The 500 body alone was
useless: the warmer fetches with curl -sf, which discards it, which is why
this went undiagnosed for weeks.

Note the branch also carries 7fddbbe (drought-watchdog hardening), which is
NOT in the deployed image - deployment is 43fc3b3 + the nest dial-leak fix
+ these three.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ajplotkin added a commit to ajplotkin/nest-homekit-cameras that referenced this pull request Jul 27, 2026
New fork tag nestfix-1.9.14-2 adds the two fixes also filed upstream:

- AlexxIT/go2rtc#2378 - rtcConn never closed the PeerConnection on any
  error path, so a failed dial leaked it along with its ICE agent's mDNS
  :5353 sockets. A powered-off camera retrying every ~2 min accumulated
  142 leaked sockets, which starved host mDNS and left every HomeKit
  accessory at "No Response". Verified 142 -> 2, flat across 16 failed
  dials.
- AlexxIT/go2rtc#2380 - RTPDepay fed pion whatever packet arrived first,
  so a consumer attaching mid-fragmented-NAL got orphan tail fragments
  assembled under a synthesized NAL header: a head-truncated keyframe that
  passes IsKeyframe but cannot decode. /api/frame.jpeg builds a fresh
  depayloader per request, so this produced intermittent 500s and grey
  snapshots.

Also surfaces ffmpeg's stderr on a failed transcode instead of a bare
"exit status N", which is what made the second bug diagnosable at all.

patches/go2rtc-nest.patch regenerated against the new tag and verified: it
applies cleanly to a pristine v1.9.14 checkout and the result builds. The
adopted community PRs (#2351 tillo, #2194/#2193 MechanicalCoderX, #2327
zephleggett) are unchanged and still credited in the header and README.

The tag deliberately excludes the drought-watchdog hardening on the dev
branch, which is not running in production and is unvalidated.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant