chore(video): emit frames with real timestamps, drop frame number quantization - #42480
chore(video): emit frames with real timestamps, drop frame number quantization#42480Pavel Feldman (pavelfeldman) wants to merge 1 commit into
Conversation
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.
…ntization Cluster timestamps are computed relative to the recorder creation time, which is also written into the webm creation_time metadata. Firefox and WebKit screencast timestamps are monotonic, so they are anchored to the wall clock at the first frame.
aaa09b1 to
85a23ee
Compare
| const h = this._size.height; | ||
| const videoFilterArgs = page.getFFmpegVideoFilterArgs?.({ width: w, height: h }) ?? `pad=${w}:${h}:0:0:gray,crop=${w}:${h}:0:0`; | ||
| 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 ${videoFilterArgs}`.split(' '); | ||
| args.push('-metadata', `creation_time=${new Date(this._creationTimeMs).toISOString()}`); |
There was a problem hiding this comment.
creation_time is captured before the first frame, and ffmpeg rebases the first input timestamp to zero, so if this is meant to supplant #42478 then this would be earlier than desired
| const timestampMs = Math.max(0, Math.round(frameNumber * 1000 / fps)); | ||
| this._process!.stdin!.write(writeClusterHeader(timestampMs, frame.length)); | ||
| private _emitFrame(frame: Buffer, timestampMs: number) { | ||
| this._process!.stdin!.write(writeClusterHeader(Math.max(0, Math.round(timestampMs)), frame.length)); |
There was a problem hiding this comment.
this removes the explicit video frame slot selection added in 7070565 and rounds timestamps before ffmpeg resamples to 25fps, so frames near a 40ms boundary may land in a different output video frame slot
everything may end up working just fine, but id suggest adding a test to make sure
There was a problem hiding this comment.
Could you help me understand the problem? Do we not trust ffmpeg to resample?
There was a problem hiding this comment.
i dont really know about ffmpeg enough to know whether it'll do the right thing or not
i was just trying to understand why this explicit slot mapping existed and whether these changes would cause a regression
Test results for "tests 1"4 flaky51278 passed, 1241 skipped Merge workflow run. |
Test results for "MCP"2 failed 8303 passed, 1367 skipped Merge workflow run. |
🟢 CI is clear — no failures caused by this PRHi, I'm the Playwright bot and I took a look at the failing CI here. This PR only reworks video frame timestamps ( DetailsOverall: nothing in the failing set exercises the changed code. The diff is confined to the Pre-existing flake / infra
Flaky only — nothing to triage (but worth a note, since it's the PR's area)
Triaged by the Playwright bot - agent run |
3 participants
Summary
-r 25output resampling handles frame timing. Drops the frame number quantization, same-slot coalescing and the one-frame emission delay.creation_timemetadata.frameSwapWallTimeis now anchored to the wall clock at the first frame.Fixes: #42361