[vlc-devel] [PATCH 2/3] decoder: don't control the vout when not started

[Thomas Guillem]

On Thu, Dec 17, 2020, at 07:57, Steve Lhomme wrote:
> On 2020-12-16 18:34, Thomas Guillem wrote:
> > Calling vout functions that need to Hold the vout_control will deadlock
> > if the vout is not yet started.
> > ---
> >   src/input/decoder.c | 26 ++++++++++++++++++++------
> >   1 file changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/input/decoder.c b/src/input/decoder.c
> > index b9cdc7d4a81..488998fc35e 100644
> > --- a/src/input/decoder.c
> > +++ b/src/input/decoder.c
> > @@ -128,6 +128,7 @@ struct vlc_input_decoder_t
> >       audio_output_t *p_aout;
> >   
> >       vout_thread_t   *p_vout;
> > +    bool             vout_started;
> >       enum vlc_vout_order vout_order;
> >   
> >       /* -- Theses variables need locking on read *and* write -- */
> > @@ -476,12 +477,20 @@ static int ModuleThread_UpdateVideoFormat( decoder_t *p_dec, vlc_video_context *
> >                                      &has_started);
> >       if (p_vout != NULL)
> >       {
> > +        vlc_mutex_lock( &p_owner->lock );
> > +        p_owner->vout_started = true;
> > +        vlc_mutex_unlock( &p_owner->lock );
> 
> It seems that it should change with the has_started below. If it was 
> started there's no need to change it.

Naup, if the vout is recycled (2 medias played one after the other, has_started will always be false with the second media, and yet the vout will be started. That's the difference between "has_started" and "started".

> 
> Same thing when set to false. It should also match the calls to 
> on_vout_stopped.
> 
> > +
> >           if (has_started)
> >               decoder_Notify(p_owner, on_vout_started, p_vout, p_owner->vout_order);
> >           return 0;
> >       }
> >   
> >   error:
> > +    vlc_mutex_lock( &p_owner->lock );
> > +    p_owner->vout_started = false;
> > +    vlc_mutex_unlock( &p_owner->lock );
> > +
> >       if (p_owner->vctx != NULL)
> >       {
> >           vlc_video_context_Release(p_owner->vctx);
> > @@ -1023,6 +1032,8 @@ static int ModuleThread_PlayVideo( vlc_input_decoder_t *p_owner, picture_t *p_pi
> >       }
> >   
> >       vlc_mutex_lock( &p_owner->lock );
> > +    assert( p_owner->vout_started );
> > +
> >       bool prerolled = p_owner->i_preroll_end != PREROLL_NONE;
> >       if( prerolled && p_owner->i_preroll_end > p_picture->date )
> >       {
> > @@ -1474,7 +1485,7 @@ static void DecoderThread_Flush( vlc_input_decoder_t *p_owner )
> >       }
> >       else if( p_dec->fmt_out.i_cat == VIDEO_ES )
> >       {
> > -        if( p_owner->p_vout )
> > +        if( p_owner->p_vout && p_owner->vout_started )
> >               vout_FlushAll( p_owner->p_vout );
> >   
> >           /* Reset the pool cancel state, previously set by
> > @@ -1504,7 +1515,7 @@ static void DecoderThread_ChangePause( vlc_input_decoder_t *p_owner, bool paused
> >       {
> >           case VIDEO_ES:
> >               vlc_mutex_lock( &p_owner->lock );
> > -            if( p_owner->p_vout != NULL )
> > +            if( p_owner->p_vout != NULL && p_owner->vout_started )
> >                   vout_ChangePause( p_owner->p_vout, paused, date );
> >               vlc_mutex_unlock( &p_owner->lock );
> >               break;
> > @@ -1530,7 +1541,7 @@ static void DecoderThread_ChangeRate( vlc_input_decoder_t *p_owner, float rate )
> >       switch( p_dec->fmt_out.i_cat )
> >       {
> >           case VIDEO_ES:
> > -            if( p_owner->p_vout != NULL )
> > +            if( p_owner->p_vout != NULL && p_owner->vout_started )
> >                   vout_ChangeRate( p_owner->p_vout, rate );
> >               break;
> >           case AUDIO_ES:
> > @@ -1562,7 +1573,7 @@ static void DecoderThread_ChangeDelay( vlc_input_decoder_t *p_owner, vlc_tick_t
> >       {
> >           case VIDEO_ES:
> >               vlc_mutex_lock( &p_owner->lock );
> > -            if( p_owner->p_vout != NULL )
> > +            if( p_owner->p_vout != NULL && p_owner->vout_started )
> >                   vout_ChangeDelay( p_owner->p_vout, delay );
> >               vlc_mutex_unlock( &p_owner->lock );
> >               break;
> > @@ -1792,6 +1803,7 @@ CreateDecoder( vlc_object_t *p_parent,
> >       p_owner->cbs_userdata = cbs_userdata;
> >       p_owner->p_aout = NULL;
> >       p_owner->p_vout = NULL;
> > +    p_owner->vout_started = false;
> >       p_owner->i_spu_channel = VOUT_SPU_CHANNEL_INVALID;
> >       p_owner->i_spu_order = 0;
> >       p_owner->p_sout = p_sout;
> > @@ -2151,7 +2163,8 @@ void vlc_input_decoder_Delete( vlc_input_decoder_t *p_owner )
> >        *
> >        * This unblocks the thread, allowing the decoder module to join all its
> >        * worker threads (if any) and the decoder thread to terminate. */
> > -    if( p_dec->fmt_in.i_cat == VIDEO_ES && p_owner->p_vout != NULL )
> > +    if( p_dec->fmt_in.i_cat == VIDEO_ES && p_owner->p_vout != NULL
> > +     && p_owner->vout_started )
> >       {
> >           if (p_owner->out_pool)
> >               picture_pool_Cancel( p_owner->out_pool, true );
> > @@ -2324,7 +2337,8 @@ void vlc_input_decoder_Flush( vlc_input_decoder_t *p_owner )
> >            * after being unstuck. */
> >   
> >           vlc_mutex_lock( &p_owner->lock );
> > -        if( p_owner->dec.fmt_out.i_cat == VIDEO_ES && p_owner->p_vout )
> > +        if( p_owner->dec.fmt_out.i_cat == VIDEO_ES && p_owner->p_vout
> > +         && p_owner->vout_started )
> >               vout_FlushAll( p_owner->p_vout );
> >           vlc_mutex_unlock( &p_owner->lock );
> >       }
> > -- 
> > 2.29.2
> > 
> > _______________________________________________
> > 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