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

[Tristan Matthews]

On Thu, May 2, 2013 at 10:22 AM, Rémi Denis-Courmont <remi at remlab.net> wrote:
> 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?

Aside from affecting the ringbuffer, I guess it's not related.
I will send it as a separate 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>.

Ok, will fix and resend.

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



--
Tristan Matthews
web: http://tristanswork.blogspot.com