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

[Marc-André Lureau]

Hi

On Tue, Jun 21, 2011 at 3:22 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 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.

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).

>> @@ -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)

We need to save current volume unconditionnaly so that later client
connections can receive it.

-- 
Marc-André Lureau