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

[Rémi Denis-Courmont]

Le jeudi 21 juin 2012 19:25:35 Alexandre Ratchov, vous avez écrit :
> > 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.

I have not computed the line numbers but I doubt that. For one thing renaming 
variables causes a lot of noise. If you really need to rename variables or do 
other cosmetics, you really need to split patches.

Reviewers are even more sparse a resource than developers, live with it.

> > > +    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.

msg_Dbg() is fine but I suspect most of the data you print there is already in 
the common audio debug messages.

> > > +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.

That's exactly why it will underflow.

> > > -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.

The point of flush is to stop streaming immediately. Without flush, the sound 
would continue until the buffers are empty, up two seconds after the Stop, 
Prev or Next button press... That's terrible user experience.

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis