AV1 decoder #920
AV1 decoder #920
Conversation
christriants
force-pushed
the
feat/av1-decoder
branch
2 times, most recently
from
26dca8d to
06f9b31
Compare
christriants
force-pushed
the
feat/av1-decoder
branch
from
06f9b31 to
8a81510
Compare
| display_ratio_width: None, | ||
| display_ratio_height: None, | ||
| optimize_for_latency: None, | ||
| container: hang::catalog::Container::Legacy, |
There was a problem hiding this comment.
Is Legacy correct here?
There was a problem hiding this comment.
It's basically a big fat TODO to future me.
At the very least timestamps should be delta encoded.
WalkthroughAdds AV1 support behind a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@rs/moq-mux/src/import/av01.rs`:
- Around line 136-186: init_from_av1c currently always creates a new track and
leaks any previous one; change it to mirror init() by (1) comparing the newly
built config with self.config and returning early if equal, and (2) if unequal,
lock the catalog, remove the old track from the catalog/broadcast (use the same
removal calls used in init() for removing the prior track referenced by
self.track), then create the new catalog track with catalog.video.create_track
and call self.broadcast.create_track, and finally update self.config and
self.track. Ensure you use the same helper/method names used in init() for
removing tracks so behavior is consistent.
- Around line 113-121: The av1C detection in initialize is using the wrong
marker byte (0x0a); update the conditional that currently checks data[0] == 0x0a
to check data[0] == 0x81 so the AV1CodecConfigurationRecord is correctly
identified, leaving the existing length check and subsequent call to
init_from_av1c(data) and buf.advance(data.len()) unchanged; ensure the change is
made in the initialize<T: Buf + AsRef<[u8]>> method where init_from_av1c is
invoked.
- Line 75: The ColorConfig's full_range is being hardcoded to false; update the
code that constructs the ColorConfig (where full_range: false is set) to read
the value from the sequence header by using
seq_header.color_config.full_color_range instead, matching how other fields like
bit_depth, color_primaries, and transfer_characteristics are populated.
- Around line 236-246: The SequenceHeader OBU handling incorrectly always slices
payload as &obu_data[1..] and sets self.current.contains_keyframe = true even on
parse failure; update the payload offset using header.extension_header (if true
use &obu_data[2..] else &obu_data[1..]) when calling
SequenceHeaderObu::parse(header, ...), and only call self.init(&seq_header) and
set self.current.contains_keyframe = true inside the Ok arm (do not set the
keyframe flag on Err); references: ObuType::SequenceHeader,
header.extension_header, SequenceHeaderObu::parse, self.init, and
self.current.contains_keyframe.
- Around line 400-441: In ObuIterator::next you must account for the OBU
extension byte before parsing the LEB128 size: read the header byte, detect the
obu_extension_flag (same test used in decode_obu), and set the initial size
offset to 2 when the extension flag is present (otherwise 1); update the
subsequent bounds checks (data.len(), self.buf.remaining()) and LEB128 loop to
use that computed offset so the computed total_size and copy_to_bytes call use
the correct start of the size field and avoid corrupting OBU boundaries (mirror
the logic in decode_obu to locate the header/extension/size bytes).
🧹 Nitpick comments (2)
rs/moq-mux/src/import/av01.rs (1)
255-258: Redundantdata.is_empty()check.On line 255,
obu_data.len() > obu_header_sizeguarantees thatdata(a subslice starting atobu_header_size) is non-empty. Theif data.is_empty()check at line 257 is unreachable.♻️ Simplify
let is_keyframe = if obu_data.len() > obu_header_size { let data = &obu_data[obu_header_size..]; - if data.is_empty() { - false - } else { - let first_byte = data[0]; - let show_existing_frame = (first_byte >> 7) & 1; - if show_existing_frame == 1 { - self.current.contains_keyframe - } else { - let frame_type = (first_byte >> 5) & 0b11; - frame_type == 0 - } - } + let first_byte = data[0]; + let show_existing_frame = (first_byte >> 7) & 1; + if show_existing_frame == 1 { + self.current.contains_keyframe + } else { + let frame_type = (first_byte >> 5) & 0b11; + frame_type == 0 + } } else {rs/moq-mux/src/import/mod.rs (1)
17-17:aacandopusmodules are not feature-gated, though their features exist in Cargo.toml.
mod aacandmod opus(and theirpub usere-exports) are compiled unconditionally, even thoughaacandopusfeature flags are defined inCargo.toml. The other codecs (h264, h265, mp4, av1) are all guarded. This appears to be a pre-existing inconsistency rather than something introduced in this PR, but worth noting since you're touching the module wiring.Also applies to: 31-31, 33-33, 45-45
| fn init_from_av1c(&mut self, data: &[u8]) -> anyhow::Result<()> { | ||
| // Parse av1C box structure | ||
| let seq_profile = (data[1] >> 5) & 0x07; | ||
| let seq_level_idx = data[1] & 0x1F; | ||
| let tier = ((data[2] >> 7) & 0x01) == 1; | ||
| let high_bitdepth = ((data[2] >> 6) & 0x01) == 1; | ||
| let twelve_bit = ((data[2] >> 5) & 0x01) == 1; | ||
|
|
||
| let config = hang::catalog::VideoConfig { | ||
| // Resolution unknown from av1C - will be updated when first sequence header arrives | ||
| coded_width: None, | ||
| coded_height: None, | ||
| codec: hang::catalog::AV1 { | ||
| profile: seq_profile, | ||
| level: seq_level_idx, | ||
| tier: if tier { 'H' } else { 'M' }, | ||
| bitdepth: if high_bitdepth { | ||
| if twelve_bit { 12 } else { 10 } | ||
| } else { | ||
| 8 | ||
| }, | ||
| mono_chrome: ((data[2] >> 4) & 0x01) == 1, | ||
| chroma_subsampling_x: ((data[2] >> 3) & 0x01) == 1, | ||
| chroma_subsampling_y: ((data[2] >> 2) & 0x01) == 1, | ||
| chroma_sample_position: data[2] & 0x03, | ||
| color_primaries: 1, | ||
| transfer_characteristics: 1, | ||
| matrix_coefficients: 1, | ||
| full_range: false, | ||
| } | ||
| .into(), | ||
| description: None, | ||
| framerate: None, | ||
| bitrate: None, | ||
| display_ratio_width: None, | ||
| display_ratio_height: None, | ||
| optimize_for_latency: None, | ||
| container: hang::catalog::Container::Legacy, | ||
| jitter: None, | ||
| }; | ||
|
|
||
| let mut catalog = self.catalog.lock(); | ||
| let track = catalog.video.create_track("av01", config.clone()); | ||
| drop(catalog); | ||
|
|
||
| let track = self.broadcast.create_track(track); | ||
| self.config = Some(config); | ||
| self.track = Some(track.into()); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
init_from_av1c doesn't clean up existing track or deduplicate config, unlike init().
init() (line 88–97) checks for config equality and removes the old track before creating a new one. init_from_av1c always creates a new track without cleanup. If initialize() is called more than once with av1C data, the previous track leaks in the catalog.
🛡️ Suggested: add cleanup logic matching init()
fn init_from_av1c(&mut self, data: &[u8]) -> anyhow::Result<()> {
// ... config construction ...
+ if let Some(old) = &self.config && old == &config {
+ return Ok(());
+ }
+
+ if let Some(track) = self.track.take() {
+ tracing::debug!(name = ?track.info.name, "reinitializing track");
+ self.catalog.lock().video.remove_track(&track.info);
+ }
+
let mut catalog = self.catalog.lock();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn init_from_av1c(&mut self, data: &[u8]) -> anyhow::Result<()> { | |
| // Parse av1C box structure | |
| let seq_profile = (data[1] >> 5) & 0x07; | |
| let seq_level_idx = data[1] & 0x1F; | |
| let tier = ((data[2] >> 7) & 0x01) == 1; | |
| let high_bitdepth = ((data[2] >> 6) & 0x01) == 1; | |
| let twelve_bit = ((data[2] >> 5) & 0x01) == 1; | |
| let config = hang::catalog::VideoConfig { | |
| // Resolution unknown from av1C - will be updated when first sequence header arrives | |
| coded_width: None, | |
| coded_height: None, | |
| codec: hang::catalog::AV1 { | |
| profile: seq_profile, | |
| level: seq_level_idx, | |
| tier: if tier { 'H' } else { 'M' }, | |
| bitdepth: if high_bitdepth { | |
| if twelve_bit { 12 } else { 10 } | |
| } else { | |
| 8 | |
| }, | |
| mono_chrome: ((data[2] >> 4) & 0x01) == 1, | |
| chroma_subsampling_x: ((data[2] >> 3) & 0x01) == 1, | |
| chroma_subsampling_y: ((data[2] >> 2) & 0x01) == 1, | |
| chroma_sample_position: data[2] & 0x03, | |
| color_primaries: 1, | |
| transfer_characteristics: 1, | |
| matrix_coefficients: 1, | |
| full_range: false, | |
| } | |
| .into(), | |
| description: None, | |
| framerate: None, | |
| bitrate: None, | |
| display_ratio_width: None, | |
| display_ratio_height: None, | |
| optimize_for_latency: None, | |
| container: hang::catalog::Container::Legacy, | |
| jitter: None, | |
| }; | |
| let mut catalog = self.catalog.lock(); | |
| let track = catalog.video.create_track("av01", config.clone()); | |
| drop(catalog); | |
| let track = self.broadcast.create_track(track); | |
| self.config = Some(config); | |
| self.track = Some(track.into()); | |
| Ok(()) | |
| } | |
| fn init_from_av1c(&mut self, data: &[u8]) -> anyhow::Result<()> { | |
| // Parse av1C box structure | |
| let seq_profile = (data[1] >> 5) & 0x07; | |
| let seq_level_idx = data[1] & 0x1F; | |
| let tier = ((data[2] >> 7) & 0x01) == 1; | |
| let high_bitdepth = ((data[2] >> 6) & 0x01) == 1; | |
| let twelve_bit = ((data[2] >> 5) & 0x01) == 1; | |
| let config = hang::catalog::VideoConfig { | |
| // Resolution unknown from av1C - will be updated when first sequence header arrives | |
| coded_width: None, | |
| coded_height: None, | |
| codec: hang::catalog::AV1 { | |
| profile: seq_profile, | |
| level: seq_level_idx, | |
| tier: if tier { 'H' } else { 'M' }, | |
| bitdepth: if high_bitdepth { | |
| if twelve_bit { 12 } else { 10 } | |
| } else { | |
| 8 | |
| }, | |
| mono_chrome: ((data[2] >> 4) & 0x01) == 1, | |
| chroma_subsampling_x: ((data[2] >> 3) & 0x01) == 1, | |
| chroma_subsampling_y: ((data[2] >> 2) & 0x01) == 1, | |
| chroma_sample_position: data[2] & 0x03, | |
| color_primaries: 1, | |
| transfer_characteristics: 1, | |
| matrix_coefficients: 1, | |
| full_range: false, | |
| } | |
| .into(), | |
| description: None, | |
| framerate: None, | |
| bitrate: None, | |
| display_ratio_width: None, | |
| display_ratio_height: None, | |
| optimize_for_latency: None, | |
| container: hang::catalog::Container::Legacy, | |
| jitter: None, | |
| }; | |
| if let Some(old) = &self.config && old == &config { | |
| return Ok(()); | |
| } | |
| if let Some(track) = self.track.take() { | |
| tracing::debug!(name = ?track.info.name, "reinitializing track"); | |
| self.catalog.lock().video.remove_track(&track.info); | |
| } | |
| let mut catalog = self.catalog.lock(); | |
| let track = catalog.video.create_track("av01", config.clone()); | |
| drop(catalog); | |
| let track = self.broadcast.create_track(track); | |
| self.config = Some(config); | |
| self.track = Some(track.into()); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In `@rs/moq-mux/src/import/av01.rs` around lines 136 - 186, init_from_av1c
currently always creates a new track and leaks any previous one; change it to
mirror init() by (1) comparing the newly built config with self.config and
returning early if equal, and (2) if unequal, lock the catalog, remove the old
track from the catalog/broadcast (use the same removal calls used in init() for
removing the prior track referenced by self.track), then create the new catalog
track with catalog.video.create_track and call self.broadcast.create_track, and
finally update self.config and self.track. Ensure you use the same helper/method
names used in init() for removing tracks so behavior is consistent.
That's an existing race condition. I'm not sure what causes it, but it's harmless. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
The MoQ stuff looks gud. You figure out if the AI bot is hallucinating; I haven't used AV1 before.
rs/moq-mux/src/import/decoder.rs
Outdated
| #[cfg(feature = "mp4")] | ||
| "fmp4" | "cmaf" => Ok(StreamFormat::Fmp4), | ||
| #[cfg(feature = "av1")] | ||
| "av01" | "av1" => Ok(StreamFormat::Av01), |
There was a problem hiding this comment.
Maybe throw in av1C too since both are supported.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@rs/moq-mux/src/import/av01.rs`:
- Around line 161-164: The init_from_av1c implementation is hardcoding
color_primaries/transfer_characteristics/matrix_coefficients/full_range to
BT.709; update init_from_av1c to parse the av1C box's configOBUs (bytes 4+) to
extract those color metadata values (or if parsing is not implemented, remove
the hardcoded defaults and leave them unset/None or document why defaults are
acceptable) so init() won't be misled by incorrect defaults; look for symbols
init_from_av1c, av1C, configOBUs, and the fields
color_primaries/transfer_characteristics/matrix_coefficients/full_range to make
the change.
- Around line 350-359: The wall-clock PTS is currently captured once per
decode_stream/decode_frame and reused for all OBUs flushed in that call,
producing zero-duration frames; to fix, stop capturing a single pts at the start
and instead call pts() each time a new frame is started so each OBU flush gets a
fresh timestamp. Locate the code paths that call pts() (the pts method and its
current single-call site in decode_stream/decode_frame) and move/invoke
self.pts(None) at the point where frames are actually created/flushed (e.g.,
inside maybe_start_frame or right before constructing a frame for a
TemporalDelimiter OBU) so each frame gets a new Timestamp based on current
zero.elapsed(); keep the zero field and pts() semantics but ensure it is called
per-OBU flush rather than once per decode invocation.
- Around line 246-259: The payload_offset currently set to 1 or 2 ignores the
LEB128 size field and causes SequenceHeaderObu::parse and frame-type detection
to read the size bytes instead of the payload; fix by using the reader position
returned/advanced by ObuHeader::parse (i.e., use the actual payload start
computed by ObuHeader parsing) as the payload_offset when calling
SequenceHeaderObu::parse and when handling the FrameHeader | Frame arm, and
remove the manual obu_header_size calculation that tries to rederive the header
length so both arms consistently slice the true payload bytes.
🧹 Nitpick comments (2)
rs/moq-mux/src/import/av01.rs (2)
268-292: Redundantdata.is_empty()check after already verifyingobu_data.len() > obu_header_size.Line 270 checks
data.is_empty()butdatais&obu_data[obu_header_size..]which is guaranteed non-empty by the outerif obu_data.len() > obu_header_sizecondition. This is dead code.Also, note that the bit extraction at lines 275 and 280 is affected by the payload offset bug flagged above — once the offset is corrected, the frame type detection logic itself is sound.
294-296: Redundant keyframe propagation.
if is_keyframe || self.current.contains_keyframe { self.current.contains_keyframe = true; }can be simplified toself.current.contains_keyframe |= is_keyframe;— it's equivalent and clearer.Proposed simplification
- if is_keyframe || self.current.contains_keyframe { - self.current.contains_keyframe = true; - } + self.current.contains_keyframe |= is_keyframe;
christriants
force-pushed
the
feat/av1-decoder
branch
from
4508f8f to
2aca94f
Compare
This should be fixed with 56606ee. Good coderabbitai. |
|
Thanks Chris! Looks awesome |
2 participants
Related to: #740
MoQ OBS Plugin update: moq-dev/obs#20
This PR adds AV1 (av01) decoder support with inline sequence headers, following the format of the
avc3decoder. Key implementation details:Core Decoder (av01.rs):
Dual format support: Handles both av1C (MP4 container format) and raw OBU streams
initialize()- Detects format by examining first bytes: 0x0a signature with 16-byte length indicates av1C, otherwise treats as raw OBUsinit_from_av1c()- Parses 16-byteav1Cbox to extract codec parameters (profile, level, tier, etc). Resolution is set to None and deferred to inline sequence headers sinceav1Cdoesn't contain dimension infodecode_obu()- Processes individual Open Bitstream Units, handling sequence headers, frame headers, temporal delimiters, and tile groupsav1Cinitialization: Parses 16-byte av1C box to extract codec parameters (profile, level, tier, bit depth, chroma subsampling), defers resolution extraction to inline sequence headersSequence header handling: Uses
scuffle_av1crate to parse inline sequence headers for resolution and color configurationFrame detection:
show_existing_frameflag (bit 7) which OBS uses for display reordering - preserves keyframe state from preceding sequence header rather than parsing referenced framemaybe_start_frame()- Flushes completed frames to track when temporal delimiter or metadata OBU is encounteredOBU iteration: Custom iterator that parses LEB128-encoded size fields to split OBU boundaries in streams
API Alignment:
new(),initialize(),decode_stream(), anddecode_frame()methodsA couple of bugs I am trying to sort out / understand:
invalidStateError: Failed to execute 'clone' on 'VideoFrame': Cannot clone closed VideoFrame. at #runRender (renderer.ts:90:29)- I think we can fix this separately.