Hi
On Tue, Jun 21, 2011 at 3:22 PM, Christophe Fergeau <cferg...@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
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
