ipodlinux-devel

[Ipodlinux-devel] PATCH: fix for a rare audio playback bug

[Gobber W. <gob...@gm...>]

I've found a bug in the circular buffer management code. It is in the
routine ipodaudio_write in the file linux/arch/armnommu/mach-ipod/audio.c.
The bug is not in the ipod1-1 and earlier kernels, but is present in ipod1-=
2
and all later kernels. The change that introduced the bug is CVS revision
1.9 to audio.c from Bernard:

fixed circular buffer bugs to clean up audio output

http://cvs.sourceforge.net/viewcvs.py/ipodlinux/linux/arch/armnommu/mach-ip=
od/audio.c?rev=3D1.9&view=3Dmarkup

It happens when a write *exactly* fills the buffer up to the end. This coul=
d
be because there is exactly that amount of data to write, or because the
read pointer is at the beginning of the audio buffer (ie. full). This cause=
s
the value *DMA_WRITE_OFF to point to one element past the end of the buffer=
.
This is not a problem for the writing code, it will just wrap around to the
beginning of the buffer when there is more data to write or more space to
write into. However, the reading code looks like this:

        while ( *r_off !=3D *w_off ) {
                if ( (inl(0xc000251c) & 0x7800000) =3D=3D 0 ) {
                        outl(inl(0xc000251c)|(1<<9), 0xc000251c);
                        return;
                }

                outl(((unsigned)dma_buf[*r_off]) << 16, 0xc0002540);
                if ( !stereo ) {
                        outl(((unsigned)dma_buf[*r_off]) << 16, 0xc0002540)=
;
                }

                *r_off =3D (*r_off + 1) % BUF_LEN;
        }

If the value of *w_off is outside the buffer (eg. one element past its end)=
,
then there is nothing to stop the read code from looping around the same se=
t
of samples in the buffer. Depending on the timing between the writer and
reader, it could loop around one extra time or multiple times before a writ=
e
re-establishes a valid write offset. The overall effect is one or more
replays of the last ~0.25 seconds worth of audio (assuming 46KB buffer and
CD format audio). I think that this doesn't happpen in practice because the
writes are faster than the reads so the read pointer rarely catches up with
the write pointer. I found an obscure way that I could get this to happen
all the time giving a 0.25 second stutter, so I'd appreciate if someone can
commit the following fix for me.

Cheers!

Gobber Warts.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: RCS/audio.c,v
retrieving revision 1.1
diff -c -r1.1 audio.c
*** audio.c     2006/03/24 08:13:10     1.1
--- audio.c     2006/03/24 09:26:42
***************
*** 452,458 ****
                                rem -=3D cnt;
                                bufsp +=3D cnt;

!                               write_off_current +=3D cnt;
                        }

                        /* room at start of buffer (and more data)? */
--- 452,458 ----
                                rem -=3D cnt;
                                bufsp +=3D cnt;

!                               write_off_current =3D (write_off_current +
cnt) % BUF_LEN;
                        }

                        /* room at start of buffer (and more data)? */

Re: [Ipodlinux-devel] PATCH: fix for a rare audio playback bug

[Bernard L. <le...@bo...>]

Thanks for that, I've finally managed to get around to commiting it to
CVS.

Apologies for the delay.

cheers,
bern.

On Sun, 2006-03-26 at 10:35 -0800, Gobber Warts wrote:
> I've found a bug in the circular buffer management code. It is in the
> routine ipodaudio_write in the file
> linux/arch/armnommu/mach-ipod/audio.c. The bug is not in the ipod1-1
> and earlier kernels, but is present in ipod1-2 and all later kernels.
> The change that introduced the bug is CVS revision 1.9 to audio.c from
> Bernard:
> fixed circular buffer bugs to clean up audio output
> 
> http://cvs.sourceforge.net/viewcvs.py/ipodlinux/linux/arch/armnommu/mach-ipod/audio.c?rev=1.9&view=markup
> 
> It happens when a write *exactly* fills the buffer up to the end. This
> could be because there is exactly that amount of data to write, or
> because the read pointer is at the beginning of the audio buffer (ie.
> full). This causes the value *DMA_WRITE_OFF to point to one element
> past the end of the buffer. This is not a problem for the writing
> code, it will just wrap around to the beginning of the buffer when
> there is more data to write or more space to write into. However, the
> reading code looks like this:
> 
>         while ( *r_off != *w_off ) {
>                 if ( (inl(0xc000251c) & 0x7800000) == 0 ) {
>                         outl(inl(0xc000251c)|(1<<9), 0xc000251c);
>                         return;
>                 }
> 
>                 outl(((unsigned)dma_buf[*r_off]) << 16, 0xc0002540);
>                 if ( !stereo ) {
>                         outl(((unsigned)dma_buf[*r_off]) << 16,
> 0xc0002540);
>                 }
> 
>                 *r_off = (*r_off + 1) % BUF_LEN;
>         }
> 
> If the value of *w_off is outside the buffer (eg. one element past its
> end), then there is nothing to stop the read code from looping around
> the same set of samples in the buffer. Depending on the timing between
> the writer and reader, it could loop around one extra time or multiple
> times before a write re-establishes a valid write offset. The overall
> effect is one or more replays of the last ~0.25 seconds worth of audio
> (assuming 46KB buffer and CD format audio). I think that this doesn't
> happpen in practice because the writes are faster than the reads so
> the read pointer rarely catches up with the write pointer. I found an
> obscure way that I could get this to happen all the time giving a 0.25
> second stutter, so I'd appreciate if someone can commit the following
> fix for me.
> 
> Cheers!
> 
> Gobber Warts.
> 
> ===================================================================
> RCS file: RCS/audio.c,v
> retrieving revision 1.1
> diff -c -r1.1 audio.c
> *** audio.c     2006/03/24 08:13:10     1.1
> --- audio.c     2006/03/24 09:26:42
> ***************
> *** 452,458 ****
>                                 rem -= cnt;
>                                 bufsp += cnt;
> 
> !                               write_off_current += cnt;
>                         }
> 
>                         /* room at start of buffer (and more data)? */
> --- 452,458 ----
>                                 rem -= cnt;
>                                 bufsp += cnt;
> 
> !                               write_off_current = (write_off_current
> + cnt) % BUF_LEN;
>                         }
> 
>                         /* room at start of buffer (and more data)? */