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 <[email protected]>

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<u8>>, Vec<[Vec<u8>; 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<u8>], recon: &[[Vec<u8>; 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]

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);