hev1 decoder #813
hev1 decoder #813
Conversation
WalkthroughAdds H.265 support and Annex B parsing: introduces dependency Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 1
🧹 Nitpick comments (2)
rs/hang/src/import/hev1.rs (2)
318-328: Minor: Use implicit return for idiomatic Rust.The explicit
returnstatement is non-idiomatic in Rust when it's the last expression.🔎 Suggested fix
fn pack_constraint_flags(profile: &scuffle_h265::Profile) -> [u8; 6] { let mut flags = [0u8; 6]; flags[0] = ((profile.progressive_source_flag as u8) << 7) | ((profile.interlaced_source_flag as u8) << 6) | ((profile.non_packed_constraint_flag as u8) << 5) | ((profile.frame_only_constraint_flag as u8) << 4); // @todo: pack the rest of the optional flags in profile.additional_flags - return flags; + flags }
374-396: Consider adding inline unit tests.As per coding guidelines, Rust crates should have tests integrated within source files using inline test modules. Consider adding a
#[cfg(test)]module with tests for:
decode_nalwith various NAL typespack_constraint_flagsoutput verificationaspect_ratio_from_idcmappingsget_vui_dataextraction logicWould you like me to generate a starter test module for this file?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rs/hang/Cargo.tomlrs/hang/src/import/annexb.rsrs/hang/src/import/avc3.rsrs/hang/src/import/decoder.rsrs/hang/src/import/hev1.rsrs/hang/src/import/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
rs/**/Cargo.toml
📄 CodeRabbit inference engine (CLAUDE.md)
For Rust development, use the workspace configuration in
rs/Cargo.toml
Files:
rs/hang/Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
In Rust crates, tests should be integrated within source files using inline test modules
Files:
rs/hang/src/import/mod.rsrs/hang/src/import/avc3.rsrs/hang/src/import/annexb.rsrs/hang/src/import/decoder.rsrs/hang/src/import/hev1.rs
🧬 Code graph analysis (2)
rs/hang/src/import/decoder.rs (1)
rs/hang/src/import/hev1.rs (2)
new(32-40)pts(267-274)
rs/hang/src/import/hev1.rs (4)
rs/hang/src/import/annexb.rs (1)
new(12-14)rs/hang/src/import/avc3.rs (10)
new(30-38)init(40-98)initialize(101-113)decode_stream(121-136)pts(248-255)decode_frame(145-168)decode_nal(170-218)maybe_start_frame(220-242)is_initialized(244-246)drop(259-264)rs/hang/src/import/decoder.rs (5)
new(80-90)initialize(98-110)decode_stream(125-136)decode_frame(148-162)is_initialized(165-173)rs/hang/src/model/timestamp.rs (1)
from_micros(40-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (23)
rs/hang/src/import/mod.rs (1)
2-2: LGTM! Clean module organization.The new
annexbandhev1modules follow the existing pattern consistently with proper declarations and re-exports.Also applies to: 6-6, 11-11, 15-15
rs/hang/src/import/decoder.rs (1)
5-5: LGTM! Hev1 integration follows the established Avc3 pattern.The Hev1 decoder is properly integrated across all decoder methods with consistent handling. The deprecation warnings for "h265"/"hevc" aliases are a nice touch for backward compatibility.
Also applies to: 15-16, 33-37, 51-51, 64-65, 84-84, 102-102, 129-129, 156-156, 169-169
rs/hang/src/import/avc3.rs (1)
1-3: LGTM! Clean refactoring to use the shared annexb module.The migration from local NAL parsing logic to the centralized
annexbmodule improves code reusability without changing the logic.rs/hang/src/import/annexb.rs (6)
4-4: LGTM! Standard Annex B start code.The 4-byte start code constant is correctly defined using
Bytes::from_staticfor efficient cloning.
6-32: LGTM! Well-designed NAL iterator with flush support.The iterator caches the start code position for efficient flushing, and properly handles both cached and uncached start positions.
34-53: LGTM! Iterator implementation is correct.The iterator properly handles error propagation, advances the buffer correctly, and caches start positions for efficient iteration.
56-72: LGTM! Robust start code validation.The function correctly validates both 3-byte and 4-byte Annex B start codes with appropriate error handling for incomplete or invalid patterns.
75-135: LGTM! Clever optimization for start code detection.The algorithm efficiently searches for start codes by checking the 3rd byte first, allowing skips of up to 3 bytes when no pattern is detected. The implementation correctly handles both 3-byte and 4-byte start codes with proper edge case handling.
137-535: LGTM! Excellent test coverage.The test suite is comprehensive and covers:
- Valid 3-byte and 4-byte start codes
- Incomplete and invalid patterns
- Edge cases (empty NALs, embedded zeros, consecutive zeros)
- Realistic H.264 and H.265 streams
- Flush behavior in various scenarios
The tests provide strong confidence in the correctness of the NAL parsing logic.
rs/hang/Cargo.toml (1)
32-32: Version 0.2.2 ofscuffle-h265is valid and currently the latest published release. No known security vulnerabilities or CVEs have been identified for this package.rs/hang/src/import/hev1.rs (13)
10-29: LGTM!The struct definition follows the established pattern from
Avc3and includes appropriate documentation noting the single-layer limitation.
31-40: LGTM!Constructor follows the established pattern from
Avc3::new().
42-98: LGTM!The initialization logic correctly mirrors
Avc3::init()- handles config comparison to avoid redundant reinitialization, properly removes old tracks from the catalog before creating new ones, and extracts H.265-specific codec parameters from the SPS.
100-113: LGTM!Follows the established
Avc3::initialize()pattern exactly.
115-136: LGTM!The streaming decode implementation matches the
Avc3pattern. The TODO about PTS association is acknowledged and documented.
138-168: LGTM!Follows the established
Avc3::decode_frame()pattern - iterates NALs, flushes the final NAL, and triggers frame emission.
239-261: LGTM!Frame emission logic correctly matches
Avc3::maybe_start_frame().
263-274: LGTM!Both methods follow the
Avc3pattern. Theu128tou64cast inpts()is safe for practical stream durations.
277-284: LGTM!Proper cleanup on drop, matching
Avc3'sDropimplementation.
286-316: LGTM!The NAL type values correctly match ITU H.265 Table 7-1, and reserved ranges are appropriately excluded.
330-335: LGTM!Frame accumulation struct follows the established pattern.
337-372: LGTM!VUI parameter extraction is well-structured. The use of
NonZerotypes (via.get()) for timing values safely prevents division by zero.
374-396: LGTM!Aspect ratio mapping covers all predefined SAR values with appropriate fallbacks for unspecified and reserved values.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rs/hang/src/import/hev1.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
In Rust crates, tests should be integrated within source files using inline test modules
Files:
rs/hang/src/import/hev1.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (1)
rs/hang/src/import/hev1.rs (1)
170-237: NAL parsing logic looks solid.The NAL unit parsing correctly:
- Validates the forbidden zero bit
- Extracts the NAL unit type from the first header byte
- Handles HEVC-specific frame boundaries (SPS, AUD, PPS, SEI, keyframe slices, non-keyframe slices)
- Checks
first_slice_segment_in_pic_flagfor slice NALs to detect frame boundaries- Accumulates NALs with 4-byte start codes for downstream processing
The logic aligns well with the HEVC specification and the pattern established by the existing
avc3decoder.
| use crate as hang; | ||
| use crate::import::annexb::{NalIterator, START_CODE}; | ||
|
|
||
| use anyhow::Context; | ||
| use buf_list::BufList; | ||
| use bytes::{Buf, Bytes}; | ||
| use moq_lite as moq; | ||
| use scuffle_h265::SpsNALUnit; | ||
|
|
||
| /// A decoder for H.265 with inline SPS/PPS. | ||
| /// Only supports single layer streams, ignores VPS. | ||
| pub struct Hev1 { | ||
| // The broadcast being produced. | ||
| // This `hang` variant includes a catalog. | ||
| broadcast: hang::BroadcastProducer, | ||
|
|
||
| // The track being produced. | ||
| track: Option<hang::TrackProducer>, | ||
|
|
||
| // Whether the track has been initialized. | ||
| // If it changes, then we'll reinitialize with a new track. | ||
| config: Option<hang::catalog::VideoConfig>, | ||
|
|
||
| // The current frame being built. | ||
| current: Frame, | ||
|
|
||
| // Used to compute wall clock timestamps if needed. | ||
| zero: Option<tokio::time::Instant>, | ||
| } | ||
|
|
||
| impl Hev1 { | ||
| pub fn new(broadcast: hang::BroadcastProducer) -> Self { | ||
| Self { | ||
| broadcast, | ||
| track: None, | ||
| config: None, | ||
| current: Default::default(), | ||
| zero: None, | ||
| } | ||
| } | ||
|
|
||
| fn init(&mut self, sps: &SpsNALUnit) -> anyhow::Result<()> { | ||
| let profile = &sps.rbsp.profile_tier_level.general_profile; | ||
| let vui_data = get_vui_data(&sps.rbsp.vui_parameters); | ||
|
|
||
| let config = hang::catalog::VideoConfig { | ||
| coded_width: Some(sps.rbsp.cropped_width() as u32), | ||
| coded_height: Some(sps.rbsp.cropped_height() as u32), | ||
| codec: hang::catalog::H265 { | ||
| in_band: true, // We only support `hev1` with inline SPS/PPS for now | ||
| profile_space: profile.profile_space, | ||
| profile_idc: profile.profile_idc, | ||
| profile_compatibility_flags: profile.profile_compatibility_flag.bits().to_be_bytes(), | ||
| tier_flag: profile.tier_flag, | ||
| level_idc: profile.level_idc.context("missing level_idc in SPS")?, | ||
| constraint_flags: pack_constraint_flags(profile), | ||
| } | ||
| .into(), | ||
| description: None, | ||
| framerate: vui_data.framerate, | ||
| bitrate: None, | ||
| display_ratio_width: vui_data.display_ratio_width, | ||
| display_ratio_height: vui_data.display_ratio_height, | ||
| optimize_for_latency: None, | ||
| }; | ||
|
|
||
| if let Some(old) = &self.config { | ||
| if old == &config { | ||
| return Ok(()); | ||
| } | ||
| } | ||
|
|
||
| if let Some(track) = &self.track.take() { | ||
| tracing::debug!(name = ?track.info.name, "reinitializing track"); | ||
| self.broadcast.catalog.lock().remove_video(&track.info.name); | ||
| } | ||
|
|
||
| let track = moq::Track { | ||
| name: self.broadcast.track_name("video"), | ||
| priority: 2, | ||
| }; | ||
|
|
||
| tracing::debug!(name = ?track.name, ?config, "starting track"); | ||
|
|
||
| { | ||
| let mut catalog = self.broadcast.catalog.lock(); | ||
| let video = catalog.insert_video(track.name.clone(), config.clone()); | ||
| video.priority = 2; | ||
| } | ||
|
|
||
| let track = track.produce(); | ||
| self.broadcast.insert_track(track.consumer); | ||
|
|
||
| self.config = Some(config); | ||
| self.track = Some(track.producer.into()); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Initialize the decoder with SPS/PPS and other non-slice NALs. | ||
| pub fn initialize<T: Buf + AsRef<[u8]>>(&mut self, buf: &mut T) -> anyhow::Result<()> { | ||
| let mut nals = NalIterator::new(buf); | ||
|
|
||
| while let Some(nal) = nals.next().transpose()? { | ||
| self.decode_nal(nal, None)?; | ||
| } | ||
|
|
||
| if let Some(nal) = nals.flush()? { | ||
| self.decode_nal(nal, None)?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Decode as much data as possible from the given buffer. | ||
| /// | ||
| /// Unlike [Self::decode_frame], this method needs the start code for the next frame. | ||
| /// This means it works for streaming media (ex. stdin) but adds a frame of latency. | ||
| /// | ||
| /// TODO: This currently associates PTS with the *previous* frame, as part of `maybe_start_frame`. | ||
| pub fn decode_stream<T: Buf + AsRef<[u8]>>( | ||
| &mut self, | ||
| buf: &mut T, | ||
| pts: Option<hang::Timestamp>, | ||
| ) -> anyhow::Result<()> { | ||
| let pts = self.pts(pts)?; | ||
|
|
||
| // Iterate over the NAL units in the buffer based on start codes. | ||
| let nals = NalIterator::new(buf); | ||
|
|
||
| for nal in nals { | ||
| self.decode_nal(nal?, Some(pts))?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Decode all data in the buffer, assuming the buffer contains (the rest of) a frame. | ||
| /// | ||
| /// Unlike [Self::decode_stream], this is called when we know NAL boundaries. | ||
| /// This can avoid a frame of latency just waiting for the next frame's start code. | ||
| /// This can also be used when EOF is detected to flush the final frame. | ||
| /// | ||
| /// NOTE: The next decode will fail if it doesn't begin with a start code. | ||
| pub fn decode_frame<T: Buf + AsRef<[u8]>>( | ||
| &mut self, | ||
| buf: &mut T, | ||
| pts: Option<hang::Timestamp>, | ||
| ) -> anyhow::Result<()> { | ||
| let pts = self.pts(pts)?; | ||
| // Iterate over the NAL units in the buffer based on start codes. | ||
| let mut nals = NalIterator::new(buf); | ||
|
|
||
| // Iterate over each NAL that is followed by a start code. | ||
| while let Some(nal) = nals.next().transpose()? { | ||
| self.decode_nal(nal, Some(pts))?; | ||
| } | ||
|
|
||
| // Assume the rest of the buffer is a single NAL. | ||
| if let Some(nal) = nals.flush()? { | ||
| self.decode_nal(nal, Some(pts))?; | ||
| } | ||
|
|
||
| // Flush the frame if we read a slice. | ||
| self.maybe_start_frame(Some(pts))?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Decode a single NAL unit. Only reads the first header byte to extract nal_unit_type, | ||
| /// Ignores nuh_layer_id and nuh_temporal_id_plus1. | ||
| fn decode_nal(&mut self, nal: Bytes, pts: Option<hang::Timestamp>) -> anyhow::Result<()> { | ||
| anyhow::ensure!(nal.len() >= 2, "NAL unit is too short"); | ||
| // u16 header: [forbidden_zero_bit(1) | nal_unit_type(6) | nuh_layer_id(6) | nuh_temporal_id_plus1(3)] | ||
| let header = nal.first().context("NAL unit is too short")?; | ||
|
|
||
| let forbidden_zero_bit = (header >> 7) & 1; | ||
| anyhow::ensure!(forbidden_zero_bit == 0, "forbidden zero bit is not zero"); | ||
|
|
||
| // Bits 1-6: nal_unit_type | ||
| let nal_unit_type = (header >> 1) & 0b111111; | ||
| let nal_type = HevcNalType::try_from(nal_unit_type).ok(); | ||
|
|
||
| match nal_type { | ||
| Some(HevcNalType::Sps) => { | ||
| self.maybe_start_frame(pts)?; | ||
|
|
||
| // Try to reinitialize the track if the SPS has changed. | ||
| let sps = SpsNALUnit::parse(&mut &nal[..]).context("failed to parse SPS NAL unit")?; | ||
| self.init(&sps)?; | ||
| } | ||
| // TODO parse the SPS again and reinitialize the track if needed | ||
| Some(HevcNalType::Aud | HevcNalType::Pps | HevcNalType::SeiPrefix | HevcNalType::SeiSuffix) => { | ||
| self.maybe_start_frame(pts)?; | ||
| } | ||
| // Keyframe containing slices | ||
| Some( | ||
| HevcNalType::IdrWRadl | ||
| | HevcNalType::IdrNLp | ||
| | HevcNalType::BlaNLp | ||
| | HevcNalType::BlaWRadl | ||
| | HevcNalType::BlaWLp | ||
| | HevcNalType::Cra, | ||
| ) => { | ||
| self.current.contains_idr = true; | ||
| self.current.contains_slice = true; | ||
| } | ||
| // All other slice types (both N and R variants) | ||
| Some( | ||
| HevcNalType::TrailN | ||
| | HevcNalType::TrailR | ||
| | HevcNalType::TsaN | ||
| | HevcNalType::TsaR | ||
| | HevcNalType::StsaN | ||
| | HevcNalType::StsaR | ||
| | HevcNalType::RadlN | ||
| | HevcNalType::RadlR | ||
| | HevcNalType::RaslN | ||
| | HevcNalType::RaslR, | ||
| ) => { | ||
| // Check first_slice_segment_in_pic_flag (bit 7 of third byte, after 2-byte header) | ||
| if nal.get(2).context("NAL unit is too short")? & 0x80 != 0 { | ||
| self.maybe_start_frame(pts)?; | ||
| } | ||
| self.current.contains_slice = true; | ||
| } | ||
| _ => {} | ||
| } | ||
|
|
||
| // Rather than keeping the original size of the start code, we replace it with a 4 byte start code. | ||
| // It's just marginally easier and potentially more efficient down the line (JS player with MSE). | ||
| // NOTE: This is ref-counted and static, so it's extremely cheap to clone. | ||
| self.current.chunks.push_chunk(START_CODE.clone()); | ||
| self.current.chunks.push_chunk(nal); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn maybe_start_frame(&mut self, pts: Option<hang::Timestamp>) -> anyhow::Result<()> { | ||
| // If we haven't seen any slices, we shouldn't flush yet. | ||
| if !self.current.contains_slice { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let track = self.track.as_mut().context("expected SPS before any frames")?; | ||
| let pts = pts.context("missing timestamp")?; | ||
|
|
||
| let payload = std::mem::take(&mut self.current.chunks); | ||
| let frame = hang::Frame { | ||
| timestamp: pts, | ||
| keyframe: self.current.contains_idr, | ||
| payload, | ||
| }; | ||
|
|
||
| track.write(frame)?; | ||
|
|
||
| self.current.contains_idr = false; | ||
| self.current.contains_slice = false; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn is_initialized(&self) -> bool { | ||
| self.track.is_some() | ||
| } | ||
|
|
||
| fn pts(&mut self, hint: Option<hang::Timestamp>) -> anyhow::Result<hang::Timestamp> { | ||
| if let Some(pts) = hint { | ||
| return Ok(pts); | ||
| } | ||
|
|
||
| let zero = self.zero.get_or_insert_with(tokio::time::Instant::now); | ||
| Ok(hang::Timestamp::from_micros(zero.elapsed().as_micros() as u64)?) | ||
| } | ||
| } | ||
|
|
||
| impl Drop for Hev1 { | ||
| fn drop(&mut self) { | ||
| if let Some(track) = &self.track { | ||
| tracing::debug!(name = ?track.info.name, "ending track"); | ||
| self.broadcast.catalog.lock().remove_video(&track.info.name); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, num_enum::TryFromPrimitive)] | ||
| #[repr(u8)] | ||
| pub enum HevcNalType { | ||
| TrailN = 0, | ||
| TrailR = 1, | ||
| TsaN = 2, | ||
| TsaR = 3, | ||
| StsaN = 4, | ||
| StsaR = 5, | ||
| RadlN = 6, | ||
| RadlR = 7, | ||
| RaslN = 8, | ||
| RaslR = 9, | ||
| // 10 -> 15 reserved | ||
| BlaWLp = 16, | ||
| BlaWRadl = 17, | ||
| BlaNLp = 18, | ||
| IdrWRadl = 19, | ||
| IdrNLp = 20, | ||
| Cra = 21, | ||
| // 22 -> 31 reserved | ||
| Vps = 32, | ||
| Sps = 33, | ||
| Pps = 34, | ||
| Aud = 35, | ||
| EndOfSequence = 36, | ||
| EndOfBitstream = 37, | ||
| Filler = 38, | ||
| SeiPrefix = 39, | ||
| SeiSuffix = 40, | ||
| } // ITU H.265 V10 Table 7-1 – NAL unit type codes and NAL unit type classes | ||
|
|
||
| // Packs the constraint flags from ITU H.265 V10 Section 7.3.3 Profile, tier and level syntax | ||
| fn pack_constraint_flags(profile: &scuffle_h265::Profile) -> [u8; 6] { | ||
| let mut flags = [0u8; 6]; | ||
| flags[0] = ((profile.progressive_source_flag as u8) << 7) | ||
| | ((profile.interlaced_source_flag as u8) << 6) | ||
| | ((profile.non_packed_constraint_flag as u8) << 5) | ||
| | ((profile.frame_only_constraint_flag as u8) << 4); | ||
|
|
||
| // @todo: pack the rest of the optional flags in profile.additional_flags | ||
| flags | ||
| } | ||
|
|
||
| #[derive(Default)] | ||
| struct Frame { | ||
| chunks: BufList, | ||
| contains_idr: bool, | ||
| contains_slice: bool, | ||
| } | ||
|
|
||
| struct VuiData { | ||
| framerate: Option<f64>, | ||
| display_ratio_width: Option<u32>, | ||
| display_ratio_height: Option<u32>, | ||
| } | ||
|
|
||
| fn get_vui_data(vui: &Option<scuffle_h265::VuiParameters>) -> VuiData { | ||
| // FPS = time_scale / num_units_in_tick | ||
| let framerate = vui | ||
| .as_ref() | ||
| .and_then(|v| v.vui_timing_info.as_ref()) | ||
| .map(|t| t.time_scale.get() as f64 / t.num_units_in_tick.get() as f64); | ||
|
|
||
| let (display_ratio_width, display_ratio_height) = vui | ||
| .as_ref() | ||
| .map(|v| &v.aspect_ratio_info) | ||
| .and_then(|ar| { | ||
| match ar { | ||
| // Extended SAR has explicit arbitrary values for width and height. | ||
| scuffle_h265::AspectRatioInfo::ExtendedSar { sar_width, sar_height } => { | ||
| Some((Some(*sar_width as u32), Some(*sar_height as u32))) | ||
| } | ||
| // Predefined map to known values. | ||
| scuffle_h265::AspectRatioInfo::Predefined(idc) => { | ||
| aspect_ratio_from_idc(*idc).map(|(w, h)| (Some(w), Some(h))) | ||
| } | ||
| } | ||
| }) | ||
| .unwrap_or((None, None)); | ||
|
|
||
| VuiData { | ||
| framerate, | ||
| display_ratio_width, | ||
| display_ratio_height, | ||
| } | ||
| } | ||
|
|
||
| fn aspect_ratio_from_idc(idc: scuffle_h265::AspectRatioIdc) -> Option<(u32, u32)> { | ||
| match idc { | ||
| scuffle_h265::AspectRatioIdc::Unspecified => None, | ||
| scuffle_h265::AspectRatioIdc::Square => Some((1, 1)), | ||
| scuffle_h265::AspectRatioIdc::Aspect12_11 => Some((12, 11)), | ||
| scuffle_h265::AspectRatioIdc::Aspect10_11 => Some((10, 11)), | ||
| scuffle_h265::AspectRatioIdc::Aspect16_11 => Some((16, 11)), | ||
| scuffle_h265::AspectRatioIdc::Aspect40_33 => Some((40, 33)), | ||
| scuffle_h265::AspectRatioIdc::Aspect24_11 => Some((24, 11)), | ||
| scuffle_h265::AspectRatioIdc::Aspect20_11 => Some((20, 11)), | ||
| scuffle_h265::AspectRatioIdc::Aspect32_11 => Some((32, 11)), | ||
| scuffle_h265::AspectRatioIdc::Aspect80_33 => Some((80, 33)), | ||
| scuffle_h265::AspectRatioIdc::Aspect18_11 => Some((18, 11)), | ||
| scuffle_h265::AspectRatioIdc::Aspect15_11 => Some((15, 11)), | ||
| scuffle_h265::AspectRatioIdc::Aspect64_33 => Some((64, 33)), | ||
| scuffle_h265::AspectRatioIdc::Aspect160_99 => Some((160, 99)), | ||
| scuffle_h265::AspectRatioIdc::Aspect4_3 => Some((4, 3)), | ||
| scuffle_h265::AspectRatioIdc::Aspect3_2 => Some((3, 2)), | ||
| scuffle_h265::AspectRatioIdc::Aspect2_1 => Some((2, 1)), | ||
| scuffle_h265::AspectRatioIdc::ExtendedSar => None, | ||
| _ => None, // Reserved | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add inline tests per coding guidelines.
Per the coding guidelines, Rust crates should have integrated tests within source files using inline test modules. This file implements complex HEVC parsing logic but lacks tests. Consider adding a #[cfg(test)] module to cover:
- NAL type classification (
HevcNalType::try_from) - Frame boundary detection logic (especially the
first_slice_segment_in_pic_flagcheck) - Aspect ratio mapping (
aspect_ratio_from_idc) - VUI data extraction (
get_vui_data) - Constraint flags packing (
pack_constraint_flags)
The existing test_nal_iterator_realistic_h265 in annexb.rs provides a good example pattern to follow.
Based on coding guidelines, inline tests are required for Rust source files.
🤖 Prompt for AI Agents
rs/hang/src/import/hev1.rs lines 1-396: Missing inline tests for HEVC parsing
logic as required by project guidelines; add a #[cfg(test)] mod tests with unit
tests covering HevcNalType::try_from (various nal_unit_type values), frame
boundary detection (especially first_slice_segment_in_pic_flag behavior using
constructed NAL byte sequences to assert maybe_start_frame triggers),
aspect_ratio_from_idc mapping for key AspectRatioIdc variants, get_vui_data
extraction for timing and aspect ratio (including ExtendedSar and predefined
cases), and pack_constraint_flags output for representative
scuffle_h265::Profile values; implement tests using small byte buffers and the
existing NalIterator pattern from annexb.rs, assert expected outputs with
assert_eq!/assert!(...), and ensure tests compile by importing necessary types
and constructing minimal Hev1/Frame state where needed.
There was a problem hiding this comment.
Good call Robo Rabbit, will add!
There was a problem hiding this comment.
@mcintyrehh Great! If you need any help structuring the tests or want me to generate some starter test cases for the key functions I mentioned (like HevcNalType::try_from, aspect_ratio_from_idc, pack_constraint_flags, etc.), feel free to ask. The pattern from annexb.rs should work well as a template.
🧠 Learnings used
Learnt from: CR
Repo: moq-dev/moq PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T04:00:14.871Z
Learning: Applies to **/*.rs : In Rust crates, tests should be integrated within source files using inline test modules
| /// Decode as much data as possible from the given buffer. | ||
| /// | ||
| /// Unlike [Self::decode_frame], this method needs the start code for the next frame. | ||
| /// This means it works for streaming media (ex. stdin) but adds a frame of latency. | ||
| /// | ||
| /// TODO: This currently associates PTS with the *previous* frame, as part of `maybe_start_frame`. | ||
| pub fn decode_stream<T: Buf + AsRef<[u8]>>( | ||
| &mut self, | ||
| buf: &mut T, | ||
| pts: Option<hang::Timestamp>, | ||
| ) -> anyhow::Result<()> { | ||
| let pts = self.pts(pts)?; | ||
|
|
||
| // Iterate over the NAL units in the buffer based on start codes. | ||
| let nals = NalIterator::new(buf); | ||
|
|
||
| for nal in nals { | ||
| self.decode_nal(nal?, Some(pts))?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Address the PTS timing issue noted in the TODO comment.
The TODO comment at line 120 correctly identifies that decode_stream associates the PTS with the previous frame. This happens because maybe_start_frame is called when a boundary NAL (e.g., SPS, AUD, or first slice of next frame) is detected, but it writes the accumulated frame from the previous group of NALs. This causes Frame N to receive Frame N+1's timestamp, leading to incorrect playback timing.
Note that decode_frame (line 165) doesn't have this issue since it explicitly flushes after processing the buffer.
Do you want me to propose a fix? One approach would be to buffer the PTS and apply it when the frame is actually started (first NAL after a boundary) rather than when the boundary is detected.
🤖 Prompt for AI Agents
In rs/hang/src/import/hev1.rs around lines 115 to 136, decode_stream currently
computes pts and passes it to decode_nal immediately, which causes the PTS to be
applied to the previous frame (see TODO). Change the logic to compute pts once
at the start of decode_stream but do not apply it immediately; instead store it
as a pending_pts (add a field like self.pending_pts: Option<Timestamp>) and
change maybe_start_frame / decode_nal flow so that when a boundary NAL is seen
you do NOT tag the flushed (previous) frame with the pending_pts, but when the
first NAL of the next frame is started you consume pending_pts and attach it to
that new frame. Concretely: 1) remove passing Some(pts) into decode_nal for
boundary-detection; 2) set self.pending_pts = Some(pts) when a boundary is
detected (or at entry if buffer starts a new frame); 3) in the code path that
actually starts a frame (maybe_start_frame when opening a new accumulator /
first slice), if self.pending_pts.is_some() take() it and assign to the new
frame’s pts; 4) ensure decode_frame behavior remains unchanged and tests updated
to verify timestamps align with the frame they describe.
rs/hang/src/import/hev1.rs
Outdated
| Filler = 38, | ||
| SeiPrefix = 39, | ||
| SeiSuffix = 40, | ||
| } // ITU H.265 V10 Table 7-1 – NAL unit type codes and NAL unit type classes |
There was a problem hiding this comment.
Alternatively, we could use the scuffle NALUnitType enum.
|
looks good 2 me |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rs/hang/src/import/hev1.rs (1)
282-292: Constraint flags packing is incomplete.The
pack_constraint_flagsfunction only packs the first 4 constraint flags (bits 7-4 of byte 0), leaving the remaining 5.5 bytes as zeros. While the TODO at line 290 documents this limitation, incomplete constraint flags might cause compatibility issues with strict decoders or players that validate profile conformance.Consider extracting and packing the additional flags from
profile.additional_flagsto provide complete profile information.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rs/hang/src/import/decoder.rsrs/hang/src/import/hev1.rsrs/hang/src/import/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/hang/src/import/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
In Rust crates, tests should be integrated within source files using inline test modules
Files:
rs/hang/src/import/decoder.rsrs/hang/src/import/hev1.rs
🧠 Learnings (1)
📚 Learning: 2025-12-10T04:00:14.871Z
Learnt from: CR
Repo: moq-dev/moq PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T04:00:14.871Z
Learning: Applies to **/*.rs : In Rust crates, tests should be integrated within source files using inline test modules
Applied to files:
rs/hang/src/import/hev1.rs
🧬 Code graph analysis (2)
rs/hang/src/import/decoder.rs (1)
rs/hang/src/import/hev1.rs (3)
new(32-40)new(309-332)pts(263-270)
rs/hang/src/import/hev1.rs (3)
rs/hang/src/import/decoder.rs (5)
new(76-86)initialize(94-106)decode_stream(121-132)decode_frame(144-158)is_initialized(161-169)rs/hang/src/import/avc3.rs (10)
new(30-38)init(40-98)initialize(101-113)decode_stream(121-136)pts(248-255)decode_frame(145-168)decode_nal(170-218)maybe_start_frame(220-242)is_initialized(244-246)drop(259-264)rs/hang/src/model/timestamp.rs (1)
from_micros(40-46)
🔇 Additional comments (6)
rs/hang/src/import/decoder.rs (1)
5-5: LGTM! Clean integration following established patterns.The Hev1 decoder integration is consistent with existing decoders (Avc3, Fmp4, Aac, Opus). All necessary dispatch points are present: imports, enum variants, string parsing, Display implementation, constructor, and all decoder methods (initialize, decode_stream, decode_frame, is_initialized).
Also applies to: 15-16, 33-33, 47-47, 60-61, 80-80, 98-98, 125-125, 152-152, 165-165
rs/hang/src/import/hev1.rs (5)
1-29: Well-structured decoder implementation.The imports and struct design follow the established Avc3 pattern. The optional
trackandconfigfields enable lazy initialization and reinitialization when SPS changes, whilezeroprovides wall-clock timestamp fallback.
31-98: Initialization logic correctly handles SPS parsing and track management.The constructor and
initmethod properly extract video configuration from SPS NAL units, handle VUI parameters for timing/aspect ratio, and reinitialize tracks when configuration changes. The pattern matches Avc3 and integrates cleanly with the broadcast catalog.
100-168: Decoding methods follow the established pattern.The
initialize,decode_stream, anddecode_framemethods properly handle NAL unit iteration and frame boundaries. The explicit flush indecode_frame(line 165) correctly handles the final frame in the buffer. The PTS timing issue noted in the TODO at line 120 is already documented in past reviews.
235-280: Frame management and lifecycle methods are correctly implemented.The
maybe_start_framemethod properly guards against premature flushing, validates required state (track and PTS), and correctly constructs and writes frames. Theptshelper provides sensible timestamp fallback using wall-clock time. TheDropimplementation ensures proper catalog cleanup.
301-357: VUI data extraction and aspect ratio mapping are correctly implemented.The
VuiData::newmethod properly extracts framerate (using the correct formula:time_scale / num_units_in_tick) and handles both extended SAR and predefined aspect ratios. Theaspect_ratio_from_idcfunction provides comprehensive coverage of all HEVC aspect ratio indicators.
| /// Decode a single NAL unit. Only reads the first header byte to extract nal_unit_type, | ||
| /// Ignores nuh_layer_id and nuh_temporal_id_plus1. | ||
| fn decode_nal(&mut self, nal: Bytes, pts: Option<hang::Timestamp>) -> anyhow::Result<()> { | ||
| anyhow::ensure!(nal.len() >= 2, "NAL unit is too short"); | ||
| // u16 header: [forbidden_zero_bit(1) | nal_unit_type(6) | nuh_layer_id(6) | nuh_temporal_id_plus1(3)] | ||
| let header = nal.first().context("NAL unit is too short")?; | ||
|
|
||
| let forbidden_zero_bit = (header >> 7) & 1; | ||
| anyhow::ensure!(forbidden_zero_bit == 0, "forbidden zero bit is not zero"); | ||
|
|
||
| // Bits 1-6: nal_unit_type | ||
| let nal_unit_type = (header >> 1) & 0b111111; | ||
| let nal_type = NALUnitType::from(nal_unit_type); | ||
|
|
||
| match nal_type { | ||
| NALUnitType::SpsNut => { | ||
| self.maybe_start_frame(pts)?; | ||
|
|
||
| // Try to reinitialize the track if the SPS has changed. | ||
| let sps = SpsNALUnit::parse(&mut &nal[..]).context("failed to parse SPS NAL unit")?; | ||
| self.init(&sps)?; | ||
| } | ||
| // TODO parse the SPS again and reinitialize the track if needed | ||
| NALUnitType::AudNut | NALUnitType::PpsNut | NALUnitType::PrefixSeiNut | NALUnitType::SuffixSeiNut => { | ||
| self.maybe_start_frame(pts)?; | ||
| } | ||
| // Keyframe containing slices | ||
| NALUnitType::IdrWRadl | ||
| | NALUnitType::IdrNLp | ||
| | NALUnitType::BlaNLp | ||
| | NALUnitType::BlaWRadl | ||
| | NALUnitType::BlaWLp | ||
| | NALUnitType::CraNut => { | ||
| self.current.contains_idr = true; | ||
| self.current.contains_slice = true; | ||
| } | ||
| // All other slice types (both N and R variants) | ||
| NALUnitType::TrailN | ||
| | NALUnitType::TrailR | ||
| | NALUnitType::TsaN | ||
| | NALUnitType::TsaR | ||
| | NALUnitType::StsaN | ||
| | NALUnitType::StsaR | ||
| | NALUnitType::RadlN | ||
| | NALUnitType::RadlR | ||
| | NALUnitType::RaslN | ||
| | NALUnitType::RaslR => { | ||
| // Check first_slice_segment_in_pic_flag (bit 7 of third byte, after 2-byte header) | ||
| if nal.get(2).context("NAL unit is too short")? & 0x80 != 0 { | ||
| self.maybe_start_frame(pts)?; | ||
| } | ||
| self.current.contains_slice = true; | ||
| } | ||
| _ => {} | ||
| } | ||
|
|
||
| // Rather than keeping the original size of the start code, we replace it with a 4 byte start code. | ||
| // It's just marginally easier and potentially more efficient down the line (JS player with MSE). | ||
| // NOTE: This is ref-counted and static, so it's extremely cheap to clone. | ||
| self.current.chunks.push_chunk(START_CODE.clone()); | ||
| self.current.chunks.push_chunk(nal); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
NAL parsing logic is mostly correct, but has a confusing comment.
The NAL header parsing and slice type handling are correct:
- Extracts
nal_unit_typefrom bits 1-6 of the first byte ✓ - The
first_slice_segment_in_pic_flagcheck at line 218 correctly examines bit 7 of byte 2 (after the 2-byte NAL header) ✓ - Comprehensive coverage of HEVC slice types (IDR, BLA, CRA, and all non-keyframe slice variants) ✓
However, line 192's comment is unclear. It appears next to PpsNut handling but says "parse the SPS again," which is confusing. Consider clarifying what this TODO means or removing it if the intention is unclear.
Optional: Explicitly handle VPS NAL units
Currently, VpsNut falls through to the default case and gets appended to the frame payload (lines 229-230). While line 11 documents that VPS is ignored, consider adding an explicit case to make this behavior clearer:
| NALUnitType::RaslR => {
// Check first_slice_segment_in_pic_flag (bit 7 of third byte, after 2-byte header)
if nal.get(2).context("NAL unit is too short")? & 0x80 != 0 {
self.maybe_start_frame(pts)?;
}
self.current.contains_slice = true;
}
+ // VPS is ignored per struct documentation (line 11)
+ NALUnitType::VpsNut => {
+ // VPS NAL units are not parsed but are included in the stream
+ }
_ => {}
}
2 participants
Related to: #740
MoQ OBS Plugin update: moq-dev/obs#19
This PR adds an HEVC
hev1decoder, following the format of theavc1import. The decoders are largely identical with the exception of:init- the avc3 decoder uses the h264_parser crate to handle parsing the SPS Nal unit, this PR adds a newscuffle_h265crate to the same end. A new methodget_vui_datapulls outframerateanddisplay_ratio_width/heightfrom the VUI Parameters of the SPS. Framerate ends up getting passed through to the catalog! The FFmpeg HEVC encoder in OBS doesn't pass through anaspect_ratio_idc, so I haven't been able to confirm the display ratio fields. I am the babiest of Crab-lings so shout out to Claude for the Rust syntax help here!decode_nal- similar to the avc3 parser. Ignores the second NAL header byte containingnuh_layer_idandnuh_temporal_id_plus1. Not sure if there is anything we need to do with these?HEVC with framerate and Opus! 🤯
Other changes:
NalIteratorand its child functions (after_start_code,find_start_code) into a newannexb.rsmodule and added a newtest_nal_iterator_realistic_h265