[vlc-devel] [PATCH] jack: don't deactivate/activate client in Flush()

[Rémi Denis-Courmont]

Le jeudi 2 mai 2013 16:36:36, Tristan Matthews a écrit :
> Instead, protect the resetting of the ringbuffer with an atomic
> flag, to avoid resetting while its being read by the Process()
> thread. Fixes #8551
> ---
>  modules/audio_output/jack.c | 72
> +++++++++++---------------------------------- 1 file changed, 17
> insertions(+), 55 deletions(-)
> 
> diff --git a/modules/audio_output/jack.c b/modules/audio_output/jack.c
> index 4571394..552cc52 100644
> --- a/modules/audio_output/jack.c
> +++ b/modules/audio_output/jack.c
> @@ -37,6 +37,7 @@
>  #include <vlc_common.h>
>  #include <vlc_plugin.h>
>  #include <vlc_aout.h>
> +#include <vlc_atomic.h>
> 
>  #include <jack/jack.h>
>  #include <jack/ringbuffer.h>
> @@ -61,6 +62,7 @@ struct aout_sys_t
>      float soft_gain;
>      bool soft_mute;
>      mtime_t paused; /**< Time when (last) paused */
> +    vlc_atomic_t reset_safe;
>  };
> 
>  /*************************************************************************
> **** @@ -117,6 +119,7 @@ static int Start( audio_output_t *p_aout,
> audio_sample_format_t *restrict fmt )
> 
>      p_sys->latency = 0;
>      p_sys->paused = VLC_TS_INVALID;
> +    vlc_atomic_set( &p_sys->reset_safe, true );
> 
>      /* Connect to the JACK server */
>      snprintf( psz_name, sizeof(psz_name), "vlc_%d", getpid());
> @@ -175,6 +178,12 @@ static int Start( audio_output_t *p_aout,
> audio_sample_format_t *restrict fmt ) goto error_out;
>      }
> 
> +    if( jack_ringbuffer_mlock( p_sys->p_jack_ringbuffer ))
> +    {
> +        msg_Err( p_aout, "failed to lock JACK ringbuffer in memory" );
> +        status = VLC_EGENERIC;
> +    }
> +

What does that have to do with the rest of the patch?

>      /* Create the output ports */
>      for( i = 0; i < p_sys->i_channels; i++ )
>      {
> @@ -307,8 +316,6 @@ static void Flush(audio_output_t *p_aout, bool wait)
>  {
>      struct aout_sys_t * p_sys = p_aout->sys;
>      jack_ringbuffer_t *rb = p_sys->p_jack_ringbuffer;
> -    jack_client_t *client = p_sys->p_jack_client;
> -    int i_error;
> 
>      /* Sleep if wait was requested */
>      if( wait )
> @@ -318,60 +325,9 @@ static void Flush(audio_output_t *p_aout, bool wait)
>              msleep(delay);
>      }
> 
> -    /* FIXME: maybe the ringbuffer_reset should just be protected by
> signalling -     * with atomic variables?
> -     * Here we save and restore connections, otherwise we'd lose the
> connections -     * every seek because of the deactivate/activate */
> -
> -    /* Save connections */
> -    const char **connections_list[p_sys->i_channels];
> -
> -    for( size_t i = 0; i < p_sys->i_channels; i++ )
> -    {
> -        const char **port_connections = jack_port_get_all_connections(
> client, -                p_sys->p_jack_ports[i] );
> -        connections_list[i] = port_connections;
> -    }
> -
> -    /* Suspend audio processing */
> -    i_error = jack_deactivate( p_sys->p_jack_client );
> -    if( i_error )
> -    {
> -        msg_Err( p_aout, "failed to deactivate JACK client (error %d)",
> -                i_error );
> -        return;
> -    }
> -
>      /* reset ringbuffer read and write pointers */
> -    jack_ringbuffer_reset(rb);
> -
> -    /* Resume audio processing */
> -    i_error = jack_activate( p_sys->p_jack_client );
> -    if( i_error )
> -    {
> -        msg_Err( p_aout, "failed to activate JACK client (error %d)",
> i_error ); -        return;
> -    }
> -
> -    /* Restore connections */
> -    for( size_t i = 0; i < p_sys->i_channels; i++ )
> -    {
> -        const char **port_connections = connections_list[i];
> -        const char *psz_out = jack_port_name( p_sys->p_jack_ports[i] );
> -        for (const char **psz_in = port_connections; psz_in && *psz_in;
> -                psz_in++)
> -        {
> -            i_error = jack_connect( client, psz_out, *psz_in );
> -            if( i_error )
> -            {
> -                msg_Err( p_aout, "failed to connect ports %s and %s (error
> %d)", -                        psz_out, *psz_in, i_error );
> -            }
> -        }
> -
> -        if( port_connections )
> -            jack_free(port_connections);
> -    }
> +    if( vlc_atomic_get( &p_sys->reset_safe ) )
> +        jack_ringbuffer_reset(rb);

This does not look valid to me. I see no proof that reset_safe did not change 
to false after vlc_atomic_get() and before jack_ringbuffer_reset(). Besides, 
the same problem already exists with paused and latency so I gather the JACK 
plugin is not thread-safe (and, AFAIK, never was).

If you do not care about race conditions, then there is no point in using 
<vlc_atomic.h>.

-- 
Rémi Denis-Courmont
http://www.remlab.net/