vo: fix race condition with redraw requests #17320
Draft
Conversation
This file contains hidden or 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
Previously, request_redraw was a boolean flag. If a new redraw request occurred while render_frame was executing, the flag would sometimes be erroneously cleared at the end of the function, causing the OSD update to be lost. This was particularly noticeable during frame stepping, where the OSD would be stuck incorrectly displaying the previous frame's timestamp and a false pause state. Commit 296d40d ("vo: fix race condition with redraw requests") addressed already some cases, but it was still possible for the flag to be wrongly cleared when: 1. a new request arrived while render_frame was unlocked and rendering, 2. the pause state changed to false immediately after checking it. This change replaces the boolean flag with a counter (redraw_req_id) and adds a tracking variable for the last rendered request (last_redraw_id). render_frame now snapshots the request counter at the start and only updates last_redraw_id to match this snapshot at the end. If redraw_req_id is incremented during execution (indicating a new request came in), the resulting mismatch with last_redraw_id forces the loop to run again, ensuring the pending OSD update is drawn immediately. Additionally, we now always request a redraw if the video was paused to ensure the OSD state is up-to-date.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Disclaimer: I am not a coder and used an AI/LLM to help with this.
Previously, request_redraw was a boolean flag. If a new redraw request occurred while render_frame was executing, the flag would sometimes be erroneously cleared at the end of the function, causing the OSD update to be lost. This was particularly noticeable during frame stepping, where the OSD would be stuck incorrectly displaying the previous frame's timestamp and a false pause state.
Commit 296d40d ("vo: fix race condition with redraw requests") addressed already some cases, but it was still possible for the flag to be wrongly cleared when:
This change replaces the boolean flag with a counter (redraw_req_id) and adds a tracking variable for the last rendered request (last_redraw_id). render_frame now snapshots the request counter at the start and only updates last_redraw_id to match this snapshot at the end. If redraw_req_id is incremented during execution (indicating a new request came in), the resulting mismatch with last_redraw_id forces the loop to run again, ensuring the pending OSD update is drawn immediately.
Additionally, we now always request a redraw if the video was paused to ensure the OSD state is up-to-date.
I'm not totally confident that this commit fixes the issue. Since this only occurs during very rapid state changes,
very small execution delays could as a side effect allow the player state enough time to settle,
and inadvertently mask the issue.
So I would appreciate it if someone with a better understanding of the whole workflow like Dudemanguy could investigate this further.
Fix targets: #17288