Repository navigation
fix(engine): the drawElement self-check reads its truth on a page the render does not use - #6316
Conversation
…xt it should accept The check's ground-truth screenshot was taken after a seek that suppressed timeline callbacks, so text a callback writes (a count-up) was stale in the truth while the real render, which runs callbacks, showed it correctly. The check failed and the whole render restarted on the slower screenshot path.
|
Parked as a draft: with only the eventful seek, a composition whose callback changes the page cumulatively (a timeline call that sets text) passes the check at 70 dB but the drawElement video shows the changed state at frames before the call, because the check's own forward seeks run the callback and leave the page changed for the real render. The check has to take its truth on a page the render does not use. Reworking that here. |
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head 4fe97cae
The fix is right. The render's own DE loop calls __hf.seek(t), which defaults to eventful, and the static-verify seek already passes suppressEvents: false, so the truth now matches the render. The only blocker is that the Comments check fails on this head. ↓ inline
Scope
Read the captureDeVerificationFrames seek and the new helper and test. I checked the render-path seeks (drawElementService.ts:1092, seekPageTimeline, static verify frameCapture.ts:3643) and the runtime default (init.ts:4665). CI was still running when I posted this review.
| return [...Array.from({ length: k - 1 }, (_, i) => (i + 1) / k), 0.95]; | ||
| } | ||
|
|
||
| /** Runs timeline callbacks like the render's seek; suppressed, a count-up's text is stale in the truth. */ |
There was a problem hiding this comment.
The Comments check fails here: the comment ratchet reports frameCapture.ts at 21.8% (1153/5284) against 21.8% (1152/5281) at the base, and this added docblock line is the one extra comment line. Drop it; the helper's name and frameCapture-verifySeek.test.ts already carry the why.
| /** Runs timeline callbacks like the render's seek; suppressed, a count-up's text is stale in the truth. */ |
… render does not use Callbacks now run in the truth seek (a count-up or class swap no longer looks stale), and they run on a disposable page so a callback that changes the page cumulatively cannot change what the render draws. Compositions with videos keep the in-page path, because the video injector is tied to the render page.
miguel-heygen
changed the title
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 1 nit · head 9e6cb7d8
The Comments blocker from 4fe97cae is fixed. Reading the truth on a disposable page makes the eventful seek safe for video-free comps. That page is createStaticVerificationPage, which already serves static verify. One new blocker: a failure to open that page now fails the render instead of degrading. ↓ inline
- nit: the PR body still describes the
4fe97caeapproach (alwayssuppressEvents: falseon the render page). It doesn't mention the disposable page, or that video comps still read their truth with callbacks suppressed.
Scope
I read the rewritten captureDeVerificationFrames, createStaticVerificationPage, the finalizeDrawElementInit call path, and the producer injector. createVideoFrameInjector returns null without a frame lookup, so isolated covers the video-free render sessions, not just probes. CI (main and Windows) was still running when I posted this review.
| // A video-free comp reads its truth on a disposable page: callbacks that run there | ||
| // cannot leave the render's own page changed. The video injector is tied to the render page. | ||
| const isolated = !session.onBeforeCapture; | ||
| const truthPage = isolated ? await createStaticVerificationPage(session) : page; |
There was a problem hiding this comment.
This adds a second full page load (goto, pollHfReady, sub-composition and font waits) to every video-free drawElement init, and nothing catches it here. finalizeDrawElementInit → initializeSession has no try/catch, so a navigation timeout or a killed tab under memory pressure now fails session init in captureStage's ensureSessionReady or the streaming stage. Before, verify only took screenshots on a page that had already loaded. The static verify call to the same helper (frameCapture.ts:3795) catches this and degrades with verification_failed. Do the same here: on a creation failure, fall back to page with suppressEvents: true, which is the pre-PR behavior.
| const truthPage = isolated ? await createStaticVerificationPage(session) : page; | |
| const truthPage = isolated | |
| ? await createStaticVerificationPage(session).catch(() => null) | |
| : page; |
(…then const onTruthPage = truthPage ?? page, and suppress events and skip the close whenever the fallback was taken.)
There was a problem hiding this comment.
✅ Fixed in 3ec057b: when the truth page cannot open, the check falls back to the render page with callbacks suppressed and does not close it.
Edit accuracy: accurate 271 (base branch 271), smooth 140 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
|
Head 3ec057b: a truth page that cannot be opened now logs it and reads the truth on the render page with callbacks suppressed (the pre-change behaviour), so a failed open never fails the render. The PR body is refreshed to describe the disposable-page approach and the video-composition scope. No unit test for the fallback itself: the function has no seam without a real browser session; the open-failure path is the same |
What
The drawElement self-check reads its ground-truth screenshots on a disposable page, with timeline callbacks running, instead of on the render's own page with callbacks suppressed. Compositions with videos keep the old in-page path (the video injector is tied to the render page); a follow-up covers them.
Why
The check compared each drawElement frame with a screenshot taken after
__hf.seek(t, { suppressEvents: true }). That skips GSAP callbacks, so anything a callback draws (a count-up's text, a class swap that shows a panel, a timelinecall) was missing from the "truth" while the render, whose per-frame seek runs callbacks, drew it. The check failed (11 to 30 dB against the 32 dB bar) and the whole render restarted on the slower screenshot path.Making the seek run callbacks on the render's own page is not enough, and is unsafe: the check's forward seeks then run a callback that changes the page cumulatively (a
callthat sets text, say) and leave it changed, so the real render starts from a polluted page. In that case the check passed at 70 dB while the drawElement video showed the changed text before the callback's time. Taking the truth on a separate page (the patterncreateStaticVerificationPagealready documents for static-frame verification) removes both problems.Related work
None open. Size: a lone engine fix under the usual PR size floor; no other open PR touches the engine's capture code.
How
seekDeVerifyTruth(page, t, suppressEvents)is the one seek used for the truth.createStaticVerificationPage, seeks eventfully there (frame 0, then ascending, like the render) and closes it; the render's page is never touched.Test plan
frameCapture-verifySeek.test.tscovers the seek with callbacks running and suppressed, and a page without the seek protocol; lint, format and typecheck clean.onUpdateonStartswaps a full-screen paneltl.calltoggles full-screen panelstl.callsets text at 3 sThe last row is the one an eventful seek on the render page got wrong (it passed but showed the text after the call at 1 s). Output sizes match the screenshot render in every row.
Not covered: compositions with videos (unchanged path), and a render-level test of the open-failure fallback, which needs a real browser session.
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug