[PATCH] plugin_ops.h fixes

[PATCH] plugin_ops.h fixes

[Clemens Ladisch]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]


This macro in plugin_ops.h
	#ifdef __i386__
	#define _get_triple_le(ptr) (*(u_int32_t*)(ptr) & 0xffffff)
tries to access four bytes through the pointer. If the three bytes of
valid data are at the end of a page and the next page isn't mapped,
this results in an exception.
The companion macro
	#define _get_triple_be(ptr) (bswap_32(*(u_int32_t*)(ptr)) & 0xffffff)
suffers from the same problem, and, additionally, is wrong (it lacks
">> 8" after the bswap).
I fixed both macros by removing them. :-)

There is a typo in get16_1230_B2.

The put16_labels array is declared as having 128 elements instead
of 16.

The gets_* code doesn't correctly extend the sign of the value in every
case.

put_12_A1 should have been put_12_29, and the codes for put_0123_* use
the nonexistent macros as_s24/as_u24. (This doesn't really matter as
put_* isn't used anyway.)

The _norms function appears to be badly broken (_min and _max
interchanged, using 32/64 bits for 24/32 bit values). I didn't attempt
to fix it as it isn't used, either.

And the summing/normalization code in pcm_route.c treats the sample as
if it were unsigned.


Clemens

[-- Attachment #2: lib-pcm.diff --]
[-- Type: application/octet-stream, Size: 6090 bytes --]

Index: src/pcm/pcm_route.c
===================================================================
RCS file: /cvsroot/alsa/alsa-lib/src/pcm/pcm_route.c,v
retrieving revision 1.65
diff -u -r1.65 pcm_route.c
--- src/pcm/pcm_route.c	26 Jun 2002 02:04:12 -0000	1.65
+++ src/pcm/pcm_route.c	8 Jul 2002 08:08:48 -0000
@@ -344,7 +344,7 @@
 				sum.as_sint32 += sample;
 			goto after_sum;
 		add_int64_att:
-			sum.as_sint64 += (u_int64_t) sample * ttp->as_int;
+			sum.as_sint64 += (int64_t)sample * ttp->as_int;
 			goto after_sum;
 		add_int64_noatt:
 			if (ttp->as_int)
@@ -408,8 +408,10 @@
 
 	norm_int64_0_noatt:
 	norm_int:
-		if (sum.as_sint64 > (u_int32_t)0xffffffff)
-			sample = (u_int32_t)0xffffffff;
+		if (sum.as_sint64 >= 0x7fffffffLL)
+			sample = 0x7fffffff;
+		else if (sum.as_sint64 <= -0x80000000LL)
+			sample = 0x80000000;
 		else
 			sample = sum.as_sint64;
 		goto after_norm;
@@ -427,8 +429,10 @@
 	norm_float_0:
 	norm_float:
 		sum.as_float = floor(sum.as_float + 0.5);
-		if (sum.as_float > (u_int32_t)0xffffffff)
-			sample = (u_int32_t)0xffffffff;
+		if (sum.as_float >= 2147483647.0F)
+			sample = 0x7fffffff;
+		else if (sum.as_float <= -2147483648.0F)
+			sample = 0x80000000;
 		else
 			sample = sum.as_float;
 		goto after_norm;
Index: src/pcm/plugin_ops.h
===================================================================
RCS file: /cvsroot/alsa/alsa-lib/src/pcm/plugin_ops.h,v
retrieving revision 1.13
diff -u -r1.13 plugin_ops.h
--- src/pcm/plugin_ops.h	26 Jun 2002 02:04:12 -0000	1.13
+++ src/pcm/plugin_ops.h	8 Jul 2002 08:08:48 -0000
@@ -48,13 +48,8 @@
 #define as_floatc(ptr) (*(const float_t*)(ptr))
 #define as_doublec(ptr) (*(const double_t*)(ptr))
 
-#ifdef __i386__
-#define _get_triple_le(ptr) (*(u_int32_t*)(ptr) & 0xffffff)
-#define _get_triple_be(ptr) (bswap_32(*(u_int32_t*)(ptr)) & 0xffffff)
-#else
 #define _get_triple_le(ptr) (*(u_int8_t*)(ptr) | (u_int32_t)*((u_int8_t*)(ptr) + 1) << 8 | (u_int32_t)*((u_int8_t*)(ptr) + 2) << 16)
 #define _get_triple_be(ptr) ((u_int32_t)*(u_int8_t*)(ptr) << 16 | (u_int32_t)*((u_int8_t*)(ptr) + 1) << 8 | *((u_int8_t*)(ptr) + 2))
-#endif
 #define _put_triple_le(ptr,val) do { \
 	u_int8_t *_tmp = (u_int8_t *)(ptr); \
 	u_int32_t _val = (val); \
@@ -381,7 +376,7 @@
 get16_0123_12: sample = as_u32c(src) >> 8; goto GET16_END;
 get16_0123_92: sample = (as_u32c(src) >> 8) ^ 0x8000; goto GET16_END;
 get16_1230_32: sample = bswap_16(as_u32c(src) >> 8); goto GET16_END;
-get16_1230_B2: sample = bswap_16((as_u32c(src) >> 8) ^ 0x8000); goto GET16_END;
+get16_1230_B2: sample = bswap_16((as_u32c(src) >> 8) ^ 0x80); goto GET16_END;
 get16_1234_12: sample = as_u32c(src) >> 16; goto GET16_END;
 get16_1234_92: sample = (as_u32c(src) >> 16) ^ 0x8000; goto GET16_END;
 get16_1234_43: sample = bswap_16(as_u32c(src)); goto GET16_END;
@@ -403,7 +398,7 @@
 
 #ifdef PUT16_LABELS
 /* dst_wid dst_endswap sign_toggle */
-static void *put16_labels[4 * 2 * 2 * 4 * 2] = {
+static void *put16_labels[4 * 2 * 2] = {
 	&&put16_12_1,		 /* 16h ->  8h */
 	&&put16_12_9,		 /* 16h ^>  8h */
 	&&put16_12_1,		 /* 16h ->  8s */
@@ -664,17 +659,17 @@
 #ifdef GETS_END
 while (0) {
 gets_1_1: sample = as_s8c(src); goto GETS_END;
-gets_1_9: sample = as_s8c(src) ^ 0x80; goto GETS_END;
+gets_1_9: sample = (int8_t)(as_s8c(src) ^ 0x80); goto GETS_END;
 gets_12_12: sample = as_s16c(src); goto GETS_END;
-gets_12_92: sample = as_s16c(src) ^ 0x8000; goto GETS_END;
-gets_12_21: sample = bswap_16(as_s16c(src)); goto GETS_END;
+gets_12_92: sample = (int16_t)(as_s16c(src) ^ 0x8000); goto GETS_END;
+gets_12_21: sample = (int16_t)bswap_16(as_s16c(src)); goto GETS_END;
 gets_12_A1: sample = (int16_t)bswap_16(as_s16c(src) ^ 0x80); goto GETS_END;
-gets_0123_0123: sample = as_s32c(src); goto GETS_END;
-gets_0123_0923: sample = (as_s32c(src) ^ 0x800000); goto GETS_END;
-gets_1230_0321: sample = (int32_t)bswap_32(as_s32c(src)); goto GETS_END;
-gets_1230_0B21: sample = (int32_t)bswap_32(as_s32c(src) ^ 0x8000); goto GETS_END;
+gets_0123_0123: sample = (int32_t)(as_s32c(src) << 8) >> 8; goto GETS_END;
+gets_0123_0923: sample = (int32_t)((as_s32c(src) ^ 0x800000) << 8) >> 8; goto GETS_END;
+gets_1230_0321: sample = (int32_t)(bswap_32(as_s32c(src)) << 8) >> 8; goto GETS_END;
+gets_1230_0B21: sample = (int32_t)(bswap_32(as_s32c(src) ^ 0x8000) << 8) >> 8; goto GETS_END;
 gets_1234_1234: sample = as_s32c(src); goto GETS_END;
-gets_1234_9234: sample = as_s32c(src) ^ 0x80000000; goto GETS_END;
+gets_1234_9234: sample = (int32_t)(as_s32c(src) ^ 0x80000000); goto GETS_END;
 gets_1234_4321: sample = (int32_t)bswap_32(as_s32c(src)); goto GETS_END;
 gets_1234_C321: sample = (int32_t)bswap_32(as_s32c(src) ^ 0x80); goto GETS_END;
 }
@@ -690,7 +685,7 @@
 	&&put_12_12,		/* 16h -> 16h */
 	&&put_12_92,		/* 16h ^> 16h */
 	&&put_12_21,		/* 16h -> 16s */
-	&&put_12_A1,		/* 16h ^> 16s */
+	&&put_12_29,		/* 16h ^> 16s */
 	&&put_0123_0123,	/* 24h -> 24h */
 	&&put_0123_0923,	/* 24h ^> 24h */
 	&&put_0123_3210,	/* 24h -> 24s */
@@ -708,11 +703,12 @@
 put_12_12: as_s16(dst) = sample; goto PUT_END;
 put_12_92: as_u16(dst) = sample ^ 0x8000; goto PUT_END;
 put_12_21: as_s16(dst) = bswap_16(sample); goto PUT_END;
-put_12_A1: as_u16(dst) = bswap_16(sample ^ 0x80); goto PUT_END;
-put_0123_0123: as_s24(dst) = sample; goto PUT_END;
-put_0123_0923: as_u24(dst) = sample ^ 0x800000; goto PUT_END;
-put_0123_3210: as_s24(dst) = bswap_32(sample); goto PUT_END;
-put_0123_3290: as_u24(dst) = bswap_32(sample) ^ 0x8000; goto PUT_END;
+put_12_29: as_u16(dst) = bswap_16(sample) ^ 0x80; goto PUT_END;
+/* this always writes the unused byte in 24-bit formats as 0x00 */
+put_0123_0123: as_s32(dst) = sample & 0x00ffffff; goto PUT_END;
+put_0123_0923: as_u32(dst) = (sample & 0x00ffffff) ^ 0x800000; goto PUT_END;
+put_0123_3210: as_s32(dst) = bswap_32(sample) & 0xffffff00; goto PUT_END;
+put_0123_3290: as_u32(dst) = (bswap_32(sample) & 0xffffff00) ^ 0x8000; goto PUT_END;
 put_1234_1234: as_s32(dst) = sample; goto PUT_END;
 put_1234_9234: as_u32(dst) = sample ^ 0x80000000; goto PUT_END;
 put_1234_4321: as_s32(dst) = bswap_32(sample); goto PUT_END;

Re: [PATCH] plugin_ops.h fixes

[Takashi Iwai]

At Mon, 08 Jul 2002 11:02:25 +0200,
Clemens Ladisch wrote:
> 
> This macro in plugin_ops.h
> 	#ifdef __i386__
> 	#define _get_triple_le(ptr) (*(u_int32_t*)(ptr) & 0xffffff)
> tries to access four bytes through the pointer. If the three bytes of
> valid data are at the end of a page and the next page isn't mapped,
> this results in an exception.

right...

> The companion macro
> 	#define _get_triple_be(ptr) (bswap_32(*(u_int32_t*)(ptr)) & 0xffffff)
> suffers from the same problem, and, additionally, is wrong (it lacks
> ">> 8" after the bswap).
> I fixed both macros by removing them. :-)
 
yes, thanks.


> There is a typo in get16_1230_B2.
> 
> The put16_labels array is declared as having 128 elements instead
> of 16.
> 
> The gets_* code doesn't correctly extend the sign of the value in every
> case.
> 
> put_12_A1 should have been put_12_29, and the codes for put_0123_* use
> the nonexistent macros as_s24/as_u24. (This doesn't really matter as
> put_* isn't used anyway.)
> 
> The _norms function appears to be badly broken (_min and _max
> interchanged, using 32/64 bits for 24/32 bit values). I didn't attempt
> to fix it as it isn't used, either.
 
yes.  we can remove this stuff.
Jaroslav, any plan to use still this one?

> And the summing/normalization code in pcm_route.c treats the sample as
> if it were unsigned.

nice spot.

patches are applied to cvs.


thanks,

Takashi


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Oh, it's good to be a geek.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Abramo Bagnara]

Takashi Iwai wrote:
> 
> >
> > The _norms function appears to be badly broken (_min and _max
> > interchanged, using 32/64 bits for 24/32 bit values). I didn't attempt
> > to fix it as it isn't used, either.

min & max are interchanged indeed. My bad...
Use of 32/64 bits for 24/32 bit is wanted. Take in account that this
function is called on mix results (where I need to avoid wrap before
normalization).

> 
> yes.  we can remove this stuff.
> Jaroslav, any plan to use still this one?

Please don't remove it. I've written it for pcm_mix. Maybe one day I'll
find the time to finish it.

> 
> > And the summing/normalization code in pcm_route.c treats the sample as
> > if it were unsigned.
> 
> nice spot.

Be careful here. Use of signed vs. unsigned was an hard decision.
Can you point me exactly where you see a problem?


-- 
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:ThinkGeek
Oh, it's good to be a geek.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Clemens Ladisch]

Abramo Bagnara wrote:
> min & max are interchanged indeed. My bad...

Oh, there's no problem with using _max instead of _min  -- I just
noticed that the code for _max writes 0 for unsigned 16/24/32-bit
values ...  ;o)

> Use of 32/64 bits for 24/32 bit is wanted. Take in account that this
> function is called on mix results (where I need to avoid wrap before
> normalization).

Then it should use bigger types for 8/16 bits, too. Checking for a type
to overflow its own valid range seems to be rather pointless to me.

> > > And the summing/normalization code in pcm_route.c treats the sample as
> > > if it were unsigned.
> 
> Be careful here. Use of signed vs. unsigned was an hard decision.
> Can you point me exactly where you see a problem?

In the following code snippet

	add_int64_att:
		sum.as_sint64 += (u_int64_t) sample * ttp->as_int;

each of the three variables has a signed integer type. Converting
the sample from int32_t to u_int64_t would give wrong results for
negative values, e.g., the sample -1 (0xffffffff) would be converted
to 4294967295 (0x00000000ffffffff).

The following code

	norm_int:
		if (sum.as_sint64 > (u_int32_t)0xffffffff)
			sample = (u_int32_t)0xffffffff;
		else
			sample = sum.as_sint64;

doesn't check for negative values of sum.as_sint64. Additionally,
using 0xffffffff in case of overflow results in sample == -1.

These code snippets would be correct if the sample would be unsigned,
but my point was that this isn't the case.


Clemens


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Stuff, things, and much much more.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Abramo Bagnara]

Clemens Ladisch wrote:
> 
> Abramo Bagnara wrote:
> > min & max are interchanged indeed. My bad...
> 
> Oh, there's no problem with using _max instead of _min  -- I just
> noticed that the code for _max writes 0 for unsigned 16/24/32-bit
> values ...  ;o)

Fixed now in CVS.

> 
> > Use of 32/64 bits for 24/32 bit is wanted. Take in account that this
> > function is called on mix results (where I need to avoid wrap before
> > normalization).
> 
> Then it should use bigger types for 8/16 bits, too. Checking for a type
> to overflow its own valid range seems to be rather pointless to me.

It *does* use bigger types.

> 
> > > > And the summing/normalization code in pcm_route.c treats the sample as
> > > > if it were unsigned.
> >
> > Be careful here. Use of signed vs. unsigned was an hard decision.
> > Can you point me exactly where you see a problem?
> 
> In the following code snippet
> 
>         add_int64_att:
>                 sum.as_sint64 += (u_int64_t) sample * ttp->as_int;
> 
> each of the three variables has a signed integer type. Converting
> the sample from int32_t to u_int64_t would give wrong results for
> negative values, e.g., the sample -1 (0xffffffff) would be converted
> to 4294967295 (0x00000000ffffffff).
> 
> The following code
> 
>         norm_int:
>                 if (sum.as_sint64 > (u_int32_t)0xffffffff)
>                         sample = (u_int32_t)0xffffffff;
>                 else
>                         sample = sum.as_sint64;
> 
> doesn't check for negative values of sum.as_sint64. Additionally,
> using 0xffffffff in case of overflow results in sample == -1.
> 
> These code snippets would be correct if the sample would be unsigned,
> but my point was that this isn't the case.

I've just checked: this has been broken by Jaroslav with CVS version
1.55. I'd like to restore the original behaviour I wrote (that used
unsigned for efficiency reason), but I don't understand what Jaroslav
tried to solve.

Jaroslav?

-- 
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:ThinkGeek
Stuff, things, and much much more.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Clemens Ladisch]

Abramo Bagnara wrote:
> Clemens Ladisch wrote:
> > Abramo Bagnara wrote:
> > > Use of 32/64 bits for 24/32 bit is wanted. Take in account that this
> > > function is called on mix results (where I need to avoid wrap before
> > > normalization).
> >
> > Then it should use bigger types for 8/16 bits, too. Checking for a type
> > to overflow its own valid range seems to be rather pointless to me.
> 
> It *does* use bigger types.

In rev. 1.14 of plugin_ops.h, the beginning of the _norms() function is:

	switch (src_wid) {
	case 8:
		s = *(int8_t*)src;
		if (s >= 0x7f)
			goto _max;
		else if (s <= -0x80)
			goto _min;
		break;
	case 16:
		s = *(int16_t*)src;
		if (s >= 0x7fff)
			goto _max;
		else if (s <= -0x8000)
			goto _min;
		break;

The reads through the src pointer _are_ using 8 and 16 bits.


Anyway, nobody has complained about my changes to _get_triple_*/
get16_1230_B2/put16_labels/gets_*/put_*, so I think these can be
committed now.
Takashi?


Clemens


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Stuff, things, and much much more.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Takashi Iwai]

At Tue, 09 Jul 2002 13:23:14 +0200,
Clemens Ladisch wrote:
> 
> Anyway, nobody has complained about my changes to _get_triple_*/
> get16_1230_B2/put16_labels/gets_*/put_*, so I think these can be
> committed now.
> Takashi?

yes, already committed yesterday.
sorry, forget to announce to you.


Takashi


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Stuff, things, and much much more.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Clemens Ladisch]

Takashi Iwai wrote:
> > Anyway, nobody has complained about my changes to _get_triple_*/
> > get16_1230_B2/put16_labels/gets_*/put_*, so I think these can be
> > committed now.
> 
> yes, already committed yesterday.

I don't see them in the CVS.


Clemens


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Stuff, things, and much much more.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Takashi Iwai]

At Tue, 09 Jul 2002 14:21:58 +0200,
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > > Anyway, nobody has complained about my changes to _get_triple_*/
> > > get16_1230_B2/put16_labels/gets_*/put_*, so I think these can be
> > > committed now.
> > 
> > yes, already committed yesterday.
> 
> I don't see them in the CVS.

oops, was pending on my queue.  committed really now :)


Takashi


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Stuff, things, and much much more.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Abramo Bagnara]

Clemens Ladisch wrote:
> 
> Abramo Bagnara wrote:
> > Clemens Ladisch wrote:
> > > Abramo Bagnara wrote:
> > > > Use of 32/64 bits for 24/32 bit is wanted. Take in account that this
> > > > function is called on mix results (where I need to avoid wrap before
> > > > normalization).
> > >
> > > Then it should use bigger types for 8/16 bits, too. Checking for a type
> > > to overflow its own valid range seems to be rather pointless to me.
> >
> > It *does* use bigger types.
> 
> In rev. 1.14 of plugin_ops.h, the beginning of the _norms() function is:
> 
>         switch (src_wid) {
>         case 8:
>                 s = *(int8_t*)src;
>                 if (s >= 0x7f)
>                         goto _max;
>                 else if (s <= -0x80)
>                         goto _min;
>                 break;
>         case 16:
>                 s = *(int16_t*)src;
>                 if (s >= 0x7fff)
>                         goto _max;
>                 else if (s <= -0x8000)
>                         goto _min;
>                 break;
> 
> The reads through the src pointer _are_ using 8 and 16 bits.

Now I see... thank you. I've fixed it now.


-- 
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:ThinkGeek
Stuff, things, and much much more.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Jaroslav Kysela]

On Tue, 9 Jul 2002, Abramo Bagnara wrote:

> > > > > And the summing/normalization code in pcm_route.c treats the sample as
> > > > > if it were unsigned.
> > >
> > > Be careful here. Use of signed vs. unsigned was an hard decision.
> > > Can you point me exactly where you see a problem?
> > 
> > In the following code snippet
> > 
> >         add_int64_att:
> >                 sum.as_sint64 += (u_int64_t) sample * ttp->as_int;
> > 
> > each of the three variables has a signed integer type. Converting
> > the sample from int32_t to u_int64_t would give wrong results for
> > negative values, e.g., the sample -1 (0xffffffff) would be converted
> > to 4294967295 (0x00000000ffffffff).
> > 
> > The following code
> > 
> >         norm_int:
> >                 if (sum.as_sint64 > (u_int32_t)0xffffffff)
> >                         sample = (u_int32_t)0xffffffff;
> >                 else
> >                         sample = sum.as_sint64;
> > 
> > doesn't check for negative values of sum.as_sint64. Additionally,
> > using 0xffffffff in case of overflow results in sample == -1.
> > 
> > These code snippets would be correct if the sample would be unsigned,
> > but my point was that this isn't the case.
> 
> I've just checked: this has been broken by Jaroslav with CVS version
> 1.55. I'd like to restore the original behaviour I wrote (that used
> unsigned for efficiency reason), but I don't understand what Jaroslav
> tried to solve.
> 
> Jaroslav?

You cannot use unsigned values, because we need to sum samples (it means: 
add positive and substract negative ones). If we are in positive range, we 
can only add samples, so the final sum result is wrong.

I've fixed the normalization (hopefully) for int64 and float values. Could 
you, guys, verify new code?

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project  http://www.alsa-project.org
SuSE Linux    http://www.suse.com



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Two, two, TWO treats in one.
http://thinkgeek.com/sf

another problems with signed arithmetic snd_pcm_mmap_playback_avail

[tomasz motylewski]

> You cannot use unsigned values, because we need to sum samples (it means: 
> add positive and substract negative ones). If we are in positive range, we 
> can only add samples, so the final sum result is wrong.

Since the bug database at sourceforge seems to be completely forgotten I am
reposting it here: 
http://sourceforge.net/tracker/?func=detail&aid=576153&group_id=27464&atid=390601

snd_pcm_mmap_*_avail may return negative value on some 
occasions. CONSIDER: 

 buffer_size=80 (0x50), then boundary=0x50000000 
 let *appl.ptr= 0 and *hw.ptr=0x80000051 

      avail = *pcm->hw.ptr + pcm->buffer_size - 
          *pcm->appl.ptr; 
      // 0x80000001 == - 0x7FFFFFFF 
      if (avail < 0) 
          avail += pcm->boundary; 
      else if ((snd_pcm_uframes_t) avail >= pcm->boundary) 
          avail -= pcm->boundary; 
      // here avail == 0xD0000001 == -0x2FFFFFFF 

This is not theoretical case. My application playing at 
8 kHz crashes after 3 days (0x10000000/1000 seconds) 

Q: should the driver guarantee hw.ptr < boundary ???

I am keeping *appl.ptr at 0 and using 
snd_pcm_mmap_*_avail to give me the position to write 
into mmaped buffer. I am mixing streams directly in the 
buffer, and do not care if some streams do not come. 

These functions should simply return 
((unsigned long)avail) % boundary 
instead of that funny += -= cases. 

And why there is no function which just simply returns 
the value of 
((unsigned long)*pcm->hw.ptr)% boundary 
that is all what I need. 

I have to use at the moment: 

static inline int snd_pcm_gethwpos(snd_pcm_t *pcm) { 
    snd_pcm_sframes_t ret = snd_pcm_avail_update(pcm); 
    ret += snd_pcm_commited; 
    if(ret<0) { 
    	ret = ((unsigned long) 
       	ret)%buffer_size; 
    } 
    return snd_pcm_avail_update(pcm)%buffer_size; 
} 

Best regards,
--
Tomasz Motylewski



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Two, two, TWO treats in one.
http://thinkgeek.com/sf

Re: another problems with signed arithmetic snd_pcm_mmap_playback_avail

[tomasz motylewski]

> I have to use at the moment: 
> 
> static inline int snd_pcm_gethwpos(snd_pcm_t *pcm) { 
>     snd_pcm_sframes_t ret = snd_pcm_avail_update(pcm); 
>     ret += snd_pcm_commited; 
>     if(ret<0) { 
>     	ret = ((unsigned long) 
>        	ret)%buffer_size; 
>     } 
>     return snd_pcm_avail_update(pcm)%buffer_size; 

BUG. I should have used:

	return ret;

But anyway, what do you think about snd_pcm_avail_update returning negative
values?

Best regards,
--
Tomasz Motylewski



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Two, two, TWO treats in one.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Abramo Bagnara]

Jaroslav Kysela wrote:
> 
> On Tue, 9 Jul 2002, Abramo Bagnara wrote:
> 
> > > > > > And the summing/normalization code in pcm_route.c treats the sample as
> > > > > > if it were unsigned.
> > > >
> > > > Be careful here. Use of signed vs. unsigned was an hard decision.
> > > > Can you point me exactly where you see a problem?
> > >
> > > In the following code snippet
> > >
> > >         add_int64_att:
> > >                 sum.as_sint64 += (u_int64_t) sample * ttp->as_int;
> > >
> > > each of the three variables has a signed integer type. Converting
> > > the sample from int32_t to u_int64_t would give wrong results for
> > > negative values, e.g., the sample -1 (0xffffffff) would be converted
> > > to 4294967295 (0x00000000ffffffff).
> > >
> > > The following code
> > >
> > >         norm_int:
> > >                 if (sum.as_sint64 > (u_int32_t)0xffffffff)
> > >                         sample = (u_int32_t)0xffffffff;
> > >                 else
> > >                         sample = sum.as_sint64;
> > >
> > > doesn't check for negative values of sum.as_sint64. Additionally,
> > > using 0xffffffff in case of overflow results in sample == -1.
> > >
> > > These code snippets would be correct if the sample would be unsigned,
> > > but my point was that this isn't the case.
> >
> > I've just checked: this has been broken by Jaroslav with CVS version
> > 1.55. I'd like to restore the original behaviour I wrote (that used
> > unsigned for efficiency reason), but I don't understand what Jaroslav
> > tried to solve.
> >
> > Jaroslav?
> 
> You cannot use unsigned values, because we need to sum samples (it means:
> add positive and substract negative ones). If we are in positive range, we
> can only add samples, so the final sum result is wrong.

Wow, I've never realized this...
So mixing can only be done with signed values, otherwise we have bogus
value for overranges. Right?

Taken this in account, I'm tempted to consider unsigned formats as
second class citizen (i.e. format hard to handle and with some added
difficulties wrt manipulation). I'm wrong?

-- 
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:ThinkGeek
Two, two, TWO treats in one.
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Jaroslav Kysela]

On Thu, 11 Jul 2002, Abramo Bagnara wrote:

> Jaroslav Kysela wrote:
> > 
> > On Tue, 9 Jul 2002, Abramo Bagnara wrote:
> > 
> > > > > > > And the summing/normalization code in pcm_route.c treats the sample as
> > > > > > > if it were unsigned.
> > > > >
> > > > > Be careful here. Use of signed vs. unsigned was an hard decision.
> > > > > Can you point me exactly where you see a problem?
> > > >
> > > > In the following code snippet
> > > >
> > > >         add_int64_att:
> > > >                 sum.as_sint64 += (u_int64_t) sample * ttp->as_int;
> > > >
> > > > each of the three variables has a signed integer type. Converting
> > > > the sample from int32_t to u_int64_t would give wrong results for
> > > > negative values, e.g., the sample -1 (0xffffffff) would be converted
> > > > to 4294967295 (0x00000000ffffffff).
> > > >
> > > > The following code
> > > >
> > > >         norm_int:
> > > >                 if (sum.as_sint64 > (u_int32_t)0xffffffff)
> > > >                         sample = (u_int32_t)0xffffffff;
> > > >                 else
> > > >                         sample = sum.as_sint64;
> > > >
> > > > doesn't check for negative values of sum.as_sint64. Additionally,
> > > > using 0xffffffff in case of overflow results in sample == -1.
> > > >
> > > > These code snippets would be correct if the sample would be unsigned,
> > > > but my point was that this isn't the case.
> > >
> > > I've just checked: this has been broken by Jaroslav with CVS version
> > > 1.55. I'd like to restore the original behaviour I wrote (that used
> > > unsigned for efficiency reason), but I don't understand what Jaroslav
> > > tried to solve.
> > >
> > > Jaroslav?
> > 
> > You cannot use unsigned values, because we need to sum samples (it means:
> > add positive and substract negative ones). If we are in positive range, we
> > can only add samples, so the final sum result is wrong.
> 
> Wow, I've never realized this...
> So mixing can only be done with signed values, otherwise we have bogus
> value for overranges. Right?
> 
> Taken this in account, I'm tempted to consider unsigned formats as
> second class citizen (i.e. format hard to handle and with some added
> difficulties wrt manipulation). I'm wrong?

Yes, unsigned formats are dangerous in the sense of arithmetics.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project  http://www.alsa-project.org
SuSE Linux    http://www.suse.com



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
PC Mods, Computing goodies, cases & more
http://thinkgeek.com/sf

Re: [PATCH] plugin_ops.h fixes

[Clemens Ladisch]

Jaroslav Kysela wrote:
> I've fixed the normalization (hopefully) for int64 and float values. Could
> you, guys, verify new code?

In the following test

	else if (sum.as_xxx < (int64_t)-0x80000000)

0x80000000 is an unsigned integer, and, according to C rules, negating an
unsigned integer results in an unsigned integer, in this case 0x80000000.
Converting this to 64 bits yields 0x0000000080000000. *Ouch*

Replacing both tests with

	else if (sum.as_xxx < (int64_t)(int32_t)0x80000000)

should finally work.


And
> > > (...) the following code snippet
> > >
> > >         add_int64_att:
> > >                 sum.as_sint64 += (u_int64_t) sample * ttp->as_int;
> > >
still treats the sample as unsigned.


Clemens



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
PC Mods, Computing goodies, cases & more
http://thinkgeek.com/sf