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?
WalkthroughThis pull request introduces AV1 video codec support to the moq-mux crate. A new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
| color_primaries: seq_header.color_config.color_primaries, | ||
| transfer_characteristics: seq_header.color_config.transfer_characteristics, | ||
| matrix_coefficients: seq_header.color_config.matrix_coefficients, | ||
| full_range: false, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n rs/moq-mux/src/import/av01.rs | head -100Repository: moq-dev/moq
Length of output: 3470
🏁 Script executed:
rg -n "full_range\|color_range\|ColorConfig" --type rust rs/moq-mux/src/import/av01.rsRepository: moq-dev/moq
Length of output: 37
🏁 Script executed:
rg -n "struct ColorConfig\|pub.*color" --type rust | head -20Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
rg -n "full_range_flag\|full_range\|video_full_range\|ColorConfig" --type rust -A 5 | head -50Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Search in dependencies or external crates
rg -n "struct ColorConfig" --type rust -A 10Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Check if there's a Cargo.lock or Cargo.toml that shows scuffle_av1 version
cat rs/moq-mux/Cargo.toml | grep -A 2 "scuffle_av1"Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
find rs -name "Cargo.toml" -type fRepository: moq-dev/moq
Length of output: 286
🏁 Script executed:
cat rs/moq-mux/Cargo.tomlRepository: moq-dev/moq
Length of output: 1561
🏁 Script executed:
rg "ColorConfig" --type rustRepository: moq-dev/moq
Length of output: 37
🏁 Script executed:
rg "color_config\." --type rust -B 2 -A 2Repository: moq-dev/moq
Length of output: 985
🏁 Script executed:
# Check if there are any examples or tests using color_config
rg "seq_header\|SequenceHeaderObu" --type rust -A 5 | head -60Repository: moq-dev/moq
Length of output: 37
🌐 Web query:
scuffle_av1 0.1.4 ColorConfig full_range field documentation
💡 Result:
In scuffle-av1 0.1.4, ColorConfig does not have a full_range field; the field is named:
pub full_color_range: bool— documented as AV1color_range(1 bit) in AV1-Spec-2 §5.5.2. [1]
Per the AV1 bitstream specification, color_range is binary:
0= studio swing (limited/legal range)1= full swing (full range)
So, full_color_range: true corresponds to full range, and false corresponds to limited range. [2]
Sources:
[1] scuffle-av1 0.1.4 ColorConfig rustdoc (docs.rs) (docs.rs)
[2] AV1 Bitstream & Decoding Process Specification (color_range semantics) (aomediacodec.github.io)
Citations:
- 1: https://docs.rs/scuffle-av1/latest/scuffle_av1/seq/struct.ColorConfig.html
- 2: https://aomediacodec.github.io/av1-spec/?utm_source=openai
🌐 Web query:
scuffle_av1 crate color_config fields AV1 specification
💡 Result:
In scuffle_av1, seq::ColorConfig is a Rust representation of AV1’s color_config() structure (AV1 Spec §5.5.2). Its fields map as follows:
scuffle_av1::seq::ColorConfig field |
AV1 spec name / variable in color_config() |
Notes |
|---|---|---|
bit_depth: i32 |
BitDepth |
Derived from seq_profile, high_bitdepth, and (if present) twelve_bit in color_config(). |
mono_chrome: bool |
mono_chrome |
1 bit. If true, NumPlanes becomes 1 and chroma-related values are forced/inferred by the spec. |
num_planes: u8 |
NumPlanes |
mono_chrome ? 1 : 3. |
color_primaries: u8 |
color_primaries |
Present only if color_description_present_flag == 1, else defaults to CP_UNSPECIFIED. |
transfer_characteristics: u8 |
transfer_characteristics |
Same presence/defaulting rules as color_primaries. |
matrix_coefficients: u8 |
matrix_coefficients |
Same presence/defaulting rules as color_primaries. If MC_IDENTITY, conformance requires subsampling_x = 0 and subsampling_y = 0. |
full_color_range: bool |
color_range |
1 bit (aka “video full range flag” / full swing vs studio swing). |
subsampling_x: bool |
subsampling_x |
Chroma subsampling signaled/inferred per seq_profile and BitDepth (and forced for monochrome / identity-RGB cases). |
subsampling_y: bool |
subsampling_y |
Same as above. |
chroma_sample_position: u8 |
chroma_sample_position |
Only present when subsampling_x && subsampling_y (i.e., 4:2:0). |
separate_uv_delta_q: bool |
separate_uv_delta_q |
1 bit; whether U and V can have separate delta quantizers. |
Sources: scuffle_av1::seq::ColorConfig docs/fields [1], AV1 color_config() syntax + semantics [2], and how these values are carried into ISOBMFF (e.g., videoFullRangeFlag, chroma_sample_position rules) [3].
References:
[1] https://docs.rs/scuffle-av1/latest/scuffle_av1/seq/struct.ColorConfig.html
[2] https://aomediacodec.github.io/av1-spec/
[3] https://aomediacodec.github.io/av1-isobmff/
Read full_range from sequence header's color_config.full_color_range instead of hardcoding to false.
The ColorConfig struct in scuffle_av1 exposes full_color_range: bool, which maps to the AV1 specification's color_range field. This determines whether the video uses full swing (full range) or studio swing (limited range) color. Hardcoding this to false causes incorrect color metadata for any stream that uses full-range color.
Change line 75 from full_range: false, to full_range: seq_header.color_config.full_color_range, to match the pattern used for other color config fields (bit_depth, color_primaries, transfer_characteristics, etc.).
🤖 Prompt for AI Agents
In `@rs/moq-mux/src/import/av01.rs` at 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.
| pub fn initialize<T: Buf + AsRef<[u8]>>(&mut self, buf: &mut T) -> anyhow::Result<()> { | ||
| let data = buf.as_ref(); | ||
|
|
||
| // Handle av1C format (MP4/container initialization) | ||
| if data.len() >= 4 && data[0] == 0x0a && data.len() == 16 { | ||
| self.init_from_av1c(data)?; | ||
| buf.advance(data.len()); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AV1CodecConfigurationRecord av1C first byte marker specification
💡 Result:
In AV1CodecConfigurationRecord (the payload of the ISOBMFF av1C box), the first byte is specified as:
marker= 1 bit, must be 1 (the MSB, bit 7)version= 7 bits, must be 1 (bits 6..0)
So the first byte value is:
- binary:
1 0000001 - hex:
0x81
The spec note says the marker bit is set so the first byte can’t be confused with an AV1 OBU header byte. [1]
Source: AOMedia “AV1 Codec ISO Media File Format Binding”, AV1CodecConfigurationRecord syntax/semantics. [1]
[1] (aomediacodec.github.io)
Citations:
🏁 Script executed:
# Examine the full context of the av01.rs file
cd rs/moq-mux && find . -name "av01.rs" -type fRepository: moq-dev/moq
Length of output: 75
🏁 Script executed:
# Look at the av01.rs file to see the full context and implementation
cat -n rs/moq-mux/src/import/av01.rs | head -150Repository: moq-dev/moq
Length of output: 5249
Incorrect av1C marker byte check — use 0x81 instead of 0x0a
The first byte of an AV1CodecConfigurationRecord (av1C box) must be 0x81 (marker=1, version=1), not 0x0a. The marker bit is explicitly set per the AOMedia specification to distinguish av1C from OBU headers. A 16-byte raw OBU starting with 0x0a (valid SequenceHeader OBU type) would be misidentified as av1C and incorrectly parsed. Change the check to data[0] == 0x81.
🤖 Prompt for AI Agents
In `@rs/moq-mux/src/import/av01.rs` around lines 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.
| 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.
| ObuType::SequenceHeader => { | ||
| match SequenceHeaderObu::parse(header, &mut &obu_data[1..]) { | ||
| Ok(seq_header) => { | ||
| self.init(&seq_header)?; | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!("Failed to parse sequence header OBU, skipping: {}", e); | ||
| } | ||
| } | ||
|
|
||
| self.current.contains_keyframe = true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the context around the mentioned lines
cat -n rs/moq-mux/src/import/av01.rs | sed -n '230,255p'Repository: moq-dev/moq
Length of output: 1033
🏁 Script executed:
# Check the ObuHeader struct definition
rg -n "struct ObuHeader\|pub.*has_extension" rs/moq-mux/src/import/av01.rs -A 10Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Check the SequenceHeaderObu::parse signature
rg -n "impl.*SequenceHeaderObu\|fn parse" rs/moq-mux/src/import/av01.rs -B 2 -A 5 | head -50Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Check for similar OBU parsing patterns in the codebase
rg -n "obu_data\[" rs/moq-mux/src/import/av01.rs -B 2 -A 2Repository: moq-dev/moq
Length of output: 757
🏁 Script executed:
# Check imports and ObuHeader definition
rg -n "use.*scuffle_av1|ObuHeader|has_extension" rs/moq-mux/src/import/av01.rs -B 1 -A 3 | head -40Repository: moq-dev/moq
Length of output: 812
🏁 Script executed:
# Check if has_extension is used anywhere in the codebase
rg -n "has_extension\|header\." rs/moq-mux/src/import/av01.rs | head -20Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Look for the definition of ObuHeader or search for its fields
find . -path "*/scuffle_av1*" -type f | head -10Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Check what fields ObuHeader might have by looking at how it's used
rg -n "header\." rs/moq-mux/src/import/av01.rs -A 1 | head -30Repository: moq-dev/moq
Length of output: 987
🏁 Script executed:
# Check Cargo.toml for scuffle_av1 dependency
cat rs/moq-mux/Cargo.toml | grep -A 5 scuffle_av1Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Search for scuffle_av1 source in the repo
find . -name "scuffle_av1*" -o -name "*scuffle*" -type d | grep -v target | head -10Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Check workspace Cargo.toml for scuffle
cat Cargo.toml | grep -A 5 scuffle_av1Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Try to find ObuHeader definition anywhere in the repo
rg -n "struct ObuHeader|pub struct ObuHeader" --type rustRepository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Read Cargo.toml directly
ls -la rs/moq-mux/Cargo.toml
cat rs/moq-mux/Cargo.tomlRepository: moq-dev/moq
Length of output: 1632
🏁 Script executed:
# Search for any Cargo.toml in the repo
find . -name "Cargo.toml" -type f | head -5Repository: moq-dev/moq
Length of output: 161
🏁 Script executed:
# Get more context around the Frame parsing to understand the pattern
cat -n rs/moq-mux/src/import/av01.rs | sed -n '250,270p'Repository: moq-dev/moq
Length of output: 785
🌐 Web query:
scuffle-av1 crate ObuHeader fields has_extension
💡 Result:
In scuffle-av1 (crate scuffle_av1), ObuHeader does not have a has_extension field. Instead, the presence of the OBU extension is represented by:
extension_header: Option<ObuExtensionHeader>—Some(...)iff the bitstream’sobu_extension_flagis1; otherwiseNone. So “has extension” is effectivelyheader.extension_header.is_some(). [1][2]
The full ObuHeader fields are: obu_type: ObuType, size: Option<u64>, and extension_header: Option<ObuExtensionHeader>. [1]
Where the flag goes: during parsing, the code reads extension_flag from the header bits and uses it only to decide whether to parse/populate extension_header; it does not store the boolean separately. [2]
Sources:
[1] ObuHeader rustdoc (fields) https://docs.rs/scuffle-av1/latest/scuffle_av1/struct.ObuHeader.html
[2] ObuHeader::parse implementation (reads extension_flag) https://docs.rs/scuffle-av1/latest/src/scuffle_av1/obu/mod.rs.html
🌐 Web query:
AV1 OBU extension flag bit position specification
💡 Result:
In the AV1 OBU header (obu_header()), obu_extension_flag is the 1-bit field immediately after obu_type:
obu_forbidden_bit (1) | obu_type (4) | obu_extension_flag (1) | obu_has_size_field (1) | obu_reserved_1bit (1) [1]
So, in the first OBU header byte (bit numbering b7 = MSB … b0 = LSB):
- b7:
obu_forbidden_bit - b6..b3:
obu_type - b2:
obu_extension_flag(mask 0x04) - b1:
obu_has_size_field(mask 0x02) - b0:
obu_reserved_1bit(mask 0x01) [1][2]
Sequence header OBU payload offset doesn't account for extension byte; keyframe flag set even on parse failure.
Two issues in this block:
-
&obu_data[1..]always skips exactly one byte. If the OBU has an extension byte, the sequence header payload starts at byte 2. Theheader.extension_headerfield indicates whether an extension is present — use it to compute the correct offset. -
Line 246 sets
contains_keyframe = trueunconditionally, even when parsing fails. A malformed sequence header shouldn't assert keyframe status, as the subsequent frame may be incorrectly tagged.
🐛 Proposed fix
ObuType::SequenceHeader => {
- match SequenceHeaderObu::parse(header, &mut &obu_data[1..]) {
+ let obu_header_size = if header.extension_header.is_some() { 2 } else { 1 };
+ match SequenceHeaderObu::parse(header, &mut &obu_data[obu_header_size..]) {
Ok(seq_header) => {
self.init(&seq_header)?;
+ self.current.contains_keyframe = true;
}
Err(e) => {
tracing::warn!("Failed to parse sequence header OBU, skipping: {}", e);
}
}
-
- self.current.contains_keyframe = true;
}📝 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.
| ObuType::SequenceHeader => { | |
| match SequenceHeaderObu::parse(header, &mut &obu_data[1..]) { | |
| Ok(seq_header) => { | |
| self.init(&seq_header)?; | |
| } | |
| Err(e) => { | |
| tracing::warn!("Failed to parse sequence header OBU, skipping: {}", e); | |
| } | |
| } | |
| self.current.contains_keyframe = true; | |
| ObuType::SequenceHeader => { | |
| let obu_header_size = if header.extension_header.is_some() { 2 } else { 1 }; | |
| match SequenceHeaderObu::parse(header, &mut &obu_data[obu_header_size..]) { | |
| Ok(seq_header) => { | |
| self.init(&seq_header)?; | |
| self.current.contains_keyframe = true; | |
| } | |
| Err(e) => { | |
| tracing::warn!("Failed to parse sequence header OBU, skipping: {}", e); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@rs/moq-mux/src/import/av01.rs` around lines 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.
| let header = data[0]; | ||
| let has_size = (header >> 1) & 1 == 1; | ||
|
|
||
| if !has_size { | ||
| let remaining = self.buf.remaining(); | ||
| let obu = self.buf.copy_to_bytes(remaining); | ||
| return Some(Ok(obu)); | ||
| } | ||
|
|
||
| // Parse LEB128 size | ||
| let mut size: usize = 0; | ||
| let mut offset = 1; | ||
| let mut shift = 0; | ||
|
|
||
| loop { | ||
| if offset >= data.len() { | ||
| return None; | ||
| } | ||
|
|
||
| let byte = data[offset]; | ||
| offset += 1; | ||
|
|
||
| size |= ((byte & 0x7F) as usize) << shift; | ||
| shift += 7; | ||
|
|
||
| if byte & 0x80 == 0 { | ||
| break; | ||
| } | ||
|
|
||
| if shift >= 56 { | ||
| return Some(Err(anyhow::anyhow!("OBU size too large"))); | ||
| } | ||
| } | ||
|
|
||
| let total_size = offset + size; | ||
|
|
||
| if total_size > self.buf.remaining() { | ||
| return None; | ||
| } | ||
|
|
||
| let obu = self.buf.copy_to_bytes(total_size); | ||
| Some(Ok(obu)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the ObuIterator context and see the full method
cat -n rs/moq-mux/src/import/av01.rs | sed -n '358,442p'Repository: moq-dev/moq
Length of output: 2335
🏁 Script executed:
#!/bin/bash
# Check how SequenceHeaderObu::parse is called to understand expected input
rg -B 5 -A 5 "SequenceHeaderObu::parse" rs/moq-mux/src/import/av01.rsRepository: moq-dev/moq
Length of output: 405
🏁 Script executed:
#!/bin/bash
# Look at how other decoders (avc3, hev1) handle similar OBU/NAL unit parsing
ls -la rs/moq-mux/src/import/ | grep -E "\.(rs|toml)"Repository: moq-dev/moq
Length of output: 636
🏁 Script executed:
#!/bin/bash
# Check if there are similar iterator patterns in other decoders
rg -n "has_size\|has_extension" rs/moq-mux/src/import/Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Look at the full decode_obu function to see how data is being sliced before passing to parse functions
cat -n rs/moq-mux/src/import/av01.rs | sed -n '227,305p'Repository: moq-dev/moq
Length of output: 2679
🏁 Script executed:
#!/bin/bash
# Check the scuffle_av1 dependency version and explore its crate
rg "scuffle" rs/moq-mux/Cargo.tomlRepository: moq-dev/moq
Length of output: 215
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation in the code about OBU format handling
rg -B 3 -A 3 "obu_extension" rs/moq-mux/src/import/av01.rsRepository: moq-dev/moq
Length of output: 217
🏁 Script executed:
#!/bin/bash
# Check if hev1.rs or avc3.rs handle NAL unit extension bytes for comparison
rg -n "extension\|offset" rs/moq-mux/src/import/hev1.rs | head -20Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Let's verify the consistency: Check if decode_obu is being applied correctly and whether the review's concern is valid
# by examining if there are other places in the code that handle OBU parsing
rg -n "ObuHeader::parse\|SequenceHeaderObu::parse\|FrameHeaderObu::parse" rs/moq-mux/src/import/Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check the actual API of scuffle_av1 to understand what parse functions expect
# First, let's see if there are any test cases or examples
rg -B 5 -A 10 "ObuHeader::parse" rs/moq-mux/src/import/av01.rsRepository: moq-dev/moq
Length of output: 587
🏁 Script executed:
#!/bin/bash
# Critical: Let's check if the extension byte is indeed being skipped incorrectly in SequenceHeaderObu::parse
# by looking at similar patterns in scuffle-h265 usage in hev1.rs
cat -n rs/moq-mux/src/import/hev1.rs | head -100Repository: moq-dev/moq
Length of output: 3721
🏁 Script executed:
#!/bin/bash
# Let's understand the ObuHeader::parse behavior by checking if it returns info about extension byte
# and verify the actual position after parsing
cat -n rs/moq-mux/src/import/av01.rs | sed -n '227,245p'Repository: moq-dev/moq
Length of output: 811
🏁 Script executed:
#!/bin/bash
# Let's check if there's test code or integration tests that would catch this ObuIterator bug
find rs/moq-mux -name "*.rs" -type f | xargs grep -l "ObuIterator\|test.*av01\|#\[test\]" | head -5Repository: moq-dev/moq
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Check if the extension byte is being checked in ObuIterator (which is the snippet in question)
# Looking at lines around 400-441 more carefully
cat -n rs/moq-mux/src/import/av01.rs | sed -n '358,442p' | grep -E "extension|offset|header"Repository: moq-dev/moq
Length of output: 406
🏁 Script executed:
#!/bin/bash
# Let's trace through exactly what happens: check if ObuIterator is actually being used
# and whether it processes OBUs before or after they're individually extracted
cat -n rs/moq-mux/src/import/av01.rs | sed -n '188,225p'Repository: moq-dev/moq
Length of output: 1187
🏁 Script executed:
#!/bin/bash
# Verify the OBU header format specification by checking comments and structure
# and confirm the bit positions for extension flag
cat -n rs/moq-mux/src/import/av01.rs | sed -n '393,402p'Repository: moq-dev/moq
Length of output: 353
🏁 Script executed:
#!/bin/bash
# Cross-check: In decode_obu, does the code handle extension bytes when it receives OBUs from ObuIterator?
# This will tell us if ObuIterator is supposed to handle extension bytes
cat -n rs/moq-mux/src/import/av01.rs | sed -n '250,260p'Repository: moq-dev/moq
Length of output: 487
🏁 Script executed:
#!/bin/bash
# Let's verify if scuffle_av1::ObuHeader::parse includes information about extension byte size
# by examining how the parsed header is used
rg -A 15 "let header = scuffle_av1::ObuHeader::parse" rs/moq-mux/src/import/av01.rs | head -20Repository: moq-dev/moq
Length of output: 529
🏁 Script executed:
#!/bin/bash
# Examine what the header type contains
rg "ObuHeader" rs/moq-mux/src/import/av01.rsRepository: moq-dev/moq
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Check if there are any test files that verify ObuIterator behavior
find rs/moq-mux -name "*.rs" -exec grep -l "ObuIterator\|#\[cfg(test)\]" {} \;Repository: moq-dev/moq
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Let's look at the specific bit calculation for extension flag in both places
# to confirm they're consistent
rg ">> 2.*& 1\|extension_flag" rs/moq-mux/src/import/av01.rsRepository: moq-dev/moq
Length of output: 37
OBU extension byte not accounted for when parsing the LEB128 size field in ObuIterator.
The LEB128 size field offset is hardcoded to 1 (line 411), but when obu_extension_flag is set in the header byte, the extension byte occupies the second byte, so the size field actually starts at offset 2. This causes incorrect OBU boundary calculation for any OBU with an extension byte, corrupting all subsequent OBU splits.
The decode_obu function already demonstrates the correct pattern for handling this (lines 252–253), where it checks the extension flag and adjusts the header size accordingly. The same logic should be applied in ObuIterator::next.
🐛 Proposed fix
let header = data[0];
let has_size = (header >> 1) & 1 == 1;
if !has_size {
let remaining = self.buf.remaining();
let obu = self.buf.copy_to_bytes(remaining);
return Some(Ok(obu));
}
// Parse LEB128 size
let mut size: usize = 0;
-let mut offset = 1;
+let has_extension = (header >> 2) & 1 == 1;
+let mut offset = if has_extension { 2 } else { 1 };
let mut shift = 0;📝 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.
| let header = data[0]; | |
| let has_size = (header >> 1) & 1 == 1; | |
| if !has_size { | |
| let remaining = self.buf.remaining(); | |
| let obu = self.buf.copy_to_bytes(remaining); | |
| return Some(Ok(obu)); | |
| } | |
| // Parse LEB128 size | |
| let mut size: usize = 0; | |
| let mut offset = 1; | |
| let mut shift = 0; | |
| loop { | |
| if offset >= data.len() { | |
| return None; | |
| } | |
| let byte = data[offset]; | |
| offset += 1; | |
| size |= ((byte & 0x7F) as usize) << shift; | |
| shift += 7; | |
| if byte & 0x80 == 0 { | |
| break; | |
| } | |
| if shift >= 56 { | |
| return Some(Err(anyhow::anyhow!("OBU size too large"))); | |
| } | |
| } | |
| let total_size = offset + size; | |
| if total_size > self.buf.remaining() { | |
| return None; | |
| } | |
| let obu = self.buf.copy_to_bytes(total_size); | |
| Some(Ok(obu)) | |
| let header = data[0]; | |
| let has_size = (header >> 1) & 1 == 1; | |
| if !has_size { | |
| let remaining = self.buf.remaining(); | |
| let obu = self.buf.copy_to_bytes(remaining); | |
| return Some(Ok(obu)); | |
| } | |
| // Parse LEB128 size | |
| let mut size: usize = 0; | |
| let has_extension = (header >> 2) & 1 == 1; | |
| let mut offset = if has_extension { 2 } else { 1 }; | |
| let mut shift = 0; | |
| loop { | |
| if offset >= data.len() { | |
| return None; | |
| } | |
| let byte = data[offset]; | |
| offset += 1; | |
| size |= ((byte & 0x7F) as usize) << shift; | |
| shift += 7; | |
| if byte & 0x80 == 0 { | |
| break; | |
| } | |
| if shift >= 56 { | |
| return Some(Err(anyhow::anyhow!("OBU size too large"))); | |
| } | |
| } | |
| let total_size = offset + size; | |
| if total_size > self.buf.remaining() { | |
| return None; | |
| } | |
| let obu = self.buf.copy_to_bytes(total_size); | |
| Some(Ok(obu)) |
🤖 Prompt for AI Agents
In `@rs/moq-mux/src/import/av01.rs` around lines 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).
1 participant
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.