Misc display sync fixes #15259
Open
Misc display sync fixes #15259
+27
−11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Download the artifacts for this pull request: |
kasper93
reviewed
Nov 4, 2024
video/out/vo.c
Outdated
| @@ -489,11 +489,14 @@ static void update_vsync_timing_after_swap(struct vo *vo, | |||
| } | |||
|
|
|||
| in->num_successive_vsyncs++; | |||
| if (in->num_successive_vsyncs <= DELAY_VSYNC_SAMPLES) | |||
| if (in->num_successive_vsyncs <= DELAY_VSYNC_SAMPLES) { | |||
| in->base_vsync += in->vsync_interval; | |||
There was a problem hiding this comment.
I don't think it is correct to increment base_vsync here. It is updated in update_vsync_timing_after_swap and this function makes sure valid value is used. base_vsync is last valid point, incrementing with with interval will accumulate a lot of drift, making this value unreliable.
There was a problem hiding this comment.
I guess it makes more sense to set it to the actual vsync instead of incrementing by nominal vsync interval. Updated the patch
llyyr
force-pushed
the
display-sync-shitmess
branch
from
2c6e957
to
0e15601
Compare
into get_display_synced_frame_end. Used in next commit.
llyyr
force-pushed
the
display-sync-shitmess
branch
from
0e15601
to
b2503ef
Compare
This is still used for caclulating a/v sync and delay remaining even for initial samples, so we should always update it to the actual vsync for those initial samples so we have something to work with at least. And if we receive bogus values, also reset it to 0 along with prev_vsync. Not having base_vsync set to _some_ value completely breaks vsync_skip_detection, and mpv stays stuck in a permanent mistimed state where every frame is marked as delayed and never recovers from it.
llyyr
force-pushed
the
display-sync-shitmess
branch
from
b2503ef
to
4c0afdb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First two commits are entirely theoretical, it's more correct but it probably doesn't matter much because this code is only used to detect underruns and to wait for a frame to finish before applying new image_params.
Third commit fixes a real world problem with bad drivers that I encountered while testing various wip fifo-v1/commit-timing implementations. By not always updating base_vsync, we could potentially end up in a permanent underflow state and never recover from it even once the driver has stopped giving us bogus values.