Display lock S03E02
Related issues/MR: #25603 (closed) #25479 (closed) #25694 (closed) #25716 (closed) #25925 !159 (closed) !324 !340 (merged) !353 (closed) !1176 (closed) !1226 (closed)
During the workshop this week-end (April 2nd, 2022), we discussed about how to fix the problems caused by the display lock.
Concretely, in the previous workshop (December 6th/7th 2021), for VLC 4 we agreed to interrupt the wait between prepare() and display() on resize (so display the picture early) in order to release the display_lock as soon as possible. So far, so good.
However, this mechanism was not sufficient: even if interrupting the wait makes the vout thread release the display_lock quickly (at the end of a vout loop), it almost immediately re-acquires it in the next vout loop. As a consequence, the other threads (in practice, the UI thread) often have to wait several vout loops to acquire the lock.
We discussed two proposals to fix the issue. We could not reach an agreement, so here I provide an implementation for both as a support for discussions.
Common parts
Both branches have common parts:
- split the display lock (and use the new lock for protecting the clock)
- interrupt the wait between
prepare()anddisplay()on UI resize requests - re-render ASAP in that case
(although their implementation differ due to branches differences)
Proposal 1: fairness mechanism
Branch display_lock.prop1
I initially proposed a priority mechanism in !1176 (closed). Forget it, it is superseded by a "fairness" mechanism using an additional lock (as discussed). The purpose is the same: avoid to wait several vout loops for acquiring the display_lock.
Proposal 2: asynchronous requests (no display_lock)
Branch display_lock.prop2
Instead of acquiring the display_lock from the UI thread for performing the action, just write the request that will be handled by the vout thread for the next rendering. The UI thread may optionally wait for the current rendering (if any) to be complete.
As a consequence, all vout display callbacks are called from the vout thread (or serialized with the vout thread via the vout_control_Hold() mechanism), so the display_lock becomes useless and is removed.
Discussions
As mentioned in the workshop, I have a strong preference towards proposal 2, because it does not block the UI thread for performing the requests.
For example, if a crop change is requested, proposal 1 (like master) blocks the UI thread until any current rendering on the vout thread is complete, while there is no reason to do so.
On resize, there might be a reason to block the UI thread: if a callback (cb) is passed to vout_ChangeDisplaySize(). In that case, proposal 2 blocks until the current rendering is complete, but does not block otherwise.
EDIT: Note that the lag in viewpoint changes (as implemented on master) in 360 videos (#25479 (closed)) is fixed by proposal 2, but not by proposal 1 (even if we include vout: translate mouse state from the vout thread).
There were mainly two arguments expressed against proposal 2, I will try to answer them.
Complexity
The first objection was that proposal 2 adds complexity.
This is true to some degree, handling requests asynchronously is more complex than performing the actions synchronously, because we must write a request from the UI thread, and perform that request on the vout thread. However, IMO the mechanism is quite straightforward, and this is justified by not blocking the UI thread when we don't need to. The only slightly complex part is the mechanism to wait for the end of a specific rendering (identified by its render_id).
In return, this also brings some simplificaitons: all vout display callbacks are called serialized with the vout thread, and the display_lock disappears.
Of course, proposal 2 has more commits and code changes, since it (semantically) reverts (but without using the control queue) the commits which made the requests synchronous (see !340 (comment 272630)) and removes unnecessary waits.
To get an concrete idea of the result, here is the (current) diff between the two proposals (the "compare" feature from gitlab does not work as expected):
diff between prop1 and prop2
git diff --patience rom1v/display_lock.prop1 rom1v/display_lock.prop2diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
index 179f9018ad..34499691c6 100644
--- a/src/video_output/video_output.c
+++ b/src/video_output/video_output.c
@@ -62,6 +62,13 @@
#include "chrono.h"
#include "control.h"
+enum vout_thread_state
+{
+ VOUT_THREAD_STATE_STOPPED,
+ VOUT_THREAD_STATE_IDLE,
+ VOUT_THREAD_STATE_RENDERING,
+};
+
typedef struct vout_thread_sys_t
{
struct vout_thread_t obj;
@@ -136,25 +143,63 @@ typedef struct vout_thread_sys_t
/* Video output window */
bool window_enabled;
- unsigned window_width; /* protected by display_lock */
- unsigned window_height; /* protected by display_lock */
+ unsigned window_width;
+ unsigned window_height;
vlc_mutex_t window_lock;
vlc_decoder_device *dec_device;
/* Video output display */
vout_display_cfg_t display_cfg;
vout_display_t *display;
- vlc_mutex_t display_lock;
- /* Use an additional lock to guarantee some "fairness" in display_lock
- * acquisition */
- vlc_mutex_t fairness_lock;
-
- /* Additionally protects sys->display and sys->clock even without
- * display_lock */
+ /* Protects sys->display and sys->clock */
vlc_mutex_t sys_lock;
- atomic_uint urgent_pending;
+ bool clock_nowait; /* protected by vlc_clock_Lock()/vlc_clock_Unlock() */
+
+ vlc_mutex_t req_lock;
+ vlc_cond_t req_cond;
+ /* Incremented after each rendering */
+ uint64_t render_id;
+ enum vout_thread_state state;
+
+ struct vout_req {
+#define VOUT_REQ_PENDING_RESIZE (1 << 0)
+#define VOUT_REQ_PENDING_FILL_DISPLAY (1 << 1)
+#define VOUT_REQ_PENDING_ZOOM (1 << 2)
+#define VOUT_REQ_PENDING_DAR (1 << 3)
+#define VOUT_REQ_PENDING_CROP (1 << 4)
+#define VOUT_REQ_PENDING_VIEWPOINT (1 << 5)
+ unsigned pending;
+
+ struct {
+ unsigned width;
+ unsigned height;
+ } resize;
+
+ struct {
+ bool value;
+ } fill_display;
+
+ struct {
+ unsigned num;
+ unsigned den;
+ } zoom;
+
+ struct {
+ unsigned num;
+ unsigned den;
+ } dar;
+
+ struct {
+ struct vout_crop value;
+ } crop;
+
+ struct {
+ vlc_viewpoint_t value;
+ } viewpoint;
+ } req;
+
bool wait_interrupted;
/* Video filter2 chain */
@@ -196,13 +241,6 @@ typedef struct vout_thread_sys_t
/* Better be in advance when awakening than late... */
#define VOUT_MWAIT_TOLERANCE VLC_TICK_FROM_MS(4)
-static inline void DisplayLockFair(struct vout_thread_sys_t *sys)
-{
- vlc_mutex_lock(&sys->fairness_lock);
- vlc_mutex_lock(&sys->display_lock);
- vlc_mutex_unlock(&sys->fairness_lock);
-}
-
/* */
static bool VoutCheckFormat(const video_format_t *src)
{
@@ -222,6 +260,21 @@ static void VoutFixFormat(video_format_t *dst, const video_format_t *src)
video_format_FixRgb(dst);
}
+static void VoutRenderWakeUpUrgent(vout_thread_sys_t *sys)
+{
+ /* The assignment to sys->clock is protected by sys->lock */
+ vlc_mutex_lock(&sys->sys_lock);
+ if (sys->clock)
+ {
+ /* Wake up the clock-wait between prepare() and display() */
+ vlc_clock_Lock(sys->clock);
+ sys->clock_nowait = true;
+ vlc_clock_Wake(sys->clock);
+ vlc_clock_Unlock(sys->clock);
+ }
+ vlc_mutex_unlock(&sys->sys_lock);
+}
+
static bool VideoFormatIsCropArEqual(video_format_t *dst,
const video_format_t *src)
{
@@ -462,33 +515,23 @@ void vout_ChangeDisplaySize(vout_thread_t *vout,
assert(!sys->dummy);
- /* The assignment to sys->clock is protected by sys->lock */
- vlc_mutex_lock(&sys->sys_lock);
- if (sys->clock)
- {
- /* Wake up the clock-wait between prepare() and display() */
- vlc_clock_Lock(sys->clock);
- atomic_fetch_add_explicit(&sys->urgent_pending, 1,
- memory_order_relaxed);
- vlc_clock_Wake(sys->clock);
- vlc_clock_Unlock(sys->clock);
- }
- vlc_mutex_unlock(&sys->sys_lock);
-
- /* DO NOT call this outside the vout window callbacks */
- DisplayLockFair(sys);
-
- atomic_fetch_sub_explicit(&sys->urgent_pending, 1, memory_order_relaxed);
-
- sys->window_width = width;
- sys->window_height = height;
-
- if (sys->display != NULL)
- vout_display_SetSize(sys->display, width, height);
+ vlc_mutex_lock(&sys->req_lock);
+ sys->req.pending |= VOUT_REQ_PENDING_RESIZE;
+ sys->req.resize.width = width;
+ sys->req.resize.height = height;
+
+ if (sys->state == VOUT_THREAD_STATE_RENDERING)
+ VoutRenderWakeUpUrgent(sys);
+
+ /* Must wait until a rendering at the new size is complete */
+ uint64_t target_render_id = sys->render_id + 1 + (sys->state == VOUT_THREAD_STATE_RENDERING);
+ while (sys->state != VOUT_THREAD_STATE_STOPPED
+ && sys->render_id < target_render_id)
+ vlc_cond_wait(&sys->req_cond, &sys->req_lock);
if (cb != NULL)
cb(opaque);
- vlc_mutex_unlock(&sys->display_lock);
+ vlc_mutex_unlock(&sys->req_lock);
}
void vout_ChangeDisplayFilled(vout_thread_t *vout, bool is_filled)
@@ -500,12 +543,11 @@ void vout_ChangeDisplayFilled(vout_thread_t *vout, bool is_filled)
sys->display_cfg.is_display_filled = is_filled;
/* no window size update here */
- DisplayLockFair(sys);
+ vlc_mutex_lock(&sys->req_lock);
+ sys->req.pending |= VOUT_REQ_PENDING_FILL_DISPLAY;
+ sys->req.fill_display.value = true;
+ vlc_mutex_unlock(&sys->req_lock);
vlc_mutex_unlock(&sys->window_lock);
-
- if (sys->display != NULL)
- vout_SetDisplayFilled(sys->display, is_filled);
- vlc_mutex_unlock(&sys->display_lock);
}
void vout_ChangeZoom(vout_thread_t *vout, unsigned num, unsigned den)
@@ -534,12 +576,12 @@ void vout_ChangeZoom(vout_thread_t *vout, unsigned num, unsigned den)
vout_UpdateWindowSizeLocked(sys);
- DisplayLockFair(sys);
+ vlc_mutex_lock(&sys->req_lock);
+ sys->req.pending |= VOUT_REQ_PENDING_ZOOM;
+ sys->req.zoom.num = num;
+ sys->req.zoom.den = den;
+ vlc_mutex_unlock(&sys->req_lock);
vlc_mutex_unlock(&sys->window_lock);
-
- if (sys->display != NULL)
- vout_SetDisplayZoom(sys->display, num, den);
- vlc_mutex_unlock(&sys->display_lock);
}
static void vout_SetAspectRatio(vout_thread_sys_t *sys,
@@ -560,12 +602,12 @@ void vout_ChangeDisplayAspectRatio(vout_thread_t *vout,
vout_UpdateWindowSizeLocked(sys);
- DisplayLockFair(sys);
+ vlc_mutex_lock(&sys->req_lock);
+ sys->req.pending |= VOUT_REQ_PENDING_DAR;
+ sys->req.dar.num = dar_num;
+ sys->req.dar.den = dar_den;
+ vlc_mutex_unlock(&sys->req_lock);
vlc_mutex_unlock(&sys->window_lock);
-
- if (sys->display != NULL)
- vout_SetDisplayAspect(sys->display, dar_num, dar_den);
- vlc_mutex_unlock(&sys->display_lock);
}
void vout_ChangeCrop(vout_thread_t *vout,
@@ -578,12 +620,11 @@ void vout_ChangeCrop(vout_thread_t *vout,
sys->source.crop = *crop;
vout_UpdateWindowSizeLocked(sys);
- DisplayLockFair(sys);
+ vlc_mutex_lock(&sys->req_lock);
+ sys->req.pending |= VOUT_REQ_PENDING_CROP;
+ sys->req.crop.value = *crop;
+ vlc_mutex_unlock(&sys->req_lock);
vlc_mutex_unlock(&sys->window_lock);
-
- if (sys->display != NULL)
- vout_SetDisplayCrop(sys->display, crop);
- vlc_mutex_unlock(&sys->display_lock);
}
void vout_ControlChangeFilters(vout_thread_t *vout, const char *filters)
@@ -651,12 +692,12 @@ void vout_ChangeViewpoint(vout_thread_t *vout,
vlc_mutex_lock(&sys->window_lock);
sys->display_cfg.viewpoint = *p_viewpoint;
/* no window size update here */
- vlc_mutex_unlock(&sys->window_lock);
- DisplayLockFair(sys);
- if (sys->display != NULL)
- vout_SetDisplayViewpoint(sys->display, p_viewpoint);
- vlc_mutex_unlock(&sys->display_lock);
+ vlc_mutex_lock(&sys->req_lock);
+ sys->req.pending |= VOUT_REQ_PENDING_VIEWPOINT;
+ sys->req.viewpoint.value = *p_viewpoint;
+ vlc_mutex_unlock(&sys->req_lock);
+ vlc_mutex_unlock(&sys->window_lock);
}
/* */
@@ -1228,16 +1269,42 @@ static int RenderPicture(vout_thread_sys_t *sys, bool render_now)
if (!filtered)
return VLC_EGENERIC;
- DisplayLockFair(sys);
+ vlc_mutex_lock(&sys->req_lock);
+ struct vout_req req = sys->req;
+ sys->req.pending = 0;
+ sys->state = VOUT_THREAD_STATE_RENDERING;
+ vlc_mutex_unlock(&sys->req_lock);
+
+ if (req.pending) {
+ if (req.pending & VOUT_REQ_PENDING_RESIZE)
+ {
+ sys->window_width = req.resize.width;
+ sys->window_height = req.resize.height;
+ vout_display_SetSize(sys->display, req.resize.width,
+ req.resize.height);
+ }
+
+ if (req.pending & VOUT_REQ_PENDING_FILL_DISPLAY)
+ vout_SetDisplayFilled(sys->display, req.fill_display.value);
+
+ if (req.pending & VOUT_REQ_PENDING_ZOOM)
+ vout_SetDisplayZoom(sys->display, req.zoom.num, req.zoom.den);
+
+ if (req.pending & VOUT_REQ_PENDING_DAR)
+ vout_SetDisplayAspect(sys->display, req.dar.num, req.dar.den);
+
+ if (req.pending & VOUT_REQ_PENDING_CROP)
+ vout_SetDisplayCrop(sys->display, &req.crop.value);
+
+ if (req.pending & VOUT_REQ_PENDING_VIEWPOINT)
+ vout_SetDisplayViewpoint(sys->display, &req.viewpoint.value);
+ }
picture_t *todisplay;
subpicture_t *subpic;
int ret = PrerenderPicture(sys, filtered, &render_now, &todisplay, &subpic);
if (ret != VLC_SUCCESS)
- {
- vlc_mutex_unlock(&sys->display_lock);
return ret;
- }
vlc_tick_t system_now = vlc_tick_now();
const vlc_tick_t pts = todisplay->date;
@@ -1296,8 +1363,7 @@ static int RenderPicture(vout_thread_sys_t *sys, bool render_now)
deadline = max_deadline;
}
- if (atomic_load_explicit(&sys->urgent_pending,
- memory_order_relaxed))
+ if (sys->clock_nowait)
{
/* A caller (the UI thread) awaits for the rendering to
* complete urgently, do not wait. */
@@ -1324,7 +1390,17 @@ static int RenderPicture(vout_thread_sys_t *sys, bool render_now)
/* Display the direct buffer returned by vout_RenderPicture */
vout_display_Display(vd, todisplay);
- vlc_mutex_unlock(&sys->display_lock);
+
+ vlc_mutex_lock(&sys->req_lock);
+ sys->state = VOUT_THREAD_STATE_IDLE;
+ ++sys->render_id;
+ vlc_cond_broadcast(&sys->req_cond);
+
+ vlc_clock_Lock(sys->clock);
+ sys->clock_nowait = false;
+ vlc_clock_Unlock(sys->clock);
+
+ vlc_mutex_unlock(&sys->req_lock);
picture_Release(todisplay);
@@ -1511,10 +1587,8 @@ static void vout_FlushUnlocked(vout_thread_sys_t *vout, bool below,
picture_fifo_Flush(sys->decoder_fifo, date, below);
- vlc_mutex_lock(&sys->display_lock);
if (sys->display != NULL)
vout_FilterFlush(sys->display);
- vlc_mutex_unlock(&sys->display_lock);
if (sys->clock != NULL)
{
@@ -1605,9 +1679,7 @@ static void ProcessMouseState(vout_thread_sys_t *p_vout,
/* Translate window coordinates to video coordinates */
vlc_mouse_t win_mouse = *mouse;
- vlc_mutex_lock(&sys->display_lock);
vout_display_TranslateMouseState(sys->display, &win_mouse, mouse);
- vlc_mutex_unlock(&sys->display_lock);
/* pass mouse coordinates in the filter chains. */
m = &win_mouse;
@@ -1687,10 +1759,8 @@ static int vout_Start(vout_thread_sys_t *vout, vlc_video_context *vctx, const vo
crop = sys->source.crop;
num = sys->source.dar.num;
den = sys->source.dar.den;
- vlc_mutex_lock(&sys->display_lock);
vlc_mutex_unlock(&sys->window_lock);
- /* Setup the window size, protected by the display_lock */
dcfg.display.width = sys->window_width;
dcfg.display.height = sys->window_height;
@@ -1704,7 +1774,6 @@ static int vout_Start(vout_thread_sys_t *vout, vlc_video_context *vctx, const vo
vlc_mutex_unlock(&sys->sys_lock);
if (sys->display == NULL) {
- vlc_mutex_unlock(&sys->display_lock);
goto error;
}
@@ -1712,7 +1781,6 @@ static int vout_Start(vout_thread_sys_t *vout, vlc_video_context *vctx, const vo
if (num != 0 && den != 0)
vout_SetDisplayAspect(sys->display, num, den);
- vlc_mutex_unlock(&sys->display_lock);
assert(sys->private.display_pool != NULL && sys->private.private_pool != NULL);
@@ -1767,6 +1835,11 @@ static void *Thread(void *object)
vout_thread_sys_t *vout = object;
vout_thread_sys_t *sys = vout;
+ vlc_mutex_lock(&sys->req_lock);
+ assert(sys->state == VOUT_THREAD_STATE_STOPPED);
+ sys->state = VOUT_THREAD_STATE_IDLE;
+ vlc_mutex_unlock(&sys->req_lock);
+
vlc_tick_t deadline = VLC_TICK_INVALID;
for (;;) {
@@ -1793,6 +1866,12 @@ static void *Thread(void *object)
vout_SetInterlacingState(&vout->obj, &sys->private, picture_interlaced);
}
+
+ vlc_mutex_lock(&sys->req_lock);
+ assert(sys->state == VOUT_THREAD_STATE_IDLE);
+ sys->state = VOUT_THREAD_STATE_STOPPED;
+ vlc_mutex_unlock(&sys->req_lock);
+
return NULL;
}
@@ -1809,13 +1888,11 @@ static void vout_ReleaseDisplay(vout_thread_sys_t *vout)
if (sys->private.display_pool != NULL)
vout_FlushUnlocked(vout, true, VLC_TICK_MAX);
- vlc_mutex_lock(&sys->display_lock);
vlc_mutex_lock(&sys->sys_lock);
vout_CloseWrapper(&vout->obj, &sys->private, sys->display);
sys->display = NULL;
sys->clock = NULL;
vlc_mutex_unlock(&sys->sys_lock);
- vlc_mutex_unlock(&sys->display_lock);
/* Destroy the video filters */
DelAllFilterCallbacks(vout);
@@ -2020,11 +2097,16 @@ vout_thread_t *vout_Create(vlc_object_t *object)
/* Display */
sys->display = NULL;
- vlc_mutex_init(&sys->display_lock);
- vlc_mutex_init(&sys->fairness_lock);
vlc_mutex_init(&sys->sys_lock);
- atomic_init(&sys->urgent_pending, 0);
+ sys->clock_nowait = false;
+
+ vlc_mutex_init(&sys->req_lock);
+ vlc_cond_init(&sys->req_cond);
+ sys->render_id = 0;
+ sys->state = VOUT_THREAD_STATE_STOPPED;
+ sys->req.pending = 0;
+
sys->wait_interrupted = false;
/* Window */
diff --git a/src/video_output/video_window.c b/src/video_output/video_window.c
index a37a96534e..0c6566d522 100644
--- a/src/video_output/video_window.c
+++ b/src/video_output/video_window.c
@@ -64,10 +64,16 @@ static void vout_display_window_ResizeNotify(vout_window_t *window,
{
vout_display_window_t *state = window->owner.sys;
vout_thread_t *vout = state->vout;
- struct vout_window_ack_data data = { window, cb, width, height, opaque };
msg_Dbg(window, "resized to %ux%u", width, height);
- vout_ChangeDisplaySize(vout, width, height, vout_window_Ack, &data);
+ if (cb) {
+ struct vout_window_ack_data data = {
+ window, cb, width, height, opaque
+ };
+ vout_ChangeDisplaySize(vout, width, height, vout_window_Ack, &data);
+ } else {
+ vout_ChangeDisplaySize(vout, width, height, NULL, NULL);
+ }
}
static void vout_display_window_CloseNotify(vout_window_t *window)VLC5 direction
The other main argument against proposal 2 is that this mechanism avoids to block the UI thread, but in theory for "frame-perfect" (which we would like for VLC5) we have to block anyway.
Firstly, we don't have to block for all requests, even with "frame-perfect": a crop or an aspect ratio change have no reason to block the UI thread until the current rendering is complete.
Besides, it is assumed that locking the display_lock from the UI thread is more "frame-perfect-friendly", but for example locking the display_lock on the UI thread on resize will not allow to wait for the completion of a rendering at the new size performed on the vout thread (the display_lock must be released so that the vout thread could acquire it). By contrast, with proposal 2, we can wait for a specific rendering completion (via its render_id), so we could wait for the rendering at the new size to be complete. So it seems to me that proposal 1 (and master) is further from this goal than proposal 2.
In any case, in VLC4, we should not block the UI thread more than necessary without additional benefits.
Next steps
I hope this thread clarifies the current state (more than the discussions during the workshop, I felt like I wasn't very clear).
Please post your remarks/opinions, then we should probably make a decision.
I will then open a MR with either proposal 1 or 2 depending on what we decide, to allow a proper review of the implementation.
Thank you.