[vlc-devel] [PATCH 1/2] vout: add a resize acknowledgement callback

Steve Lhomme robux4 at ycbcr.xyz
Wed Feb 10 06:38:11 UTC 2021


On 2021-02-09 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);

Missing some documentation of the new parameter. In particular when it 
will be called.

>   
>       /**
>        * 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);
>   }

IMO this function should always expose the new parameter or have a new 
variant with the extra params. Just calling the callback manually (as 
done in 2/2) defeats the purpose of having a helper in the first place.

>   /**
> 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)

There's no guarantee this callback signature will match 
vout_window_ack_cb if it changes. There should be a cast to that type.

>   {
>       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);

Using a local stack structure forces vout_ChangeDisplaySize() to be 
synchronous. Which is kind of contradictory to using a callback that 
look asynchronous (but is not).

To paraphrase your commit log, that is "leaking the internal 
implementation".

>   }
>   
>   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