Re: [hatari-devel] Falcon left-right sound swapper bug?

On 17 April 2014 12:53, Douglas Little <doug694@xxxxxxxxxxxxxx> wrote:

I had a very quick look at the function DmaSnd_FIFO_Refill(void) in dmaSnd.c (...perhaps too quick, but I'm also chasing down my own audio problems so I'm interested to see how that works...)

It seems there is a FIFO buffer which is 8 bytes deep, and is refilled on each HBL event. This seems to be handling the audio bytes themselves, and updates to the new DMA buffer address on looping.

The FIFO is only 8 'bytes' deep. Not samples, but bytes. A stereo 16bit sample consumes 4 bytes, so that makes space for only 2 samples maximum. Since it is a ring buffer, that probably means room for 1 new sample without collisions.

At 50khz, there are approx 1000 samples per video frame at 50Hz. There are (IIRC) 312 HBLs per video frame @ 50hz, so that's already a potential 3 samples per HBL event.

How do those 3 samples fit in the FIFO? Am I missing something here?

I'll re-read the code when I get come, in case I missed a detail here or there. Most probably :)

Below is a reminder of unexplained noise in Hatari's codec output from my last post (albeit, probably 2 different kinds - one HF noise at 50khz quality setting, and one erratic/occasional glitches at all quality settings, but both using 12292hz codec rate, which should be ok with the code as it is, regardless of any issues indicated above!):

https://dl.dropboxusercontent.com/u/12947585/wavefm.png

D

On 17 April 2014 07:27, Eero Tamminen <oak@xxxxxxxxxxxxxx> wrote:

Hi,

On torstai 17 huhtikuu 2014, David Savinkoff wrote:
> crossbar.c is the only file that has:
> dac.readPosition :: dac.writePosition
> and:
> adc2dac_readBufferPosition :: adc2dac_writeBufferPosition
>
> These circular buffer indexes must check that they do not over-run each
> other.
>
> dmaSnd.c maintains the circular buffer indexes with:
> static void DmaSnd_FIFO_Refill(void);
> static Sint8 DmaSnd_FIFO_PullByte(void);
>
> crossbar.c has incomplete functions:
> void Crossbar_GetMicrophoneDatas(Sint16 *micro_bufferL, Sint16
> *micro_bufferR, Uint32 microBuffer_size); static void
> Crossbar_SendDataToDAC(Sint16 value, Uint16 sample_pos); void
> Crossbar_GenerateSamples(int nMixBufIdx, int nSamplesToGenerate);

There's following old bug in Hatari Crossbar DMA code:
http://listengine.tuxfamily.org/lists.tuxfamily.org/hatari-
devel/2012/02/msg00082.html

It doesn't check the indeces given by emulated code
which is really bad because emulated code can then
corrupt Hatari's own memory. That might then have also
other effects.

- Eero

> Sincerely,
> David Savinkoff
>
> ----- Laurent Sallafranque wrote:
> > Hi David,
> >
> > I introduced these 0 later in the crossbar code, because sometimes,
> > when the 68030 was crashing, there was an horrible repeating sound in
> > hatari. In the real hardware, when a value is read into the little 32
> > bytes memory, I think it doesn't exist anymore after, so that's why I
> > introduced these 0.
> >
> > Removing them doesn't remove the problem. Instead, when there's the
> > problem, we can hear an echo (repeating of the latest 0.5 s or so (0.5
> > is not an exact value here))
> >
> > 2 things I find stange :
> >
> > - when I try to change #define DACBUFFER_SIZE 2048 by 4096 or
> > more, I have no sound or a really really dirty sound
> >
> > - I've placed a little test which detects a value !=0 and then count
> > when there are more than 6 consecutive 0 in a sound (and then display
> > it
> >
> > found a case) :
> > int detect_in = 0;
> > int count_in = 0;
> >
> > // Detect in
> > if (dac_LeftData != 0)
> >
> > detect_in = 1;
> >
> > if (detect_in == 1) {
> >
> > if (dac_LeftData == 0) {
> >
> > count_in ++;
> > if (count_in > 6) {
> >
> > fprintf(stderr, "IN > 6\n");
> > detect_in = 0;
> > count_in = 0;
> >
> > }
> >
> > }
> >
> > }
> >
> > I've placed this code into Crossbar_GenerateSamples :
> > I've places it at the end to detect if there's a 0 hole in the
> >
> > dac_LeftData and in the MixBuffer[nBufIdx][0]
> >
> > I get some few holes when the sound is OK and a lot of holes when
> > the
> >
> > sound is dirty.
> >
> > So, the echo when I apply your patch (or the 0 value when not) + this
> >
> > test seems to indicate :
> > - we're sometimes reading the buffer backward (or too far at once,
> >
> > wich, with the modulo DACBUFFER_SIZE returns before the latest values
> > played)
> > or
> >
> > - we're sometimes reading the buffer many times at the same place
> >
> > (which would be strange according to the code).
> >
> > I've given a look at nSamplesToGenerate (in the function parameters).
> > It's always 882 (with good or wrong sound)
> >
> > I continue to look at this.
> >
> > Laurent
> >
> > Le 16/04/2014 21:12, David Savinkoff a écrit :
> > > ----- Laurent Sallafranque wrote:
> > >> Hi David,
> > >>
> > >> I don't think the bug is here :
> > >>
> > >> The aim of this code is to separate the sound into a left and a
> > >> right part.
> > >
> > > Hi Laurent,
> > >
> > > Thank you for the explanation. I have been comparing dmaSnd.c with
> > > crossbar.c and finding ways to improve crossbar.c
> > >
> > > I noticed that zeros are being written back to a buffer after the
> > > data is read so that a subsequent read would be zero... to avoid
> > > introducing a zero order hold filtering effect. However; if this
> > > buffer is 'actual' Falcon memory, there is a problem..
> > >
> > > Please see the enclosed patch.
> > >
> > > Sincerely,
> > > David Savinkoff