[vlc-devel] [remi at remlab.net: Re: [PATCH] sndio: Updated sndio backend which actually works.]

[Alexandre Ratchov]

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