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

[Christophe Fergeau]

Hi,

I finally read the patch in full ;) You'll find a few minor comments
inline.

On Mon, Jun 20, 2011 at 11:40:01PM +0200, Marc-André Lureau wrote:
> @@ -141,14 +145,22 @@ struct SndWorker {
>      int active;
>  };
>  
> +typedef struct SpiceVolumeState {
> +    uint8_t volume_nchannels;
> +    uint16_t *volume;
> +    int mute;

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.

> @@ -823,6 +912,38 @@ static void snd_set_command(SndChannel *channel, uint32_t command)
>      channel->command |= command;
>  }
>  
> +__visible__ void spice_server_playback_set_volume(SpicePlaybackInstance *sin,
> +                                                  uint8_t nchannels,
> +                                                  uint16_t *volume)
> +{
> +    SpiceVolumeState *st = &sin->st->volume;
> +    SndChannel *channel = sin->st->worker.connection;
> +    PlaybackChannel *playback_channel = SPICE_CONTAINEROF(channel, PlaybackChannel, base);
> +
> +    st->volume_nchannels = nchannels;
> +    free(st->volume);
> +    st->volume = spice_memdup(volume, sizeof(uint16_t) * nchannels);
> +
> +    if (!channel)
> +        return;
> +
> +    snd_playback_send_volume(playback_channel);
> +}

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)

The rest looks good to me.

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/0ae6c1c5/attachment.pgp>