[vlc-devel] [PATCH 1/2] vout: add a resize acknowledgement callback
Thomas Guillem
thomas at gllm.fr
Thu Feb 11 09:14:41 UTC 2021
Sorry, I reviewed too fast. I have one small comment.
On Tue, Feb 9, 2021, at 18:04, remi at remlab.net wrote:
> From: Rémi Denis-Courmont <remi at remlab.net>
>
> For perfect rendering synchronisation, the change of window size must
> be acknowledged after the last picture is rendered in the old size,
> and before the first picture is rendered in the new size.
>
> As of c9d6d95f291b1e2c0e4f374f87790af091282c1e, the window resize is
> processed synchronously. This provides for the "after" requirement. But
> it failed to address the "before" requirement: the video output thread
> can end render a picture so soon after the resize so the window
> callback thread has not had time to perform acknowledgement procedures.
>
> This adds a callback to the window resize event which is called before
> the display lock (or equivalent) is released. This is guaranteed to be
> invoked before the video output thread has the opportunity to render
> anything.
>
> Other options were considered:
>
> 1) Invoking a callback to the window provider from the video output
> thread code code.
> -> This can delay the callback unreasonably.
> -> Taking the window lock on the video output thread is undesirable
> if at all possible.
>
> 2) Invoking a windowing system type-specific callback to the window
> provider from every display of that windowing system.
> -> Same delay and locking problems as 1.
> -> Requires patching several display plugins.
>
> 3) Exposing the display lock to the window plugin, via new callbacks.
> -> This leaks internal implementation of the core to window
> providers.
> -> This gets really confusing as some callbacks would need explicit
> locking and others not.
>
> In comparison, this patch seems like the least of evils.
>
> The callback is passed a parameter rather than as a property of the
> window to distinguish it from the existing window callbacks which are
> not reentrant.
>
> Refs #25112.
> ---
> include/vlc_vout_window.h | 7 +++++--
> modules/video_output/splitter.c | 6 +++++-
> src/video_output/opengl.c | 6 +++++-
> src/video_output/video_output.c | 6 +++++-
> src/video_output/vout_internal.h | 3 ++-
> src/video_output/window.c | 21 +++++++++++++++++++--
> 6 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/include/vlc_vout_window.h b/include/vlc_vout_window.h
> index ea16d4b414..7d0db591e5 100644
> --- a/include/vlc_vout_window.h
> +++ b/include/vlc_vout_window.h
> @@ -173,6 +173,8 @@ typedef struct vout_window_cfg_t {
>
> } vout_window_cfg_t;
>
> +typedef void (*vout_window_ack_cb)(struct vout_window_t *, void *);
> +
> /**
> * Window event callbacks structure.
> *
> @@ -200,7 +202,8 @@ struct vout_window_callbacks {
> * of external events from the windowing system, or deferred processing of
> * a size change request.
> */
> - void (*resized)(struct vout_window_t *, unsigned width, unsigned height);
> + void (*resized)(struct vout_window_t *, unsigned width, unsigned height,
> + vout_window_ack_cb cb, void *opaque);
Could you add documentation (include)?
Otherwise, OK with the set.
>
> /**
> * Callback for window closing.
> @@ -543,7 +546,7 @@ void vout_window_Disable(vout_window_t *window);
> static inline void vout_window_ReportSize(vout_window_t *window,
> unsigned width, unsigned height)
> {
> - window->owner.cbs->resized(window, width, height);
> + window->owner.cbs->resized(window, width, height, NULL, NULL);
> }
>
> /**
> diff --git a/modules/video_output/splitter.c b/modules/video_output/splitter.c
> index 6ca82e34c1..9a2a1544e7 100644
> --- a/modules/video_output/splitter.c
> +++ b/modules/video_output/splitter.c
> @@ -135,7 +135,8 @@ static void vlc_vidsplit_Close(vout_display_t *vd)
> }
>
> static void vlc_vidsplit_window_Resized(vout_window_t *wnd,
> - unsigned width, unsigned height)
> + unsigned width, unsigned height,
> + vout_window_ack_cb cb, void *opaque)
> {
> struct vlc_vidsplit_part *part = wnd->owner.sys;
>
> @@ -145,6 +146,9 @@ static void vlc_vidsplit_window_Resized(vout_window_t *wnd,
>
> if (part->display != NULL)
> vout_display_SetSize(part->display, width, height);
> +
> + if (cb != NULL)
> + cb(wnd, opaque);
> vlc_sem_post(&part->lock);
> }
>
> diff --git a/src/video_output/opengl.c b/src/video_output/opengl.c
> index 8355c16135..bb10ca6302 100644
> --- a/src/video_output/opengl.c
> +++ b/src/video_output/opengl.c
> @@ -124,7 +124,8 @@ typedef struct vlc_gl_surface
> } vlc_gl_surface_t;
>
> static void vlc_gl_surface_ResizeNotify(vout_window_t *surface,
> - unsigned width, unsigned height)
> + unsigned width, unsigned height,
> + vout_window_ack_cb cb, void *opaque)
> {
> vlc_gl_surface_t *sys = surface->owner.sys;
>
> @@ -133,6 +134,9 @@ static void
> vlc_gl_surface_ResizeNotify(vout_window_t *surface,
> vlc_mutex_lock(&sys->lock);
> sys->width = width;
> sys->height = height;
> +
> + if (cb != NULL)
> + cb(surface, opaque);
> vlc_mutex_unlock(&sys->lock);
> }
>
> diff --git a/src/video_output/video_output.c
> b/src/video_output/video_output.c
> index d04d3f4d11..fb7f54e385 100644
> --- a/src/video_output/video_output.c
> +++ b/src/video_output/video_output.c
> @@ -543,7 +543,8 @@ void vout_ChangeWindowState(vout_thread_t *vout,
> unsigned st)
> }
>
> void vout_ChangeDisplaySize(vout_thread_t *vout,
> - unsigned width, unsigned height)
> + unsigned width, unsigned height,
> + void (*cb)(void *), void *opaque)
> {
> vout_thread_sys_t *sys = VOUT_THREAD_TO_SYS(vout);
>
> @@ -557,6 +558,9 @@ void vout_ChangeDisplaySize(vout_thread_t *vout,
>
> if (sys->display != NULL)
> vout_display_SetSize(sys->display, width, height);
> +
> + if (cb != NULL)
> + cb(opaque);
> vlc_mutex_unlock(&sys->display_lock);
> }
>
> diff --git a/src/video_output/vout_internal.h
> b/src/video_output/vout_internal.h
> index fb24b51ee8..95274f2409 100644
> --- a/src/video_output/vout_internal.h
> +++ b/src/video_output/vout_internal.h
> @@ -150,7 +150,8 @@ bool GetAspectRatio(const char *ar_str, unsigned
> *num, unsigned *den);
> void vout_ChangeFullscreen(vout_thread_t *, const char *id);
> void vout_ChangeWindowed(vout_thread_t *);
> void vout_ChangeWindowState(vout_thread_t *, unsigned state);
> -void vout_ChangeDisplaySize(vout_thread_t *, unsigned width, unsigned
> height);
> +void vout_ChangeDisplaySize(vout_thread_t *, unsigned width, unsigned
> height,
> + void (*ack_cb)(void *), void *opaque);
> void vout_ChangeDisplayFilled(vout_thread_t *, bool is_filled);
> void vout_ChangeZoom(vout_thread_t *, unsigned num, unsigned den);
> void vout_ChangeDisplayAspectRatio(vout_thread_t *, unsigned num,
> unsigned den);
> diff --git a/src/video_output/window.c b/src/video_output/window.c
> index 9c2df5c1e6..c866fe7af4 100644
> --- a/src/video_output/window.c
> +++ b/src/video_output/window.c
> @@ -198,6 +198,20 @@ void vout_window_ReportFullscreen(vout_window_t
> *window, const char *id)
> window->owner.cbs->fullscreened(window, id);
> }
>
> +struct vout_window_ack_data {
> + vout_window_t *window;
> + vout_window_ack_cb callback;
> + void *opaque;
> +};
> +
> +static void vout_window_Ack(void *data)
> +{
> + struct vout_window_ack_data *cb_data = data;
> +
> + if (cb_data->callback != NULL)
> + cb_data->callback(cb_data->window, cb_data->opaque);
> +}
> +
> /* Video output display integration */
> #include <vlc_vout.h>
> #include <vlc_vout_display.h>
> @@ -214,13 +228,16 @@ typedef struct vout_display_window
> } vout_display_window_t;
>
> static void vout_display_window_ResizeNotify(vout_window_t *window,
> - unsigned width, unsigned height)
> + unsigned width, unsigned height,
> + vout_window_ack_cb cb,
> + void *opaque)
> {
> vout_display_window_t *state = window->owner.sys;
> vout_thread_t *vout = state->vout;
> + struct vout_window_ack_data data = { window, cb, opaque };
>
> msg_Dbg(window, "resized to %ux%u", width, height);
> - vout_ChangeDisplaySize(vout, width, height);
> + vout_ChangeDisplaySize(vout, width, height, vout_window_Ack, &data);
> }
>
> static void vout_display_window_CloseNotify(vout_window_t *window)
> --
> 2.30.0
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list