[vlc-devel] [PATCH 4/4] avcodec: fix deadlock when closing with mt

[Thomas Guillem]

On Tue, Nov 17, 2015, at 20:03, Rémi Denis-Courmont wrote:
> Le 2015-11-17 21:34, Thomas Guillem a écrit :
> > lavc_GetFrame can be stuck in decoder_GetPicture when decoder is 
> > closing.
> > ---
> >  modules/codec/avcodec/video.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/modules/codec/avcodec/video.c 
> > b/modules/codec/avcodec/video.c
> > index 69d6f6e..8ebc21c 100644
> > --- a/modules/codec/avcodec/video.c
> > +++ b/modules/codec/avcodec/video.c
> > @@ -74,6 +74,7 @@ struct decoder_sys_t
> >
> >      /* */
> >      bool b_flush;
> > +    atomic_bool flushing_buffers;
> >
> >      /* VA API */
> >      vlc_va_t *p_va;
> > @@ -458,6 +459,7 @@ int InitVideoDec( decoder_t *p_dec,
> > AVCodecContext *p_context,
> >      p_sys->b_first_frame = true;
> >      p_sys->b_flush = false;
> >      p_sys->i_late_frames = 0;
> > +    atomic_init( &p_sys->flushing_buffers, false );
> >
> >      /* Set output properties */
> >      p_dec->fmt_out.i_cat = VIDEO_ES;
> > @@ -536,7 +538,11 @@ static picture_t *DecodeVideo( decoder_t *p_dec,
> > block_t **pp_block )
> >              /* NOTE: data is good only the timeline changed so do
> > not flush decoder */
> >              post_mt( p_sys );
> >              if( p_block->i_flags & BLOCK_FLAG_DISCONTINUITY )
> > +            {
> > +                atomic_store( &p_sys->flushing_buffers, true );
> 
> This assumes that libavcodec won't need new buffers while flushing. I 
> don't know if that is currently the case, and in any case, that seems 
> like an implementation detail of libavcodec (threaded) decoders that we 
> cannot rely on.

Before DecoderIsFlushing was removed, we relied on it. Indeed,
lavc_GetFrame returned -1 when Decoder was flushing (via
decoder_NewPicture that returned NULL).

> 
> >                  avcodec_flush_buffers( p_context );
> > +                atomic_store( &p_sys->flushing_buffers, false );
> > +            }
> >              wait_mt( p_sys );
> >  #endif
> >              if( p_block->i_flags & BLOCK_FLAG_CORRUPTED )
> > @@ -858,7 +864,10 @@ void EndVideoDec( decoder_t *p_dec )
> >
> >      /* do not flush buffers if codec hasn't been opened
> > (theora/vorbis/VC1) */
> >      if( p_sys->p_context->codec )
> > +    {
> > +        atomic_store( &p_sys->flushing_buffers, true );
> 
> This is racy.

Indeed, lavc_GetFrame can already be stuck when setting flushing_buffers
to true.

We need to find a way to unblock lavc_GetFrame when flushing or closing.

> 
> >          avcodec_flush_buffers( p_sys->p_context );
> > +    }
> >
> >      wait_mt( p_sys );
> >
> > @@ -1065,6 +1074,12 @@ static int lavc_GetFrame(struct AVCodecContext
> > *ctx, AVFrame *frame, int flags)
> >      frame->opaque = NULL;
> >
> >      wait_mt(sys);
> > +    if (atomic_load(&sys->flushing_buffers))
> > +    {
> > +        /* If decoder is flushing buffers, abort current
> > lavc_GetFrame call */
> > +        post_mt(sys);
> > +        return -1;
> > +    }
> >      if (sys->p_va == NULL)
> >      {
> >          if (!sys->b_direct_rendering)
> 
> -- 
> Rémi Denis-Courmont
> http://www.remlab.net/
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel