Skip to content

Commit 0f66d73

Browse files
committed
fix(gif): one trailer per file, clean up a failed export, revert lockfile churn
- finish() wrote 0x3B and Drop wrote it again, so every GIF ended 3B 3B. Strict decoders reject that. Guard Drop with a finished flag; tests cover both the finish() path and the bail-out path that relies on Drop. - A mid-export error left a truncated .gif at the user's destination. The MP4 path removes it (discard_partial_output); do the same here. - Cargo.lock carried ~15 unrelated transitive bumps plus a new syn 3.0.3. The feature adds no direct dependency, so restore the lockfile to base.
1 parent 3586469 commit 0f66d73

2 files changed

Lines changed: 102 additions & 51 deletions

File tree

crates/Cargo.lock

Lines changed: 34 additions & 45 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/compositor/src/gif_export.rs

Lines changed: 68 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -146,13 +146,31 @@ impl Default for GifExportParams {
146146
/// Drive a single-clip GIF export end-to-end. Mirrors the shape of
147147
/// `pipeline::run_composited` so the bench can compare apples to
148148
/// apples once the readback cost has been measured.
149+
/// A failed run leaves a truncated GIF under exactly the name the user thinks
150+
/// they exported. Remove it rather than leave it lying around — same contract
151+
/// as `discard_partial_output` on the MP4 path.
149152
pub fn export_gif(
150153
screen: &str,
151154
webcam: &str,
152155
cursor_json: Option<&str>,
153156
out_path: &Path,
154157
params: &GifExportParams,
155158
progress: &mut dyn FnMut(u64),
159+
) -> Result<GifStats> {
160+
let result = export_gif_inner(screen, webcam, cursor_json, out_path, params, progress);
161+
if result.is_err() {
162+
let _ = std::fs::remove_file(out_path);
163+
}
164+
result
165+
}
166+
167+
fn export_gif_inner(
168+
screen: &str,
169+
webcam: &str,
170+
cursor_json: Option<&str>,
171+
out_path: &Path,
172+
params: &GifExportParams,
173+
progress: &mut dyn FnMut(u64),
156174
) -> Result<GifStats> {
157175
let width = params.width.unwrap_or(DEFAULT_GIF_WIDTH);
158176
let height = params.height.unwrap_or(DEFAULT_GIF_HEIGHT);
@@ -332,14 +350,21 @@ struct GifWriter<W: Write> {
332350
w: W,
333351
width: u16,
334352
height: u16,
353+
/// Set by `finish`, so `Drop` does not append a second trailer.
354+
finished: bool,
335355
}
336356

337357
impl<W: Write> GifWriter<W> {
338358
fn new(w: W, width: u16, height: u16) -> Result<Self> {
339359
if width == 0 || height == 0 {
340360
bail!("gif: dimensions must be > 0 (got {width}x{height})");
341361
}
342-
Ok(GifWriter { w, width, height })
362+
Ok(GifWriter {
363+
w,
364+
width,
365+
height,
366+
finished: false,
367+
})
343368
}
344369

345370
/// Write the GIF89a header + Logical Screen Descriptor. No global
@@ -434,6 +459,10 @@ impl<W: Write> GifWriter<W> {
434459
/// path that wants an explicit "we're done, no more frames"
435460
/// signal.
436461
fn finish(&mut self) -> Result<()> {
462+
if self.finished {
463+
return Ok(());
464+
}
465+
self.finished = true;
437466
self.w.write_all(&[0x3B])?;
438467
self.w.flush()?;
439468
Ok(())
@@ -442,12 +471,15 @@ impl<W: Write> GifWriter<W> {
442471

443472
impl<W: Write> Drop for GifWriter<W> {
444473
fn drop(&mut self) {
445-
// Best-effort trailer; if the buffer failed before, this is
446-
// also the path that records the failure. We intentionally
447-
// don't propagate the result — `Drop` can't return errors.
448-
// A failed write is logged and the process continues; the
449-
// resulting file will be truncated/invalid, which the
474+
// Best-effort trailer for the paths that bail out before calling
475+
// `finish`. Skipped when `finish` already wrote one — two trailer
476+
// bytes are tolerated by most decoders but rejected by strict ones.
477+
// We intentionally don't propagate the result — `Drop` can't return
478+
// errors. The resulting file will be truncated/invalid, which the
450479
// caller will detect on the next read.
480+
if self.finished {
481+
return;
482+
}
451483
let _ = self.w.write_all(&[0x3B]);
452484
let _ = self.w.flush();
453485
}
@@ -920,6 +952,36 @@ mod tests {
920952
/// The most basic round-trip: write a 2×2 frame and check the
921953
/// file is well-formed GIF89a. No decode — we just walk the
922954
/// output bytes and confirm the structural shape.
955+
#[test]
956+
fn gif_writer_writes_exactly_one_trailer() {
957+
// `finish` writes 0x3B, and so does `Drop`. Without the guard the file
958+
// ends `3B 3B`, which strict decoders reject.
959+
let mut buf = Vec::new();
960+
{
961+
let mut gw = GifWriter::new(&mut buf, 2, 2).unwrap();
962+
gw.write_header().unwrap();
963+
gw.finish().unwrap();
964+
}
965+
assert_eq!(buf.last(), Some(&0x3B));
966+
assert_ne!(
967+
buf[buf.len() - 2],
968+
0x3B,
969+
"trailer written twice: {:02X?}",
970+
&buf[buf.len() - 2..]
971+
);
972+
}
973+
974+
#[test]
975+
fn gif_writer_drop_still_terminates_without_finish() {
976+
// The bail-out paths never call `finish`; `Drop` must still close the file.
977+
let mut buf = Vec::new();
978+
{
979+
let mut gw = GifWriter::new(&mut buf, 2, 2).unwrap();
980+
gw.write_header().unwrap();
981+
}
982+
assert_eq!(buf.last(), Some(&0x3B));
983+
}
984+
923985
#[test]
924986
fn gif_writer_writes_minimal_header() {
925987
let mut buf = Vec::new();

0 commit comments

Comments
 (0)