[vlc-devel] [PATCH 2/2] decoder: fix input_DecoderWait deadlock

[Petri Hintukainen]

On pe, 2015-12-11 at 11:35 +0100, Thomas Guillem wrote:
> I don't feely ready to get rid of input_DecoderWait and rewrite
> es_out, at
> least, not for 3.0.
> 
> With this patch, input_DecoderWait will only lock one mutex, the
> owner one, and
> will be signaled when the decoder is idle and has nothing to decode.
> ---
>  src/input/decoder.c | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/src/input/decoder.c b/src/input/decoder.c
> index f8b4b46..c8ab8d5 100644
> --- a/src/input/decoder.c
> +++ b/src/input/decoder.c
> @@ -106,6 +106,8 @@ struct decoder_owner_sys_t
>  
>      /* Waiting */
>      bool b_waiting;
> +    /* atomic in order to avoid a double lock from lock and fifo */
> +    atomic_bool wait_abort;
>      bool b_first;
>      bool b_has_data;
>  
> @@ -113,7 +115,6 @@ struct decoder_owner_sys_t
>      bool flushing;
>      bool b_draining;
>      atomic_bool drained;
> -    bool b_idle;
>  
>      /* CC */
>      struct
> @@ -1484,10 +1485,19 @@ static void *DecoderThread( void *p_data )
>          if( p_block == NULL )
>          {
>              if( likely(!p_owner->b_draining) )
> -            {   /* Wait for a block to decode (or a request to
> drain) */
> -                p_owner->b_idle = true;
> +            {
> +                /* Wake up and abort input_DecoderWait */
> +                atomic_store( &p_owner->wait_abort, true );
> +                vlc_fifo_Unlock( p_owner->p_fifo );
> +
> +                vlc_mutex_lock( &p_owner->lock );
> +                vlc_cond_signal( &p_owner->wait_acknowledge );
> +                vlc_mutex_unlock( &p_owner->lock );
> +
> +                vlc_fifo_Lock( p_owner->p_fifo );
> +
> +                /* Wait for a block to decode (or a request to
> drain) */

fifo was unlocked after vlc_fifo_DequeueUnlocked(), so we don't know if
it is empty anymore.

>                  vlc_fifo_Wait( p_owner->p_fifo );
> -                p_owner->b_idle = false;
>                  continue;
>              }
>              /* We have emptied the FIFO and there is a pending
> request to
> @@ -1570,13 +1580,13 @@ static decoder_t * CreateDecoder(
> vlc_object_t *p_parent,
>      p_owner->frames_countdown = 0;
>  
>      p_owner->b_waiting = false;
> +    atomic_init( &p_owner->wait_abort, false );
>      p_owner->b_first = true;
>      p_owner->b_has_data = false;
>  
>      p_owner->flushing = false;
>      p_owner->b_draining = false;
>      atomic_init( &p_owner->drained, false );
> -    p_owner->b_idle = false;
>  
>      es_format_Init( &p_owner->fmt, UNKNOWN_ES, 0 );
>  
> @@ -1925,6 +1935,7 @@ void input_DecoderDecode( decoder_t *p_dec,
> block_t *p_block, bool b_do_pace )
>              vlc_fifo_WaitCond( p_owner->p_fifo, &p_owner->wait_fifo
> );
>      }
>  
> +    atomic_store( &p_owner->wait_abort, false );
>      vlc_fifo_QueueUnlocked( p_owner->p_fifo, p_block );
>      vlc_fifo_Unlock( p_owner->p_fifo );
>  }
> @@ -2129,6 +2140,7 @@ void input_DecoderStopWait( decoder_t *p_dec )
>  
>      vlc_mutex_lock( &p_owner->lock );
>      p_owner->b_waiting = false;
> +    atomic_store( &p_owner->wait_abort, false );
>      vlc_cond_signal( &p_owner->wait_request );
>      vlc_mutex_unlock( &p_owner->lock );
>  }
> @@ -2140,22 +2152,13 @@ void input_DecoderWait( decoder_t *p_dec )
>      assert( p_owner->b_waiting );
>  
>      vlc_mutex_lock( &p_owner->lock );
> -    while( !p_owner->b_has_data )
> -    {
> -        /* Don't need to lock p_owner->paused since it's only
> modified by the
> -         * owner */
> -        if( p_owner->paused )
> -            break;
> -        vlc_fifo_Lock( p_owner->p_fifo );
> -        if( p_owner->b_idle && vlc_fifo_IsEmpty( p_owner->p_fifo ) )
> -        {
> -            msg_Err( p_dec, "buffer deadlock prevented" );
> -            vlc_fifo_Unlock( p_owner->p_fifo );
> -            break;
> -        }
> -        vlc_fifo_Unlock( p_owner->p_fifo );
> +
> +    /* Wait until decoder has data, decoder is paused or wait is
> aborted.
> +     * Don't need to lock p_owner->paused with fifo lock since it's
> only
> +     * modified by the owner */
> +    while( !p_owner->b_has_data && !p_owner->paused
> +        && !atomic_load( &p_owner->wait_abort ) )
>          vlc_cond_wait( &p_owner->wait_acknowledge, &p_owner->lock );
> -    }
>      vlc_mutex_unlock( &p_owner->lock );
>  }
>