⚗️ 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.
| try { | ||
| hash = computeImageHash(canvas, configuration?.hashingMaxDimension ?? 100) | ||
| } catch { | ||
| return // capture failed; leave it dirty |
There was a problem hiding this comment.
+1 to Codex's comment here; if computeImageHash() throws, it'd be best to note that the canvas is tainted and stop trying to snapshot its contents. I'd go further than Codex recommended ; there's no need to try again even on draw or resize. Once a canvas becomes tainted, it stays that way.
| const nodeId = scope.nodeIds.get(canvas) | ||
| if (nodeId === undefined) { | ||
| canvasManager.markCanvasClean(canvas) | ||
| return | ||
| } |
There was a problem hiding this comment.
I'd make this check earlier.
| } | ||
|
|
||
| /* eslint-disable no-bitwise */ | ||
| function fnv1aHash(data: ArrayLike<number>): string { |
There was a problem hiding this comment.
SubtleCrypto.digest() is preferred if our browser support matrix lets us use it. It will almost certainly be significantly faster. (Note that it's not necessary to use the same hash function mobile uses.)
| 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.
| const currentNodePrivacyLevel = getNodePrivacyLevel(canvas, scope.configuration.defaultPrivacyLevel) | ||
| if (currentNodePrivacyLevel !== NodePrivacyLevel.ALLOW) { | ||
| canvasManager.markCanvasClean(canvas) | ||
| return // Do not emit pixels if the canvas became privacy level other than allow during capture | ||
| } |
There was a problem hiding this comment.
I think it'd be OK to remove this. The async part of toBlob() is the image encoding part, but the image data you're encoding is a snapshot of the canvas as it existed while it was still NodePrivacyLevel.ALLOW.
| const previousHashes = new WeakMap<HTMLCanvasElement, string>() | ||
| const inFlightCaptures = new WeakSet<HTMLCanvasElement>() |
There was a problem hiding this comment.
I'd store this kind of state on the CanvasManager, probably. That way RecordingScope#reset() will reset it naturally when a new full snapshot is taken. (We probably don't want to reset our knowledge about which hashes have been uploaded to the server, but that's a different thing than previousHashes, as I understand it.)
I think CanvasManager knowing more about canvases than just whether they were dirty might let you simplify some of the logic here. If it also knew whether canvas were tainted (so they should never be captured, because we can't see their contents) and whether a capture for them was in flight, then you could handle much of the logic for deciding which canvases are ready for capture totally within CanvasManager. You might have a getCapturableCanvases() getter that does some of the filtering you're doing here up front. Not saying other approaches can't work, but that kind of thing might feel nicer as you start to move more state onto CanvasManager.
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