Re: [Qemu-devel] [PATCH 1/2] Introduce AUD_set_volume

[Jan Kiszka <jan.kiszka@web.de>]

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

andrzej zaborowski wrote:
> On 02/05/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
>> andrzej zaborowski wrote:
>>  > The user still needs to disable NOVOL if they really want volume control.
>>
>> What is the "user" here? wm8750? musicpal? I just hope it is not the
>>  real user... :)
> 
> Whoever builds qemu.  You don't want it enabled by default and globally.

That's the totally wrong way from the usability perspective, IMHO. What
are the remaining downsides of mixer support? If there are any, I guess
they are far less than the advantages, so this switch should at least
become an option which is default off.

> 
>>  [ QEMU audio setup is already weird enough: I recently tried to get ALSA
>>  running to overcome outdated OSS - well, tricky. Needs fixing. ]
> 
> Yup, I had alsa running but normally (for example now) I just run qemu
> through "aoss".

I had problems with parallel usage by several apps, and only true ALSA
mode resolved it. I think AOSS is purely for unconvertible legacy apps,
and I don't want to put QEMU into this category. ;)

Will follow up on this the next days.

> 
>>  > diff --git a/audio/audio.c b/audio/audio.c
>>  > index 9e7fe80..1231ec5 100644
>>  > --- a/audio/audio.c
>>  > +++ b/audio/audio.c
>>  > @@ -1955,3 +1955,21 @@ void AUD_del_capture (CaptureVoiceOut *cap,
>>  > void *cb_opaque)
>>  >          }
>>  >      }
>>  >  }
>>  > +
>>  > +void AUD_set_volume_out (SWVoiceOut *sw, int mute, uint8_t lvol, uint8_t rvol)
>>  > +{
>>  > +    if (sw) {
>>  > +        sw->vol.mute = mute;
>>  > +        sw->vol.l = nominal_volume.l * lvol / 255;
>>  > +        sw->vol.r = nominal_volume.r * rvol / 255;
> 
> Oops, it may be wrong indeed because it might overflow on machines
> with sizeof(int) >= sizeof(int64_t) and no FLOAT_MIXENG.  I first
> wrote (nominal_volume.l / 255) * lvol and then immediately changed it,
> but that was better.

Thought a bit about it (still untested, though), and I think it should
be fine to just fix the initialization of nominal_volume from UINT_MAX
to (1ULL << 32) (because we are shifting back by 32 bits at runtime). I
had that wrong in my patch as well, BTW.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]