Skip to content
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.

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

options: change video-sync-max-factor to 1 #15326

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kasper93
Copy link
Contributor

Allow changing the video speed only if it results in an integer multiple of the display FPS by default. It makes little sense to adjust the video speed if it does not result in the target multiple.

For example, a 25 fps video on a 144 Hz display would be sped up by 0.17% before this change, resulting in a ratio of approximately 5.75 instead of 5.76 without the speed change...

Allow changing the video speed only if it results in an integer multiple
of the display FPS by default. It makes little sense to adjust the video
speed if it does not result in the target multiple.

For example, a 25 fps video on a 144 Hz display would be sped up by
0.17% before this change, resulting in a ratio of approximately 5.75
instead of 5.76 without the speed change...
Copy link

Download the artifacts for this pull request:

Windows
macOS

@kasper93 kasper93 marked this pull request as draft November 17, 2024 09:26
@Dudemanguy
Copy link
Member

Didn't check the math but wouldn't 1 only allow exact integers? e.g. 23.976 fps against a 72.00 HZ is surely desirable so you'd still want that to work.

@kasper93
Copy link
Contributor Author

Didn't check the math but wouldn't 1 only allow exact integers? e.g. 23.976 fps against a 72.00 HZ is surely desirable so you'd still want that to work.

I'm not sure what is 72 Hz in your example. But let me explain what is going on in this code.

It tries find speed adjustment for video to match multiple of display fps. So for example for 144 Hz:

factor 1 tries to match 144 Hz
factor 2 tries to match 288 Hz
factor 3 tries to match 432 Hz
factor 4 tries to match 576 Hz
factor 5 tries to match 720 Hz
and so on...

Assuming your example was about 72 Hz display. It would match at factor 1 because speed adjustment is below threshold. So it would get adjusted to 24 fps @ 72 Hz. Also this is perfect case, because after small adjustment we have multiple of display Hz, so factor doesn't even matter.

Now more interesting case is when we cannot adjust speed enough (due to limit) and start going into multiples of display Hz. For example. 25 fps @ 144 Hz would try to:

factor speed change % video fps after adjust vsync ratio
no adjustment 0.0000 25.0000 5.7600
1 -4.0000 24.0000 6.0000
2 -4.0000 24.0000 6.0000
3 1.6471 25.4118 5.6667
4 0.1739 25.0435 5.7500
5 -0.6897 24.8276 5.8000

1-3 is rejected, because it is too big speed change and we end up using factor 4. Which is targeting video fps to be multiple of 576 Hz. 576 / 25.0435 = 23.

Sync code will fit number of reapeats for current vsync duration. So if we cannot target multiple of this duration we will accumulate error either way, and have at some point 5 instead of 6 repeats. note that in all those cases it would do the same, but if we adjust the speed to factor 4, we in theory after repeating frames 23 times (6+6+6+5), cancel out error and align on vsync again. We also have drift and another sync innacuracies, so it won't be perfect anyway. I made this draft, because after doing 2nd take on this, it might actually be better this way, I'm not sure though if 5 is not too big number, but theoretically it makes sense actually.

Another example would be 23.976 fps on 60 Hz. With factor 2 it would adjust it to 24 fps, which makes it clean 3:2 rate. Else we would accumulate error and after a while (every 17 seconds) we would need to do drop one repeat.

Either way, I think we can skip this patch probably, I still not sure if this is the best way to adjust speed, but it is what we have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants