[Spice-devel] [PATCH v10 01/11] sound: Convert SndChannelClient to GObject

[Frediano Ziglio]

> 
> On Tue, Jan 03, 2017 at 11:00:54AM +0000, Frediano Ziglio wrote:
> > @@ -618,25 +401,26 @@ static int snd_send_volume(SndChannelClient *client,
> > uint32_t cap, int msg)
> >  {
> >      SpiceMsgAudioVolume *vol;
> >      uint8_t c;
> > -    SpiceVolumeState *st = &client->channel->volume;
> > -    SpiceMarshaller *m = client->send_data.marshaller;
> > +    RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> > +    SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> > +    SndChannel *channel =
> > SND_CHANNEL(red_channel_client_get_channel(rcc));
> > +    SpiceVolumeState *st = &channel->volume;
> >  
> > -    if (!red_channel_client_test_remote_cap(client->channel_client, cap))
> > {
> > -        return TRUE;
> > +    if (!red_channel_client_test_remote_cap(rcc, cap)) {
> > +        return FALSE;
> >      }
> 
> Quickly mentioned on IRC that the returned value changed here, but you
> told me this change. Mentioning this here in case someone wonders the
> same thing ;)
> 

The return logic is a bit changed. Before was "return FALSE if we can't sent
a message" now is "return TRUE if a message has been sent" (so we can decide
to send another if not sent).

> > @@ -1509,52 +1316,79 @@ static void on_new_record_channel(SndChannel
> > *channel, SndChannelClient *snd_cha
> >  {
> >      spice_assert(snd_channel);
> >  
> > -    channel->connection = snd_channel ;
> > +    channel->connection = snd_channel;
> >      if (channel->volume.volume_nchannels) {
> > -        snd_set_command(snd_channel, SND_VOLUME_MASK);
> > +        snd_set_command(snd_channel, SND_VOLUME_MUTE_MASK);
> 
> Is this mask change intentional?
> 

Actually this is not a change. Previously there was a single bit
(VOLUME) for both VOLUME and MUTE (messages were always sent together).
This actually in some conditions (network buffers full) could cause a
single VOLUME message to be sent and MUTE discarded (for instance if
you mute the line). Now there are 2 bits and for compatibility you
queue both VOLUME and MUTE.

> Christophe
> 

I think the main problem reviewing this patch is that people keeps
thinking that this big patch is the results of many small changes.
As explained is not so the patch is big and it's better to consider
the overall code before and after instead of the patch itself.

Frediano