[vlc-devel] [PATCH 2/5] video_output: move the dead (don't push) control handling in video_output

[Alexandre Janniaux]

Hi,

As Rémi mentioned, it introduces a race where the pusher
thread would check if it's dead and determine that it's
not, enter the condition while the control thread is setting
is_dead to true, and push a control with the is_dead=true
state.

I'd prefer that you describe this new race case and explain
why it's still legitimate if it is. As Rémi highlighted, it's
always ringing a bell when you replace a mutex by an atomic,
especially if you don't use exchange variants to signal an
atomic change of state.

> Only the code that pushes code need to know it's not supposed to push anything
> anymore. So far it was under a lock of the control thread. We use an atomic

I'm also not sure of what this means, if only the code that
pushes code need to know it's not supposed to push, then
there's no need for locks or atomics, so it's probably a bit
confusing.

Regards,
--
Alexandre Janniaux
Videolabs

On Thu, Dec 17, 2020 at 04:39:16PM +0100, Steve Lhomme wrote:
> Only the code that pushes code need to know it's not supposed to push anything
> anymore. So far it was under a lock of the control thread. We use an atomic
> boolean instead.
> ---
>  src/video_output/control.c      | 14 ++------------
>  src/video_output/control.h      |  2 --
>  src/video_output/video_output.c | 10 +++++++---
>  3 files changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/src/video_output/control.c b/src/video_output/control.c
> index fa5210bba9f..b22d0a03f66 100644
> --- a/src/video_output/control.c
> +++ b/src/video_output/control.c
> @@ -38,7 +38,6 @@ void vout_control_Init(vout_control_t *ctrl)
>
>      ctrl->is_held = false;
>      ctrl->is_waiting = false;
> -    ctrl->is_dead = false;
>      ctrl->can_sleep = true;
>      ARRAY_INIT(ctrl->cmd);
>  }
> @@ -49,20 +48,11 @@ void vout_control_Clean(vout_control_t *ctrl)
>      ARRAY_RESET(ctrl->cmd);
>  }
>
> -void vout_control_Dead(vout_control_t *ctrl)
> -{
> -    vlc_mutex_lock(&ctrl->lock);
> -    ctrl->is_dead = true;
> -    vlc_mutex_unlock(&ctrl->lock);
> -}
> -
>  static void vout_control_Push(vout_control_t *ctrl, vout_control_cmd_t *cmd)
>  {
>      vlc_mutex_lock(&ctrl->lock);
> -    if (!ctrl->is_dead) {
> -        ARRAY_APPEND(ctrl->cmd, *cmd);
> -        vlc_cond_signal(&ctrl->wait_request);
> -    }
> +    ARRAY_APPEND(ctrl->cmd, *cmd);
> +    vlc_cond_signal(&ctrl->wait_request);
>      vlc_mutex_unlock(&ctrl->lock);
>  }
>
> diff --git a/src/video_output/control.h b/src/video_output/control.h
> index 644a849b3e5..bb0ba999ea1 100644
> --- a/src/video_output/control.h
> +++ b/src/video_output/control.h
> @@ -43,7 +43,6 @@ typedef struct {
>      vlc_cond_t  wait_available;
>
>      /* */
> -    bool is_dead;
>      bool can_sleep;
>      bool is_waiting;
>      bool is_held;
> @@ -65,6 +64,5 @@ void vout_control_Release(vout_control_t *);
>
>  /* control inside of the vout thread */
>  int vout_control_Pop(vout_control_t *, vout_control_cmd_t *, vlc_tick_t deadline);
> -void vout_control_Dead(vout_control_t *);
>
>  #endif
> diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
> index 29b8b140e09..f6afd17681d 100644
> --- a/src/video_output/video_output.c
> +++ b/src/video_output/video_output.c
> @@ -118,6 +118,7 @@ typedef struct vout_thread_sys_t
>
>      /* Thread & synchronization */
>      vout_control_t  control;
> +    atomic_bool     control_dead;
>      vlc_thread_t    thread;
>
>      struct {
> @@ -392,7 +393,8 @@ void vout_MouseState(vout_thread_t *vout, const vlc_mouse_t *mouse)
>          video_mouse = *mouse;
>      vlc_mutex_unlock(&sys->display_lock);
>
> -    vout_control_PushMouse(&sys->control, &video_mouse);
> +    if (!atomic_load(&sys->control_dead))
> +        vout_control_PushMouse(&sys->control, &video_mouse);
>  }
>
>  void vout_PutSubpicture( vout_thread_t *vout, subpicture_t *subpic )
> @@ -1971,7 +1973,8 @@ void vout_StopDisplay(vout_thread_t *vout)
>  {
>      vout_thread_sys_t *sys = VOUT_THREAD_TO_SYS(vout);
>
> -    vout_control_PushTerminate(&sys->control);
> +    if (!atomic_load(&sys->control_dead))
> +        vout_control_PushTerminate(&sys->control);
>      vlc_join(sys->thread, NULL);
>
>      vout_ReleaseDisplay(sys);
> @@ -2010,7 +2013,7 @@ void vout_Close(vout_thread_t *vout)
>
>      vout_IntfDeinit(VLC_OBJECT(vout));
>      vout_snapshot_End(sys->snapshot);
> -    vout_control_Dead(&sys->control);
> +    atomic_store(&sys->control_dead, true);
>      vout_chrono_Clean(&sys->render);
>
>      if (sys->spu)
> @@ -2117,6 +2120,7 @@ vout_thread_t *vout_Create(vlc_object_t *object)
>                 spu_Create(vout, vout) : NULL;
>
>      vout_control_Init(&sys->control);
> +    atomic_init(&sys->control_dead, false);
>
>      sys->title.show     = var_InheritBool(vout, "video-title-show");
>      sys->title.timeout  = var_InheritInteger(vout, "video-title-timeout");
> --
> 2.29.2
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel