[Spice-devel] [PATCH v3] sound: Change snd_playback_start/snd_record_start

[Frediano Ziglio]

> 
> The content of these functions almost exclusively deals with channel
> client functionality except one line where the channel's active state is
> set to TRUE.
> 
> These functions are called in two different places.
> 
> The first place is from the public API spice_server_record_start() and
> spice_server_playback_start(). These functions should alter the
> channel's active state, and then set the associated channel client to
> active.
> 
> The second place is when a new channel client is created. In this
> case, it is only called if the channel is already active, so it doesn't
> make much sense to set the channel's active state inside of the
> function.
> 
> To simplify things (and enable some future refactoring), this function
> now only deals with the SndChannelClient. The functions have also been
> renamed to reflect this fact. The SndChannel's active state is now only
> modified from the public API functions.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> Acked-by: Pavel Grunt <pgrunt at redhat.com>
> ---
> Changes since v2:
>  - renamed functions due to a comment by Frediano from his review of patch 6
> 
>  server/sound.c | 47 ++++++++++++++++++++++-------------------------
>  1 file changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 64072b4..dcee24d 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -238,8 +238,6 @@ G_DEFINE_TYPE(RecordChannelClient, record_channel_client,
> TYPE_SND_CHANNEL_CLIEN
>  /* A list of all Spice{Playback,Record}State objects */
>  static GList *snd_channels;
>  
> -static void snd_playback_start(SndChannel *channel);
> -static void snd_record_start(SndChannel *channel);
>  static void snd_send(SndChannelClient * client);
>  
>  static RedsState* snd_channel_get_server(SndChannelClient *client)
> @@ -862,15 +860,9 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_set_mute(SpicePlaybackInstance *si
>      snd_channel_set_mute(&sin->st->channel, mute);
>  }
>  
> -static void snd_playback_start(SndChannel *channel)
> +static void snd_channel_client_start(SndChannelClient *client)
>  {
> -    SndChannelClient *client = channel->connection;
> -
> -    channel->active = true;
> -    if (!client)
> -        return;
>      spice_assert(!client->active);
> -    reds_disable_mm_time(snd_channel_get_server(client));
>      client->active = true;
>      if (!client->client_active) {
>          snd_set_command(client, SND_CTRL_MASK);
> @@ -880,9 +872,21 @@ static void snd_playback_start(SndChannel *channel)
>      }
>  }
>  
> +static void playback_channel_client_start(SndChannelClient *client)
> +{
> +    if (!client)
> +        return;
> +

brackets

> +    reds_disable_mm_time(snd_channel_get_server(client));

OT: don't understand why this is needed.

> +    snd_channel_client_start(client);
> +

why this empty line?

> +}
> +
>  SPICE_GNUC_VISIBLE void spice_server_playback_start(SpicePlaybackInstance
>  *sin)
>  {
> -    return snd_playback_start(&sin->st->channel);
> +    SndChannel *channel = &sin->st->channel;
> +    channel->active = true;
> +    return playback_channel_client_start(channel->connection);
>  }
>  
>  SPICE_GNUC_VISIBLE void spice_server_playback_stop(SpicePlaybackInstance
>  *sin)
> @@ -1074,7 +1078,7 @@ playback_channel_client_constructed(GObject *object)
>      }
>  
>      if (channel->active) {
> -        snd_playback_start(channel);
> +        playback_channel_client_start(scc);
>      }
>      snd_send(scc);
>  }
> @@ -1126,30 +1130,23 @@ SPICE_GNUC_VISIBLE void
> spice_server_record_set_mute(SpiceRecordInstance *sin, u
>      snd_channel_set_mute(&sin->st->channel, mute);
>  }
>  
> -static void snd_record_start(SndChannel *channel)
> +static void record_channel_client_start(SndChannelClient *client)
>  {
> -    SndChannelClient *client = channel->connection;
> -
> -    channel->active = true;
>      if (!client) {
>          return;
>      }
> +
>      RecordChannelClient *record_client = RECORD_CHANNEL_CLIENT(client);
> -    spice_assert(!client->active);
>      record_client->read_pos = record_client->write_pos = 0;   //todo:
>      improve by
>                                                                //stream
>                                                                generation
> -    client->active = true;
> -    if (!client->client_active) {
> -        snd_set_command(client, SND_CTRL_MASK);
> -        snd_send(client);
> -    } else {
> -        client->command &= ~SND_CTRL_MASK;
> -    }
> +    snd_channel_client_start(client);
>  }
>  
>  SPICE_GNUC_VISIBLE void spice_server_record_start(SpiceRecordInstance *sin)
>  {
> -    snd_record_start(&sin->st->channel);
> +    SndChannel *channel = &sin->st->channel;
> +    channel->active = true;
> +    record_channel_client_start(channel->connection);
>  }
>  
>  SPICE_GNUC_VISIBLE void spice_server_record_stop(SpiceRecordInstance *sin)
> @@ -1266,7 +1263,7 @@ record_channel_client_constructed(GObject *object)
>      }
>  
>      if (channel->active) {
> -        snd_record_start(channel);
> +        record_channel_client_start(scc);
>      }
>      snd_send(scc);
>  }

Beside that looks fine.

Frediano