[Spice-devel] [PATCH spice-server v2 3/4] sound: Remove sin field from SpicePlaybackState and SpiceRecordState

Christophe Fergeau cfergeau at redhat.com
Tue Nov 15 10:23:48 UTC 2016


This would have deserved a "why?" in the log even if it's just a few
lines (something like "it's redundant because all we need is sin->st
which we can pass directly/get directly from yyy").

In my opinion, commits with only a short log are only acceptable
for the most trivial of commits, which this one is not.

Christophe

On Mon, Nov 14, 2016 at 09:32:11AM +0000, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/sound.c | 63 ++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 35 insertions(+), 28 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index c078f28..915756f 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -166,12 +166,10 @@ struct SndWorker {
>  
>  struct SpicePlaybackState {
>      struct SndWorker worker;
> -    SpicePlaybackInstance *sin;
>  };
>  
>  struct SpiceRecordState {
>      struct SndWorker worker;
> -    SpiceRecordInstance *sin;
>  };
>  
>  typedef struct RecordChannel {
> @@ -190,6 +188,8 @@ typedef struct RecordChannel {
>  static SndWorker *workers;
>  
>  static void snd_receive(SndChannel *channel);
> +static void snd_playback_start(SpicePlaybackState *st);
> +static void snd_record_start(SpiceRecordState *st);
>  
>  static SndChannel *snd_channel_ref(SndChannel *channel)
>  {
> @@ -393,11 +393,11 @@ static int snd_record_handle_message(SndChannel *channel, size_t size, uint32_t
>          return snd_record_handle_write((RecordChannel *)channel, size, message);
>      case SPICE_MSGC_RECORD_MODE: {
>          SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
> -        SpiceRecordState *st = SPICE_CONTAINEROF(channel->worker, SpiceRecordState, worker);
> +        SndWorker *worker = channel->worker;
>          record_channel->mode_time = mode->time;
>          if (mode->mode != SPICE_AUDIO_DATA_MODE_RAW) {
> -            if (snd_codec_is_capable(mode->mode, st->worker.frequency)) {
> -                if (snd_codec_create(&record_channel->codec, mode->mode, st->worker.frequency,
> +            if (snd_codec_is_capable(mode->mode, worker->frequency)) {
> +                if (snd_codec_create(&record_channel->codec, mode->mode, worker->frequency,
>                                       SND_CODEC_DECODE) == SND_CODEC_OK) {
>                      record_channel->mode = mode->mode;
>                  } else {
> @@ -1035,25 +1035,29 @@ SPICE_GNUC_VISIBLE void spice_server_playback_set_mute(SpicePlaybackInstance *si
>      snd_playback_send_mute(playback_channel);
>  }
>  
> -SPICE_GNUC_VISIBLE void spice_server_playback_start(SpicePlaybackInstance *sin)
> +static void snd_playback_start(SpicePlaybackState *st)
>  {
> -    SndChannel *channel = sin->st->worker.connection;
> -    PlaybackChannel *playback_channel = SPICE_CONTAINEROF(channel, PlaybackChannel, base);
> +    SndChannel *channel = st->worker.connection;
>  
> -    sin->st->worker.active = 1;
> +    st->worker.active = 1;
>      if (!channel)
>          return;
> -    spice_assert(!playback_channel->base.active);
> +    spice_assert(!channel->active);
>      reds_disable_mm_time(snd_channel_get_server(channel));
> -    playback_channel->base.active = TRUE;
> -    if (!playback_channel->base.client_active) {
> -        snd_set_command(&playback_channel->base, SND_PLAYBACK_CTRL_MASK);
> -        snd_playback_send(&playback_channel->base);
> +    channel->active = TRUE;
> +    if (!channel->client_active) {
> +        snd_set_command(channel, SND_PLAYBACK_CTRL_MASK);
> +        snd_playback_send(channel);
>      } else {
> -        playback_channel->base.command &= ~SND_PLAYBACK_CTRL_MASK;
> +        channel->command &= ~SND_PLAYBACK_CTRL_MASK;
>      }
>  }
>  
> +SPICE_GNUC_VISIBLE void spice_server_playback_start(SpicePlaybackInstance *sin)
> +{
> +    return snd_playback_start(sin->st);
> +}
> +
>  SPICE_GNUC_VISIBLE void spice_server_playback_stop(SpicePlaybackInstance *sin)
>  {
>      SndChannel *channel = sin->st->worker.connection;
> @@ -1245,7 +1249,7 @@ static void snd_set_playback_peer(RedChannel *channel, RedClient *client, RedsSt
>      }
>  
>      if (worker->active) {
> -        spice_server_playback_start(st->sin);
> +        snd_playback_start(st);
>      }
>      snd_playback_send(worker->connection);
>  }
> @@ -1299,26 +1303,31 @@ SPICE_GNUC_VISIBLE void spice_server_record_set_mute(SpiceRecordInstance *sin, u
>      snd_record_send_mute(record_channel);
>  }
>  
> -SPICE_GNUC_VISIBLE void spice_server_record_start(SpiceRecordInstance *sin)
> +static void snd_record_start(SpiceRecordState *st)
>  {
> -    SndChannel *channel = sin->st->worker.connection;
> +    SndChannel *channel = st->worker.connection;
>      RecordChannel *record_channel = SPICE_CONTAINEROF(channel, RecordChannel, base);
>  
> -    sin->st->worker.active = 1;
> +    st->worker.active = 1;
>      if (!channel)
>          return;
> -    spice_assert(!record_channel->base.active);
> -    record_channel->base.active = TRUE;
> +    spice_assert(!channel->active);
>      record_channel->read_pos = record_channel->write_pos = 0;   //todo: improve by
>                                                                  //stream generation
> -    if (!record_channel->base.client_active) {
> -        snd_set_command(&record_channel->base, SND_RECORD_CTRL_MASK);
> -        snd_record_send(&record_channel->base);
> +    channel->active = TRUE;
> +    if (!channel->client_active) {
> +        snd_set_command(channel, SND_RECORD_CTRL_MASK);
> +        snd_record_send(channel);
>      } else {
> -        record_channel->base.command &= ~SND_RECORD_CTRL_MASK;
> +        channel->command &= ~SND_RECORD_CTRL_MASK;
>      }
>  }
>  
> +SPICE_GNUC_VISIBLE void spice_server_record_start(SpiceRecordInstance *sin)
> +{
> +    snd_record_start(sin->st);
> +}
> +
>  SPICE_GNUC_VISIBLE void spice_server_record_stop(SpiceRecordInstance *sin)
>  {
>      SndChannel *channel = sin->st->worker.connection;
> @@ -1465,7 +1474,7 @@ static void snd_set_record_peer(RedChannel *channel, RedClient *client, RedsStre
>  
>      on_new_record_channel(worker, &record_channel->base);
>      if (worker->active) {
> -        spice_server_record_start(st->sin);
> +        snd_record_start(st);
>      }
>      snd_record_send(worker->connection);
>  }
> @@ -1513,7 +1522,6 @@ void snd_attach_playback(RedsState *reds, SpicePlaybackInstance *sin)
>      ClientCbs client_cbs = { NULL, };
>  
>      sin->st = spice_new0(SpicePlaybackState, 1);
> -    sin->st->sin = sin;
>      playback_worker = &sin->st->worker;
>      playback_worker->frequency = SND_CODEC_CELT_PLAYBACK_FREQ; /* Default to the legacy rate */
>  
> @@ -1543,7 +1551,6 @@ void snd_attach_record(RedsState *reds, SpiceRecordInstance *sin)
>      ClientCbs client_cbs = { NULL, };
>  
>      sin->st = spice_new0(SpiceRecordState, 1);
> -    sin->st->sin = sin;
>      record_worker = &sin->st->worker;
>      record_worker->frequency = SND_CODEC_CELT_PLAYBACK_FREQ; /* Default to the legacy rate */
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161115/a8d6a3c9/attachment-0001.sig>


More information about the Spice-devel mailing list