Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions crates/vdo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -453,9 +453,17 @@ impl StreamBuffer<'_> {
Ok(slice)
}

/// Returns a copy of the frame data, excluding the header if one is present.
/// Returns a copy of the frame data, excluding the header if one is present
/// (the `[0, header_size())` prefix — see [`header_bytes()`](StreamBuffer::header_bytes)).
///
/// Use [`as_slice()`](StreamBuffer::as_slice) for a raw view of the whole buffer.
/// **On H.264/H.265 key frames this excludes the codec parameter sets.**
/// VDO carries the SPS/PPS (and VPS for H.265) in that stripped header, so
/// `data_copy()` on a key frame returns only the coded slice — the
/// parameter sets are *not* in the result. To recover them (e.g. to build
/// an `avcC`/`hvcC`, an MP4 init segment, or an SDP `fmtp`), read
/// [`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> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

let data = unsafe { vdo_sys::vdo_buffer_get_data(self.raw) };
if data.is_null() {
Expand All @@ -475,6 +483,32 @@ impl StreamBuffer<'_> {
Ok(slice.to_vec())
}

/// Returns the frame's header bytes — the `[0, header_size())` prefix that

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[0, header_size())

Personally I would leave out details about where in the buffer the header bytes are from the commit message

/// [`data_copy()`](StreamBuffer::data_copy) excludes — or an empty slice
/// when the frame has no header.
///
/// On H.264/H.265 **key frames** this header carries the codec parameter
/// sets (SPS/PPS, and VPS for H.265) in Annex B, so this is where to read
/// them to build an `avcC`/`hvcC`, an MP4 init segment, or an SDP `fmtp`.
/// 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> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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>

let data = unsafe { vdo_sys::vdo_buffer_get_data(self.raw) };
if data.is_null() {
return Err(Error::NullPointer);
}
let header = self.header_size().unwrap_or(0);
assert!(
header <= self.capacity(),
"expect header to fit within buffer capacity"
);
// SAFETY: 0..header lies within the mapped region of capacity bytes,
// which is fully initialized at allocation time.
let slice = unsafe { std::slice::from_raw_parts(data as *const u8, header) };
Ok(slice)
}

pub fn frame_type(&self) -> VdoFrameType {
unsafe { vdo_sys::vdo_frame_get_frame_type(self.raw) }
}
Expand All @@ -499,6 +533,9 @@ impl StreamBuffer<'_> {
}

/// Returns the header size in bytes, or `None` if the frame has no header.
///
/// For H.264/H.265 key frames the header holds the codec parameter sets;
/// see [`header_bytes()`](StreamBuffer::header_bytes).
pub fn header_size(&self) -> Option<usize> {
let size = unsafe { vdo_sys::vdo_frame_get_header_size(self.raw) };
if size < 0 {
Expand Down
Loading