Skip to content

player/video: rewrite display-sync audio drift compensation#17776

Open
kasper93 wants to merge 4 commits intompv-player:masterfrom
kasper93:audio-sync-fix
Open

player/video: rewrite display-sync audio drift compensation#17776
kasper93 wants to merge 4 commits intompv-player:masterfrom
kasper93:audio-sync-fix

Conversation

@kasper93
Copy link
Copy Markdown
Member

No description provided.

@Dudemanguy Dudemanguy self-requested a review April 20, 2026 00:09
Comment thread player/video.c Outdated
Comment thread player/video.c Outdated
// recovery loop noticeably.
const double AVD_FILTER_TIME = 1.0;
// Target exponential decay time of av_diff_lp toward zero.
const double AVD_RECOVERY_TIME = 15.0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Based on my testing it now takes much longer to return to AV sync because of this large time constant, combined with delayed reaction caused by the LP filter.

The maximum correction speed can be calculated with max slew and opts->sync_max_audio_change so I think this is not needed.

Copy link
Copy Markdown
Member Author

@kasper93 kasper93 Apr 20, 2026

Choose a reason for hiding this comment

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

Based on my testing it now takes much longer to return to AV sync because of this large time constant, combined with delayed reaction caused by the LP filter.

That's correct, but in playback, we prefer stability, over short term correction. Even if it takes some seconds more, in normal playback scenario we just want to gently adjust audio speed to keep the sync. Note that for big difference we would drop frames anyway, same as with old code. Also note, that it correct quite slow when we are close to the perfect sync, because we are already in sync in practice.

I will retest, with more aggressive correction. But current version works well for me.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For the record, this is before and after for me:

image image

This is when it is correcting:
image

Note, that my main objective was to avoid constantly doing big audio speed changes. Which you may argue is not audible in our default sync limits, but still it's not nice to do that to resampler. Also avoid overshooting and never actually arriving at the steady state. While obviously we were still in sync, it was doing more than it should, and making things worse.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's correct, but in playback, we prefer stability, over short term correction. Even if it takes some seconds more, in normal playback scenario we just want to gently adjust audio speed to keep the sync.

This is already controlled by opts->sync_max_audio_change, which should have a default that is "safe". In my testing of 25 fps video on 60 fps display, the speed of change never reaches this max value, essentially making it useless.

Also note, that it correct quite slow when we are close to the perfect sync, because we are already in sync in practice.

This does not require exp decay. When max slew is known, the place where the correction needs to decelerate can be calculated, such that when correction reaches target, correction speed also reaches zero, so a steady state is achieved.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is already controlled by opts->sync_max_audio_change, which should have a default that is "safe". In my testing of 25 fps video on 60 fps display, the speed of change never reaches this max value, essentially making it useless.

For me 24 fps on 240 fps, ping-pongs between min/max correction and never actually settles down.

This does not require exp decay. When max slew is known, the place where the correction needs to decelerate can be calculated, such that when correction reaches target, correction speed also reaches zero, so a steady state is achieved.

I had this in previous version, but I found decaying over time more universal for all corner cases in my testing. Also the biggest issues is what to do around the zero, when there is little systemic drift. So we can calculate how to correct towards zero, but exp decay works nicely to smooth out correction when we are in the steady state. Note that even though in many cases vspeed==aspeed is actually good to keep sync, by constantly adjusting the aspeed we never actually have to worry about drifting away.

Those values are often very small, but it's still good information to
have how they are changing.

This brings those values closes to old `%+.05f%%` that were used here.
To better see close to zero values, instead of just clamping to 0.
@kasper93 kasper93 force-pushed the audio-sync-fix branch 2 times, most recently from b7eeec0 to 267c741 Compare April 20, 2026 02:55
The old code had several interacting problems that prevented A/V from
reaching a steady state.

When the A/V difference was large, it applied the maximum allowed
compensation in one step. That was fine for catching up, but the value
was never re-evaluated, so audio speed routinely overshot the target.
Once the difference crossed zero (or re-entered the acceptable range) it
would run a linear regression over recent av_diff samples and snap the
audio speed to the estimated drift. The fatal flaw was that those
samples already included the controller's own compensation: even when
the underlying drift was zero, the controller would "detect" the drift
it had just induced and try to cancel it. Between these rare, sparse
updates audio speed stayed frozen, so in practice the controller was
introducing drift more than it was fixing it. On high fps outputs,
where the vsync range is tight, this degenerated into a ping-pong:
overshoot, snap back, overshoot again, never settling.

The new code replaces the bang-bang direction controller and one-shot
regression with a proportional controller driven by a low-pass-filtered
av_diff. The LP filter (1 s time constant) rejects per-frame vsync
jitter so the compensation no longer chases noise. The target then
decays av_diff exponentially toward zero with a 10 s recovery time.
Adaptive slew caps per-frame motion to the desired correction magnitude,
so large excursions are absorbed quickly while near-equilibrium motion
stays below audibility. The result is a continuous, gentle control loop
that actually reaches steady state and does so without audible
audio-speed modulation.
This helps with video fps estimation, for containers that doesn't
provide accurate timestamps.
@sfan5
Copy link
Copy Markdown
Member

sfan5 commented Apr 21, 2026

With a 60fps video if I drag it from my 60Hz to my 144Hz display (X11 btw), then it takes ~17 seconds until the sync ratio reaches the target value of 2.4. Likewise the other way around.
This doesn't seem to have any ill effects, but is this intentional?

Haven't noticed anything else in a bit of testing.

@kasper93
Copy link
Copy Markdown
Member Author

With a 60fps video if I drag it from my 60Hz to my 144Hz display (X11 btw), then it takes ~17 seconds until the sync ratio reaches the target value of 2.4. Likewise the other way around. This doesn't seem to have any ill effects, but is this intentional?

Haven't noticed anything else in a bit of testing.

Yes, it prefers slow adjustment. Also it should got into +-vsync/2 range quite quicky, while the remaining is just smoothing, we are already in sync.

If that's an issue we want to fix, I can make it more aggressive. But, yes, this was intentional to have slow audio speed changes. This also smoothes all possible perturbation that may happen mostly during playback start.

@Jules-A
Copy link
Copy Markdown

Jules-A commented Apr 22, 2026

If that's an issue we want to fix, I can make it more aggressive.

Can it be user adjustable?

@kasper93
Copy link
Copy Markdown
Member Author

If that's an issue we want to fix, I can make it more aggressive.

Can it be user adjustable?

I don't see user need to adjust any of this beyond the current settings.

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.

4 participants