Conversation
There was a problem hiding this comment.
Pull request overview
This PR turns the repository from a template into the initial hwdecode crate: a cross-platform, hardware-only video decoder built on ffmpeg-next, with a safe Frame wrapper and support code/docs/tests/examples/benchmarks.
Changes:
- Added core decoding implementation (
VideoDecoder) with backend auto-probe, strict HW selection, and probe replay logic. - Added
Frame+pix_fmtmodules to expose CPU-side frame access without constructing FFmpeg enums from runtime values. - Added integration tests, an example app, and a Criterion benchmark; removed leftover template placeholder files/docs.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib.rs |
New crate-level docs + module wiring and public re-exports. |
src/backend.rs |
Defines supported HW backends and per-OS probe order. |
src/decoder.rs |
Main VideoDecoder implementation (probe, replay, transfer, buffering/caps). |
src/frame.rs |
Safe CPU-side Frame wrapper with raw-field access + plane sizing. |
src/ffi.rs |
Unsafe FFmpeg shims/callbacks centralized for auditability. |
src/error.rs |
Error model for open/probe/device init failures. |
src/pix_fmt.rs |
Stable i32 pixel-format constants for post-transfer frames. |
tests/decode.rs |
Integration test decoding sample video when env var is set. |
tests/hw_smoke.rs |
#[ignore] HW smoke test gated by env var + working HW backend. |
examples/decode.rs |
CLI example: decode frames and print per-frame info. |
benches/decode.rs |
Criterion bench comparing SW baseline vs HW decode when available. |
docs/design.md |
Design/spec document added (with historical context). |
README.md |
Replaced template README with crate overview, usage, and requirements. |
Cargo.toml |
Renamed crate to hwdecode, added dependencies, example/bench entries. |
tests/foo.rs |
Removed template placeholder test file. |
examples/foo.rs |
Removed template placeholder example. |
benches/foo.rs |
Removed template placeholder bench. |
README-zh_CN.md |
Removed template Chinese README. |
CHANGELOG.md |
Removed template changelog stub. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name = "hwdecode" | ||
| version = "0.0.0" | ||
| edition = "2021" | ||
| repository = "https://github.com/al8n/template-rs" | ||
| homepage = "https://github.com/al8n/template-rs" | ||
| documentation = "https://docs.rs/template-rs" | ||
| description = "A template for creating Rust open-source repo on GitHub" | ||
| rust-version = "1.95" | ||
| description = "Cross-platform hardware-only video decoder built on top of ffmpeg-next, with auto-probe across HW backends. Callers handle software fallback." |
There was a problem hiding this comment.
PR title is "0.1.0" but Cargo.toml still sets version = "0.0.0". Please bump the crate version (and any related docs/CHANGELOG) so metadata matches the intended release.
| ## Public API | ||
|
|
||
| ```rust | ||
| pub struct VideoDecoder { /* private */ } | ||
|
|
||
| impl VideoDecoder { | ||
| /// Auto-probe HW backends in platform order. Returns | ||
| /// `Error::AllBackendsFailed` if no backend can decode this stream; | ||
| /// caller falls back to software decoder of choice. On success, | ||
| /// `backend()` reports the one that won. | ||
| pub fn open(parameters: ffmpeg::codec::Parameters) -> Result<Self, Error>; | ||
|
|
||
| /// Force a specific backend. No probe, no fallback. | ||
| pub fn open_with(parameters: ffmpeg::codec::Parameters, backend: Backend) -> Result<Self, Error>; | ||
|
|
||
| pub fn backend(&self) -> Backend; | ||
| pub fn width(&self) -> u32; | ||
| pub fn height(&self) -> u32; | ||
| pub fn format(&self) -> ffmpeg::format::Pixel; | ||
| pub fn time_base(&self) -> ffmpeg::Rational; | ||
| pub fn frame_rate(&self) -> ffmpeg::Rational; | ||
|
|
||
| pub fn send_packet(&mut self, packet: &ffmpeg::Packet) -> Result<(), Error>; | ||
| pub fn send_eof(&mut self) -> Result<(), Error>; | ||
|
|
||
| /// Receive a CPU-side frame. Internally calls | ||
| /// `av_hwframe_transfer_data` and copies PTS/timing onto the result; | ||
| /// output format is NV12 (8-bit) or P010 (10-bit) per the HW backend's | ||
| /// `AVHWFramesContext::sw_format`. | ||
| pub fn receive_frame(&mut self, frame: &mut Frame) -> Result<(), Error>; | ||
|
|
||
| pub fn flush(&mut self); | ||
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] | ||
| pub enum Backend { | ||
| Software, | ||
| VideoToolbox, // macOS, iOS, iPadOS, tvOS | ||
| Vaapi, // Linux (Intel/AMD) | ||
| Cuda, // Linux/Windows (NVIDIA) | ||
| D3d11va, // Windows | ||
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { | ||
| #[error("ffmpeg error: {0}")] | ||
| Ffmpeg(#[from] ffmpeg::Error), | ||
| #[error("no decoder for codec id {0:?}")] | ||
| NoCodec(ffmpeg::codec::Id), | ||
| #[error("hardware device init failed for {backend:?}: {source}")] | ||
| HwDeviceInitFailed { backend: Backend, source: ffmpeg::Error }, | ||
| #[error("all backends failed; attempts: {attempts:?}")] | ||
| AllBackendsFailed { attempts: Vec<(Backend, ffmpeg::Error)> }, | ||
| } | ||
| ``` | ||
|
|
||
| ## Behavior | ||
|
|
||
| ### Probe order | ||
|
|
||
| | Target | Order tried | | ||
| | ------------------- | -------------------------------------------- | | ||
| | macOS, iOS, tvOS | `[VideoToolbox, Software]` | | ||
| | Linux | `[Vaapi, Cuda, Software]` | | ||
| | Windows | `[D3d11va, Cuda, Software]` | | ||
| | Other | `[Software]` | | ||
|
|
There was a problem hiding this comment.
docs/design.md includes a large "Public API" code block and probe-order table that still mention Backend::Software, a format() method, and in-crate software fallback. Even with the status note, this is likely to confuse readers because it presents invalid, non-compiling API. Consider either updating the snippet/table to match the current crate, or clearly labeling the entire section as outdated/for history and removing/shortening the incorrect code block.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name = "hwdecode" | ||
| version = "0.0.0" | ||
| edition = "2021" | ||
| repository = "https://github.com/al8n/template-rs" | ||
| homepage = "https://github.com/al8n/template-rs" | ||
| documentation = "https://docs.rs/template-rs" | ||
| description = "A template for creating Rust open-source repo on GitHub" | ||
| rust-version = "1.95" | ||
| description = "Cross-platform hardware-only video decoder built on top of ffmpeg-next, with auto-probe across HW backends. Callers handle software fallback." |
There was a problem hiding this comment.
PR title suggests a 0.1.0 release, but the crate version in Cargo.toml is still 0.0.0. If this PR is meant to cut 0.1.0, please bump package.version accordingly (and ensure any release notes/changelog expectations are aligned).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// TODO: when frames significantly exceed typical sizes, consider | ||
| /// memmap-backed pending buffers (write transferred frames to a temp file | ||
| /// or shared-memory segment) so the resident set stays bounded even when | ||
| /// the byte cap is raised. Out of scope for v0.0.0. |
There was a problem hiding this comment.
The comment says “Out of scope for v0.0.0”, but the crate version is now 0.1.0. Please update this to the correct version (or make it version-agnostic) so it doesn’t go stale again.
| /// the byte cap is raised. Out of scope for v0.0.0. | |
| /// the byte cap is raised. Out of scope for now. |
| // even a single retained frame is significant. Future direction: | ||
| // memmap-backed pending frames (write to a temp file or shared | ||
| // memory segment) so the resident set stays bounded even when the | ||
| // byte cap is raised. Out of scope for v0.0.0. |
There was a problem hiding this comment.
This inline TODO comment still references “Out of scope for v0.0.0” even though the crate is 0.1.0. Update it to the current release series (or remove the specific version) to avoid misleading future readers.
| // byte cap is raised. Out of scope for v0.0.0. | |
| // byte cap is raised. Out of scope for now. |
| #[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos"))] | ||
| #[test] | ||
| fn apple_probe_order() { | ||
| assert_eq!(probe_order(), &[Backend::VideoToolbox]); | ||
| } |
There was a problem hiding this comment.
probe_order() includes target_os = "visionos", but the Apple probe-order unit test is not enabled for visionOS. Add visionos to the test’s #[cfg(any(...))] so the expected probe order is validated on all supported Apple targets.
| codec::{ | ||
| self, | ||
| packet::{Mut as PacketMut, Ref as PacketRef}, | ||
| Context, | ||
| }, |
There was a problem hiding this comment.
PacketMut and PacketRef are imported but never used. This triggers warnings and makes the import list harder to audit; remove these imports (and any other unused ones) to keep the module clean.
| codec::{ | |
| self, | |
| packet::{Mut as PacketMut, Ref as PacketRef}, | |
| Context, | |
| }, | |
| codec::{self, Context}, |
No description provided.