[Spice-devel] [PATCH v2] sndworker: add AudioVolume/AudioMute messages

[Christophe Fergeau]

On Tue, Jun 21, 2011 at 03:32:58PM +0200, Marc-André Lureau wrote:
> > It's the only place where mute is an int, why not make it a uint8_t as
> > everywhere else?
> > Another issue I have with mute is that I assume it's a boolean, however
> > there is no place at all where it's made obvious, it could very well be a 0
> > to 255 value and me making bad assumptions about it. If it's actually a
> > boolean, maybe we could use bool here instead (though "bool" is only used
> > in one other place in server/), or maybe it could be treated more like a
> > boolean ? Eg use msg->mute = !!mute when sending the value to the server.
> > Or naming it is_muted. Or simply adding a comment :) But I really think it
> > would be nice to make this slightly more obvious.
> 
> I fully agree with you, but to be consistent with the code around, I
> used int (like "celt_allowed", which I will remove in a following
> patch in favour a caps check function).

On the other hand, the "celt_allowed" name hints that it's a boolean, which
"mute" does not. Can we use "is_muted" or have a comment in the struct
mentioning it's a boolean value?

> > snd_playback_send_volume has a return value to indicate failure. For now it
> > won't happen: the only case when this could happen is if channel is NULL,
> > which is tested here, and anyway the code would crash before. But maybe we
> > should be robust against future changes and start by trying to call this
> > function, and then update sin->st->volume if it succeeded, otherwise report
> > an error? (same comment for the other public _set_ functions)
> 
> We need to save current volume unconditionnaly so that later client
> connections can receive it.

Ah yeah, I was thinking it was a client => server message for some reason,
not the other way :-/

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20110621/f74962ea/attachment.pgp>