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

Thomas Guillem thomas at gllm.fr
Mon Jun 3 15:59:57 CEST 2019


On Mon, Jun 3, 2019, at 15:57, Steve Lhomme wrote:
> 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 ?

maybe

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

No, rate and delay should go into the spu_t. But First I need to handle that for each channels. cf. Roland patches about dual subtitles.

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