feat(tracing): clip canvas contents from screenshots
#33119
Conversation
tests/library/trace-viewer.spec.ts
Outdated
| await page.waitForTimeout(1000); // ensure we could take a screenshot | ||
| }); | ||
|
|
||
| await traceViewer.page.pause(); |
There was a problem hiding this comment.
this test isn't gonna stay, obviously. but you can use it to play around with the feature: npm run ctest -- --grep 'clipping' --headed
@dgozman what's the best way of testing this - should we trace a page with a canvas and then take a screenshot of the trace, to ensure the screenshot isn't blank? Or somehow compute if it's blank without storing a golden image?
There was a problem hiding this comment.
I think that would be a flaky test, unfortunately. Perhaps just check some test-specific logging, like we do in recorder tests:
if (isUnderTest) console.error(`drawn 15;23-184;566 screenshot rect on the canvas`);There was a problem hiding this comment.
thanks, good idea! implemented
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| const { wallTime, timestamp, pageId } = snapshot.snapshot(); | ||
| const page = this._pages.get(pageId); | ||
| if (page) { | ||
| const closestFrame = (wallTime && page.screencastFrames[0]?.frameSwapWallTime) ? findClosest(page.screencastFrames, frame => frame.frameSwapWallTime!, wallTime) : findClosest(page.screencastFrames, frame => frame.timestamp, timestamp); |
There was a problem hiding this comment.
I'd prefer this code to live in SnapshotRenderer instead.
There was a problem hiding this comment.
How would SnapshotRenderer get access to screencastFrames? Would we pass them into .render(), or do we pass them into the constructor?
There was a problem hiding this comment.
I've since moved the code a little, but it's still inside the server. Let me know how to proceed.
There was a problem hiding this comment.
we discussed this offline and ended up with 45d1b54.
| if (page) { | ||
| const closestFrame = (wallTime && page.screencastFrames[0]?.frameSwapWallTime) ? findClosest(page.screencastFrames, frame => frame.frameSwapWallTime!, wallTime) : findClosest(page.screencastFrames, frame => frame.timestamp, timestamp); | ||
| if (closestFrame) | ||
| screenshotUrl = new URL(`./sha1/${closestFrame.sha1}`, swScope); |
There was a problem hiding this comment.
Injecting swScope here is unfortunate. Perhaps we should have a relative url for the screenshot, e.g. /screenshot/<pageOrFrameId>, and route it through sw main?
There was a problem hiding this comment.
I had that in a first iteration, but that requires making sw main listen to not just swScope, but also to requests from the iframe origin. Would that be fine? Alternatively, we could also use a fake origin like playwright.sw.
There was a problem hiding this comment.
made it work with a new route in the service worker 👍
| const xEnd = boundingRect.right / window.innerWidth; | ||
| const yEnd = boundingRect.bottom / window.innerHeight; | ||
|
|
||
| context.drawImage(img, xStart * img.width, yStart * img.height, (xEnd - xStart) * img.width, (yEnd - yStart) * img.height, 0, 0, canvas.width, canvas.height); |
There was a problem hiding this comment.
- Is this going to work nicely for the out-of-viewport canvas? Perhaps we should say "not captured" there instead?
- This code must run after
scrollLeft/scrollTopare restored inonLoad(). - We should still have a warning somewhere that would explain that image might be wrong.
tests/library/trace-viewer.spec.ts
Outdated
| await page.waitForTimeout(1000); // ensure we could take a screenshot | ||
| }); | ||
|
|
||
| await traceViewer.page.pause(); |
There was a problem hiding this comment.
I think that would be a flaky test, unfortunately. Perhaps just check some test-specific logging, like we do in recorder tests:
if (isUnderTest) console.error(`drawn 15;23-184;566 screenshot rect on the canvas`);| const canvases = root.querySelectorAll('canvas'); | ||
| if (canvases.length > 0 && screenshotURL) { | ||
| const img = new Image(); | ||
| img.onload = () => { |
There was a problem hiding this comment.
Please handle img.onerror as well.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
feedback from today:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| const canvases = root.querySelectorAll('canvas'); | ||
| if (canvases.length > 0 && screenshotURL) { | ||
| const img = new Image(); | ||
| img.onload = () => { |
| const xEnd = boundingRect.right / window.innerWidth; | ||
| const yEnd = boundingRect.bottom / window.innerHeight; | ||
|
|
||
| context.drawImage(img, xStart * img.width, yStart * img.height, (xEnd - xStart) * img.width, (yEnd - yStart) * img.height, 0, 0, canvas.width, canvas.height); |
| const { wallTime, timestamp, pageId } = snapshot.snapshot(); | ||
| const page = this._pages.get(pageId); | ||
| if (page) { | ||
| const closestFrame = (wallTime && page.screencastFrames[0]?.frameSwapWallTime) ? findClosest(page.screencastFrames, frame => frame.frameSwapWallTime!, wallTime) : findClosest(page.screencastFrames, frame => frame.timestamp, timestamp); |
There was a problem hiding this comment.
I've since moved the code a little, but it's still inside the server. Let me know how to proceed.
| if (page) { | ||
| const closestFrame = (wallTime && page.screencastFrames[0]?.frameSwapWallTime) ? findClosest(page.screencastFrames, frame => frame.frameSwapWallTime!, wallTime) : findClosest(page.screencastFrames, frame => frame.timestamp, timestamp); | ||
| if (closestFrame) | ||
| screenshotUrl = new URL(`./sha1/${closestFrame.sha1}`, swScope); |
There was a problem hiding this comment.
made it work with a new route in the service worker 👍
tests/library/trace-viewer.spec.ts
Outdated
| await page.waitForTimeout(1000); // ensure we could take a screenshot | ||
| }); | ||
|
|
||
| await traceViewer.page.pause(); |
There was a problem hiding this comment.
thanks, good idea! implemented
|
I've seen that this doesn't work perfectly on Firefox - gonna investigate more. |
Seems to be a known issue with screencast frames in headed Firefox. |
This comment has been minimized.
This comment has been minimized.
|
Alright, this is ready for one last review! |
This comment has been minimized.
This comment has been minimized.
tests/library/trace-viewer.spec.ts
Outdated
| test('canvas clipping', async ({ runAndTrace, page, server }) => { | ||
| const traceViewer = await runAndTrace(async () => { | ||
| await page.goto(server.PREFIX + '/screenshots/canvas.html#canvas-on-edge'); | ||
| await page.waitForTimeout(1000); // ensure we could take a screenshot |
There was a problem hiding this comment.
I predict this will be flaky 😄 We usually do a few rafs in the hopes it will redraw, but even that does not always help.
There was a problem hiding this comment.
|
|
||
| if (xEnd > 1 || yEnd > 1) { | ||
| if (xStart > 1 || yStart > 1) | ||
| canvas.title = `Playwright couldn't capture canvas contents because it's located outside the viewport.`; |
There was a problem hiding this comment.
Let's skip both drawWarningBackground and drawImage calls in this case.
There was a problem hiding this comment.
I've made it skip drawImage, but I don't think we should skip drawCheckerboard - having that around is still valuable to show that there's something missing.
There was a problem hiding this comment.
I've also made it skip drawCheckerboard to align with what we discussed in the meeting. Gonna potentially change that in a follow-up. ea4116e
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"12 failed 4 flaky36395 passed, 639 skipped Merge workflow run. |
Closes #33115.
If we're happy with it, this can supercedes the "show screenshot instead of snapshot" implementation. Will remove that in a separate PR.
This PR adds logic to snapshot rendering that fetches the closest screenshot for the rendered snapshot to clip the canvas contents from it. This is best-effort, and the time alignment won't always be perfect. This is fine if there's no scrolling and the canvas has static contents. We show a warning icon to let the user know about that. Because it's based on viewport screenshots, we don't have data outside of it. Parts of the canvas that aren't available will be shown with a checkered background. Here's a demo:
Screen.Recording.2024-10-17.at.12.44.42.mov