[vlc-devel] jack output: patch for #8126
Rémi Denis-Courmont
remi at remlab.net
Wed Apr 10 17:00:24 CEST 2013
Hello,
Looks quite nice overall. Comments inline...
> diff --git a/modules/audio_output/Modules.am
> b/modules/audio_output/Modules.am
> index 38e9794..eb0bdc6 100644
> --- a/modules/audio_output/Modules.am
> +++ b/modules/audio_output/Modules.am
> @@ -53,7 +53,7 @@ if HAVE_PULSE
> libvlc_LTLIBRARIES += libpulse_plugin.la
> endif
>
> -libjack_plugin_la_SOURCES = jack.c packet.c volume.h
> +libjack_plugin_la_SOURCES = jack.c volume.h
> libjack_plugin_la_CFLAGS = $(AM_CFLAGS) $(JACK_CFLAGS)
> libjack_plugin_la_LIBADD = $(AM_LIBADD) $(JACK_LIBS) $(LIBM)
> EXTRA_LTLIBRARIES += libjack_plugin.la
> diff --git a/modules/audio_output/jack.c b/modules/audio_output/jack.c
> index 22200d5..39e64df 100644
> --- a/modules/audio_output/jack.c
> +++ b/modules/audio_output/jack.c
> @@ -39,6 +39,7 @@
> #include <vlc_aout.h>
>
> #include <jack/jack.h>
> +#include <jack/ringbuffer.h>
>
> typedef jack_default_audio_sample_t jack_sample_t;
>
> @@ -50,14 +51,17 @@ typedef jack_default_audio_sample_t jack_sample_t;
>
>
*****************************************************************************/
> struct aout_sys_t
> {
> - aout_packet_t packet;
> + jack_ringbuffer_t *p_jack_ringbuffer;
> jack_client_t *p_jack_client;
> jack_port_t **p_jack_ports;
> jack_sample_t **p_jack_buffers;
> unsigned int i_channels;
> + int i_rate;
unsigned?
> jack_nframes_t latency;
> float soft_gain;
> bool soft_mute;
> + mtime_t paused; /**< Time when (last) paused */
> + size_t bytes_per_frame;
size_t is OK but a tad overkill.
> };
>
>
>
/*****************************************************************************
> @@ -65,6 +69,10 @@ struct aout_sys_t
>
>
*****************************************************************************/
> static int Open ( vlc_object_t * );
> static void Close ( vlc_object_t * );
> +static void Play ( audio_output_t * p_aout, block_t * p_block
);
> +static void Pause ( audio_output_t *aout, bool paused, mtime_t
> date );
> +static void Flush ( audio_output_t *p_aout, bool wait );
> +static int TimeGet ( audio_output_t *, mtime_t * );
> static int Process ( jack_nframes_t i_frames, void *p_arg );
> static int GraphChange ( void *p_arg );
>
> @@ -109,6 +117,7 @@ static int Start( audio_output_t *p_aout,
> audio_sample_format_t *restrict fmt )
> int i_error;
>
> p_sys->latency = 0;
> + p_sys->paused = VLC_TS_INVALID;
>
> /* Connect to the JACK server */
> snprintf( psz_name, sizeof(psz_name), "vlc_%d", getpid());
> @@ -130,17 +139,16 @@ static int Start( audio_output_t *p_aout,
> audio_sample_format_t *restrict fmt )
> /* JACK only supports fl32 format */
> fmt->i_format = VLC_CODEC_FL32;
> // TODO add buffer size callback
> - fmt->i_rate = jack_get_sample_rate( p_sys->p_jack_client );
> -
> - p_aout->time_get = aout_PacketTimeGet;
> - p_aout->play = aout_PacketPlay;
> - p_aout->pause = NULL;
> - p_aout->flush = aout_PacketFlush;
> - aout_PacketInit( p_aout, &p_sys->packet,
> - jack_get_buffer_size( p_sys->p_jack_client ), fmt
);
> + p_sys->i_rate = fmt->i_rate = jack_get_sample_rate(
> p_sys->p_jack_client );
> +
> + p_aout->play = Play;
> + p_aout->pause = Pause;
> + p_aout->flush = Flush;
> + p_aout->time_get = TimeGet;
> aout_SoftVolumeStart( p_aout );
>
> p_sys->i_channels = aout_FormatNbChannels( fmt );
> + aout_FormatPrepare(fmt);
>
> p_sys->p_jack_ports = malloc( p_sys->i_channels *
> sizeof(jack_port_t *) );
> @@ -158,6 +166,16 @@ static int Start( audio_output_t *p_aout,
> audio_sample_format_t *restrict fmt )
> goto error_out;
> }
>
> + p_sys->bytes_per_frame = fmt->i_bytes_per_frame;
> + const size_t buf_sz = (AOUT_MAX_ADVANCE_TIME / CLOCK_FREQ *
> fmt->i_rate * fmt->i_bytes_per_frame);
This seems to assume that AOUT_MAX_ADVANCE_TIME is a multiple of
CLOCK_FREQ. AOUT_MAX_ADVANCE_TIME * fmt->i_rate * fmt->i_bytes_per_frame /
CLOCK_FREQ is likely better.
Also VLC code is normally wrapped to 79 columns.
> + p_sys->p_jack_ringbuffer = jack_ringbuffer_create( buf_sz );
> +
> + if( p_sys->p_jack_ringbuffer == NULL )
> + {
> + status = VLC_ENOMEM;
> + goto error_out;
> + }
> +
> /* Create the output ports */
> for( i = 0; i < p_sys->i_channels; i++ )
> {
> @@ -231,39 +249,151 @@ error_out:
> {
> jack_deactivate( p_sys->p_jack_client );
> jack_client_close( p_sys->p_jack_client );
> - aout_PacketDestroy( p_aout );
> }
> + if( p_sys->p_jack_ringbuffer)
> + jack_ringbuffer_free( p_sys->p_jack_ringbuffer );
> +
> free( p_sys->p_jack_ports );
> free( p_sys->p_jack_buffers );
> }
> return status;
> }
>
> +static void Play (audio_output_t * p_aout, block_t * p_block)
> +{
> + struct aout_sys_t *p_sys = p_aout->sys;
> +
> + while (p_block->i_buffer > 0) {
> +
> + /* move data to buffer */
> + const size_t bytes = __MIN(
> jack_ringbuffer_write_space(p_sys->p_jack_ringbuffer), p_block->i_buffer
);
The __MIN macro is not expansion-safe, so this is probably wrong.
> + if (unlikely(jack_ringbuffer_write( p_sys->p_jack_ringbuffer,
> (const char *) p_block->p_buffer, bytes ) < bytes)) {
> + msg_Warn(p_aout, "Some audio was dropped");
You should try to keep message style consistent across the plugin.
I do not understand how this loop won't always fail if it ever iterates
more than once.
> + break;
> + }
> +
> + p_block->i_nb_samples -= bytes / sizeof(jack_sample_t);
> + p_block->p_buffer += bytes;
> + p_block->i_buffer = p_block->i_buffer < bytes ? 0 :
> p_block->i_buffer - bytes;
How can this possibly overflow?
> + }
> +
> + block_Release(p_block);
> +}
> +
> +/**
> + * Pause or unpause playback
> + */
> +static void Pause(audio_output_t *aout, bool paused, mtime_t date)
> +{
> + aout_sys_t *sys = aout->sys;
> +
> + if (paused) {
> + sys->paused = date;
> + } else {
> + date -= sys->paused;
> + msg_Dbg(aout, "resuming after %"PRId64" us", date);
> + sys->paused = VLC_TS_INVALID;
> + }
> +}
> +
> +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;
> + int i_error;
> +
> + /* Sleep until the ring buffer has been emptied */
> + if( wait )
> + {
> + const mtime_t wait_time =
> jack_frames_to_time(p_sys->p_jack_client,
> jack_get_buffer_size(p_sys->p_jack_client));
> + while( jack_ringbuffer_write_space( rb ) != rb->size )
> + msleep(wait_time);
wait_time is probably wrong from the second iteration.
> + }
> +
> + /* FIXME: maybe the ringbuffer_reset should just be protected by
> signalling with atomic variables? */
> +
> + /* Save connections */
> + const char ***connections_list = malloc( p_sys->i_channels *
> sizeof(const char **) );
The maximum channels count is small enough that you can allocate on the
stack. If your religion forbids variable-sized arrays, there is
AOUT_CHAN_MAX or something.
> + if( connections_list == NULL )
> + {
> + msg_Err( p_aout, "Out of memory" );
> + return;
> + }
> +
> + for(size_t i = 0; i < p_sys->i_channels; i++ )
> + {
> + const char **port_connections = jack_port_get_all_connections(
> p_sys->p_jack_client, p_sys->p_jack_ports[i] );
> + connections_list[i] = port_connections;
> + }
> +
> + /* Suspend audio processing */
> + i_error = jack_deactivate( p_sys->p_jack_client );
I don't know how JACK works. From the name, this function looks like it
belongs in Stop().
> + 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 );
Does it make sense before Start()?
> + if( i_error )
> + {
> + msg_Err( p_aout, "failed to activate JACK client (error %d)",
> i_error );
> + return;
> + }
> +
> + /* Restore connections */
Are you trying to preserve connections across audio streams? This makes
sense; in fact, it is another known bug. This would require a rehaul of
Open() vs Start() and Close() vs Open(). Maybe this whole Flush() blob
should be postponed to a separate patch...
> + 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( p_sys->p_jack_client, psz_out,
> *psz_in );
> + if( i_error )
> + {
> + msg_Err( p_aout, "failed to connect port %s to port %s
> (error %d)",
> + psz_out, *psz_in, i_error );
> + }
> + }
> +
> + if (port_connections)
> + jack_free(port_connections);
> + }
> +
> + free(connections_list);
> +}
> +
> +static int TimeGet(audio_output_t *p_aout, mtime_t *delay)
> +{
> + struct aout_sys_t * p_sys = p_aout->sys;
> + jack_ringbuffer_t *rb = p_sys->p_jack_ringbuffer;
> + if( p_sys->bytes_per_frame == 0 )
> + return -1;
> +
> + *delay = (p_sys->latency + (jack_ringbuffer_read_space(rb) /
> p_sys->bytes_per_frame)) * CLOCK_FREQ / p_sys->i_rate;
> +
> + return 0;
> +}
>
>
>
/*****************************************************************************
> * Process: callback for JACK
>
>
*****************************************************************************/
> int Process( jack_nframes_t i_frames, void *p_arg )
> {
> - unsigned int i, j, i_nb_samples = 0;
> + unsigned int i, j, frames_from_rb = 0;
> + size_t bytes_read = 0;
> + size_t frames_read;
> audio_output_t *p_aout = (audio_output_t*) p_arg;
> struct aout_sys_t *p_sys = p_aout->sys;
> - jack_sample_t *p_src = NULL;
>
> - jack_nframes_t dframes = p_sys->latency
> - - jack_frames_since_cycle_start(
> p_sys->p_jack_client );
> + /* Get the next audio data buffer unless paused */
>
> - jack_time_t dtime = dframes * 1000 * 1000 / jack_get_sample_rate(
> p_sys->p_jack_client );
> - mtime_t play_date = mdate() + (mtime_t) ( dtime );
> -
> - /* Get the next audio data buffer */
> - block_t *p_buffer = aout_PacketNext( p_aout, play_date );
> -
> - if( p_buffer != NULL )
> - {
> - p_src = (jack_sample_t *)p_buffer->p_buffer;
> - i_nb_samples = p_buffer->i_nb_samples;
> - }
> + if( p_sys->paused == VLC_TS_INVALID)
> + frames_from_rb = i_frames;
>
> /* Get the JACK buffers to write to */
> for( i = 0; i < p_sys->i_channels; i++ )
> @@ -273,28 +403,27 @@ int Process( jack_nframes_t i_frames, void *p_arg
)
> }
>
> /* Copy in the audio data */
> - for( j = 0; j < i_nb_samples; j++ )
> + for( j = 0; j < frames_from_rb; j++ )
> {
> for( i = 0; i < p_sys->i_channels; i++ )
> {
> - jack_sample_t *p_dst = p_sys->p_jack_buffers[i];
> - p_dst[j] = *p_src;
> - p_src++;
> + jack_sample_t *p_dst = p_sys->p_jack_buffers[i] + j;
> + bytes_read += jack_ringbuffer_read(
p_sys->p_jack_ringbuffer,
> + (char *) p_dst, sizeof(jack_sample_t) );
> }
> }
>
> /* Fill any remaining buffer with silence */
> - if( i_nb_samples < i_frames )
> + frames_read = (bytes_read / sizeof(jack_sample_t)) /
> p_sys->i_channels;
> + if( frames_read < i_frames )
> {
> for( i = 0; i < p_sys->i_channels; i++ )
> {
> - memset( p_sys->p_jack_buffers[i] + i_nb_samples, 0,
> - sizeof( jack_sample_t ) * (i_frames - i_nb_samples)
> );
> + memset( p_sys->p_jack_buffers[i] + frames_read, 0,
> + sizeof( jack_sample_t ) * (i_frames - frames_read)
);
> }
> }
>
> - if( p_buffer )
> - block_Release( p_buffer );
> return 0;
> }
>
> @@ -308,15 +437,15 @@ static int GraphChange( void *p_arg )
> audio_output_t *p_aout = (audio_output_t*) p_arg;
> struct aout_sys_t *p_sys = p_aout->sys;
> unsigned int i;
> - jack_nframes_t port_latency;
> + jack_latency_range_t port_latency;
>
> p_sys->latency = 0;
>
> for( i = 0; i < p_sys->i_channels; ++i )
> {
> - port_latency = jack_port_get_total_latency( p_sys->p_jack_client,
> -
p_sys->p_jack_ports[i]
> );
> - p_sys->latency = __MAX( p_sys->latency, port_latency );
> + jack_port_get_latency_range( p_sys->p_jack_ports[i],
> JackPlaybackLatency,
> + &port_latency );
> + p_sys->latency = __MAX( p_sys->latency, port_latency.max );
> }
>
> msg_Dbg(p_aout, "JACK graph reordered. Our maximum latency=%d.",
> p_sys->latency);
> @@ -345,7 +474,8 @@ static void Stop( audio_output_t *p_aout )
> }
> free( p_sys->p_jack_ports );
> free( p_sys->p_jack_buffers );
> - aout_PacketDestroy( p_aout );
> + if( p_sys->p_jack_ringbuffer )
Isn't this a tautology?
> + jack_ringbuffer_free( p_sys->p_jack_ringbuffer );
> }
>
> static int Open(vlc_object_t *obj)
> diff --git a/modules/audio_output/packet.c
b/modules/audio_output/packet.c
> deleted file mode 100644
> index 48f7491..0000000
I would recommend deleting packet.c in a separate patch, because this is
not JACK-specific. Furthermore, the corresponding bits in
include/vlc_aout.h need to removed as well.
--
Rémi Denis-Courmont
Sent from my collocated server
More information about the vlc-devel
mailing list