vo: change vsync base to nanoseconds

[kasper93]

No description provided.

[Dudemanguy]

UST is actually "Unadjusted System Time". It has nothing to do with microseconds (i.e. please don't change that).

[kasper93]

UST is actually "Unadjusted System Time". It has nothing to do with microseconds (i.e. please don't change that).

Ok, fixed. My bad about the name change. For drm I reverted all changes, because apparently it works with microsecond timestamps. wayland and x11 goes through mpv's present_sync so it is fine to not truncate the value in case of wayland. I don't know what resolution is in XPresentCompleteNotifyEvent, but it was working with microseconds before so I assume that.

EDIT:

Changed XPresentCompleteNotifyEvent to nanoseconds too. This needs testing.

[sfan5]

What issue is that actually meant to solve? I don't see anything concrete mentioned anywhere.

[kasper93]

What issue is that actually meant to solve? I don't see anything concrete mentioned anywhere.

Same as bda8439 fixed truncation of nominal_vsync_interval by changing it to double, this commit fixes the same truncation when calculating estimated_vsync_interval. But it is more principal way. We get at least ns resolution from Wayland/X11/D3D11 feedback, there is no need to truncate this. Which is historical baggage, because our timers were maxed at microseconds. While main platforms mpv targets provide higher resolution timers, there is literally no need to truncate this.

Also this is a base for further improving sync code, but first I want to have at least native timer resolution and not dealing with truncated values. Again same fix as bda8439 but with broader scope.

I'm traveling this week, so will not have much time to improve/test it. But if someone wants to try feel free.

I'm mostly thinking that we should have timer sync logic, to infer clock base of UST instead of assuming it's base, because it doesn't seem to be in fact defined. But baby steps, I want to incrementally work on this.

[Dudemanguy]

The presentation stuff does seem a bit messed up (wayland and x11). Honestly just looking at it I'm not sure exactly why. I guess maybe a missed conversion somewhere.

[sfan5]

Same as bda8439 fixed truncation of nominal_vsync_interval by changing it to double, this commit fixes the same truncation when calculating estimated_vsync_interval.

Alright, for some reason I thought that was different. Indeed the same 16666(.66666) truncation applies when you use microseconds over a smaller unit or floating-point.

[kasper93]

@Dudemanguy: So the main issue, was the assumption, same as the code currently assumes, that feedback_presented gives us presentation feedback for the past events. And I actually wraparound uint64_t value on the subtraction.

You can reproduce this on master adding following code to the feedback_presented function. It gives feedback for the future.

    struct timespec ts;
    if (clock_gettime(CLOCK_MONOTONIC, &ts)) {
        return;
    }
    uint64_t now_monotonic = ts.tv_sec * UINT64_C(1000000) + ts.tv_nsec / 1000;
    assert(now_monotonic >= ust);

I reviewed sync code in general and made few notes of things that I'd like to change. I have vague plan in head, but this PR is not it at this moment.

[Dudemanguy]

I actually noticed that happening on wayland as well the other day. I'm not sure how we should interpret such a value; it reeks of nonsense honestly.

[kasper93]

I need to read docs how exactly it is defined for wayland. But on Windows DwmGetCompositionTimingInfo returns the qpcVBlank field which is most of the time in the future. (we don't use it, just an example) I think once it is scheduled the value is already available. But I don't want to think about Windows specifics rn....

[Dudemanguy]

On another look, the changes LGTM. Works fine too. The only thing is the mac stuff which I'm not sure about. The README claims we support macOS 10.8 (no idea if this is even true), but clock_gettime apparently requires 10.12. I doubt we really care about ancient mac versions though.

[avih]

Nevermind.

I still doubt that nanosec accuracy is required because the measuring itself is noisy to begin with, nano or u sec, so accounting for that noise is needed either way, and I doubt that accounting for usec noise would yield worse results than accounting for nanosec noise, but whatever.

[kasper93]

I still doubt that nanosec accuracy is required

The original change was initiated by people complaining that 1 / 60 in usec is not accurate enough. Does it really fix anything, probably no. But if anything it is cosmetic change that gives users peace of mind.

because the measuring itself is noisy to begin with, nano or u sec, so accounting for that noise is needed either way, and I doubt that accounting for usec noise would yield worse results than accounting for nanosec noise

We are accounting for the noise, by averaging 200 past samples. Averaging truncated values introduces more error/bias. And adding more precision is free.

but whatever.

Yes, whatever. Thanks. I really don't understand this mentality, of not touching/improving things at all cost. Note that you did not make the changes, I did, so whatever...

Wayland gives UST in nanoseconds, Windows uses QPC for their feedback. There is literally no gain in truncating those values, just because. Same in other places where we have higher precision timebase. Nowadays all platforms provides high resolution clocks we can utilize them. It is free. It doesn't harm anything to store more digits in int64_t, but whatever.

[Dudemanguy]

There's several improvements to the vsync code that could be made. I have a few ideas in mind. But step one would be using higher precision clocks so I'm all for this. Actually I plan to follow up with some more unit changes after this PR (unless kasper beats me to it hah).

[kasper93]

@Dudemanguy: Go ahead, I don't have anything in the pipeline right now. I've seen some places that would benefit form higher precision timestamps though.

I want to refactor how win32 presentation feedback works, but I think we are not in conflict there :)

[github-actions]

Download the artifacts for this pull request:

Windows

• mpv-i686-w64-mingw32
• mpv-x86_64-w64-mingw32

[Dudemanguy]

Let's see if someone catches something.