[vlc-devel] [remi at remlab.net: Re: [PATCH] sndio: Updated sndio backend which actually works.]
Alexandre Ratchov
alex at caoua.org
Thu Jun 21 18:25:35 CEST 2012
On Thu, Jun 21, 2012 at 04:05:10AM -0400, Brad Smith wrote:
> Ignore the NEWS chunk, that was me. But please read the rest especially the
> last two comments. Please see what you can do to fix up the backend in the
> ports tree take his comments into consideration.
>
>
> ----- Forwarded message from R?mi Denis-Courmont <remi at remlab.net> -----
>
> Date: Thu, 21 Jun 2012 06:20:52 +0300
> From: R?mi Denis-Courmont <remi at remlab.net>
> To: vlc-devel at videolan.org
> Cc: Brad Smith <brad at comstyle.com>
> Subject: Re: [vlc-devel] [PATCH] sndio: Updated sndio backend which actually works.
> User-Agent: KMail/1.13.7 (Linux/3.4.2-basile; KDE/4.8.3; i686; ; )
>
> Le jeudi 21 juin 2012 03:05:24 Brad Smith, vous avez ?crit :
> > sndio: Updated sndio backend which actually works.
> >
> > Provided by Alexandre Ratchov <alex at caoua.org>
>
> This patch defeats my reviewing skills because it dilutes 5% fixes and 5%
> arguable changes in 90% cosmetic changes.
>
Come on, only few are cosmetic.
> > ---
> > NEWS | 2 +-
> > configure.ac | 7 +-
> > modules/LIST | 2 +-
> > modules/audio_output/sndio.c | 237
> > +++++++++++++++++++++++++++--------------- 4 files changed, 156
> > insertions(+), 92 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index ecde695..6bbf5d0 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -31,7 +31,7 @@ Demuxers:
> >
> > Audio output:
> > * Windows Audio Session API audio output support
> > - * OpenBSD sndio audio output support
> > + * sndio audio output support
>
> Why? Far more people know what OpenBSD is, than what sndio is.
>
> > + vlc_fourcc_to_char (aout->format.i_format, fourcc);
> > + do {
> > + if (fourcc[0] == 's')
> > + par.sig = 1;
> > + else if (fourcc[0] == 'u')
> > + par.sig = 0;
> > + else
> > + break;
> > + if (fourcc[1] == '8' && fourcc[2] == ' ' && fourcc[3] == ' ') {
> > + par.bits = 8;
> > + break;
> > + } else if (fourcc[1] == '1' && fourcc[2] == '6') {
> > + par.bits = 16;
> > + } else if (fourcc[1] == '2' && fourcc[2] == '4') {
> > + par.bits = 24;
> > + par.bps = 3;
> > + } else if (fourcc[1] == '3' && fourcc[2] == '2') {
> > + par.bits = 32;
> > + }
> > + if (fourcc[3] == 'l')
> > + par.le = 1;
> > + else if (fourcc[3] == 'b')
> > + par.le = 0;
> > + else
> > + break;
> > + } while (0);
>
> You're not supposed to "parse" FOURCC like that. Use switch statement.
OK, thanks.
> > break;
> > default:
> > msg_Err (aout, "unknown %u channels map", par.pchan);
> > goto error;
> > }
>
> That chunk is purely cosmetic and belongs in a separate patch.
>
OK.
> > - f.i_original_channels = f.i_physical_channels = chans;
> > - aout_FormatPrepare (&f);
> > -
> > - aout->format = f;
> > - aout->sys = (void *)sio;
> > + sys = malloc (sizeof(struct aout_sys_t));
> > + if (sys == NULL) {
> > + msg_Err (aout, "failed to allocate sndio structure");
> > + goto error;
> > + }
> > + sys->hdl = hdl;
> > + sys->bpf = par.bps * par.pchan;
> > + sys->delay = 0;
> > + aout->sys = sys;
> > + aout->format.i_format =
> > + VLC_FOURCC(fourcc[0], fourcc[1], fourcc[2], fourcc[3]);
> > + msg_Warn(aout, "pchan = %u, rate = %u, bufsz = %u, round = %u\n",
> > + par.pchan, par.rate, par.bufsz, par.round);
> > + msg_Warn(aout, "orig_chans = 0x%x, phys_chans = 0x%x, chans = 0x%x\n",
> > + aout->format.i_original_channels,
> > + aout->format.i_physical_channels,
> > + chans);
>
> Warning can't be right.
This should be msg_Dbg(), right? It seems VLC fails on certain rare
mkv files and I could't reproduce it to figure out whether it's the
sndio module fault. This is for people to be able to report details.
> > +static void Pause (audio_output_t *aout, bool pause, mtime_t date)
> > +{
> > + struct aout_sys_t *sys = (struct aout_sys_t *)aout->sys;
> > + static char zeros[100];
> > + unsigned int todo, n;
> >
> > - block->p_buffer += bytes;
> > - block->i_buffer -= bytes;
> > - /* Note that i_nb_samples and i_pts are corrupted here. */
> > + if (pause) {
> > + sio_stop (sys->hdl);
> > + sio_start (sys->hdl);
> > + } else {
> > + todo = sys->delay * sys->bpf;
> > + while (todo > 0) {
> > + n = todo;
> > + if (n >= sizeof(zeros))
> > + n = sizeof(zeros);
> > + sio_write(sys->hdl, zeros, n);
> > + todo -= n;
> > + }
> > }
> > - block_Release (block);
> > + (void)date;
> > }
>
> This cannot possibly work. The buffer might have underrun a million times
> before Pause(..., false, ...) is called.
>
I don't understand. Once Pause(true) is called, playback is stopped
until enough data is written and buffered. Since Play() is not called
during pauses, I don't understand what buffer would underrun.
> >
> > -static void Pause (audio_output_t *aout, bool pause, mtime_t date)
> > +static void Flush (audio_output_t *aout, bool wait)
> > {
> > - struct sio_hdl *sio = (void *)aout->sys;
> > + struct aout_sys_t *sys = (struct aout_sys_t *)aout->sys;
> > +
> > + sys->delay = 0;
> > + (void)wait;
> > +}
>
> Doing nothing in Flush() cannot be right.
>
IIRC, if playback is stopped (i.e. sio_stop() is called, which is
non-blocking), then sound doesn't work at all: Flush() keeps being
called continuously. Doing nothing in Flush() keeps audio smooth.
Any better suggestion that would work?
Thanks for your review.
-- Alexandre
More information about the vlc-devel
mailing list