vo: fix race condition with redraw requests

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 #8172 and #8350.

1 files changed, 11 insertions, 1 deletions

diff --git a/video/out/vo.c b/video/out/vo.c
index 031a4e61a2..40079049f8 100644
--- a/video/out/vo.c
+++ b/video/out/vo.c
@@ -944,6 +944,9 @@ static bool render_frame(struct vo *vo)
         in->prev_vsync = now;
     in->expecting_vsync = use_vsync;
 
+    // Store the initial value before we unlock.
+    bool request_redraw = in->request_redraw;
+
     if (in->dropped_frame) {
         in->drop_count += 1;
         wakeup_core(vo);
@@ -1004,7 +1007,14 @@ static bool render_frame(struct vo *vo)
     if (in->dropped_frame) {
         MP_STATS(vo, "drop-vo");
     } else {
-        in->request_redraw = false;
+        // If the initial redraw request was true, then we can
+        // clear it here since we just performed a redraw and are
+        // merely clearing that request. However if there initially is
+        // no redraw request, then something can change this (i.e. the OSD)
+        // while the vo was unlocked. Don't touch in->request_redraw
+        // in that case.
+        if (request_redraw)
+            in->request_redraw = false;
     }
 
     if (in->current_frame && in->current_frame->num_vsyncs &&