S24_LE is 3 bytes ? in alsa-lib example pcm.c

S24_LE is 3 bytes ? in alsa-lib example pcm.c

[Norbert van Bolhuis]

as far as I know:
sample format S24_LE is 24bit sample in 4 bytes
sample format S24_3LE is 24bit sample in 3 bytes
(why else have separate definitions), right ?

In the ALSA-LIb example /test/pcm.c however
(http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html)
there is no diff between the 2 sample formats.

I noticed this by simply showing the bytes presented to snd_pcm_writei
See:

S32_LE (for reference)
----------------------
./pcm -c 2 -o S32_LE -f 4000
Playback device is plughw:0,0
Stream parameters are 48000Hz, S32_LE, 2 channels
Sine wave rate is 4000.0000Hz
Using transfer method: write
write_loop: going to write: period_size=4096 samples
data[00:31]= 00000000 00000000 ffffff3f ffffff3f a0ebd96e a0ebd96e ffffff7f ffffff7f
data[32:63]= a0ebd96e a0ebd96e ffffff3f ffffff3f 00000000 00000000 010000c0 010000c0

S24_3LE (as expected)
----------------------
./pcm -c 2 -o S24_3LE -f 4000
Playback device is plughw:0,0
Stream parameters are 48000Hz, S24_3LE, 2 channels
Sine wave rate is 4000.0000Hz
Using transfer method: write
write_loop: going to write: period_size=4096 samples
data[00:31]= 00000000 0000ffff 3fffff3f ead96eea d96effff 7fffff7f ead96eea d96effff
data[32:63]= 3fffff3f 00000000 00000100 c00100c0 16269116 26910100 80010080 16269116

S24_LE
----------------------
./pcm -c 2 -o S24_LE -f 4000
Playback device is plughw:0,0
Stream parameters are 48000Hz, S24_LE, 2 channels
Sine wave rate is 4000.0000Hz
Using transfer method: write
write_loop: going to write: period_size=4096 samples
data[00:31]= 00000000 0000ffff 3fffff3f ead96eea d96effff 7fffff7f ead96eea d96effff
data[32:63]= 3fffff3f 00000000 00000100 c00100c0 16269116 26910100 80010080 16269116


I would expect to see
data[00:31]= 0x00000000 00000000 00ffff3f 00ffff3f 00ebd96e 00ebd96e 00ffff7f 00ffff7f
data[32:63]= 0x00ebd96e 00ebd96e 00ffff3f 00ffff3f 00000000 00000000 000000c0 000000c0
for S24_LE

do I miss something ?
is this is a fault in /test/pcm.c ?


-- 
This message has been scanned for viruses and is believed to be clean

Re: S24_LE is 3 bytes ? in alsa-lib example pcm.c

[Clemens Ladisch]

Norbert van Bolhuis wrote:
> as far as I know:
> sample format S24_LE is 24bit sample in 4 bytes
> sample format S24_3LE is 24bit sample in 3 bytes
> (why else have separate definitions), right ?

Yes.

> In the ALSA-LIb example /test/pcm.c however
> (http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html)
> there is no diff between the 2 sample formats.

That example program uses snd_pcm_format_width() although it should
have used snd_pcm_format_physical_width() when calculating memory
buffer sizes.


Regards,
Clemens

Re: S24_LE is 3 bytes ? in alsa-lib example pcm.c

[Jaroslav Kysela]

On Thu, 10 Jul 2008, Clemens Ladisch wrote:

> Norbert van Bolhuis wrote:
> > as far as I know:
> > sample format S24_LE is 24bit sample in 4 bytes
> > sample format S24_3LE is 24bit sample in 3 bytes
> > (why else have separate definitions), right ?
> 
> Yes.
> 
> > In the ALSA-LIb example /test/pcm.c however
> > (http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html)
> > there is no diff between the 2 sample formats.
> 
> That example program uses snd_pcm_format_width() although it should
> have used snd_pcm_format_physical_width() when calculating memory
> buffer sizes.

I think that the problem is in wrong area->step calculation in alsa-lib, 
but I'm still investigating where the real culprit is. The
snd_pcm_format_width() is used only for sample filling (which is OK).

						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: S24_LE is 3 bytes ? in alsa-lib example pcm.c

[Takashi Iwai]

At Thu, 10 Jul 2008 12:19:05 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Thu, 10 Jul 2008, Clemens Ladisch wrote:
> 
> > Norbert van Bolhuis wrote:
> > > as far as I know:
> > > sample format S24_LE is 24bit sample in 4 bytes
> > > sample format S24_3LE is 24bit sample in 3 bytes
> > > (why else have separate definitions), right ?
> > 
> > Yes.
> > 
> > > In the ALSA-LIb example /test/pcm.c however
> > > (http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html)
> > > there is no diff between the 2 sample formats.
> > 
> > That example program uses snd_pcm_format_width() although it should
> > have used snd_pcm_format_physical_width() when calculating memory
> > buffer sizes.
> 
> I think that the problem is in wrong area->step calculation in alsa-lib, 
> but I'm still investigating where the real culprit is. The
> snd_pcm_format_width() is used only for sample filling (which is OK).

I think Clemens is right.  The program uses snd_pcm_format_width()
wrongly.  Also, it won't work properly with strict aliasing...

Untested patch is below.


Takashi
--
diff --git a/test/pcm.c b/test/pcm.c
index cd29259..ecd0afa 100644
--- a/test/pcm.c
+++ b/test/pcm.c
@@ -38,7 +38,10 @@ static void generate_sine(const snd_pcm_channel_area_t *areas,
 	unsigned char *samples[channels], *tmp;
 	int steps[channels];
 	unsigned int chn, byte;
-	int ires;
+	union {
+		int i;
+		unsigned char c[4];
+	} ires;
 	unsigned int maxval = (1 << (snd_pcm_format_width(format) - 1)) - 1;
 	int bps = snd_pcm_format_width(format) / 8;  /* bytes per sample */
 	
@@ -59,8 +62,8 @@ static void generate_sine(const snd_pcm_channel_area_t *areas,
 	/* fill the channel areas */
 	while (count-- > 0) {
 		res = sin(phase) * maxval;
-		ires = res;
-		tmp = (unsigned char *)(&ires);
+		ires.i = res;
+		tmp = ires.c;
 		for (chn = 0; chn < channels; chn++) {
 			for (byte = 0; byte < (unsigned int)bps; byte++)
 				*(samples[chn] + byte) = tmp[byte];
@@ -868,7 +871,7 @@ int main(int argc, char *argv[])
 	if (verbose > 0)
 		snd_pcm_dump(handle, output);
 
-	samples = malloc((period_size * channels * snd_pcm_format_width(format)) / 8);
+	samples = malloc((period_size * channels * snd_pcm_format_physical_width(format)) / 8);
 	if (samples == NULL) {
 		printf("No enough memory\n");
 		exit(EXIT_FAILURE);
@@ -881,8 +884,8 @@ int main(int argc, char *argv[])
 	}
 	for (chn = 0; chn < channels; chn++) {
 		areas[chn].addr = samples;
-		areas[chn].first = chn * snd_pcm_format_width(format);
-		areas[chn].step = channels * snd_pcm_format_width(format);
+		areas[chn].first = chn * snd_pcm_format_physical_width(format);
+		areas[chn].step = channels * snd_pcm_format_physical_width(format);
 	}
 
 	err = transfer_methods[method].transfer_loop(handle, samples, areas);

Re: S24_LE is 3 bytes ? in alsa-lib example pcm.c

[Jaroslav Kysela]

On Thu, 10 Jul 2008, Takashi Iwai wrote:

> At Thu, 10 Jul 2008 12:19:05 +0200 (CEST),
> Jaroslav Kysela wrote:
> > 
> > On Thu, 10 Jul 2008, Clemens Ladisch wrote:
> > 
> > > Norbert van Bolhuis wrote:
> > > > as far as I know:
> > > > sample format S24_LE is 24bit sample in 4 bytes
> > > > sample format S24_3LE is 24bit sample in 3 bytes
> > > > (why else have separate definitions), right ?
> > > 
> > > Yes.
> > > 
> > > > In the ALSA-LIb example /test/pcm.c however
> > > > (http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html)
> > > > there is no diff between the 2 sample formats.
> > > 
> > > That example program uses snd_pcm_format_width() although it should
> > > have used snd_pcm_format_physical_width() when calculating memory
> > > buffer sizes.
> > 
> > I think that the problem is in wrong area->step calculation in alsa-lib, 
> > but I'm still investigating where the real culprit is. The
> > snd_pcm_format_width() is used only for sample filling (which is OK).
> 
> I think Clemens is right.  The program uses snd_pcm_format_width()
> wrongly.  Also, it won't work properly with strict aliasing...

Yes, the patch looks good. I overlooked that the area initialization is 
directly in pcm.c code (I looked only to generate_sine(). I tested it and 
it produces good samples. I'm getting some underruns on my notebook with 
24-bit formats (but it's probably different problem).

					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: S24_LE is 3 bytes ? in alsa-lib example pcm.c

[Takashi Iwai]

At Thu, 10 Jul 2008 12:41:39 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Thu, 10 Jul 2008, Takashi Iwai wrote:
> 
> > At Thu, 10 Jul 2008 12:19:05 +0200 (CEST),
> > Jaroslav Kysela wrote:
> > > 
> > > On Thu, 10 Jul 2008, Clemens Ladisch wrote:
> > > 
> > > > Norbert van Bolhuis wrote:
> > > > > as far as I know:
> > > > > sample format S24_LE is 24bit sample in 4 bytes
> > > > > sample format S24_3LE is 24bit sample in 3 bytes
> > > > > (why else have separate definitions), right ?
> > > > 
> > > > Yes.
> > > > 
> > > > > In the ALSA-LIb example /test/pcm.c however
> > > > > (http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html)
> > > > > there is no diff between the 2 sample formats.
> > > > 
> > > > That example program uses snd_pcm_format_width() although it should
> > > > have used snd_pcm_format_physical_width() when calculating memory
> > > > buffer sizes.
> > > 
> > > I think that the problem is in wrong area->step calculation in alsa-lib, 
> > > but I'm still investigating where the real culprit is. The
> > > snd_pcm_format_width() is used only for sample filling (which is OK).
> > 
> > I think Clemens is right.  The program uses snd_pcm_format_width()
> > wrongly.  Also, it won't work properly with strict aliasing...
> 
> Yes, the patch looks good. I overlooked that the area initialization is 
> directly in pcm.c code (I looked only to generate_sine(). I tested it and 
> it produces good samples. I'm getting some underruns on my notebook with 
> 24-bit formats (but it's probably different problem).

OK, committed now.


Takashi