⚗️ Add Canvas image capture [3/n] - #4980
Conversation
Bundles Sizes Evolution
|
|
BeltranBulbarellaDD
changed the title
BeltranBulbarellaDD
changed the title
BeltranBulbarellaDD
changed the title
144fbb9 to
af5efab
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e88d1dda23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const previousHashes = new WeakMap<HTMLCanvasElement, string>() | ||
| const inFlightCaptures = new WeakSet<HTMLCanvasElement>() |
There was a problem hiding this comment.
issue: When VIEW_CREATED triggers serializeFullSnapshot(), the scope resets all node IDs and rebuilds a fresh replay tree, but these hash and in-flight caches survive. An unchanged canvas is consequently marked clean without emitting an image for the new tree, leaving it blank, while an in-flight callback can emit the previous tree's nodeId; scope this state to the current full-snapshot generation or force a fresh canvas mapping after each reset.
Useful? React with 👍 / 👎.
| if (shouldMaskNode(canvas, nodePrivacyLevel)) { | ||
| canvasManager.markCanvasClean(canvas) | ||
| return // Do not read pixels from masked canvases |
There was a problem hiding this comment.
issue: If a canvas is dirty while explicitly masked, this branch removes its only pending capture. Removing the privacy attribute or changing a masking ancestor does not re-dirty it—serializeMutations() only does that for canvas size attributes—so a now-allowed static canvas remains blank in replay until another drawing operation happens; retain enough state to capture it when its privacy level becomes permissive.
Useful? React with 👍 / 👎.
| context.filter = 'grayscale(1)' | ||
| context.drawImage(canvas, 0, 0, width, height) | ||
|
|
||
| return fnv1aHash(context.getImageData(0, 0, width, height).data) |
There was a problem hiding this comment.
issue: The hash covers only the downscaled pixels, not the canvas dimensions. Responsive code can resize a canvas proportionally and redraw the same scene—for example, a solid 1000×1000 canvas becoming 2000×2000—producing the same 100×100 hash; the replay applies the width/height mutation, which clears its bitmap, but the hash comparison suppresses the callback that would redraw it. Include intrinsic dimensions in the change key or invalidate the cached hash on size changes.
Useful? React with 👍 / 👎.
| try { | ||
| hash = computeImageHash(canvas, configuration?.hashingMaxDimension ?? 100) | ||
| } catch { | ||
| return // capture failed; leave it dirty |
There was a problem hiding this comment.
issue: When a canvas contains cross-origin pixels without CORS, getImageData() throws a SecurityError on every attempt until the bitmap is reset. This catch deliberately leaves the canvas dirty, so every interval creates a thumbnail, draws it, and throws again—up to five times per second per canvas—even though capture cannot succeed; handle origin-security failures separately and defer retries until a later draw or size reset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04324e8b22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return undefined | ||
| } | ||
| context.imageSmoothingQuality = 'low' | ||
| context.filter = 'grayscale(1)' |
There was a problem hiding this comment.
issue: When a canvas changes between distinct colors that produce the same grayscale luminance—such as a hue animation with constant luminance—this filter maps both frames to identical thumbnail pixels. previousHashes then treats the second frame as unchanged and suppresses its image, potentially freezing or skipping visually significant color changes in the replay; hash the color channels rather than discarding chroma.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I believe more of an edge case? Mobile does not address this case.
| trackVisualViewportResize(processRecord), | ||
| trackViewEnd(lifeCycle, processRecord, flushMutations), | ||
| trackCanvasContent(scope), | ||
| trackCanvasCapture(scope), |
There was a problem hiding this comment.
issue: When the experimental canvas option is enabled, this production call omits onCanvasCapture, so the tracker uses its noop default; a repo-wide search at this commit finds no other production consumer of CanvasCapture. Every dirty canvas therefore still performs thumbnail reads and potentially PNG encoding up to maxFramesPerSecond, but every resulting blob is discarded, adding substantial main-thread and memory work without producing replay data; avoid starting this tracker until the delivery callback is wired.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not an issue.
| return undefined | ||
| } | ||
| context.imageSmoothingQuality = 'low' | ||
| context.filter = 'grayscale(1)' |
There was a problem hiding this comment.
Is the reasoning behind the grayscale filter that it'll produce a smaller image to hash? I don't think that actually works, because getImageData() is still giving you the image in RGBA format. The grayscale filter also has a cost. It's possible I'm missing something, but based on my current understanding, I think we'd likely be better off without it.
In general we may be able to get better performance using other APIs that are more directly tailored to our needs here; take a look at ImageBitmap, and in particular the resizeWidth, resizeHeight, and resizeQuality options of createImageBitmap, for an alternative approach.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc5306758c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (hash === canvasManager.getPreviousHash(canvas)) { | ||
| canvasManager.markCanvasCleanIfUnchanged(canvas, captureId) | ||
| return // unchanged: no capture/output; thumbnail is discarded naturally |
There was a problem hiding this comment.
issue: When an already-captured canvas is moved or removed and reinserted, mutation serialization deletes its old node ID and assigns a new one, but the hash remains keyed to the same DOM element. This comparison then suppresses the unchanged pixels, so no image is emitted for the new replay node and the canvas remains blank. Fresh evidence beyond the full-snapshot case is that the new reset() runs only from resetIds(), while ordinary removal calls only markCanvasClean; tie the cached hash to the node ID or invalidate it when the node identity is removed.
Useful? React with 👍 / 👎.
| markCanvasDirty: (canvas) => { | ||
| if (canvas.isConnected) { | ||
| if (canvas.isConnected && !taintedCanvases.has(canvas)) { | ||
| dirtyCanvases.add(canvas) | ||
| dirtyVersions.set(canvas, getDirtyVersion(canvas) + 1) |
There was a problem hiding this comment.
issue: When an origin-unclean canvas is restored by assigning width or height and then redrawn with capturable content, this guard ignores both the size mutation and every later 2D draw, even though resetting the bitmap restores its origin-clean state. Fresh evidence is the newly added taintedCanvases weak set, which is never cleared—even by reset()—so the attempted retry fix now leaves that canvas uncapturable for the rest of the recording; clear or retry the taint state after a bitmap reset or subsequent drawing operation.
Useful? React with 👍 / 👎.
| const subtleCrypto = globalObject.crypto?.subtle | ||
| if (!subtleCrypto) { | ||
| return Promise.reject(new Error('SubtleCrypto is not supported')) |
There was a problem hiding this comment.
issue: When SubtleCrypto is unavailable, such as on a non-secure origin, this permanently rejected promise reaches the catch handler that marks the canvas dirty, causing every interval to redraw and read the thumbnail only to reject again. The missing-createImageBitmap path similarly returns no image and re-dirties the canvas, so unsupported environments perform continuous work without ever producing a capture; disable the tracker or mark the failure non-retryable when these global capabilities are absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eed2ca243
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| forgetCanvasNode: (canvas) => { | ||
| dirtyCanvases.delete(canvas) | ||
| dirtyVersions.delete(canvas) | ||
| previousHashes.delete(canvas) | ||
| inFlightCaptures.delete(canvas) | ||
| }, |
There was a problem hiding this comment.
issue: When a tainted canvas is removed, reset while detached (for example by assigning canvas.width), and later reinserted, the detached reset is not observed by the recording MutationObserver, while this method deliberately retains the canvas in taintedCanvases. Its new serialization then calls markCanvasDirty(), which is suppressed by that stale taint entry, so the now-origin-clean canvas remains blank for the rest of the recording. Fresh evidence beyond the previously handled connected size mutation is this remove/reset/reinsert path, which never reaches markCanvasBitmapReset(); forgetting taint here would safely allow capture to be retried.
Useful? React with 👍 / 👎.
| hashingMaxDimension: { type: 'number', min: 1, max: 100, default: 100 }, | ||
| maxImageDimension: { type: 'number', min: 1, max: 1000, default: 1000 }, |
There was a problem hiding this comment.
issue: These schema entries reject hashingMaxDimension > 100 and maxImageDimension > 1000, but the newly exposed configuration fields only describe them as maximum dimensions and do not state either permitted range. A customer can therefore reasonably configure values such as maxImageDimension: 1200 and cause validateAndBuildRumConfiguration() to reject the entire RUM initialization rather than merely getting a larger replay image; document the 1–100 and 1–1000 ranges alongside the options, as is already done for maxFramesPerSecond.
Useful? React with 👍 / 👎.
| return captureCanvasImage( | ||
| canvas, | ||
| configuration?.maxImageDimension ?? 1000, |
There was a problem hiding this comment.
issue: When the canvas is redrawn while the asynchronous SubtleCrypto.digest() is pending, hash describes the earlier thumbnail but this call reads the live canvas again and encodes the newer pixels. The resulting CanvasCapture therefore pairs image B with the hash of image A; if B then remains stable, the still-dirty canvas is emitted again on the next tick under B's actual hash, and consumers cannot safely use changeHash to identify or deduplicate the blob. Capture one immutable snapshot and derive both the hash and output image from it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d830fc9856
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Clear the dirty state before the asynchronous capture. A draw occurring while the | ||
| // capture is in flight will mark the canvas dirty again and will be handled by the next tick. | ||
| canvasManager.markCanvasCleanIfUnchanged(canvas, captureId) |
There was a problem hiding this comment.
issue: For canvases rendered through WebGL or an ImageBitmapRenderingContext, this clears the dirty flag after the initial capture, but trackCanvasContent.ts only instruments CanvasRenderingContext2D methods. Subsequent rendered frames therefore never make the canvas capturable again, leaving the replay frozen on its first frame; either track these rendering contexts or keep unsupported-context canvases scheduled for change detection.
Useful? React with 👍 / 👎.
3 participants
Motivation
Canvas capture can be expensive. Once a canvas has been marked dirty, we need to periodically inspect its bitmap, avoid recapturing unchanged content, and downscale captured images before they are sent with Session Replay data.
This PR builds on #4949 and #4947, which add canvas dirty-state tracking and the experimental canvas-recording configuration.
Design diagram: View the Mermaid diagram
Changes
hashingMaxDimensionandmaxImageDimensionto the experimental canvas-recording configuration, with defaults of100and1000pixels.maxFramesPerSecond.Scope
This PR adds the capture primitive and callback interface. Wiring captured blobs into Session Replay event serialization and intake delivery is a follow-up step.
Test instructions
Checklist