| Message ID | 1331578211-18232-4-git-send-email-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 12 Mar 2012, Marc-Andr? Lureau wrote: > It's more appropriate to set the maximum value into a fitting integer. > --- > audio/audio.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/audio/audio.c b/audio/audio.c > index bd9237e..06c2384 100644 > --- a/audio/audio.c > +++ b/audio/audio.c > @@ -110,8 +110,8 @@ const struct mixeng_volume nominal_volume = { > .r = 1.0, > .l = 1.0, > #else > - .r = 1ULL << 32, > - .l = 1ULL << 32, > + .r = UINT_MAX, > + .l = UINT_MAX, > #endif > }; How's putting UINT_MAX into int64_t more appropriate? UINT_MAX is even one less than one in 32.32 fixpoint... I must be missing something here.
On Mon, Mar 12, 2012 at 9:03 PM, malc <av1474@comtv.ru> wrote: > How's putting UINT_MAX into int64_t more appropriate? UINT_MAX is > even one less than one in 32.32 fixpoint... I must be missing something > here. Right, the patch series used to have 2^32 -1, so it can fit in a 16bits or 32bits integer. So what I meant is rather G_MAXUINT32.
On Mon, 12 Mar 2012, Marc-Andr? Lureau wrote: > On Mon, Mar 12, 2012 at 9:03 PM, malc <av1474@comtv.ru> wrote: > > How's putting UINT_MAX into int64_t more appropriate? UINT_MAX is > > even one less than one in 32.32 fixpoint... I must be missing something > > here. > > Right, the patch series used to have 2^32 -1, so it can fit in a > 16bits or 32bits integer. > > So what I meant is rather G_MAXUINT32. Just leave it as is, it's perfectly fine.
On Mon, Mar 12, 2012 at 9:29 PM, malc <av1474@comtv.ru> wrote: >> So what I meant is rather G_MAXUINT32. > > Just leave it as is, it's perfectly fine. But it's not convenient to fit into a regular integer, as the number of steps is 2^32 + 1. Why is it preferrable this way rather than a MAXUINT? I must be missing something. Hardware range for example is usually 0...2^nbits-1
On Mon, 12 Mar 2012, Marc-Andr? Lureau wrote: > On Mon, Mar 12, 2012 at 9:29 PM, malc <av1474@comtv.ru> wrote: > >> So what I meant is rather G_MAXUINT32. > > > > Just leave it as is, it's perfectly fine. > > But it's not convenient to fit into a regular integer, as the number > of steps is 2^32 + 1. Why is it preferrable this way rather than a > MAXUINT? I must be missing something. Hardware range for example is > usually 0...2^nbits-1 > I really do not understand your point at all, mixeng_volume fields are int64_t's it's irrelevant what you initialize them with, they are still 64bit integers, and UINT_MAX as i already said is not even correct, it's 1/(2**32-1) less than one in fixed point.
On Mon, Mar 12, 2012 at 10:00 PM, malc <av1474@comtv.ru> wrote: > I really do not understand your point at all, mixeng_volume fields > are int64_t's it's irrelevant what you initialize them with, they > are still 64bit integers, and UINT_MAX as i already said is not even > correct, it's 1/(2**32-1) less than one in fixed point. But the current code does this: AUD_set_volume_out (SWVoiceOut *sw, int mute, uint8_t lvol..) sw->vol.l = nominal_volume.l * lvol / 255; So the range was [0..2^8-1] and then it becomes [0..2^32], which looks wrong to me. And Spice uses [0..2^16-1]. So it is simpler to stay within the range of an integer..
On Mon, 12 Mar 2012, Marc-Andr? Lureau wrote: > On Mon, Mar 12, 2012 at 10:00 PM, malc <av1474@comtv.ru> wrote: > > I really do not understand your point at all, mixeng_volume fields > > are int64_t's it's irrelevant what you initialize them with, they > > are still 64bit integers, and UINT_MAX as i already said is not even > > correct, it's 1/(2**32-1) less than one in fixed point. > > But the current code does this: > > AUD_set_volume_out (SWVoiceOut *sw, int mute, uint8_t lvol..) > > sw->vol.l = nominal_volume.l * lvol / 255; > > So the range was [0..2^8-1] and then it becomes [0..2^32], which looks > wrong to me. It actually becomes [-2^63..2^63-1] nominal_volume.l being 64 bit signed and all. > > And Spice uses [0..2^16-1]. So it is simpler to stay within the range > of an integer.. I do not get you, i really don't.
On Mon, Mar 12, 2012 at 10:11 PM, malc <av1474@comtv.ru> wrote: >> AUD_set_volume_out (SWVoiceOut *sw, int mute, uint8_t lvol..) >> >> sw->vol.l = nominal_volume.l * lvol / 255; >> >> So the range was [0..2^8-1] and then it becomes [0..2^32], which looks >> wrong to me. > > It actually becomes [-2^63..2^63-1] nominal_volume.l being 64 bit signed > and all. uint8_t [0..255] / 255 * 2^32 = [0..2^32] >> And Spice uses [0..2^16-1]. So it is simpler to stay within the range >> of an integer.. > > I do not get you, i really don't. The audio hw volume range is within a uint8 [0..2^8-1] that is then scaled into a [0..2^32], it would be easier to stay within a [0..2^n-1] range all the way.
On Mon, 12 Mar 2012, Marc-Andr? Lureau wrote: > On Mon, Mar 12, 2012 at 10:11 PM, malc <av1474@comtv.ru> wrote: > >> AUD_set_volume_out (SWVoiceOut *sw, int mute, uint8_t lvol..) > >> > >> sw->vol.l = nominal_volume.l * lvol / 255; > >> > >> So the range was [0..2^8-1] and then it becomes [0..2^32], which looks > >> wrong to me. > > > > It actually becomes [-2^63..2^63-1] nominal_volume.l being 64 bit signed > > and all. > > uint8_t [0..255] / 255 * 2^32 = [0..2^32] > nominal_volume.[lr] is int64_t... i don't get where you take this ^32 from. > >> And Spice uses [0..2^16-1]. So it is simpler to stay within the range > >> of an integer.. > > > > I do not get you, i really don't. > > The audio hw volume range is within a uint8 [0..2^8-1] that is then > scaled into a [0..2^32], it would be easier to stay within a > [0..2^n-1] range all the way. > > >
On Mon, Mar 12, 2012 at 10:42 PM, malc <av1474@comtv.ru> wrote: > nominal_volume.[lr] is int64_t... i don't get where you take this ^32 > from. from where it is defined, audio/audio.c: const struct mixeng_volume nominal_volume = { .mute = 0, #ifdef FLOAT_MIXENG .r = 1.0, .l = 1.0, #else .r = 1ULL << 32, .l = 1ULL << 32, #endif };
On Mon, 12 Mar 2012, Marc-Andr? Lureau wrote: > On Mon, Mar 12, 2012 at 10:42 PM, malc <av1474@comtv.ru> wrote: > > nominal_volume.[lr] is int64_t... i don't get where you take this ^32 > > from. > > from where it is defined, audio/audio.c: > > const struct mixeng_volume nominal_volume = { > .mute = 0, > #ifdef FLOAT_MIXENG > .r = 1.0, > .l = 1.0, > #else > .r = 1ULL << 32, > .l = 1ULL << 32, > #endif > }; > 1 << 32 = 1.0 in 64(32.32) fixed point... the type of l/r is int64_t
On Mon, Mar 12, 2012 at 11:52 PM, malc <av1474@comtv.ru> wrote:
> 1 << 32 = 1.0 in 64(32.32) fixed point... the type of l/r is int64_t
Ok, I'll leave it as is then if it is on purpose.
diff --git a/audio/audio.c b/audio/audio.c index bd9237e..06c2384 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -110,8 +110,8 @@ const struct mixeng_volume nominal_volume = { .r = 1.0, .l = 1.0, #else - .r = 1ULL << 32, - .l = 1ULL << 32, + .r = UINT_MAX, + .l = UINT_MAX, #endif };