Repository navigation
fix(engine): bound injected video frame resource retention - #5494
Conversation
somanshreddy
left a comment
There was a problem hiding this comment.
Reviewed at 3d7cf641, and re-checked the comment-only delta to 0d786bf5, a fast-forward that touches only the frameCapture.ts comments. No blockers. This is a COMMENT, not an approval: the request came through a bot relay.
Response lifetime and error cleanup — correct.
- Every paused request gets an answer: 200, 404 for an unknown or already-released URL, or
failRequestif the fulfill fails, soimg.decode()can't hang. Promise.all(pendingDecodes)runs inside the samepage.evaluate, so all frame fetches finish beforefinallydeletes the URLs.- A
Fetch.enablefailure detaches the session and evicts the cached transport promise, so the next batch retries cleanly. page.once("close")clears the map.- WebGL and WebGPU (
video-texture-compat.ts) upload from the<img>element itself and never refetchsrc. That makes deleting the URL after decode safe for GPU compositions too. - Nothing else in engine or producer uses
setRequestInterceptionorFetch, so this second-session interceptor has no competing Fetch client.
Primary-session buffer cap — correct. It is guarded by "client" in frame && frame.client instanceof CDPSession and limited to capture-owned pages. Nothing in engine or producer reads Puppeteer response bodies (response.buffer()/text()), so the 16 MB per-resource cap can't break a body read. The transport's own CDP session never enables Network, so it adds no second buffer.
Verified locally at 3d7cf641:
- The 6 changed engine test files pass, 69 tests. The real-Chrome
videoFrameTransport.browser.test.tsran 5 of 5; it was not skipped. - Mutations, each caught:
- drop
Cache-Control: no-store→ "without retaining inline image URLs" fails - drop the
finallyrelease → 4 tests fail - drop
Fetch.failRequest→ "unblocks a failed protocol response" fails - rethrow the generic error instead of
frame.error→ the same test fails - drop the buffer cap → 4 construction-ownership tests fail
- drop
Non-blocking test gap: the cross-origin path is untested. Dropping Access-Control-Allow-Origin: * passes 52/52. So does disabling img.crossOrigin = "anonymous", which passes 48/48. The browser test page is served over http, so the frame prefix is same-origin and neither header is exercised. Both only matter on the non-http fallback (https://hyperframes-render.invalid). No current capture path appears to use it: frames are injected into fileServer http pages, and setContent appears only in verifyStaticPage. If that fallback should stay supported, add a WebGL texImage2D upload test on an about:blank/setContent page. Without the headers, that upload would fail with a SecurityError. Otherwise, consider dropping the fallback.
— Somu
somanshreddy
left a comment
There was a problem hiding this comment.
I re-checked the delta from 0d786bf5 to e4adc91c. It changes one file, videoFrameTransport.browser.test.ts. Production code is unchanged, and my coverage note is resolved.
The alpha/WebGL case now also runs from a fresh about:blank page. At this head the browser test passes 6/6. Both mutations that survived before are now caught by preserves PNG alpha and GPU uploads from about:blank:
- dropping
Access-Control-Allow-Origin: * - disabling
img.crossOrigin = "anonymous"
That makes 7/7 mutants caught. There are no blockers.
— Somu
Edit accuracy: accurate 2061 (base branch 2061), smooth 1507 of thoseThe gate passes. Quarantined, measured but not gated (0) |
terencechotai2-commits
left a comment
There was a problem hiding this comment.
Reviewed at 3d7cf641, then re-checked both follow-ups up to e4adc91c. 0d786bf5 changes only comments in frameCapture.ts; e4adc91c changes only videoFrameTransport.browser.test.ts. No blockers, approving. All 11 required checks are green at e4adc91c. "Studio: edit accuracy gate" was red only because a shard's Bun download returned 504; it passed on the rerun.
I agree with Somu's review on response lifetime, the buffer cap and the mutation results. I won't repeat them here. What I added:
Edge cases run in real Chrome (headless-shell 152), with a scratch test against this branch's injectVideoFramesBatch. Each case injected a frame, then checked the pixels with getImageData and a WebGL texImage2D upload. All 7 passed:
setContent/ about:blank page: the frame came fromhttps://hyperframes-render.invalid/.... Pixels were right,texImage2Dreturned 0, and the canvas wasn't tainted.file://page: same result through the.invalidprefix.- Opaque-origin page (
Content-Security-Policy: sandbox allow-scripts, sowindow.origin === "null"): pixels were right and WebGL was OK.colorGrading.readablePixelsskips its CORS copy here, becausecrossOriginis now set. - Page with
Content-Security-Policy: img-src 'self' data:: OK. The same-origin prefix satisfies'self'. - Caller page with Puppeteer
setRequestInterception(true)that continues every request: the frame loads, and the caller'srequesthandler never sees the frame URL. A caller's own interception therefore can't stall or abort it. - Navigating to another origin after the first inject: the prefix stays on the first origin, so frames become cross-origin. They still load and upload through ACAO plus
anonymous. - Closing the page mid-inject: the call rejects with
TargetCloseErrorin under 100 ms. It doesn't hang.
Test-gap check. With img.crossOrigin removed, a WebGL upload on the about:blank path throws SecurityError. At 3d7cf641 no PR test caught that. The new about:blank case at e4adc91c fails under the same mutation, so that gap is closed.
Code that reads a render-frame image after its URL is released. The URL 404s once the batch finishes, so I looked for anything that reads or re-fetches these images afterwards:
colorGrading.readablePixelsreturns early becausecrossOriginis set.threeDProjection.rasterizeQuadfetches every non-data:<img>src into the SVG snapshot. It runs once ininitThreeDProjection, at session setup. That is before any frame is injected, and the pre-created siblings fromensureRenderFrameSiblingshave nosrcyet.- I found nothing else in core, engine or producer that clones, serializes or re-fetches these images.
Existing callers.
- Renders without inline frames never create the transport (
withVideoFrameSourcesreturns early). The only change for them is the 32/16 MiB inspection-buffer cap on capture-owned pages. That limits what DevTools keeps forNetwork.getResponseBody, not what the page loads, and nothing in engine or producer reads response bodies. - Local, distributed and Lambda renders share the same
createVideoFrameInjector→injectVideoFramesBatchpath. TherenderOrchestrator.tschange is comment-only.
Tests I ran at e4adc91c:
- The 6 changed engine test files: 70 passed. The real-Chrome browser file ran 6 of 6 cases.
- Full engine suite: 2,068 passed. 19 failed, all in
browserManager,videoFrameExtractor.{concurrency,downloadCache}andffprobetests. They fail the same way at base86521b94, because this sandbox has no ffmpeg and nobunon the spawn path. tsc --noEmitpasses.
Non-blocking note. injectVideoFramesBatch is a public engine export, and the CLI snapshot command calls it on its own page. After this PR, the injected <img> holds a short-lived URL instead of a self-contained data: URI. A caller outside this repo that serializes the DOM after injection would now get a dead link. Nothing in this repo does that. A line in the release notes would cover it.
— tai2
Long renders retain a new multi-megabyte image data URL for every injected video frame in Chrome, independently of the bounded Node LRU. Deliver extracted frame bytes through page-scoped CDP interception with
Cache-Control: no-storeinstead. Only short synthetic URLs reach the DOM; frame responses bypass the HTTP connection pool used by source-video downloads. Release registered response bytes after each batch, including decode failures and hidden-video skips.The existing decoded
<img>remains available to canvas, color grading and GPU texture uploads. Caller-provided frame URLs pass through unchanged. This shared engine path covers local and distributed capture without restoring file-server frame requests.Capture-owned pages also cap Puppeteer's original CDP session's inspection response-body buffer at 32 MiB total / 16 MiB per resource, instead of Chrome's 200 MiB total default. Network events and error diagnostics remain enabled. The runtime getter is guarded because Puppeteer omits it from the public Frame types; if unavailable, capture continues with the no-store transport and the default inspection budget. Caller-owned pages are unaffected by this extra optimization.
Closes #5492.
Validation:
about:blankpage; removing either the CORS response header or anonymous image mode fails this cross-origin case. Unit tests cover overlapping batches and protocol/init failures.Memory probe (Chrome 152 on Linux, two noisy 960×540 frame images):
The no-store probe transferred 1,786 MiB of PNG bytes using 400 deterministic source files repeatedly and a fresh URL for every update. The optimized probe transferred a further 865 MiB of PNG bytes. The first captured PNG is byte-identical across all three paths. The lower inspection budget cuts about 20% from the no-store-only renderer footprint in this probe; throughput comparisons are not controlled because other verification ran concurrently. Chrome still warms its decoded-image cache; this removes cumulative inline-source retention rather than eliminating normal image-cache memory. The reported macOS/Chrome 154 environment has not been run locally.