<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Thanks for the feedback, some questions inline:<br></div><div class="gmail_quote"><br>2013/4/10 Rémi Denis-Courmont <span dir="ltr"><<a href="mailto:remi@remlab.net" target="_blank">remi@remlab.net</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Hello,<br>
<br>
Looks quite nice overall. Comments inline...<br>
<br>
> diff --git a/modules/audio_output/Modules.am<br>
> b/modules/audio_output/Modules.am<br>
> index 38e9794..eb0bdc6 100644<br>
> --- a/modules/audio_output/Modules.am<br>
> +++ b/modules/audio_output/Modules.am<br>
> @@ -53,7 +53,7 @@ if HAVE_PULSE<br>
> libvlc_LTLIBRARIES += <a href="http://libpulse_plugin.la" target="_blank">libpulse_plugin.la</a><br>
> endif<br>
><br>
> -libjack_plugin_la_SOURCES = jack.c packet.c volume.h<br>
> +libjack_plugin_la_SOURCES = jack.c volume.h<br>
> libjack_plugin_la_CFLAGS = $(AM_CFLAGS) $(JACK_CFLAGS)<br>
> libjack_plugin_la_LIBADD = $(AM_LIBADD) $(JACK_LIBS) $(LIBM)<br>
> EXTRA_LTLIBRARIES += <a href="http://libjack_plugin.la" target="_blank">libjack_plugin.la</a><br>
> diff --git a/modules/audio_output/jack.c b/modules/audio_output/jack.c<br>
> index 22200d5..39e64df 100644<br>
> --- a/modules/audio_output/jack.c<br>
> +++ b/modules/audio_output/jack.c<br>
> @@ -39,6 +39,7 @@<br>
> #include <vlc_aout.h><br>
><br>
> #include <jack/jack.h><br>
> +#include <jack/ringbuffer.h><br>
><br>
> typedef jack_default_audio_sample_t jack_sample_t;<br>
><br>
> @@ -50,14 +51,17 @@ typedef jack_default_audio_sample_t jack_sample_t;<br>
><br>
><br>
*****************************************************************************/<br>
> struct aout_sys_t<br>
> {<br>
> - aout_packet_t packet;<br>
> + jack_ringbuffer_t *p_jack_ringbuffer;<br>
> jack_client_t *p_jack_client;<br>
> jack_port_t **p_jack_ports;<br>
> jack_sample_t **p_jack_buffers;<br>
> unsigned int i_channels;<br>
> + int i_rate;<br>
<br>
unsigned?<br></blockquote><div><br></div><div>sure.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> jack_nframes_t latency;<br>
> float soft_gain;<br>
> bool soft_mute;<br>
> + mtime_t paused; /**< Time when (last) paused */<br>
> + size_t bytes_per_frame;<br>
<br>
size_t is OK but a tad overkill.<br></blockquote><div><br></div><div>Should I use unsigned then?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> };<br>
><br>
><br>
><br>
/*****************************************************************************<br>
> @@ -65,6 +69,10 @@ struct aout_sys_t<br>
><br>
><br>
*****************************************************************************/<br>
> static int Open ( vlc_object_t * );<br>
> static void Close ( vlc_object_t * );<br>
> +static void Play ( audio_output_t * p_aout, block_t * p_block<br>
);<br>
> +static void Pause ( audio_output_t *aout, bool paused, mtime_t<br>
> date );<br>
> +static void Flush ( audio_output_t *p_aout, bool wait );<br>
> +static int TimeGet ( audio_output_t *, mtime_t * );<br>
> static int Process ( jack_nframes_t i_frames, void *p_arg );<br>
> static int GraphChange ( void *p_arg );<br>
><br>
> @@ -109,6 +117,7 @@ static int Start( audio_output_t *p_aout,<br>
> audio_sample_format_t *restrict fmt )<br>
> int i_error;<br>
><br>
> p_sys->latency = 0;<br>
> + p_sys->paused = VLC_TS_INVALID;<br>
><br>
> /* Connect to the JACK server */<br>
> snprintf( psz_name, sizeof(psz_name), "vlc_%d", getpid());<br>
> @@ -130,17 +139,16 @@ static int Start( audio_output_t *p_aout,<br>
> audio_sample_format_t *restrict fmt )<br>
> /* JACK only supports fl32 format */<br>
> fmt->i_format = VLC_CODEC_FL32;<br>
> // TODO add buffer size callback<br>
> - fmt->i_rate = jack_get_sample_rate( p_sys->p_jack_client );<br>
> -<br>
> - p_aout->time_get = aout_PacketTimeGet;<br>
> - p_aout->play = aout_PacketPlay;<br>
> - p_aout->pause = NULL;<br>
> - p_aout->flush = aout_PacketFlush;<br>
> - aout_PacketInit( p_aout, &p_sys->packet,<br>
> - jack_get_buffer_size( p_sys->p_jack_client ), fmt<br>
);<br>
> + p_sys->i_rate = fmt->i_rate = jack_get_sample_rate(<br>
> p_sys->p_jack_client );<br>
> +<br>
> + p_aout->play = Play;<br>
> + p_aout->pause = Pause;<br>
> + p_aout->flush = Flush;<br>
> + p_aout->time_get = TimeGet;<br>
> aout_SoftVolumeStart( p_aout );<br>
><br>
> p_sys->i_channels = aout_FormatNbChannels( fmt );<br>
> + aout_FormatPrepare(fmt);<br>
><br>
> p_sys->p_jack_ports = malloc( p_sys->i_channels *<br>
> sizeof(jack_port_t *) );<br>
> @@ -158,6 +166,16 @@ static int Start( audio_output_t *p_aout,<br>
> audio_sample_format_t *restrict fmt )<br>
> goto error_out;<br>
> }<br>
><br>
> + p_sys->bytes_per_frame = fmt->i_bytes_per_frame;<br>
> + const size_t buf_sz = (AOUT_MAX_ADVANCE_TIME / CLOCK_FREQ *<br>
> fmt->i_rate * fmt->i_bytes_per_frame);<br>
<br>
This seems to assume that AOUT_MAX_ADVANCE_TIME is a multiple of<br>
CLOCK_FREQ. AOUT_MAX_ADVANCE_TIME * fmt->i_rate * fmt->i_bytes_per_frame /<br>
CLOCK_FREQ is likely better.<br></blockquote><div><br></div><div>Ok will change that, test and confirm.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also VLC code is normally wrapped to 79 columns.<br></blockquote><div><br></div><div>Sounds good. I take it there is no indent script or anything aside from the VLC Coding guidelines?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + p_sys->p_jack_ringbuffer = jack_ringbuffer_create( buf_sz );<br>
> +<br>
> + if( p_sys->p_jack_ringbuffer == NULL )<br>
> + {<br>
> + status = VLC_ENOMEM;<br>
> + goto error_out;<br>
> + }<br>
> +<br>
> /* Create the output ports */<br>
> for( i = 0; i < p_sys->i_channels; i++ )<br>
> {<br>
> @@ -231,39 +249,151 @@ error_out:<br>
> {<br>
> jack_deactivate( p_sys->p_jack_client );<br>
> jack_client_close( p_sys->p_jack_client );<br>
> - aout_PacketDestroy( p_aout );<br>
> }<br>
> + if( p_sys->p_jack_ringbuffer)<br>
> + jack_ringbuffer_free( p_sys->p_jack_ringbuffer );<br>
> +<br>
> free( p_sys->p_jack_ports );<br>
> free( p_sys->p_jack_buffers );<br>
> }<br>
> return status;<br>
> }<br>
><br>
> +static void Play (audio_output_t * p_aout, block_t * p_block)<br>
> +{<br>
> + struct aout_sys_t *p_sys = p_aout->sys;<br>
> +<br>
> + while (p_block->i_buffer > 0) {<br>
> +<br>
> + /* move data to buffer */<br>
> + const size_t bytes = __MIN(<br>
> jack_ringbuffer_write_space(p_sys->p_jack_ringbuffer), p_block->i_buffer<br>
);<br>
<br>
The __MIN macro is not expansion-safe, so this is probably wrong.<br></blockquote><div><br></div><div>Ok.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + if (unlikely(jack_ringbuffer_write( p_sys->p_jack_ringbuffer,<br>
> (const char *) p_block->p_buffer, bytes ) < bytes)) {<br>
> + msg_Warn(p_aout, "Some audio was dropped");<br>
<br>
You should try to keep message style consistent across the plugin.<br></blockquote><div><br></div><div>Ok.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I do not understand how this loop won't always fail if it ever iterates<br>
more than once.<br></blockquote><div><br></div><div>jack_ringbuffer_write_space is called just before to see how much space<br>the ringbuffer has. It is possible that we can't write all our samples right away,<br>hence you might have to iterate more than once until enough write_space is available<br>
</div><div>in the ring buffer.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + break;<br>
> + }<br>
> +<br>
> + p_block->i_nb_samples -= bytes / sizeof(jack_sample_t);<br>
> + p_block->p_buffer += bytes;<br>
> + p_block->i_buffer = p_block->i_buffer < bytes ? 0 :<br>
> p_block->i_buffer - bytes;<br>
<br>
How can this possibly overflow?<br></blockquote><div><br></div><div>Ah you're right, since bytes is the smallest of i_buffer or write_space, it can't be larger<br></div><div>than i_buffer.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + }<br>
> +<br>
> + block_Release(p_block);<br>
> +}<br>
> +<br>
> +/**<br>
> + * Pause or unpause playback<br>
> + */<br>
> +static void Pause(audio_output_t *aout, bool paused, mtime_t date)<br>
> +{<br>
> + aout_sys_t *sys = aout->sys;<br>
> +<br>
> + if (paused) {<br>
> + sys->paused = date;<br>
> + } else {<br>
> + date -= sys->paused;<br>
> + msg_Dbg(aout, "resuming after %"PRId64" us", date);<br>
> + sys->paused = VLC_TS_INVALID;<br>
> + }<br>
> +}<br>
> +<br>
> +static void Flush(audio_output_t *p_aout, bool wait)<br>
> +{<br>
> + struct aout_sys_t * p_sys = p_aout->sys;<br>
> + jack_ringbuffer_t *rb = p_sys->p_jack_ringbuffer;<br>
> + int i_error;<br>
> +<br>
> + /* Sleep until the ring buffer has been emptied */<br>
> + if( wait )<br>
> + {<br>
> + const mtime_t wait_time =<br>
> jack_frames_to_time(p_sys->p_jack_client,<br>
> jack_get_buffer_size(p_sys->p_jack_client));<br>
> + while( jack_ringbuffer_write_space( rb ) != rb->size )<br>
> + msleep(wait_time);<br>
<br>
wait_time is probably wrong from the second iteration.<br></blockquote><div><br></div><div>The idea is to wait for a buffer's worth of time...I could wait<br>for time == samples left in ringbuffer, is that what you mean?<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + }<br>
> +<br>
> + /* FIXME: maybe the ringbuffer_reset should just be protected by<br>
> signalling with atomic variables? */<br>
> +<br>
> + /* Save connections */<br>
> + const char ***connections_list = malloc( p_sys->i_channels *<br>
> sizeof(const char **) );<br>
<br>
The maximum channels count is small enough that you can allocate on the<br>
stack. If your religion forbids variable-sized arrays, there is<br>
AOUT_CHAN_MAX or something.<br></blockquote><div><br></div><div>No problem with variable-sized arrays for me, did not know if they were<br></div><div>used in VLC.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + if( connections_list == NULL )<br>
> + {<br>
> + msg_Err( p_aout, "Out of memory" );<br>
> + return;<br>
> + }<br>
> +<br>
> + for(size_t i = 0; i < p_sys->i_channels; i++ )<br>
> + {<br>
> + const char **port_connections = jack_port_get_all_connections(<br>
> p_sys->p_jack_client, p_sys->p_jack_ports[i] );<br>
> + connections_list[i] = port_connections;<br>
> + }<br>
> +<br>
> + /* Suspend audio processing */<br>
> + i_error = jack_deactivate( p_sys->p_jack_client );<br>
<br>
I don't know how JACK works. From the name, this function looks like it<br>
belongs in Stop().<br>
<br>
> + if( i_error )<br>
> + {<br>
> + msg_Err( p_aout, "failed to deactivate JACK client (error %d)",<br>
> i_error );<br>
> + return;<br>
> + }<br>
> +<br>
> + /* reset ringbuffer read and write pointers */<br>
> + jack_ringbuffer_reset(rb);<br>
> +<br>
> + /* Resume audio processing */<br>
> + i_error = jack_activate( p_sys->p_jack_client );<br>
<br>
Does it make sense before Start()?<br>
<br>
> + if( i_error )<br>
> + {<br>
> + msg_Err( p_aout, "failed to activate JACK client (error %d)",<br>
> i_error );<br>
> + return;<br>
> + }<br>
> +<br>
> + /* Restore connections */<br>
<br>
Are you trying to preserve connections across audio streams? This makes<br>
sense; in fact, it is another known bug. This would require a rehaul of<br>
Open() vs Start() and Close() vs Open(). Maybe this whole Flush() blob<br>
should be postponed to a separate patch...<br></blockquote><div><br></div><div>So here the issue is that we need to reset the ringbuffer, but this cannot be<br>done safely between threads while the audio thread is reading it. So this code<br>
</div><div>temporarily suspends audio processing, saves the connections (which are lost<br>when you call deactivate), resets the ringbuffer, resumes audio processing, then<br></div><div>restores the connections.<br>This code does currently not preserve the connections across streams.<br>
</div><div>I could move the connection save/restore code into separate functions so that<br>they could be reused in the playing across streams case.<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + for(size_t i = 0; i < p_sys->i_channels; i++ )<br>
> + {<br>
> + const char **port_connections = connections_list[i];<br>
> + const char* psz_out = jack_port_name( p_sys->p_jack_ports[i] );<br>
> + for (const char **psz_in = port_connections; psz_in && *psz_in;<br>
> psz_in++)<br>
> + {<br>
> + i_error = jack_connect( p_sys->p_jack_client, psz_out,<br>
> *psz_in );<br>
> + if( i_error )<br>
> + {<br>
> + msg_Err( p_aout, "failed to connect port %s to port %s<br>
> (error %d)",<br>
> + psz_out, *psz_in, i_error );<br>
> + }<br>
> + }<br>
> +<br>
> + if (port_connections)<br>
> + jack_free(port_connections);<br>
> + }<br>
> +<br>
> + free(connections_list);<br>
> +}<br>
> +<br>
> +static int TimeGet(audio_output_t *p_aout, mtime_t *delay)<br>
> +{<br>
> + struct aout_sys_t * p_sys = p_aout->sys;<br>
> + jack_ringbuffer_t *rb = p_sys->p_jack_ringbuffer;<br>
> + if( p_sys->bytes_per_frame == 0 )<br>
> + return -1;<br>
> +<br>
> + *delay = (p_sys->latency + (jack_ringbuffer_read_space(rb) /<br>
> p_sys->bytes_per_frame)) * CLOCK_FREQ / p_sys->i_rate;<br>
> +<br>
> + return 0;<br>
> +}<br>
><br>
><br>
><br>
/*****************************************************************************<br>
> * Process: callback for JACK<br>
><br>
><br>
*****************************************************************************/<br>
> int Process( jack_nframes_t i_frames, void *p_arg )<br>
> {<br>
> - unsigned int i, j, i_nb_samples = 0;<br>
> + unsigned int i, j, frames_from_rb = 0;<br>
> + size_t bytes_read = 0;<br>
> + size_t frames_read;<br>
> audio_output_t *p_aout = (audio_output_t*) p_arg;<br>
> struct aout_sys_t *p_sys = p_aout->sys;<br>
> - jack_sample_t *p_src = NULL;<br>
><br>
> - jack_nframes_t dframes = p_sys->latency<br>
> - - jack_frames_since_cycle_start(<br>
> p_sys->p_jack_client );<br>
> + /* Get the next audio data buffer unless paused */<br>
><br>
> - jack_time_t dtime = dframes * 1000 * 1000 / jack_get_sample_rate(<br>
> p_sys->p_jack_client );<br>
> - mtime_t play_date = mdate() + (mtime_t) ( dtime );<br>
> -<br>
> - /* Get the next audio data buffer */<br>
> - block_t *p_buffer = aout_PacketNext( p_aout, play_date );<br>
> -<br>
> - if( p_buffer != NULL )<br>
> - {<br>
> - p_src = (jack_sample_t *)p_buffer->p_buffer;<br>
> - i_nb_samples = p_buffer->i_nb_samples;<br>
> - }<br>
> + if( p_sys->paused == VLC_TS_INVALID)<br>
> + frames_from_rb = i_frames;<br>
><br>
> /* Get the JACK buffers to write to */<br>
> for( i = 0; i < p_sys->i_channels; i++ )<br>
> @@ -273,28 +403,27 @@ int Process( jack_nframes_t i_frames, void *p_arg<br>
)<br>
> }<br>
><br>
> /* Copy in the audio data */<br>
> - for( j = 0; j < i_nb_samples; j++ )<br>
> + for( j = 0; j < frames_from_rb; j++ )<br>
> {<br>
> for( i = 0; i < p_sys->i_channels; i++ )<br>
> {<br>
> - jack_sample_t *p_dst = p_sys->p_jack_buffers[i];<br>
> - p_dst[j] = *p_src;<br>
> - p_src++;<br>
> + jack_sample_t *p_dst = p_sys->p_jack_buffers[i] + j;<br>
> + bytes_read += jack_ringbuffer_read(<br>
p_sys->p_jack_ringbuffer,<br>
> + (char *) p_dst, sizeof(jack_sample_t) );<br>
> }<br>
> }<br>
><br>
> /* Fill any remaining buffer with silence */<br>
> - if( i_nb_samples < i_frames )<br>
> + frames_read = (bytes_read / sizeof(jack_sample_t)) /<br>
> p_sys->i_channels;<br>
> + if( frames_read < i_frames )<br>
> {<br>
> for( i = 0; i < p_sys->i_channels; i++ )<br>
> {<br>
> - memset( p_sys->p_jack_buffers[i] + i_nb_samples, 0,<br>
> - sizeof( jack_sample_t ) * (i_frames - i_nb_samples)<br>
> );<br>
> + memset( p_sys->p_jack_buffers[i] + frames_read, 0,<br>
> + sizeof( jack_sample_t ) * (i_frames - frames_read)<br>
);<br>
> }<br>
> }<br>
><br>
> - if( p_buffer )<br>
> - block_Release( p_buffer );<br>
> return 0;<br>
> }<br>
><br>
> @@ -308,15 +437,15 @@ static int GraphChange( void *p_arg )<br>
> audio_output_t *p_aout = (audio_output_t*) p_arg;<br>
> struct aout_sys_t *p_sys = p_aout->sys;<br>
> unsigned int i;<br>
> - jack_nframes_t port_latency;<br>
> + jack_latency_range_t port_latency;<br>
><br>
> p_sys->latency = 0;<br>
><br>
> for( i = 0; i < p_sys->i_channels; ++i )<br>
> {<br>
> - port_latency = jack_port_get_total_latency( p_sys->p_jack_client,<br>
> -<br>
p_sys->p_jack_ports[i]<br>
> );<br>
> - p_sys->latency = __MAX( p_sys->latency, port_latency );<br>
> + jack_port_get_latency_range( p_sys->p_jack_ports[i],<br>
> JackPlaybackLatency,<br>
> + &port_latency );<br>
> + p_sys->latency = __MAX( p_sys->latency, port_latency.max );<br>
> }<br>
><br>
> msg_Dbg(p_aout, "JACK graph reordered. Our maximum latency=%d.",<br>
> p_sys->latency);<br>
> @@ -345,7 +474,8 @@ static void Stop( audio_output_t *p_aout )<br>
> }<br>
> free( p_sys->p_jack_ports );<br>
> free( p_sys->p_jack_buffers );<br>
> - aout_PacketDestroy( p_aout );<br>
> + if( p_sys->p_jack_ringbuffer )<br>
<br>
Isn't this a tautology?<br></blockquote><div><br></div><div>If you call jack_ringbuffer_free on a NULL pointer, it will SEGFAULT.<br></div><div>Are we guaranteed in Stop() that the ringbuffer has been created?<br></div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + jack_ringbuffer_free( p_sys->p_jack_ringbuffer );<br>
> }<br>
><br>
> static int Open(vlc_object_t *obj)<br>
> diff --git a/modules/audio_output/packet.c<br>
b/modules/audio_output/packet.c<br>
> deleted file mode 100644<br>
> index 48f7491..0000000<br>
<br>
I would recommend deleting packet.c in a separate patch, because this is<br>
not JACK-specific. Furthermore, the corresponding bits in<br>
include/vlc_aout.h need to removed as well.<br></blockquote><div><br></div><div>No problem.<br></div><div>Thanks again!<br><br>Best,<br></div><div>Tristan<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
<br>
--<br>
Rémi Denis-Courmont<br>
Sent from my collocated server<br>
_______________________________________________<br>
vlc-devel mailing list<br>
To unsubscribe or modify your subscription options:<br>
<a href="http://mailman.videolan.org/listinfo/vlc-devel" target="_blank">http://mailman.videolan.org/listinfo/vlc-devel</a><br>
</font></span></blockquote></div><br><br clear="all"><br>-- <br>Tristan Matthews<br>web: <a href="http://tristanswork.blogspot.com">http://tristanswork.blogspot.com</a><br>
</div></div>