Conversation
…r; add header_bytes() data_copy() excludes the [0, header_size()) header, and on H.264/H.265 key frames that header is where VDO carries the SPS/PPS (and VPS) parameter sets — so a caller scanning data_copy() bytes for parameter sets never finds them even on a healthy stream. Document this on data_copy()/header_size(), and add StreamBuffer::header_bytes() returning that header slice so the 'grab the codec config once' path is obvious and hard to get wrong. Closes AxisCommunications#247.
| /// The full access unit is `header_bytes()` followed by `data_copy()` | ||
| /// (equivalently, [`as_slice()`](StreamBuffer::as_slice) up to | ||
| /// [`size()`](StreamBuffer::size)). | ||
| pub fn header_bytes(&self) -> std::result::Result<&[u8], Error> { |
There was a problem hiding this comment.
I haven't looked at what interpreting these bytes looks like, but I am wondering if perhaps there is an opportunity here to encode some more information about the header?
i.e. what would it look like if we returned something like Result<Option<Header>, Error>
| /// [`header_bytes()`](StreamBuffer::header_bytes), or use | ||
| /// [`as_slice()`](StreamBuffer::as_slice) for the full `[header][data]` | ||
| /// buffer. | ||
| pub fn data_copy(&self) -> std::result::Result<Vec<u8>, Error> { |
There was a problem hiding this comment.
Maybe we should replace data_copy with data_bytes for consistency and flexibility in a follow up commit?
Adding .to_vec() to call sites is a minor inconvenience.
| Ok(slice.to_vec()) | ||
| } | ||
|
|
||
| /// Returns the frame's header bytes — the `[0, header_size())` prefix that |
There was a problem hiding this comment.
[0, header_size())
Personally I would leave out details about where in the buffer the header bytes are from the commit message
apljungquist
left a comment
apljungquist
left a comment
There was a problem hiding this comment.
I like the context in the issue btw; I didn't read the issue at first because it looked like a wall of AI text but now that I have read it that section helps convince me that there is a real developer with a real use case behind the isse+PR 😄
Cost a few debugging cycles on real hardware (the stream looked fine, key frames arrived, but SPS/PPS never appeared) before header_size made it obvious. Filing so the next person doesn't repeat it. Happy to PR the doc note + accessor.
| /// [`header_bytes()`](StreamBuffer::header_bytes), or use | ||
| /// [`as_slice()`](StreamBuffer::as_slice) for the full `[header][data]` | ||
| /// buffer. | ||
| pub fn data_copy(&self) -> std::result::Result<Vec<u8>, Error> { |
There was a problem hiding this comment.
You expressed surprise that data does not include the header and I'm thinking maybe we can help future users by choosing better names for the concept?
Having two functions with similar names e.g. header_bytes and data_bytes would also help I imagine.
3 participants
Fixes #247.
data_copy()strips the[0, header_size())header, and on H.264/H.265 key frames that header is exactly where VDO carries the SPS/PPS (and VPS) parameter sets — sodata_copy()on a key frame returns only the coded slice, and a caller scanning those bytes for parameter sets never finds them even on a perfectly healthy stream.Observed on an ARTPEC-6 / firmware-11 H.264 IDR buffer:
This PR:
data_copy()andheader_size(), andStreamBuffer::header_bytes()returning the[0, header_size())slice, so the "grab the codec config once" path is obvious and hard to get wrong.No behaviour change to existing methods;
header_bytes()mirrorsdata_copy()'s existing unsafe access pattern.