fix(chromium): make video recording duration close to real duration of the test run

[gpiszczek]

Videos recorded with playwright using chromium engine are shorter or longer than what could be expected by measuring execution time. Causes:

• with fast updating web pages, chromium can generate frames faster than playwright expects; all sent frames are pushed to ffmpeg, which sometimes exceeds the hardcoded frame rate (25 fps); in the effect video get too long; frame throttler does not help much with it because it was designed for different purpouse,
• timestamp of last added frame is taken from timestamp reported by browser which is not in 1/fps intervals; this leads to rounding errors and video files shorter than real execution time,
• the first screencast frame is send by browser with slight delay from the start of screencast; this causes drift between audio and video recording start times rendering them out of sync.

Fixes #9611 #35776 #38219

[Skn0tt]

This looks very similar to the 3-year-old #10145, which got closed by the triager. @yury-s could you take another look?

[gpiszczek]

@Skn0tt @yury-s these are actually the same changes. I am trying my luck resubmitting them again but now with easy to reproduce test case (in #38219). I will glady respond to comments.

To address previous concers:

#10145 (comment)

With the new backpressure mechanism added in #9893 this should not be necessary. If the throttler doesn't give desired effect we should adjust its parameters instead. The difference with this approach is that Chromium won't send new screencast frames until it gets acks for already sent once whereas with the approach in this PR the frames are still generated just to be dropped on the floor on the playwright end.

Can you check if the problem is still reproducible on @next version (which includes the fix mentioned above)?

Yes, the problem still presists. I do not think that tweaking framethrotter is a way to address this, because the purpouse of it is different. As I understandard it's main role is to not generate too much unncecessary data (for tracing) but allowing for bursts around actions. As per comment:

  // Aiming at 25 fps by default - each frame is 40ms, but we give some slack with 35ms.
  // When throttling for tracing, 200ms between frames, except for 10 frames around the action.

The purpose of this change is to keep the frame rate stable in line with the declared fps so "giving some slack" is not an option :).

#10145 (comment)

This shouldn't make any difference as the number of frames we feed into ffmpeg stays the same or am I missing something?

Yes, this changes the number of frames. Suppose that browser generates frame every 58 milliseconds. For every frame we receive we will end up pushing 1 frame to ffmpeg, loosing 18 ms of video time (with 25 fps we should have frame every 40 ms, 58 / 40 = 1.45 which is less than 1.5 so it is rounded to 1. With proposed change we will keep stable frame rate by adding additional frames every nth frame received. It is important to apply first change (dropping excessive frames), as this change alone could cause the frame rate to exceed the fps limit.

#10145 (comment)

This will only increase the duration of the resulting video by the time it took to launch screencast, why do we want this?

Yes but it is important in some use cases and does not hurt others. In our use case we measure time of execution of test scenarios steps/actions and present it visually to the user. Without this change we get about 150-250ms shorter video than expected. I thought it will be also valuable for people trying to sync with audio recording, but I discovered (#4870) that you are not keen on adding audio support at all.

[pavelfeldman]

Here is my take: https://github.com/microsoft/playwright/pull/38228/files. I hope it reads better.

[pavelfeldman]

Landed the alternative. Thanks for reminding and coming up with the fix though!

[gpiszczek]

@pavelfeldman about the change:

    // 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);

Unfortunatelly this change ruins synchronization of video vs. test run's real time.

I was trying to push change to add delay at the beginning of recording (calculated as difference between browsercontext creation time and timestamp of first frame received from browser), which would result in better synchronization (62bae71). In first try @yury-s (#10145 (comment)) argued against it as making video longer (about hunderds of millis at most). In the second try it was silently ignored.

My aim was to make sync of video vs. test run better and finally to get rid of local patches. With this change it's a failure for me 😞 .

Can you rethink this change, please?

PS Sorry for adding comments in many places but i don't know which will get to you.