From bc3729c862ba9849e4cdaed4807892b96bafea75 Mon Sep 17 00:00:00 2001
From: Cheng Sun <_@chengsun.uk>
Date: Fri, 21 Jan 2022 00:33:17 +0000
Subject: [PATCH] vo: Fix race condition causing redraw requests to be ignored

This manifested most frequently as a bug where the OSD fails to update
after pausing or single-stepping forwards (issues #8172, #8350).

In `render_frame`, the vo thread takes the lock once before rendering
the frame, and once after. The lock is relinquished whilst the frame is
actually rendering.

The `redraw_request` flag was being cleared whilst holding the lock
*after* the video frame has been rendered; however this frame was
rendered according to the request fetched whilst holding the lock the
first time. If the OSD was updated during the rendering of this frame,
the redraw request is cleared once the frame finishes rendering even
though the updated OSD is not included in the rendered frame.

This patch fixes the race condition by clearing the `redraw_request`
flag *before* the video frame rendering starts. That way, a redraw
request that comes in during frame rendering will not be cleared without
another call to `render_frame` or `do_redraw`. However, we need to
ensure that if the frame is dropped, we set `redraw_request` again if it
was previously set. See the newly added comment in the code.
---
 video/out/vo.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/video/out/vo.c b/video/out/vo.c
index 08d109b76832a..31e37c189e71c 100644
--- a/video/out/vo.c
+++ b/video/out/vo.c
@@ -938,12 +938,23 @@ static bool render_frame(struct vo *vo)
         in->prev_vsync = now;
     in->expecting_vsync = use_vsync;
 
+    bool prev_request_redraw = in->request_redraw;
+
     if (in->dropped_frame) {
         in->drop_count += 1;
         wakeup_core(vo);
     } else {
         in->rendering = true;
         in->hasframe_rendered = true;
+
+        // We have to clear the request_redraw flag now, even though we don't
+        // know whether we have successfully rendered the requested frame yet.
+        // This is so that we can detect new redraw requests whilst we're
+        // rendering the frame with the lock relinquished. If we later find
+        // that we had to drop this frame, we set redraw_request flag again if
+        // it was previously set (see prev_request_redraw).
+        in->request_redraw = false;
+
         int64_t prev_drop_count = vo->in->drop_count;
         // Can the core queue new video now? Non-display-sync uses a separate
         // timer instead, but possibly benefits from preparing a frame early.
@@ -997,8 +1008,7 @@ static bool render_frame(struct vo *vo)
 
     if (in->dropped_frame) {
         MP_STATS(vo, "drop-vo");
-    } else {
-        in->request_redraw = false;
+        in->request_redraw |= prev_request_redraw;
     }
 
     if (in->current_frame && in->current_frame->num_vsyncs &&