feat: migrate color / cfa / pixel_format / frame primitives to videoframe 0.2#5
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Wires mediadecode to consume duplicate type definitions from the new
videoframe crate via a workspace path dep:
- `mediadecode::color::*` → `videoframe::color`
- `mediadecode::cfa::BayerPattern` → `videoframe::cfa::BayerPattern`
- `mediadecode::frame::{Dimensions, Rect, Plane}` → `videoframe::frame`
Decoder-output types (`VideoFrame<P, E, D>`, `AudioFrame<S, C, E, D>`,
`SubtitleFrame<E, D>`) remain in `mediadecode::frame` — they carry
timestamp + backend-extras / sample format / channel layout, which is
mediadecode's domain (`videoframe` stays the pure pixel-data layer).
Existing import paths resolve via the re-exports so downstream
callers using `mediadecode::color::ColorMatrix` etc. continue to
work — no source-level break.
Note: `pixel_format::PixelFormat` is NOT migrated in this commit.
videoframe's variant is `Unknown(u32)` (lossless round-trip) while
mediadecode uses `Unknown = 0` (unit variant); the two types are
structurally incompatible and require changes to all
`_ => PixelFormat::Unknown` fallback arms in mediadecode-ffmpeg /
mediadecode-webcodecs. Tracked as a follow-up.
…tives)
Wires mediadecode to consume duplicate type definitions from the new
`videoframe` crate via a workspace path dep:
- `mediadecode::color::*` → `videoframe::color`
- `mediadecode::cfa::BayerPattern` → `videoframe::cfa::BayerPattern`
- `mediadecode::pixel_format::PixelFormat` → `videoframe::pixel_format`
- `mediadecode::frame::{Dimensions, Rect, Plane}` → `videoframe::frame`
Decoder-output types (`VideoFrame<P, E, D>`, `AudioFrame<S, C, E, D>`,
`SubtitleFrame<E, D>`) stay in `mediadecode::frame` — they carry
timestamp + backend-extras / sample format / channel layout, which is
mediadecode's domain. `videoframe` stays the pure pixel-data layer.
videoframe's `PixelFormat::Unknown(u32)` is a tuple variant
(lossless wire round-trip), unlike the prior unit-variant
`Unknown`. Updated downstream callers in `mediadecode-ffmpeg` and
`mediadecode-webcodecs`:
- Boundary mapping fallback: `_ => PixelFormat::Unknown` →
`_ => PixelFormat::Unknown(raw as u32)` (preserves the raw FFmpeg /
WebCodecs identifier through the cast).
- Test assertions: `assert_eq!(x, PixelFormat::Unknown)` →
`assert!(matches!(x, PixelFormat::Unknown(_)))` (wildcard match
on the tuple variant).
- Default-frame placeholders / function-argument bare-Unknown sites:
→ `PixelFormat::Unknown(0)`.
Existing `mediadecode::color::ColorMatrix` / `mediadecode::PixelFormat`
import paths continue to resolve via the re-exports — no source-level
break beyond the Unknown variant shape.
…pace layout) videoframe restructured into a Cargo workspace; the actual lib crate moved from the repo root to videoframe/videoframe/. Update path dep to match.
54757cf to
e9fbf9e
Compare
videoframe v0.1.0 was published to crates.io. Switch the workspace dep spec from the local path (`../videoframe/videoframe`) to the registry version. Caret semver allows future 0.1.x patches without further edits.
e9fbf9e to
eec6dbb
Compare
videoframe 0.2.0 reorganized the public surface: the `cfa` module was removed; `BayerPattern` now lives at `videoframe::frame::bayer` and is re-exported via `frame::*`. Update mediadecode's re-export in `cfa.rs` to use the new path. The 0.2.0 release also tightened error shapes (newtype-tuple variants with thiserror::Error on each payload, ref/ref_mut accessors on every *FrameError) and the unicode-glyph normalization that landed alongside, but nothing mediadecode consumes directly changed in a way that needs further code adjustments — the workspace builds clean and all 47 + 71 lib tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mediadecode and mediadecode-ffmpeg both move to 0.2.0 alongside the videoframe migration; mediadecode-webcodecs stays pre-release at 0.0.0. CHANGELOGs: - mediadecode/CHANGELOG.md — write the [0.2.0] section covering the PixelFormat::Unknown shape change (unit -> Unknown(u32)), the re-export pivot for color/cfa/frame primitives onto videoframe, and the explicit "decoder-output types stay in mediadecode" invariant. ~270-variant PixelFormat (videoframe-sourced, FFmpeg n8.1 coverage) called out so consumers know the previously-closed enum subset is now a strict superset. - mediadecode-ffmpeg/CHANGELOG.md — add [0.2.0] noting the bump to mediadecode 0.2 + the boundary mapping update preserving the raw AVPixelFormat identifier via Unknown(raw as u32). - mediadecode-webcodecs/CHANGELOG.md — same boundary preservation note; crate stays scaffolded pre-publish. Cargo.toml: - mediadecode-ffmpeg 0.1.0 -> 0.2.0 (type aliases shift with the PixelFormat shape change, so the version must move with mediadecode). READMEs: - root README crates table — add mediadecode-webcodecs row, add a note about videoframe sourcing the pixel/color vocabulary. - mediadecode README — fix stale `version = "0.0.0"` dep example to `"0.2"`; rewrite the "Pixel and sample formats" bullet to describe the ~270-variant videoframe-sourced PixelFormat with Unknown(u32). - mediadecode-webcodecs README — note unpublished status; fix the `mediadecode-webcodecs = "0.1"` example to `"0.0"` (crate is at 0.0.0 and not yet on crates.io). Verified locally: cargo build --workspace clean; cargo test --workspace --lib reports 47 + 71 lib tests passing; cargo fmt --check clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Issue #4 reported 4 findings (0 critical/high, 1 medium, 3 low): 1. MEDIUM — `plane_count` not validated against array capacity in `VideoFrame::new` / `AudioFrame::new`. Out-of-range values would panic later inside `planes()` / `samples()`. Already addressed in 902ec1c (pre-existing) with `assert!` on both constructors. Confirmed; no further code change. 2. LOW — no `Debug` on `mediadecode_ffmpeg::Frame`. Already addressed in 902ec1c via a manual `impl core::fmt::Debug` showing dimensions / pixel format. Confirmed; no further code change. 3. LOW — missing `#[must_use]` on consuming `with_*` builders. Added to all 59 `with_NAME(mut self, ...) -> Self` methods across the workspace (mediadecode core: frame/packet/subtitle/ channel; mediadecode-ffmpeg: decoder/extras; mediadecode- webcodecs: extras). Setters that return `&mut Self` are not must-use candidates and were left alone. 4. LOW — webcodecs crate has no native-target tests. Added `mediadecode-webcodecs/tests/native_stub.rs` with two tests verifying the empty-stub behavior under `#[cfg(not(target_arch = "wasm32"))]`. Tests pass; clean compile gate on native hosts. CHANGELOGs updated with cross-references to issue #4. Verified: cargo build --workspace, cargo test --workspace --lib (47 + 71 + 5 + 2 = 125 lib/native tests pass; one pre-existing integration test in mediadecode-ffmpeg/tests/audio_pcm_fixtures.rs fails on this worktree because the referenced fixture file isn't checked in — unrelated to this work). cargo fmt --check + cargo clippy --workspace --all-features -- -D warnings clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Panic-free counterparts to the existing `new` constructors,
returning `Result<Self, FrameError>` instead of asserting on
out-of-range `plane_count`. The existing `new` methods stay
`const fn` and keep their panicking behavior for statically-
known call sites (consts, tests with literal arrays); `try_new`
is for runtime-checked callers like the FFmpeg / WebCodecs
adapters that map decoder output into frames.
`try_new` itself is not `const fn` — returning `Result<Self, _>`
would require dropping the moved generic-typed `planes` /
`pixel_format` / `extra` on the error branch, which the const
evaluator can't prove safe for arbitrary `P` / `E` / `S` / `C` /
`D`. Rationale documented inline on each method.
New types:
- `mediadecode::frame::FrameError` — `non_exhaustive` enum with
`TooManyVideoPlanes { plane_count }` and
`TooManyAudioPlanes { plane_count }` variants. Derives
`IsVariant` and `thiserror::Error` per the crate's existing
convention.
New tests (4 added; total: 51 lib tests):
- `video_frame_try_new_returns_err_for_too_many_planes`
- `video_frame_try_new_accepts_valid_plane_count`
- `audio_frame_try_new_returns_err_for_too_many_planes`
- `audio_frame_try_new_accepts_valid_plane_count` (boundary at 8)
SubtitleFrame::try_new was intentionally not added — its
constructor has no validation step and would either return
`Result<Self, Infallible>` (lying about possible failure) or
require a separate stub variant. The crate's own README scopes
`try_*` to "where construction can fail", so this is consistent.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the convention used in `videoframe`: struct-style variants
are split into a payload struct + a newtype-tuple enum variant.
Each payload struct carries private fields with `pub const fn`
getters and (where useful) `into_*` consumers, derives
`thiserror::Error` with the moved `#[error("...")]` message, and
the enum variant uses `#[error(transparent)]` to delegate.
Affected variants (all originally struct-style):
- `mediadecode::frame::FrameError::TooManyVideoPlanes`
- `mediadecode::frame::FrameError::TooManyAudioPlanes`
- `mediadecode_ffmpeg::Error::HwDeviceInitFailed`
- `mediadecode_ffmpeg::Error::AllBackendsFailed`
- `mediadecode_ffmpeg::Error::FallbackFailed`
Pure tuple variants (Ffmpeg, NoCodec, BackendUnsupportedByCodec)
unchanged.
Callers destructuring `Err(Error::AllBackendsFailed { attempts, .. })`
must switch to `Err(Error::AllBackendsFailed(p))` then call
`p.attempts()` / `p.unconsumed_packets()` / `p.into_parts()`.
Owning-move paths preserved via `into_parts()` /
`into_unconsumed_packets()` so non-seekable callers can still
relinquish the `Vec<Packet>` without cloning.
The manual `Debug` impl on `Error` previously printed `[N packets]`
instead of dumping per-packet bytes; this Debug logic now lives
on the payload structs (`AllBackendsFailed` and `FallbackFailed`
have hand-rolled Debug for the same reason - `ffmpeg::Packet:
!Debug`). The enum itself can derive Debug normally now.
Construction sites in `decoder.rs` / `video.rs` updated (~25).
Destructuring sites in tests / examples / benches / internal
matches updated (~18). README and rustdoc examples updated to
show the new pattern.
CHANGELOG entries reflect the BREAKING change for 0.2.0.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `#[from]` to each newtype-tuple payload variant in `FrameError` and `mediadecode_ffmpeg::Error`, so inner helpers returning the payload type as their error can be `?`-propagated into the enum directly. `Ffmpeg(#[from] ffmpeg_next::Error)` already had this; the other five payload-wrapping variants now match the pattern. Affected variants: - `mediadecode::frame::FrameError::TooManyVideoPlanes` - `mediadecode::frame::FrameError::TooManyAudioPlanes` - `mediadecode_ffmpeg::Error::HwDeviceInitFailed` - `mediadecode_ffmpeg::Error::AllBackendsFailed` - `mediadecode_ffmpeg::Error::FallbackFailed` Auto-generated impls: - `impl From<TooManyVideoPlanes> for FrameError` - `impl From<TooManyAudioPlanes> for FrameError` - `impl From<HwDeviceInitFailed> for Error` - `impl From<AllBackendsFailed> for Error` - `impl From<FallbackFailed> for Error` Each payload type is unique to its variant, so no conflicting `From` impls are produced. The simple primitive / enum variants (`NoCodec(u32)`, `BackendUnsupportedByCodec(Backend)`) are intentionally left without `#[from]` — `u32` would conflict broadly and `Backend` isn't naturally a "result of a failed operation" (it's deliberately constructed at sites that want to reject a requested backend). CHANGELOGs updated to mention the new `From` impls so consumers know they exist. Verified: cargo build --workspace clean; cargo test --workspace --lib (51 + 71 + 2 pass); cargo clippy --workspace --all-features --all-targets -- -D warnings clean; cargo fmt --check clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The macOS test job started failing with
`ld: warning: search path '/opt/homebrew/Cellar/ffmpeg/8.1_1/lib'
not found; ld: library 'avutil' not found`. Root cause:
`ffmpeg-sys-next`'s `build.rs` reads `pkg-config` once and bakes
the absolute brew-installed library path into its linker hints.
The cached output points at the brew FFmpeg path that was current
when the cache was first populated; when brew bumps FFmpeg to a
new patch version on the runner, that path no longer exists and
the linker fails as soon as a downstream test/example binary
gets linked.
Two fixes per affected job (build / test / coverage):
1. **Cache-key prefix bump** (`ffmpeg-{build,test,coverage}-` →
`-v2-`). One-shot invalidation that retires the stuck caches
carrying the stale brew path.
2. **`cargo clean -p ffmpeg-sys-next -p ffmpeg-next -p mediadecode-ffmpeg`**
after rust install but before the build/test/tarpaulin step
in the test + coverage jobs. Forces re-execution of the
ffmpeg-sys-next build script on every run so the linker
hints always match the currently-installed brew FFmpeg.
Skipped on clippy (no link step) and build (rlib only).
The clean cost is one rebuild of the FFmpeg binding stack per
run — small relative to the full dependency build that was
already happening, and the only reliable answer short of pinning
a specific brew FFmpeg version (which has its own brittleness).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Migrates mediadecode's pixel-vocabulary primitives (PixelFormat, color enums, BayerPattern, Dimensions/Rect/Plane) to re-exports from the new videoframe 0.2 crate so that colconv, mediadecode, and scenesdetect share one canonical definition. Adapts the FFmpeg and WebCodecs adapters to the new PixelFormat::Unknown(u32) tuple variant, and reshapes Error/FrameError variants into newtype-tuple form wrapping payload structs (with accessors and into_* methods to preserve owning-move ergonomics for non-seekable callers replaying rescued packets).
Changes:
- Replace
mediadecode/src/{color,cfa,pixel_format}.rsand the structural bits offrame.rswith thin re-exports fromvideoframe::{color,cfa,pixel_format,frame}; bumpmediadecodeto 0.2.0 andmediadecode-ffmpegto 0.2.0. - Update 17 boundary / test / default-frame sites in
mediadecode-ffmpegandmediadecode-webcodecsto the newPixelFormat::Unknown(u32)shape (Unknown(raw as u32)at adapter fallbacks;matches!(_, Unknown(_))in tests). - Reshape
Error::{HwDeviceInitFailed, AllBackendsFailed, FallbackFailed}into newtype-tuple variants over payload structs with accessor +into_partsmethods; add a parallelFrameError+VideoFrame::try_new/AudioFrame::try_newpanic-free constructors; add#[must_use]on every consumingwith_*builder.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Adds videoframe = "0.2" workspace dep, bumps mediadecode to 0.2. |
| mediadecode/Cargo.toml | Bumps to 0.2.0, adds videoframe dep. |
| mediadecode/src/{color,cfa,pixel_format}.rs | Replace local definitions with videoframe::* re-exports. |
| mediadecode/src/frame.rs | Re-export structural primitives; add FrameError + try_new constructors; add #[must_use] on builders. |
| mediadecode/src/{packet,channel}.rs | Add #[must_use] on consuming builders. |
| mediadecode/{README,CHANGELOG}.md | Document the videoframe migration. |
| mediadecode-ffmpeg/src/error.rs | Reshape error variants into newtype-tuple form over payload structs; move hand-written Debug onto payloads. |
| mediadecode-ffmpeg/src/{decoder,video,boundary,frame,extras}.rs | Adapt to new Error shape and PixelFormat::Unknown(u32); add #[must_use] on builders. |
| mediadecode-ffmpeg/{tests,examples,benches}/* | Update destructuring to new Error::AllBackendsFailed(p) shape. |
| mediadecode-ffmpeg/{README,CHANGELOG}.md, Cargo.toml | Document migration; bump to 0.2.0. |
| mediadecode-webcodecs/src/{boundary,extras}.rs | Adapt Unknown fallback to Unknown(0); add #[must_use]. |
| mediadecode-webcodecs/tests/native_stub.rs | New non-wasm stub-linkage test. |
| mediadecode-webcodecs/{README,CHANGELOG}.md | Document scaffolded status and migration. |
| .github/workflows/ci-ffmpeg.yml | Bump cache key prefix to -v2- and add cargo clean -p ffmpeg-sys-next step to work around stale brew-FFmpeg path baked into the sys crate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Wires mediadecode to consume duplicate type definitions from the
new `videoframe` crate
(crates.io). videoframe is
the lowest-layer shared vocabulary crate; mediadecode keeps its
decoder/codec abstractions and consumes the pixel-format / color
types via re-exports.
Changes
Cargo.toml: adds `videoframe = { version = "0.2", default-features = false, features = ["frame"] }`
at the workspace root; the `mediadecode` core member consumes it
via `videoframe = { workspace = true }`.
Replaces `mediadecode/src/{color,cfa,pixel_format}.rs` with
thin re-exports from `videoframe::{color,cfa,pixel_format}` (~725
LoC of duplicated definitions deleted).
`mediadecode/src/frame.rs`: removes the local `Dimensions`,
`Rect`, `Plane` definitions in favor of
`videoframe::frame::{Dimensions, Rect, Plane}` re-exports.
Decoder-output types (`VideoFrame<P, E, D>`, `AudioFrame<S, C, E, D>`,
`SubtitleFrame<E, D>`) stay in mediadecode — they carry timestamp +
backend-extras / sample-format / channel-layout, which is mediadecode's
domain.
PixelFormat::Unknown shape change
videoframe's `PixelFormat::Unknown(u32)` is a tuple variant
(lossless wire round-trip), unlike the prior unit-variant `Unknown`.
Updated 17 sites in `mediadecode-ffmpeg` and `mediadecode-webcodecs`:
`_ => PixelFormat::Unknown(raw as u32)` (preserves the raw FFmpeg /
WebCodecs identifier through the cast).
`assert!(matches!(x, PixelFormat::Unknown(_)))`.
→ `PixelFormat::Unknown(0)`.
Tests
Breaking changes (pre-publish, acceptable)
matching on it must use `Unknown(_)` or `Unknown(raw)`.
`mediadecode::PixelFormat`, etc.) continue to resolve via the
re-exports.
🤖 Generated with Claude Code