vo: fix race condition with redraw requests #11395
Merged
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
Dudemanguy
force-pushed
the
vo-request-race
branch
from
ce4cabf to
f63cd42
Compare
There is a very subtle race in vo that can manifest itself on pause events. In the renderloop, render_frame, unsurprisingly, does the heavy lifting of actually queuing up and flipping the frames. This is called during normal playback. Sometimes various parts of the player can make a redraw request which will latter trigger another render of the frame later down in the loop (do_redraw). Because these requests can happen at essentially anytime, sometimes the redraw request will happen *before* do_redraw and it'll be caught in render_frame. When this happens, the existing render_frame run works perfectly fine as a redraw so it clears out the request which is sensible. Normally this is all locked of course, but there's one catch. render_frame has to unlock itself when propagating down into specific VOs/backends. That's what causes this bug. While render_frame is unlocked, other parts of the player can send redraw requests which will cause in->request_redraw to become true. The logic in the code always clears out this parameter after a successful render, but this isn't correct. When in->request_become becomes true in the middle of render_frame, there needs to be one more draw afterwards to reflect whatever actually changed (usually the OSD). Instead, this gets simply discarded. If you rapidly spam pause while rendering things to the OSD at the same time, it's possible to for the last render to be behind a frame and appear as if your osd event was ignored. Once you realize what is happening, the fix is quite simple. Just store the initial value of in->request_redraw before the unlock step. After we do the render step and unlock again, only set in->request_redraw to false if there was an initial redraw request. We just finished doing a redraw, so it is safe to clear. Otherwise, leave in->request_redraw alone. If it is initially false, then it will still be false and nothing changes. However if it updated to true in the middle of the rendering, this value is now preserved so we can go and call do_redraw later and show what that last frame was meant to be when you pause. One unfortunate thing about this design is that it is technically possible for other internal things in vo to update during that unlocked period. Hopefully, that doesn't actually happen and only redraw requests work like this. Fixes mpv-player#8172 and mpv-player#8350.
Dudemanguy
force-pushed
the
vo-request-race
branch
from
f63cd42 to
da36a8b
Compare
sfan5
approved these changes
Mar 2, 2023
This was referenced Mar 2, 2023
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.
See the commit message for the full explanation.
This looks very similar to #9735, but is different in a few important ways. While the user did correctly identify the problem, the fix was not correct and erroneously resulted in unneeded redraw events. This approach only sets the extra redraw when it is necessary (i.e. to fix the bug caused by the race).
Fixes: #8172 and #8350