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

[Rémi Denis-Courmont]

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.

>                  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.

>          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/