[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