Skip to content

fix(video): map video time to frames to reduce skew #38228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 17, 2025
+18 −12
Merged

fix(video): map video time to frames to reduce skew #38228

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions packages/playwright-core/src/server/chromium/videoRecorder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ export class VideoRecorder {
private _process: ChildProcess | null = null;
private _gracefullyClose: (() => Promise<void>) | null = null;
private _lastWritePromise: Promise<void> = Promise.resolve();
private _lastFrameTimestamp: number = 0;
private _lastFrameBuffer: Buffer | null = null;
private _lastWriteTimestamp: number = 0;
private _firstFrameTimestamp: number = 0;
private _lastFrame: { timestamp: number, frameNumber: number, buffer: Buffer } | null = null;
private _lastWriteNodeTime: number = 0;
private _frameQueue: Buffer[] = [];
private _isStopped = false;
private _ffmpegPath: string;
Expand Down Expand Up @@ -122,17 +122,20 @@ export class VideoRecorder {
if (this._isStopped)
return;

if (this._lastFrameBuffer) {
const durationSec = timestamp - this._lastFrameTimestamp;
const repeatCount = Math.max(1, Math.round(fps * durationSec));
if (!this._firstFrameTimestamp)
this._firstFrameTimestamp = timestamp;

const frameNumber = Math.floor((timestamp - this._firstFrameTimestamp) * fps);
Copy link
Member

@yury-s yury-s Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may end up losing frames if you always round down.


if (this._lastFrame) {
const repeatCount = frameNumber - this._lastFrame.frameNumber;
for (let i = 0; i < repeatCount; ++i)
this._frameQueue.push(this._lastFrameBuffer);
this._frameQueue.push(this._lastFrame.buffer);
this._lastWritePromise = this._lastWritePromise.then(() => this._sendFrames());
}

this._lastFrameBuffer = frame;
this._lastFrameTimestamp = timestamp;
this._lastWriteTimestamp = monotonicTime();
this._lastFrame = { buffer: frame, timestamp, frameNumber };
this._lastWriteNodeTime = monotonicTime();
}

private async _sendFrames() {
Expand All @@ -148,9 +151,12 @@ export class VideoRecorder {
}

async stop() {
if (this._isStopped)
if (this._isStopped || !this._lastFrame)
return;
this.writeFrame(Buffer.from([]), this._lastFrameTimestamp + (monotonicTime() - this._lastWriteTimestamp) / 1000);
// Pad with at least 1s of the last frame in the end for convenience.
// This also ensures non-empty videos with 1 frame.
const addTime = Math.max((monotonicTime() - this._lastWriteNodeTime) / 1000, 1);
this.writeFrame(Buffer.from([]), this._lastFrame.timestamp + addTime);
Copy link

@gpiszczek gpiszczek Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a reasoning behind this, please? I do not see benefits but on the other hand it breaks synchronization of video with test run.

I am a little bit surprised by this change, I was trying to push change to add delay at the beginning of recording, which would result in better synchronization (62bae71). In first try @yury-s (#10145 (comment)) argued against it as making video longer (we are talking about hunderds of millis at most). In the second try it was silently ignored.

Now you added 1s of padding at the end of EVERY recording just like that.

Can you rethink this change, please?

this._isStopped = true;
await this._lastWritePromise;
await this._gracefullyClose!();
Expand Down
Loading