Skip to content

Drop H264 FU-A fragments without a start - #372

Open
hsdfat wants to merge 2 commits into
pion:mainfrom
hsdfat:h264-fua-require-start
Open

hsdfat wants to merge 2 commits into
pion:mainfrom
hsdfat:h264-fua-require-start

Conversation

@hsdfat

@hsdfat hsdfat commented Sep 24, 2026

Copy link
Copy Markdown

H264Packet appended every FU-A fragment to its buffer even when the
fragment carrying the S bit was never received, then emitted the tail
of the NAL unit behind a synthesized header once the E bit arrived.

Following RFC 6184 section 5.8 (and matching the H265 depacketizer):

  • an S fragment resets the buffer, discarding any incomplete NAL unit
  • fragments that arrive with no start are dropped
  • a single NAL or STAP-A packet discards a pending NAL unit, since FUs
    must be sent back to back

Orphan fragments return an empty payload rather than an error, so
callers that stop on errors (h264writer, save-to-disk) keep running
after a mid-keyframe join or a lost start packet. If you'd rather
surface errExpectFragmentationStartUnit like H265 does, it is a one-line
change.

Fixes #370

AI disclosure: I used AI assistants (Claude, OpenAI models) while writing
and reviewing this change; I checked the RFC, the tests and the diff
myself and am responsible for it.

H264Packet appended every FU-A fragment to its buffer, even when the
fragment with the S bit had never been seen. When the E bit arrived it
emitted the tail of the NAL unit behind a synthesized header, which
decoders reject (for example an IDR slice with no slice header).

Follow RFC 6184 section 5.8, as the H265 depacketizer already does:
reset the buffer on an S fragment, drop fragments that arrive with no
start, and discard a pending NAL unit when a single NAL or STAP-A
packet shows that its end was lost.

Fixes pion#370
@JoTurk

JoTurk commented Sep 24, 2026

Copy link
Copy Markdown
Member

AI disclosure: I used AI assistants (Claude, OpenAI models) while writing
and reviewing this change; I checked the RFC, the tests and the diff
myself and am responsible for it.

Thank you :)

@JoTurk
JoTurk self-requested a review September 24, 2026 17:15
@codecov

codecov Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.47%. Comparing base (d312bf6) to head (ac980bb).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   87.45%   87.47%   +0.01%     
==========================================
  Files          28       28              
  Lines        3173     3177       +4     
==========================================
+ Hits         2775     2779       +4     
  Misses        398      398              
Flag Coverage Δ
go 87.47% <100.00%> (+0.01%) ⬆️

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.

@JoTurk JoTurk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change is minimal and does look good to me and seem to follow the spec, we should merge this after we add media tests to https://github.com/pion/browser-tests should do it this weekend.

Thank you.

Signed-off-by: phatlc <phatle.hsd@gmail.com>
@hsdfat

hsdfat commented Oct 3, 2026

Copy link
Copy Markdown
Author

@JoTurk I merged current main (d312bf6) into this branch in ac980bb, so it includes the new AV1 padding fix and is up to date. The full race-enabled tests, go vet, and lint for changes all pass.

I also noticed that the e2e media SRTP test in pion/browser-tests#111 has landed since your review. Does that cover the media validation you wanted for this change, or is there a separate H264 check still needed?

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.

codecs: H264Packet reassembles FU-A fragments it never saw the start of, producing a NAL with a fabricated header

2 participants