Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

H264: Ignore if reserved bit is set FU-A payload #109

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nemosupremo
Copy link
Contributor

We found in certain cameras with the HeroSpeed firmware were setting this bit.

The spec says:

The Reserved bit MUST be equal to 0 and MUST be ignored by the receiver.

This patch just ignores the reserved bit instead of throwing an error.

@scottlamb
Copy link
Owner

I just found the following in RFC 6184 section 5.8:

A receiver in an endpoint or in a MANE MAY aggregate the first n-1
fragments of a NAL unit to an (incomplete) NAL unit, even if fragment
n of that NAL unit is not received. In this case, the
forbidden_zero_bit of the NAL unit MUST be set to one to indicate a
syntax violation.

I wonder if that's what you're encountering. If so, we should probably treat it similar to loss, and discard the whole frame.

Could you add a test?

@nemosupremo
Copy link
Contributor Author

  1. gortsplib does not drop the entire frame https://github.com/bluenviron/gortsplib/blob/07039eb7d9fc8f54ef7f63f90b27d44641e485bd/pkg/format/rtph264/decoder.go#L87 (it seems to ignore the header entirely).
  2. From the quoted section, I'm not sure what the spec is implying - it doesn't seem to imply to drop the frame. It seems to say that the reserved bit must be set while the receiver is aggregating the frame. I'm not sure how this is relevant because retina does not expose partially aggregated NAL units. Furthermore, I don't think part of the spec concerns what happens when the sender sends a NAL with the forbidden zero bit.

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.

2 participants