[vlc-devel] jack output: patch for #8126
Tristan Matthews
le.businessman at gmail.com
Wed Apr 10 17:42:15 CEST 2013
Thanks for the feedback, some questions inline:
2013/4/10 Rémi Denis-Courmont <remi at remlab.net>
> 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?
>
sure.
>
> > 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.
>
Should I use unsigned then?
>
> > };
> >
> >
> >
>
> /*****************************************************************************
> > @@ -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.
>
Ok will change that, test and confirm.
>
> Also VLC code is normally wrapped to 79 columns.
>
Sounds good. I take it there is no indent script or anything aside from the
VLC Coding guidelines?
>
> > + 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.
>
Ok.
>
> > + 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.
>
Ok.
>
> I do not understand how this loop won't always fail if it ever iterates
> more than once.
>
jack_ringbuffer_write_space is called just before to see how much space
the ringbuffer has. It is possible that we can't write all our samples
right away,
hence you might have to iterate more than once until enough write_space is
available
in the ring buffer.
>
> > + 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?
>
Ah you're right, since bytes is the smallest of i_buffer or write_space, it
can't be larger
than i_buffer.
>
> > + }
> > +
> > + 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.
>
The idea is to wait for a buffer's worth of time...I could wait
for time == samples left in ringbuffer, is that what you mean?
>
> > + }
> > +
> > + /* 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.
>
No problem with variable-sized arrays for me, did not know if they were
used in VLC.
>
> > + 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...
>
So here the issue is that we need to reset the ringbuffer, but this cannot
be
done safely between threads while the audio thread is reading it. So this
code
temporarily suspends audio processing, saves the connections (which are lost
when you call deactivate), resets the ringbuffer, resumes audio processing,
then
restores the connections.
This code does currently not preserve the connections across streams.
I could move the connection save/restore code into separate functions so
that
they could be reused in the playing across streams case.
>
> > + 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?
>
If you call jack_ringbuffer_free on a NULL pointer, it will SEGFAULT.
Are we guaranteed in Stop() that the ringbuffer has been created?
>
> > + 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.
>
No problem.
Thanks again!
Best,
Tristan
>
>
> --
> Rémi Denis-Courmont
> Sent from my collocated server
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
>
--
Tristan Matthews
web: http://tristanswork.blogspot.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20130410/0b79e8d9/attachment.html>
More information about the vlc-devel
mailing list