Repository navigation
fix(producer): keep page timers on the film's schedule at every worker count - #7390
Conversation
There was a problem hiding this comment.
Request changes at 68b5cd2f. The only blocker is in the PR description. The fix itself looks correct.
Blocker: the fails without the fix: line doesn't name a test. The line has to name the test that fails without the fix. Line 19 of the body only describes the symptom: "on main the chain has not fired at 133 ms ([], expected [100])…". The test's name is in the sentence before it, not on the line. Putting it on the line clears this:
fails without the fix: packages/producer/src/services/fileServer-virtualTimers.test.ts › "virtual-time shim timers" › "keeps the film's timer schedule on a worker whose first frame is past the start" (on main: [] instead of [100] at 133 ms)
Editing only the body doesn't change the head, so I'll re-review at this same SHA.
What I verified:
- At the head,
fileServer-virtualTimers.test.ts,fileServer-seededRandom.test.tsandfileServer.test.tsgive 77 pass, 0 fail. - With
fileServer.tstaken frommain(b87a01f6a), the new test fails (8 pass, 1 fail). The other tests in that file still pass.
Code review found no other issues:
- The extra
seekToTime(0)happens only on the first seek after__renderReady, becauserenderingis never reset. The inner call seesrendering === true, so it can't recurse again. - A one-worker render's first seek is 0, so its path is unchanged.
- With seeding on, the frame-0 pass runs
reseedRngFromTime(0)before the target's reseed. That matches the sequence a one-worker render sees. - Timers due between 0 and the target still fire in due order when the target seek runs
fireDueTimers. - The
seekToTimeReseedCallindent change only follows the function moving out of the object literal.
Note, not blocking: the frame-0 pass also runs the page's frame-0 requestAnimationFrame callbacks on every worker past frame 0. That's the intent, and it matches a one-worker render, but it's a small cost per chunk.
CI was still running when I reviewed: CI, CodeQL, regression and Windows render verification. preview-regression, Player perf, PR captures and Comments passed.
There was a problem hiding this comment.
Approve at 68b5cd2f. The blocker is resolved.
- The
fails without the fix:line now namesfileServer-virtualTimers.test.ts› "virtual-time shim timers" › "keeps the film's timer schedule on a worker whose first frame is past the start". I reproduced this earlier: withmain'sfileServer.tsthat test fails (8 pass, 1 fail). At the head the threefileServer*suites give 77 pass, 0 fail. - The head is unchanged since my last review, so the code findings there still apply.
- Actions at this head: CI, CodeQL, regression, preview-regression, Windows render verification, Player perf, PR captures and Comments all passed. The body edit started a second Windows render verification run, which was still running when I approved. The first run on this head passed.
Port the adopted upstream fixes from f5bde95 to the current upstream tip: heygen-com#7375 multipart cloud upload, heygen-com#7382 minParallelChunks, heygen-com#7349 HLG header SEI, heygen-com#7369 skip unmounted compositions in lint/check, heygen-com#7381 filesystem-path media in the cloud archive, heygen-com#7383 Safari cut-point pre-roll revert, heygen-com#7373 clock steering between audio time updates, heygen-com#7384 injector test isolation, heygen-com#7378 preview seek retention, heygen-com#7390 virtual timer schedule per worker, heygen-com#7394 CDN script failure naming, heygen-com#7395 menu row radius, heygen-com#7119 decode timeout caller test, heygen-com#7397 authored tween order, heygen-com#7402 capture page clip loading, heygen-com#7412 muted video audio track at cuts, heygen-com#6150 template root variable test, heygen-com#7442 held tail seek once. Deliberate local differences: heygen-com#7067's audio-start hold is not carried into clock.ts, mountedSources comes from heygen-com#6827 as a helper only, cloud/_gen/types.ts omits the image-request types the fork does not generate, and the lint symlink test skips through symlinkOrSkip where the account cannot create links. Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
What
Page
setTimeoutandsetIntervalnow keep the film's schedule at every worker count, not only with one worker.Why
#7362 runs page timers on film time during a render. A timer a page starts in its first animation frame is scheduled from that frame's time. With one worker, the first frame a page renders is the film's frame 0, so the schedule starts at 0. With several workers, each worker's page starts at its own first frame. A worker whose first frame is at 133 ms schedules a 100 ms chain from 133 ms, so its ticks land at 233, 333 and so on, a step behind the film for the rest of that worker's frames.
The
timers-on-film-timeregression film shows it. Its bars match the reference video on every frame at 1 worker, and on 33 of 90 frames they are a step short at 6 workers.How
seekToTimein the virtual-time shim is now a named function. On the seek that starts rendering, if the target is past 0, it first plays the film's frame 0: it fires timers due by 0 and flushes animation frames at 0. Then it seeks to the target as before, which fires every timer due in between, in due order. A one-worker render is unchanged, because its first seek is already 0.Tests
New in
fileServer-virtualTimers.test.ts: "keeps the film's timer schedule on a worker whose first frame is past the start". A page starts a 100 ms chain in its first animation frame, and the first seek is to frame 4 (133 ms).fails without the fix: packages/producer/src/services/fileServer-virtualTimers.test.ts › "virtual-time shim timers" › "keeps the film's timer schedule on a worker whose first frame is past the start" (on main:
[]instead of[100]at 133 ms, because the chain was scheduled from 133 ms).Rendered the
timers-on-film-timefilm from this branch on Linux and compared every bar's width on every frame with the film's reference video:Also run:
fileServer-virtualTimers.test.ts,fileServer-seededRandom.test.tsandfileServer.test.ts: 77 of 77, three runs in a row.typecheck, oxfmt and the comment ratchet.Size
About 25 lines in one function and one test.