Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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)-0x8000) 0x8000 is an unsigned integer, and, according to C rules, negating an unsigned integer results in an unsigned integer, in this case 0x8000. Converting this to 64 bits yields 0x8000. *Ouch* Replacing both tests with else if (sum.as_xxx < (int64_t)(int32_t)0x8000) 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 ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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 (0x) would be converted > > > > to 4294967295 (0x). > > > > > > > > The following code > > > > > > > > norm_int: > > > > if (sum.as_sint64 > (u_int32_t)0x) > > > > sample = (u_int32_t)0x; > > > > else > > > > sample = sum.as_sint64; > > > > > > > > doesn't check for negative values of sum.as_sint64. Additionally, > > > > using 0x 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 <[EMAIL PROTECTED]> Linux Kernel Sound Maintainer ALSA Project http://www.alsa-project.org SuSE Linuxhttp://www.suse.com --- This sf.net email is sponsored by:ThinkGeek PC Mods, Computing goodies, cases & more http://thinkgeek.com/sf ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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 (0x) would be converted > > > to 4294967295 (0x). > > > > > > The following code > > > > > > norm_int: > > > if (sum.as_sint64 > (u_int32_t)0x) > > > sample = (u_int32_t)0x; > > > else > > > sample = sum.as_sint64; > > > > > > doesn't check for negative values of sum.as_sint64. Additionally, > > > using 0x 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:[EMAIL PROTECTED] 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 ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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 (0x) would be converted > > to 4294967295 (0x). > > > > The following code > > > > norm_int: > > if (sum.as_sint64 > (u_int32_t)0x) > > sample = (u_int32_t)0x; > > else > > sample = sum.as_sint64; > > > > doesn't check for negative values of sum.as_sint64. Additionally, > > using 0x 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 <[EMAIL PROTECTED]> Linux Kernel Sound Maintainer ALSA Project http://www.alsa-project.org SuSE Linuxhttp://www.suse.com --- This sf.net email is sponsored by:ThinkGeek Two, two, TWO treats in one. http://thinkgeek.com/sf ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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:[EMAIL PROTECTED]
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
___
Alsa-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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 ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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 ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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 ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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
___
Alsa-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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 (0x) would be converted > to 4294967295 (0x). > > The following code > > norm_int: > if (sum.as_sint64 > (u_int32_t)0x) > sample = (u_int32_t)0x; > else > sample = sum.as_sint64; > > doesn't check for negative values of sum.as_sint64. Additionally, > using 0x 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:[EMAIL PROTECTED] 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 ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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 (0x) would be converted to 4294967295 (0x). The following code norm_int: if (sum.as_sint64 > (u_int32_t)0x) sample = (u_int32_t)0x; else sample = sum.as_sint64; doesn't check for negative values of sum.as_sint64. Additionally, using 0x 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 ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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:[EMAIL PROTECTED] 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 ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
Re: [Alsa-devel] [PATCH] plugin_ops.h fixes
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) & 0xff) > 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)) & 0xff) > 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 ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
