mirror of
https://github.com/storytold/filmcraft.git
synced 2026-10-22 18:22:22 +00:00
#854: GIF frame delays add up to the sequence duration
Problem: the GIF exporter gave every frame the same delay of 1000·den/num ms, which the encoder truncates to whole centiseconds. At 24 fps each 41.67 ms frame became 4 cs, so a 4.0 s sequence played in 3.84 s (4 % fast); other non-integer rates drifted the same way. Fix: each frame's delay now runs from its rounded start time to the next frame's (gif_delay_cs), so delays alternate (4, 4, 5, 4, 4, 4 cs at 24 fps) and add up to the exact duration. Checked i128 math also replaces the unchecked `(1000 * den) as u32` cast. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
1 parent
2ff3f5e266
commit
7625ddf4de
2 files changed
+40
-2
No files matched your search
@@ -1671,6 +1671,19 @@ pub fn frame_span(rate: FrameRate, range: TimeRange) -> (i64, i64) {
|
||||
(f0, f1.max(f0))
|
||||
}
|
||||
|
||||
/// The delay of the `i`th GIF frame (from the start of the export) in centiseconds, the unit GIF
|
||||
/// stores. Each frame runs from its rounded start time to the next frame's, so the delays add up
|
||||
/// to the exact duration (24 fps: 4, 4, 5, 4, 4, 4 cs …) instead of every frame truncating
|
||||
/// 4.17 cs to 4 cs and the animation playing 4 % fast (#854).
|
||||
fn gif_delay_cs(rate: FrameRate, i: i64) -> u16 {
|
||||
let num = i128::from(rate.num.max(1));
|
||||
let den = i128::from(rate.den.max(0));
|
||||
// start of frame `k` in centiseconds, rounded half up
|
||||
let at = |k: i64| i128::from(k.max(0)).saturating_mul(200).saturating_mul(den).saturating_add(num) / num.saturating_mul(2);
|
||||
let cs = at(i.saturating_add(1)).saturating_sub(at(i));
|
||||
u16::try_from(cs.clamp(0, i128::from(u16::MAX))).unwrap_or(u16::MAX)
|
||||
}
|
||||
|
||||
/// An in-memory writer that stays reachable after the encoder that owns a clone is dropped.
|
||||
#[derive(Clone, Default)]
|
||||
struct SharedBuf(Arc<Mutex<Vec<u8>>>);
|
||||
@@ -1809,7 +1822,6 @@ pub fn export(project: &Arc<Project>, seq: ItemId, settings: &ExportSettings, so
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let delay = image::Delay::from_numer_denom_ms((1000 * pipe.rate.den) as u32, pipe.rate.num as u32);
|
||||
let mut f = f0;
|
||||
while f < f1 {
|
||||
if cancelled() {
|
||||
@@ -1818,8 +1830,9 @@ pub fn export(project: &Arc<Project>, seq: ItemId, settings: &ExportSettings, so
|
||||
let end = (f + batch).min(f1);
|
||||
if let Some(enc) = gif.as_mut() {
|
||||
let frames: Vec<Vec<u8>> = (f..end).into_par_iter().map(|fi| pipe.frame(fi, sources).0).collect();
|
||||
for rgba in frames {
|
||||
for (fi, rgba) in (f..end).zip(frames) {
|
||||
let img = image::RgbaImage::from_raw(w, h, rgba).ok_or_else(|| ExportError::Encode("frame".into()))?;
|
||||
let delay = image::Delay::from_numer_denom_ms(u32::from(gif_delay_cs(pipe.rate, fi - f0)) * 10, 1);
|
||||
enc.encode_frame(image::Frame::from_parts(img, 0, 0, delay)).map_err(|e| ExportError::Encode(e.to_string()))?;
|
||||
}
|
||||
} else {
|
||||
|
||||
@@ -89,6 +89,31 @@ fn wav_png_gif() {
|
||||
assert!(std::path::Path::new(&tmp("seq005.png")).exists());
|
||||
}
|
||||
|
||||
/// GIF delays are whole centiseconds (#854): at 24 fps they alternate 4/4/5 cs so a second of
|
||||
/// frames lasts 1000 ms, not 24 × 40 ms = 960 ms.
|
||||
#[test]
|
||||
fn gif_frame_delays_add_up_to_the_duration() {
|
||||
let d: Vec<u16> = (0..6).map(|i| gif_delay_cs(FrameRate::FPS_24, i)).collect();
|
||||
assert_eq!(d, [4, 4, 5, 4, 4, 4]);
|
||||
let ntsc: u64 = (0..30).map(|i| u64::from(gif_delay_cs(FrameRate::FPS_29_97, i))).sum();
|
||||
assert_eq!(ntsc, 100, "30 frames at 29.97 fps last 1.001 s");
|
||||
// hostile rates neither panic nor overflow
|
||||
assert_eq!(gif_delay_cs(FrameRate { num: 0, den: 0 }, 3), 0);
|
||||
let _ = gif_delay_cs(FrameRate { num: 1, den: i64::MAX }, i64::MAX);
|
||||
|
||||
let (p, seq, m) = project();
|
||||
let path = tmp("timing.gif");
|
||||
let mut s = ExportSettings { format: Format::Gif, path: path.clone(), scale: 0.25, ..Default::default() };
|
||||
s.range = Some(TimeRange::new(Tick::ZERO, FrameRate::FPS_24.tick_of(24)));
|
||||
export(&p, seq, &s, &m, &Progress::default()).unwrap();
|
||||
use image::AnimationDecoder;
|
||||
let dec = image::codecs::gif::GifDecoder::new(std::io::Cursor::new(std::fs::read(&path).unwrap())).unwrap();
|
||||
let frames = dec.into_frames().collect_frames().unwrap();
|
||||
assert_eq!(frames.len(), 24);
|
||||
let ms: u32 = frames.iter().map(|f| f.delay().numer_denom_ms()).map(|(n, d)| n / d.max(1)).sum();
|
||||
assert_eq!(ms, 1000);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prores_export_roundtrip() {
|
||||
let (p, seq, m) = project();
|
||||
|
||||
Reference in new issue
Block a user