-
Notifications
You must be signed in to change notification settings - Fork 1.1k
source frame width/height from ffmpeg for avassetreader #1351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughTwo modifications are made: Windows is removed from the clippy CI job matrix (with commented history preserved), and AVAssetReaderDecoder struct now tracks image width and height dimensions, propagating them through FFmpeg decoder initialization, reader track output configuration, and the asset reader output dictionary. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/video-decode/src/avassetreader.rs (1)
25-44: Consider validating that width and height are non-zero.While FFmpeg's
decoder.width()anddecoder.height()typically return valid dimensions, corrupt or unusual video files could potentially have zero dimensions. Consider adding validation to return an error early if either dimension is zero, preventing downstream issues with the asset reader configuration.Example validation:
let (pixel_format, width, height) = { // ... existing decoder setup ... let width = decoder.width(); let height = decoder.height(); if width == 0 || height == 0 { return Err(format!("Invalid video dimensions: {}x{}", width, height)); } ( pixel_to_pixel_format(decoder.format()), width, height, ) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)crates/video-decode/src/avassetreader.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/video-decode/src/avassetreader.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/video-decode/src/avassetreader.rs
🧬 Code graph analysis (1)
crates/video-decode/src/avassetreader.rs (2)
crates/rendering/src/decoder/avassetreader.rs (2)
new(45-51)new(177-182)crates/video-decode/src/ffmpeg.rs (3)
new(21-83)decoder(103-105)decoder(122-124)
⏰ 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). (2)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (7)
.github/workflows/ci.yml (1)
106-108: Windows-specific clippy lints will no longer run.While this reduces CI resource consumption, be aware that Rust clippy lints specific to Windows will not be caught until later in the pipeline (build-desktop job). Consider monitoring for any Windows-specific issues that might slip through.
crates/video-decode/src/avassetreader.rs (6)
19-20: LGTM - Width and height fields added.The addition of width and height fields to track video dimensions is appropriate. These fields are properly typed as
u32and will store the frame dimensions sourced from the FFmpeg decoder.
48-48: LGTM - Constructor properly passes dimensions.The updated call to
get_reader_track_outputcorrectly passes the extracted width and height values.
56-57: LGTM - Struct fields properly initialized.The width and height fields are correctly initialized with the values extracted from the FFmpeg decoder.
68-69: LGTM - Reset correctly preserves dimensions.The reset method appropriately reuses the stored width and height values, maintaining consistency with the original decoder configuration.
80-81: LGTM - Function signature updated to accept dimensions.The function signature correctly adds width and height parameters that are needed for configuring the asset reader track output.
108-117: LGTM - Dictionary correctly configured with width and height.The asset reader track output dictionary is properly expanded to include width and height alongside pixel format. The parallel arrays of keys and values are correctly aligned, and the Core Video pixel buffer keys are appropriately used for dimension configuration.
Summary by CodeRabbit
Bug Fixes
Chores