Skip to content

Commit 902ec1c

Browse files
committed
fix(frame): validate plane_count + add Debug for ffmpeg Frame
Audit issue #4 findings: 1. (LOW, accepted) `VideoFrame::new` / `AudioFrame::new` accept `plane_count: u8` but the `planes` arrays are fixed at 4 / 8 slots. A caller passing `plane_count > 4` (or `> 8`) would later trip the slice indexing inside `.planes()` far from the construction site. Add explicit `assert!` at construction so the failure mode names `plane_count` directly. Both constructors gain `# Panics` docs and dedicated `should_panic` tests. 2. (LOW, accepted) `mediadecode_ffmpeg::Frame` lacked a `Debug` impl because the inner `ffmpeg_next::frame::Video` doesn't derive one. Add a manual impl that reports width, height, pixel format, plane count, and PTS via the public accessors — useful for debug-print sites without surfacing raw FFI internals. Other audit recommendations (INFO `Dimensions` overflow, `#[must_use]` sweep on builder methods, native webcodecs tests) intentionally not applied — informational or stylistic, low value relative to churn.
1 parent 6f3ff58 commit 902ec1c

2 files changed

Lines changed: 62 additions & 0 deletions

File tree

mediadecode-ffmpeg/src/frame.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,23 @@ pub struct Frame {
7575
inner: frame::Video,
7676
}
7777

78+
impl core::fmt::Debug for Frame {
79+
/// `frame::Video` (from `ffmpeg_next`) doesn't itself implement
80+
/// `Debug`, so route through the public accessors. Shows the
81+
/// dimensions, pixel format, plane count, and PTS — enough to
82+
/// distinguish frames at debug-print sites without surfacing
83+
/// raw FFI internals.
84+
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
85+
f.debug_struct("Frame")
86+
.field("width", &self.width())
87+
.field("height", &self.height())
88+
.field("pix_fmt", &self.pix_fmt())
89+
.field("planes", &self.planes())
90+
.field("pts", &self.pts())
91+
.finish()
92+
}
93+
}
94+
7895
impl Frame {
7996
/// Construct an empty frame, suitable as the destination passed to
8097
/// [`crate::VideoDecoder::receive_frame`].

mediadecode/src/frame.rs

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,14 @@ impl<P, E, D> VideoFrame<P, E, D> {
289289
/// `dimensions` is the coded width/height pair (see
290290
/// [`Dimensions`] and [`Self::dimensions`] for the visible-vs-
291291
/// coded distinction).
292+
///
293+
/// # Panics
294+
///
295+
/// Panics if `plane_count > 4`. The fixed-size `planes` array
296+
/// has four slots; passing a larger `plane_count` would later
297+
/// trip the slice indexing inside [`Self::planes`] far from
298+
/// the construction site. Asserting here fails fast with a
299+
/// clear message instead.
292300
#[cfg_attr(not(tarpaulin), inline(always))]
293301
pub const fn new(
294302
dimensions: Dimensions,
@@ -297,6 +305,10 @@ impl<P, E, D> VideoFrame<P, E, D> {
297305
plane_count: u8,
298306
extra: E,
299307
) -> Self {
308+
assert!(
309+
plane_count as usize <= 4,
310+
"VideoFrame::new: plane_count exceeds the fixed 4-plane array",
311+
);
300312
Self {
301313
pts: None,
302314
duration: None,
@@ -459,6 +471,13 @@ pub struct AudioFrame<S, C, E, D> {
459471

460472
impl<S, C, E, D> AudioFrame<S, C, E, D> {
461473
/// Constructs an `AudioFrame`.
474+
///
475+
/// # Panics
476+
///
477+
/// Panics if `plane_count > 8`. The fixed-size `planes` array
478+
/// has eight slots; passing a larger `plane_count` would
479+
/// later trip the slice indexing inside [`Self::planes`] far
480+
/// from the construction site.
462481
#[allow(clippy::too_many_arguments)]
463482
#[cfg_attr(not(tarpaulin), inline(always))]
464483
pub const fn new(
@@ -471,6 +490,10 @@ impl<S, C, E, D> AudioFrame<S, C, E, D> {
471490
plane_count: u8,
472491
extra: E,
473492
) -> Self {
493+
assert!(
494+
plane_count as usize <= 8,
495+
"AudioFrame::new: plane_count exceeds the fixed 8-plane array",
496+
);
474497
Self {
475498
pts: None,
476499
duration: None,
@@ -780,6 +803,28 @@ mod tests {
780803
]
781804
}
782805

806+
#[test]
807+
#[should_panic(expected = "plane_count exceeds the fixed 4-plane array")]
808+
fn video_frame_rejects_plane_count_above_array_size() {
809+
let _f: VideoFrame<u32, (), &[u8]> =
810+
VideoFrame::new(Dimensions::new(64, 64), 0u32, empty_planes(), 5, ());
811+
}
812+
813+
#[test]
814+
#[should_panic(expected = "plane_count exceeds the fixed 8-plane array")]
815+
fn audio_frame_rejects_plane_count_above_array_size() {
816+
let _f: AudioFrame<u32, u32, (), &[u8]> = AudioFrame::new(
817+
48_000,
818+
1024,
819+
2,
820+
0u32,
821+
0u32,
822+
audio_planes(),
823+
9,
824+
(),
825+
);
826+
}
827+
783828
#[test]
784829
fn audio_frame_construct_and_access() {
785830
// AudioFrame<S, C, E, D>: S=u32 (SampleFormat), C=u32 (ChannelLayout),

0 commit comments

Comments
 (0)