Put a sound on the frame it is actually on

src/export/markers.test.ts

@@ -0,0 +1,59 @@
+import { describe, expect, it } from 'vitest';
+import { emptyProject, makeCue } from '../timeline/project.ts';
+import type { CueSource, Project } from '../timeline/types.ts';
+import { markerCsv } from './markers.ts';
+
+function projectWith(times: readonly number[], fps = 30): Project {
+  return {
+    ...emptyProject(),
+    fps,
+    duration: 30,
+    cues: times.map((at) => makeCue(at, 'impacts', { kind: 'design', name: 'impact' } as CueSource)),
+  };
+}
+
+/** The rows, split into columns, without the header. */
+function rows(csv: string): string[][] {
+  return csv.trim().split('\n').slice(1).map((line) => line.split(','));
+}
+
+/*
+ * A marker list is handed to somebody laying the sound against picture, and
+ * every column in it is a claim about which frame a sound is on.
+ */
+describe('the marker list', () => {
+  /*
+   * The fault this was written for.
+   *
+   * The frame column was rounded from the time and the timecode column was
+   * floored from the fraction of a second, so the two disagreed whenever
+   * floating point put them either side of a boundary. A sound at 2.3
+   * seconds was written as frame 69 next to a timecode of 00:00:02:08, which
+   * is frame 68. Whoever reads that has to pick one, with nothing to say
+   * which is right.
+   */
+  it('says the same frame in both of its frame columns', () => {
+    const fps = 30;
+    const times: number[] = [];
+    for (let frame = 0; frame < fps * 60; frame++) times.push(frame / fps);
+    const disagreed = rows(markerCsv(projectWith(times, fps)))
+      .filter((row) => row[3] !== 'end of sound')
+      .filter((row) => Number(row[0].split(':')[3]) !== Number(row[2]) % fps)
+      .map((row) => `${row[0]} vs frame ${row[2]}`);
+    expect(disagreed.slice(0, 8), `${disagreed.length} rows disagreed with themselves`).toEqual([]);
+  });
+
+  it('puts a sound on the frame it was placed on', () => {
+    const [first, second] = rows(markerCsv(projectWith([2.3, 7.3])));
+    expect(first[0]).toBe('00:00:02:09');
+    expect(first[2]).toBe('69');
+    expect(second[0]).toBe('00:00:07:09');
+    expect(second[2]).toBe('219');
+  });
+
+  it('still writes a header and a closing row', () => {
+    const csv = markerCsv(projectWith([1]));
+    expect(csv.split('\n')[0]).toBe('timecode,seconds,frame,sound,layer,lands,length,level');
+    expect(rows(csv).at(-1)?.[3]).toBe('end of sound');
+  });
+});

src/export/markers.ts

@@ -1,4 +1,4 @@
-import { audibleCues, cueLength, cueStart } from '../timeline/project.ts';
+import { audibleCues, cueLength, cueStart, frameAt, timecode } from '../timeline/project.ts';
 import type { Project } from '../timeline/types.ts';
 
 /**
@@ -22,11 +22,22 @@ export function markerCsv(project: Project): string {
   const cues = [...audibleCues(project)].sort((a, b) => a.time - b.time);
 
   for (const cue of cues) {
+    /*
+     * One frame number, written twice.
+     *
+     * The two columns used to be worked out separately -- the frame rounded
+     * from the time, the timecode floored from the fraction of a second --
+     * and disagreed whenever floating point put those on either side of a
+     * boundary. A sound at 2.3 seconds was written as frame 69 on the same
+     * row that its timecode called 00:00:02:08, which is frame 68. Somebody
+     * laying that up has to pick one and has no way to tell which.
+     */
+    const frame = frameAt(cue.time, project.fps);
     rows.push(
       [
-        smpte(cue.time, project.fps),
+        timecode(cue.time, project.fps),
         cue.time.toFixed(3),
-        String(Math.round(cue.time * project.fps)),
+        String(frame),
         csv(String(cue.source.name)),
         csv(layers.get(cue.layerId) ?? cue.layerId),
         // Where the marker is relative to the sound, which is the difference
@@ -42,27 +53,13 @@ export function markerCsv(project: Project): string {
   // file up knows whether anything is still ringing past the end of the video.
   const last = cues.reduce((max, cue) => Math.max(max, cueStart(cue) + cueLength(cue)), 0);
   rows.push(
-    [smpte(last, project.fps), last.toFixed(3), String(Math.round(last * project.fps)),
+    [timecode(last, project.fps), last.toFixed(3), String(frameAt(last, project.fps)),
      'end of sound', '', '', '', ''].join(','),
   );
 
   return `${rows.join('\n')}\n`;
 }
 
-/** Hours, minutes, seconds and frames, which is the form editors read. */
-function smpte(time: number, fps: number): string {
-  const rate = fps || 30;
-  const safe = Math.max(0, time);
-  const whole = Math.floor(safe);
-  const pad = (value: number): string => String(value).padStart(2, '0');
-  return [
-    pad(Math.floor(whole / 3600)),
-    pad(Math.floor((whole % 3600) / 60)),
-    pad(whole % 60),
-    pad(Math.floor((safe - whole) * rate)),
-  ].join(':');
-}
-
 /** Quote a field only where it would otherwise break the row. */
 function csv(value: string): string {
   return /[",\n]/.test(value) ? `"${value.replace(/"/g, '""')}"` : value;

src/timeline/project.ts

@@ -174,13 +174,53 @@ function applySnap(time: number, snap: SnapMode, project: Project): number {
  * and it costs three characters.
  */
 export function timecode(time: number, fps: number): string {
-  const safe = Math.max(0, time);
-  const hours = Math.floor(safe / 3600);
-  const minutes = Math.floor((safe % 3600) / 60);
-  const seconds = Math.floor(safe % 60);
-  const frames = Math.floor((safe % 1) * (fps || DEFAULT_FPS));
+  const rate = fps || DEFAULT_FPS;
+  /*
+   * Frames are counted in whole slots even when the rate is not whole.
+   *
+   * 29.97 fills thirty slots and takes 1.001 seconds to do it, so its
+   * timecode drifts from the wall clock by about two seconds an hour. That
+   * drift is what non-drop timecode is, and it is what an edit suite shows,
+   * so following it is the point rather than a rounding convenience. Taking
+   * the remainder against 29.97 itself would print a fraction of a frame,
+   * which is not a thing any of them can read.
+   */
+  const slots = Math.max(1, Math.round(rate));
+  const frames = frameAt(time, rate);
+  const whole = Math.floor(frames / slots);
   const pad = (value: number): string => String(value).padStart(2, '0');
-  return `${pad(hours)}:${pad(minutes)}:${pad(seconds)}:${pad(frames)}`;
+  return [
+    pad(Math.floor(whole / 3600)),
+    pad(Math.floor((whole % 3600) / 60)),
+    pad(whole % 60),
+    pad(frames % slots),
+  ].join(':');
+}
+
+/**
+ * Which frame a time falls on.
+ *
+ * Counted from the start rather than from the second it sits in, which is the
+ * whole of the fix this replaced. Taking the fraction first — `(time % 1) *
+ * fps` — asks binary floating point for a number it does not hold: two point
+ * three minus two is 0.2999999999999998, and thirty of those floor to frame
+ * 8 rather than 9. Nearly half of every frame position at 30fps came out a
+ * frame early that way, and the marker list disagreed with itself, its
+ * timecode column saying 00:00:02:08 on the same row its frame column said
+ * 69.
+ *
+ * The tolerance is what makes a snapped time land on its own frame. A cue at
+ * frame 41 is held as 41/30 seconds, which multiplies back to 41.00000000004
+ * on a good day and 40.99999999999 on a bad one, and flooring the second of
+ * those loses the frame again. A millionth of a frame is 33 nanoseconds at
+ * 30fps — far below anything the app can place — so it can absorb the error
+ * without reaching a real difference.
+ *
+ * Flooring rather than rounding, because this also reads a playhead that is
+ * still moving: two thirds of the way through frame 68 is frame 68, not 69.
+ */
+export function frameAt(time: number, fps: number): number {
+  return Math.floor(Math.max(0, time) * (fps || DEFAULT_FPS) + 1e-6);
 }
 
 /**

src/timeline/timecode.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from 'vitest';
-import { parseTimecode, timecode } from './project.ts';
+import { frameAt, parseTimecode, timecode } from './project.ts';
 
 /*
  * The format is the whole point of these.
@@ -107,3 +107,85 @@ describe('parseTimecode', () => {
     expect(parseTimecode('00:00:01:15', 0)).toBeCloseTo(1.5, 6);
   });
 });
+
+/*
+ * The frame a time is actually on.
+ *
+ * These are the ones that were wrong. `timecode` used to take the fraction of
+ * a second first and multiply that by the rate, which asks binary floating
+ * point for numbers it does not hold: 2.3 minus 2 is 0.2999999999999998, and
+ * thirty of those floor to 8. Nearly half of every frame position at 30fps
+ * came out a frame early, and it went unnoticed because a frame is a
+ * thirtieth of a second and the readout looked plausible either way.
+ *
+ * The app's whole job is landing a sound on the frame a cut happens on, so
+ * the sweep is the test that matters: not a handful of cases someone thought
+ * to write down, but every frame position over ten minutes at each rate.
+ */
+describe('frameAt', () => {
+  it('puts every frame-snapped time on its own frame', () => {
+    for (const fps of [24, 25, 30, 48, 50, 60]) {
+      const wrong: number[] = [];
+      for (let frame = 0; frame < fps * 600; frame++) {
+        if (frameAt(frame / fps, fps) !== frame) wrong.push(frame);
+      }
+      expect(wrong.slice(0, 8), `${fps}fps, ${wrong.length} wrong of ${fps * 600}`).toEqual([]);
+    }
+  });
+
+  it('does not round up to a frame that has not started', () => {
+    // Two thirds of the way through frame 68 is still frame 68.
+    expect(frameAt(68.67 / 30, 30)).toBe(68);
+    expect(frameAt(0.999, 30)).toBe(29);
+  });
+
+  it('treats a time before the start as the start', () => {
+    expect(frameAt(-10, 30)).toBe(0);
+  });
+});
+
+describe('timecode and frameAt agree', () => {
+  /*
+   * The two used to be worked out separately, and the marker list wrote both
+   * on the same row: a sound at 2.3 seconds was frame 69 in one column and
+   * 00:00:02:08 -- frame 68 -- in the next.
+   */
+  it('names the same frame, over ten minutes', () => {
+    const fps = 30;
+    const wrong: string[] = [];
+    for (let frame = 0; frame < fps * 600; frame++) {
+      const at = frame / fps;
+      const shown = Number(timecode(at, fps).split(':')[3]);
+      if (shown !== frameAt(at, fps) % fps) wrong.push(`${frame}: ${timecode(at, fps)}`);
+    }
+    expect(wrong.slice(0, 8), `${wrong.length} rows disagreed`).toEqual([]);
+  });
+
+  it('the case that was wrong on screen', () => {
+    expect(timecode(2.3, 30)).toBe('00:00:02:09');
+    expect(timecode(7.3, 30)).toBe('00:00:07:09');
+    expect(frameAt(2.3, 30)).toBe(69);
+  });
+});
+
+/*
+ * Rates that are not whole numbers still have to print two digits.
+ *
+ * 29.97 fills thirty frame slots and takes 1.001 seconds over it, which is
+ * what non-drop timecode is. Taking the remainder against 29.97 itself prints
+ * a fraction of a frame, which no edit suite can read.
+ */
+describe('timecode at broadcast rates', () => {
+  it('writes whole frames, never a fraction of one', () => {
+    for (const fps of [23.976, 29.97, 59.94]) {
+      for (const at of [0, 0.5, 1, 61, 3600]) {
+        expect(timecode(at, fps), `${at}s at ${fps}`).toMatch(/^\d\d:\d\d:\d\d:\d\d$/);
+      }
+    }
+  });
+
+  it('counts thirty slots at 29.97, not 29.97 of them', () => {
+    expect(timecode(29 / 29.97, 29.97)).toBe('00:00:00:29');
+    expect(timecode(30 / 29.97, 29.97)).toBe('00:00:01:00');
+  });
+});