Re: emu10k1 bug and patch (0.9.0rc7)

[Abramo Bagnara <abramo.bagnara@libero.it>]

Takashi Iwai wrote:
> 
> At Thu, 13 Feb 2003 20:57:40 +0100 (CET),
> Jaroslav wrote:
> >
> > On Thu, 13 Feb 2003, Takashi Iwai wrote:
> >
> > > At Mon, 10 Feb 2003 18:21:12 +0100,
> > > Arnaud de Bossoreille de Ribou wrote:
> > > >
> > > > Hi, I discovered a bug in the emu10k1 driver which I'll explain here:
> > > >
> > > > I was developing an application which uses the timestamps given in the
> > > > status of the device to send S/PDIF data to it. This app worked pretty
> > > > well except that sometimes I heard sound discontinuities and then a
> > > > constant time delay between the sound and the video.
> > > >
> > > > I finally found where was the problem, my results is based on the
> > > > emu10k1-debug.patch file attached. The "frame" argument is equal to 0
> > > > when the app gets the status of the device. With this patch applied I
> > > > saw some output on the console exactly at the same time the bug occured.
> > > > Adding a "else" after the "if" to prevent sw_ready from being updated
> > > > fixed the problem and the output looked like
> > > >
> > > > ----------------
> > > > plop 0 -1536 A B
> > > > plop 0 1536 B A
> > > > ----------------
> > > >
> > > > where B == A - 1536 (1536 is the period_size). These two lines were
> > > > repeated a few times during playback.
> > > >
> > > > So the bug looks like a signedness problem since sw_ready is unsigned
> > > > and there is a while(sw_ready > 0), which explain the constant delay,
> > > > next in the "snd_emu10k1_fx8010_playback_transfer" function.
> > >
> > > this is because of the incorrect check of boundary-wrap.
> > > the comparison below must be <= instead of <.
> > > (or, it can be simply "diff < 0".)
> > > if there only two periods, the original code cannot detect the
> > > boundary-wrap.
> > >
> > >     if (diff) {
> > > ==>         if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
> > >                     diff += runtime->boundary;
> > >             pcm->sw_ready += diff;
> > >     }
> > >
> > > sw_ready should be unsigned safely.
> > > please try the change above with the unsigned sw_ready.
> >
> > Not really. Note that the application can move the appl_ptr backward
> > (using snd_pcm_rewind()). The problem is that pcm->appl_ptr is updated
> > wrongly, thus calling function with frames == 0 twice or more causes
> > different results. I'm working on a proper fix.
> 
> ah, rewind take the appl_ptr back...
> 
> regarding to another appl_ptr update:
> 
> in pcm_lib.c snd_pcm_lib_read1/write1(), appl_ptr is set to 0 if it
> comes over boundary.
> 
>                 appl_ptr += frames;
>                 if (appl_ptr >= runtime->boundary) {
>                         runtime->control->appl_ptr = 0;
>                 } else {
>                         runtime->control->appl_ptr = appl_ptr;
>                 }
> 
> i'm not sure it's always safe (whether frame increment is aligned to
> the boundary size).  is there problem to do like below?
> 
>                 if (appl_ptr >= runtime->boundary) {
>                         runtime->control->appl_ptr = appl_ptr - runtime->boundary;
>                 } else {
>                         ...

It's definitely harmless, but in this way you'd hide elsewhere placed
bugs.

-- 
Abramo Bagnara                       mailto:abramo.bagnara@libero.it

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy


-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en