[vlc-devel] [PATCH 2/5] vout: lock only when needed

[Steve Lhomme]

On 2019-06-03 15:41, Thomas Guillem wrote:
> ---
>   src/video_output/video_output.c | 72 +++++++++++++++++++++------------
>   1 file changed, 47 insertions(+), 25 deletions(-)
> 
> diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
> index d9f91e9e68..0a89f11651 100644
> --- a/src/video_output/video_output.c
> +++ b/src/video_output/video_output.c
> @@ -259,12 +259,14 @@ void vout_PutSubpicture( vout_thread_t *vout, subpicture_t *subpic )
>       vout_thread_sys_t *sys = vout->p;
>       assert(!sys->dummy);
>   
> -    vlc_mutex_lock(&sys->spu_lock);
>       if (sys->spu != NULL)
> +    {
> +        vlc_mutex_lock(&sys->spu_lock);
>           spu_PutSubpicture(sys->spu, subpic);
> +        vlc_mutex_unlock(&sys->spu_lock);
> +    }
>       else
>           subpicture_Delete(subpic);
> -    vlc_mutex_unlock(&sys->spu_lock);
>   }
>   
>   int vout_RegisterSubpictureChannel( vout_thread_t *vout )
> @@ -272,10 +274,12 @@ int vout_RegisterSubpictureChannel( vout_thread_t *vout )
>       assert(!vout->p->dummy);
>       int channel = VOUT_SPU_CHANNEL_AVAIL_FIRST;
>   
> -    vlc_mutex_lock(&vout->p->spu_lock);
>       if (vout->p->spu)
> +    {
> +        vlc_mutex_lock(&vout->p->spu_lock);
>           channel = spu_RegisterChannel(vout->p->spu);
> -    vlc_mutex_unlock(&vout->p->spu_lock);
> +        vlc_mutex_unlock(&vout->p->spu_lock);
> +    }
>   
>       return channel;
>   }
> @@ -283,10 +287,12 @@ int vout_RegisterSubpictureChannel( vout_thread_t *vout )
>   void vout_SetSubpictureClock( vout_thread_t *vout, vlc_clock_t *clock )
>   {
>       assert(!vout->p->dummy);
> -    vlc_mutex_lock(&vout->p->spu_lock);
>       if (vout->p->spu)
> +    {
> +        vlc_mutex_lock(&vout->p->spu_lock);
>           spu_clock_Set(vout->p->spu, clock);
> -    vlc_mutex_unlock(&vout->p->spu_lock);
> +        vlc_mutex_unlock(&vout->p->spu_lock);
> +    }
>   }
>   
>   void vout_FlushSubpictureChannel( vout_thread_t *vout, int channel )
> @@ -294,20 +300,24 @@ void vout_FlushSubpictureChannel( vout_thread_t *vout, int channel )
>       vout_thread_sys_t *sys = vout->p;
>       assert(!sys->dummy);
>   
> -    vlc_mutex_lock(&sys->spu_lock);
>       if (sys->spu != NULL)
> +    {
> +        vlc_mutex_lock(&sys->spu_lock);
>           spu_ClearChannel(vout->p->spu, channel);
> -    vlc_mutex_unlock(&sys->spu_lock);
> +        vlc_mutex_unlock(&sys->spu_lock);
> +    }
>   }
>   
>   void vout_SetSpuHighlight( vout_thread_t *vout,
>                           const vlc_spu_highlight_t *spu_hl )
>   {
>       assert(!vout->p->dummy);
> -    vlc_mutex_lock(&vout->p->spu_lock);
>       if (vout->p->spu)
> +    {
> +        vlc_mutex_lock(&vout->p->spu_lock);
>           spu_SetHighlight(vout->p->spu, spu_hl);
> -    vlc_mutex_unlock(&vout->p->spu_lock);
> +        vlc_mutex_unlock(&vout->p->spu_lock);
> +    }
>   }
>   
>   /**
> @@ -583,19 +593,23 @@ void vout_ControlChangeFilters(vout_thread_t *vout, const char *filters)
>   void vout_ControlChangeSubSources(vout_thread_t *vout, const char *filters)
>   {
>       assert(!vout->p->dummy);
> -    vlc_mutex_lock(&vout->p->spu_lock);
>       if (likely(vout->p->spu != NULL))
> +    {
> +        vlc_mutex_lock(&vout->p->spu_lock);
>           spu_ChangeSources(vout->p->spu, filters);
> -    vlc_mutex_unlock(&vout->p->spu_lock);
> +        vlc_mutex_unlock(&vout->p->spu_lock);
> +    }
>   }
>   
>   void vout_ControlChangeSubFilters(vout_thread_t *vout, const char *filters)
>   {
>       assert(!vout->p->dummy);
> -    vlc_mutex_lock(&vout->p->spu_lock);
>       if (likely(vout->p->spu != NULL))
> +    {
> +        vlc_mutex_lock(&vout->p->spu_lock);
>           spu_ChangeFilters(vout->p->spu, filters);
> -    vlc_mutex_unlock(&vout->p->spu_lock);
> +        vlc_mutex_unlock(&vout->p->spu_lock);
> +    }
>   }
>   
>   void vout_ChangeSubMargin(vout_thread_t *vout, int margin)
> @@ -1070,7 +1084,8 @@ static int ThreadDisplayRenderPicture(vout_thread_t *vout, bool is_forced)
>   
>       video_format_t fmt_spu_rot;
>       video_format_ApplyRotation(&fmt_spu_rot, &fmt_spu);
> -    subpicture_t *subpic = spu_Render(sys->spu,
> +    subpicture_t *subpic = !sys->spu ? NULL
> +                         : spu_Render(sys->spu,
>                                         subpicture_chromas, &fmt_spu_rot,
>                                         &vd->source, system_now,
>                                         render_subtitle_date, sys->spu_rate,
> @@ -1306,13 +1321,13 @@ static void vout_FlushUnlocked(vout_thread_t *vout, bool below,
>       vlc_clock_Reset(vout->p->clock);
>       vlc_clock_SetDelay(vout->p->clock, vout->p->delay);
>   
> -    vlc_mutex_lock(&vout->p->spu_lock);
>       if (vout->p->spu)
>       {
> +        vlc_mutex_lock(&vout->p->spu_lock);
>           spu_clock_Reset(vout->p->spu);
>           spu_clock_SetDelay(vout->p->spu, vout->p->spu_delay);
> +        vlc_mutex_unlock(&vout->p->spu_lock);

Apart from this one that makes 2 calls (non atomic), all the others seem 
to be equivalent to the lock that already exists in the spu. Maybe one 
of them can be removed ?

>       }
> -    vlc_mutex_unlock(&vout->p->spu_lock);
>   }
>   
>   void vout_Flush(vout_thread_t *vout, vlc_tick_t date)
> @@ -1373,19 +1388,24 @@ void vout_ChangeRate(vout_thread_t *vout, float rate)
>   void vout_ChangeSpuDelay(vout_thread_t *vout, vlc_tick_t delay)
>   {
>       assert(!vout->p->dummy);
> -    vlc_mutex_lock(&vout->p->spu_lock);
>       if (vout->p->spu)
> +    {
> +        vlc_mutex_lock(&vout->p->spu_lock);
>           spu_clock_SetDelay(vout->p->spu, delay);
> -    vout->p->spu_delay = delay;
> -    vlc_mutex_unlock(&vout->p->spu_lock);
> +        vout->p->spu_delay = delay;
> +        vlc_mutex_unlock(&vout->p->spu_lock);
> +    }
>   }
>   
>   void vout_ChangeSpuRate(vout_thread_t *vout, float rate)
>   {
>       assert(!vout->p->dummy);
> -    vlc_mutex_lock(&vout->p->spu_lock);
> -    vout->p->spu_rate = rate;
> -    vlc_mutex_unlock(&vout->p->spu_lock);
> +    if (vout->p->spu)
> +    {
> +        vlc_mutex_lock(&vout->p->spu_lock);
> +        vout->p->spu_rate = rate;
> +        vlc_mutex_unlock(&vout->p->spu_lock);
> +    }

This change doesn't seem necessary. If the goal is to protect the 
variable maybe it could be a vlc_atomic_float ?

>   }
>   
>   static void ThreadProcessMouseState(vout_thread_t *vout,
> @@ -1725,10 +1745,12 @@ void vout_Close(vout_thread_t *vout)
>       vout_control_Dead(&sys->control);
>       vout_chrono_Clean(&sys->render);
>   
> -    vlc_mutex_lock(&sys->spu_lock);
>       if (sys->spu)
> +    {
> +        vlc_mutex_lock(&sys->spu_lock);
>           spu_Destroy(sys->spu);
> -    vlc_mutex_unlock(&sys->spu_lock);
> +        vlc_mutex_unlock(&sys->spu_lock);
> +    }
>   
>       vout_Release(vout);
>   }
> -- 
> 2.20.1
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>