chore(bidi): implement screencasting using browsingContext.captureScreenshot#41569
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| const w = this._size.width; | ||
| const h = this._size.height; | ||
| const args = `-loglevel error -f matroska -fpsprobesize 0 -probesize 32 -analyzeduration 0 -i pipe:0 -y -an -r ${fps} -c:v vp8 -qmin 0 -qmax 50 -crf 8 -deadline realtime -speed 8 -b:v 1M -threads 1 -vf pad=${w}:${h}:0:0:gray,crop=${w}:${h}:0:0`.split(' '); | ||
| const args = `-loglevel error -f matroska -fpsprobesize 0 -probesize 32 -analyzeduration 0 -i pipe:0 -y -an -r ${fps} -c:v vp8 -qmin 0 -qmax 50 -crf 8 -deadline realtime -speed 8 -b:v 1M -threads 1 -vf scale=w='min(iw,${w})':h='min(ih,${h})':force_original_aspect_ratio=decrease:eval=frame,pad=${w}:${h}:0:0:gray`.split(' '); |
There was a problem hiding this comment.
This file and especially these args are scary to touch, so we'll want to do some careful QA and eyeballing on the video results before merging. We already touched them once in this release, i'm unsure if that's good or bad.
There was a problem hiding this comment.
Did a QA pass. For even, in-bounds frames the new no-op scale is a bit-exact passthrough, zero pixels changed (800×600, 640×480, 800×400, 400×600 all identical).
On odd-dimensioned frames there's some difference because now libswscale engages, but all our existing producers will give even-dimensioned frames. (the comment on that is contradicting the impl)
There was a problem hiding this comment.
On odd-dimensioned frames there's some difference
Does this block this PR? Or is there anything else you want me to fix?
| } | ||
|
|
||
| this._waitingForScreenshot = true; | ||
| const payload = await this._session.sendMayFail('browsingContext.captureScreenshot', { |
There was a problem hiding this comment.
This seems very expensive, especially on typical QA pages where paints are <25fps. Have you checked performance? I wonder how this compares to non-bidi Firefox, for example on todomvc, and on media and animation heavy pages like microsoft.com or github.com.
There was a problem hiding this comment.
Yes, I've checked and didn't see any issues even at higher resolutions and frame rates. At some point it'll start "dropping frames" (i.e. skipping captureScreenshot calls if the previous one hasn't finished yet) but Playwright's screencast pipeline is designed to handle these "gaps" and the worst thing I saw happening was a slight stutter in animations in the video.
In my experiments, Firefox with Juggler always sends frames at 25fps, even on static pages. Compared to that, the only downsides with this implementation are the lack of browser-side scaling and the overhead of having to send a command for every screenshot (instead of receiving them as events).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Skn0tt
left a comment
Skn0tt
left a comment
There was a problem hiding this comment.
We discussed this on thursday with the team. Please inject the custom ffmpeg args from bidiPage.ts, so we don't need to worry about non-bidi video for this.
In general, it seems this PR is working around bidi protocol limitations, specifically missing dimensions on browsingContext.captureScreenshot, and a missing event-style delivery for frames. We think this area is where the bidirectionality of bidi can shine, for example by letting the producer decide the number of frames.
Let's land this in the spirit of progress, but please consider improving the protocol in this area. Video is only becoming more important with agents.
Test results for "MCP"2 failed 7089 passed, 1009 skipped Merge workflow run. |
Test results for "tests 1"4 flaky49494 passed, 1164 skipped Merge workflow run. |
2 participants
This PR implements screencasting by requesting
browsingContext.captureScreenshotevery 40 milliseconds.Unlike the implementations for other protocols, the screenshots are not scaled to the requested video size by the browser because
browsingContext.captureScreenshotdoesn't have a scaling option yet, so the trace viewer and theonFrame()callback of the Screencast API receive unscaled screenshots but theVideoRecorderscales them using ffmpeg.I've changed the ffmpeg filter to use
scaleinstead ofcropto deal with screenshots that are larger than the requested video size. This should not make any difference with the other protocols: the screenshots from those protocols are never larger than the requested size and theVideoRecorderwould have failed with larger screenshots anyway because the current filter expression uses thepadfilter first which fails for frames that are larger than the requested size.Fixes the following tests:
library/browsertype-connect.spec.ts:library/multiclient.spec.ts:library/trace-viewer.spec.ts:library/tracing.spec.ts:library/video.spec.ts:library/screencast.spec.ts: