diff --git a/src/export/markers.test.ts b/src/export/markers.test.ts new file mode 100644 index 0000000..1d9cecc --- /dev/null +++ b/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'); + }); +}); diff --git a/src/export/markers.ts b/src/export/markers.ts index a952194..dba5497 100644 --- a/src/export/markers.ts +++ b/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; diff --git a/src/timeline/project.ts b/src/timeline/project.ts index 0f8a913..b215ce7 100644 --- a/src/timeline/project.ts +++ b/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); } /** diff --git a/src/timeline/timecode.test.ts b/src/timeline/timecode.test.ts index fdd5b5d..cf83e80 100644 --- a/src/timeline/timecode.test.ts +++ b/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'); + }); +});