From e90f81850ca7bb9559ad12f2f523804887ba9ea7 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 14:15:33 +0700 Subject: [PATCH] VP9: frames never end in a byte that reads as a superframe marker (#161) A frame whose last byte happened to match the superframe marker pattern (0b110xxxxx, Annex B) was taken by decoders for a superframe with an index at its end: ffmpeg rejected it ("Invalid le value at frame_sizes[i]"), libvpx reported a corrupt frame, and later frames lost their reference. In the reported WebM, 3 of 60 frames (10, 22, 24) ended in 0xc8, 0xc0 and 0xca. The render still reported success. Such frames now get a zero byte after the last tile; the last tile's bool-coded data may end in zero padding (9.2.3). Test: frames_never_end_in_a_superframe_marker encodes sequences until a frame needs the padding, checks no frame ends in a marker byte, and that the padded frames decode exactly with FilmCraft's decoder and with ffmpeg (clean error log). It failed before ("96x64 q40 frame 22 ends in 0xde"). The ffmpeg IVF oracle is shared with the existing inter test. Checked with the reporter's steps: 60/60 frames, no errors from ffmpeg's vp9 or libvpx, RGB and RGB + Alpha. Co-Authored-By: Claude Opus 5.5 --- crates/export/tests/vp9_roundtrip.rs | 77 ++++++++++++++++++++++------ crates/vp9enc/src/lib.rs | 6 +++ 2 files changed, 67 insertions(+), 16 deletions(-) diff --git a/crates/export/tests/vp9_roundtrip.rs b/crates/export/tests/vp9_roundtrip.rs index 5bf46e0a..e7c2b034 100644 --- a/crates/export/tests/vp9_roundtrip.rs +++ b/crates/export/tests/vp9_roundtrip.rs @@ -259,11 +259,22 @@ fn inter_frames_are_much_smaller_than_intra() { eprintln!("{report:#?}"); } -#[test] -fn ffmpeg_decodes_inter_frames_like_the_encoder() { - let (w, h) = (96usize, 64usize); - let mut e = Vp9Encoder::new(EncoderConfig { width: w as u32, height: h as u32, quality: 75, keyframe_interval: 5, ..Default::default() }); - let n = 7; +/// Encode `n` frames of `moving(.., kind)`: (frames, the encoder's reconstruction of each). +fn encode_moving(cfg: EncoderConfig, n: usize, kind: u32) -> (Vec>, Vec<[Vec; 3]>) { + let (w, h) = (cfg.width as usize, cfg.height as usize); + let mut e = Vp9Encoder::new(cfg); + (0..n) + .map(|t| { + let (y, u, v) = moving(w, h, t, kind); + let f = e.encode_frame(&y, &u, &v, false); + (f.data, e.reconstruction().expect("recon")) + }) + .unzip() +} + +/// Decode `frames` with ffmpeg (an IVF file) and check every frame against `recon`, with nothing +/// on ffmpeg's error log. Skipped without ffmpeg. +fn ffmpeg_matches(w: usize, h: usize, frames: &[Vec], recon: &[[Vec; 3]], name: &str) { let mut ivf = Vec::new(); ivf.extend_from_slice(b"DKIF"); ivf.extend_from_slice(&0u16.to_le_bytes()); @@ -273,18 +284,14 @@ fn ffmpeg_decodes_inter_frames_like_the_encoder() { ivf.extend_from_slice(&(h as u16).to_le_bytes()); ivf.extend_from_slice(&30u32.to_le_bytes()); ivf.extend_from_slice(&1u32.to_le_bytes()); - ivf.extend_from_slice(&(n as u32).to_le_bytes()); + ivf.extend_from_slice(&(frames.len() as u32).to_le_bytes()); ivf.extend_from_slice(&0u32.to_le_bytes()); - let mut recon = vec![]; - for t in 0..n { - let (y, u, v) = moving(w, h, t, 0); - let f = e.encode_frame(&y, &u, &v, false); - ivf.extend_from_slice(&(f.data.len() as u32).to_le_bytes()); + for (t, f) in frames.iter().enumerate() { + ivf.extend_from_slice(&(f.len() as u32).to_le_bytes()); ivf.extend_from_slice(&(t as u64).to_le_bytes()); - ivf.extend_from_slice(&f.data); - recon.push(e.reconstruction().expect("recon")); + ivf.extend_from_slice(f); } - let dir = std::env::temp_dir().join(format!("effectcraft-vp9-inter-{}", std::process::id())); + let dir = std::env::temp_dir().join(format!("effectcraft-vp9-{name}-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); let path = dir.join("a.ivf"); std::fs::write(&path, &ivf).unwrap(); @@ -297,9 +304,9 @@ fn ffmpeg_decodes_inter_frames_like_the_encoder() { eprintln!("ffmpeg not found: skipping oracle"); return; }; - assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr)); + assert!(out.status.success() && out.stderr.is_empty(), "{}", String::from_utf8_lossy(&out.stderr)); let fsz = w * h + 2 * w.div_ceil(2) * h.div_ceil(2); - assert_eq!(out.stdout.len(), fsz * n); + assert_eq!(out.stdout.len(), fsz * frames.len()); for (t, r) in recon.iter().enumerate() { let frame = &out.stdout[t * fsz..(t + 1) * fsz]; assert_eq!(&frame[..w * h], &r[0][..], "ffmpeg luma of frame {t}"); @@ -307,6 +314,44 @@ fn ffmpeg_decodes_inter_frames_like_the_encoder() { } } +#[test] +fn ffmpeg_decodes_inter_frames_like_the_encoder() { + let (w, h) = (96usize, 64usize); + let cfg = EncoderConfig { width: w as u32, height: h as u32, quality: 75, keyframe_interval: 5, ..Default::default() }; + let (frames, recon) = encode_moving(cfg, 7, 0); + ffmpeg_matches(w, h, &frames, &recon, "inter"); +} + +/// A frame whose data would end in a byte that reads as a superframe marker (0b110xxxxx) gets a +/// zero byte after it; otherwise decoders take its tail for a superframe index and reject it +/// (#161: 3 of 60 frames of a WebM failed to decode). Such frames still decode exactly. +#[test] +fn frames_never_end_in_a_superframe_marker() { + let marker = |b: u8| b & 0xe0 == 0xc0; + let mut padded = None; + for (w, h) in [(96usize, 64usize), (130, 66), (160, 96)] { + for q in [40u8, 60, 75, 90] { + let cfg = EncoderConfig { width: w as u32, height: h as u32, quality: q, keyframe_interval: 12, ..Default::default() }; + let (frames, recon) = encode_moving(cfg, 24, 1); + for (t, f) in frames.iter().enumerate() { + assert!(!f.last().copied().is_some_and(marker), "{w}x{h} q{q} frame {t} ends in {:#x}", f[f.len() - 1]); + } + let needed = frames.iter().any(|f| f.len() >= 2 && f[f.len() - 1] == 0 && marker(f[f.len() - 2])); + if needed && padded.is_none() { + padded = Some((w, h, q, frames, recon)); + } + } + } + let Some((w, h, q, frames, recon)) = padded else { panic!("no frame needed the padding: widen the search") }; + // The padded frames decode exactly, with FilmCraft's decoder and with ffmpeg. + let mut d = Decoder::new(); + for (t, (f, r)) in frames.iter().zip(&recon).enumerate() { + let mut pics = d.decode(f, t as i64).unwrap_or_else(|e| panic!("{w}x{h} q{q} frame {t}: {e:?}")); + assert_eq!(planes_of(&pics.remove(0), w, h), *r, "{w}x{h} q{q} frame {t}"); + } + ffmpeg_matches(w, h, &frames, &recon, "marker"); +} + /// Encoder speed on 720p (run with `--ignored --nocapture`). #[test] #[ignore] diff --git a/crates/vp9enc/src/lib.rs b/crates/vp9enc/src/lib.rs index f438cbf1..9364dfaa 100644 --- a/crates/vp9enc/src/lib.rs +++ b/crates/vp9enc/src/lib.rs @@ -341,6 +341,12 @@ impl Vp9Encoder { } out.extend_from_slice(&t); } + // A frame mustn't end in a byte that reads as a superframe marker (0b110xxxxx, Annex B): + // decoders would take its last bytes for a superframe index and reject the frame. The + // last tile's bool-coded data may end in zero padding (9.2.3), so a zero byte fixes it. + if out.last().is_some_and(|b| b & 0xe0 == 0xc0) { + out.push(0); + } // State for the next frame. self.mvs = mi.iter().map(|m| if m.inter { m.mv } else { Mv::ZERO }).collect(); self.reference = Some(planes);