[Spice-devel] [spice-server v3 07/10] sound: Use RedChannelClient to receive/send data

[Frediano Ziglio]

> 
> You can see that SndChannelClient has much less field
> as the code to read/write from/to client is reused from
> RedChannelClient instead of creating a fake RedChannelClient
> just to make the system happy.
> 
> One of the different between the old sound code and all other
> RedChannelClient objects was that the sound channel don't use
> a queue while RedChannelClient use RedPipeItem object. This was
> the main reason why RedChannelClient was not used. To implement
> the old behaviour a "persistent_pipe_item" is used. This RedPipeItem
> will be queued to RedChannelClient (only one!) so signal code we
> have data to send. The {playback,record}_channel_send_item will
> then send the messages to the client using RedChannelClient functions.
> For this reason snd_reset_send_data is replaced by a call to
> red_channel_client_init_send_data and snd_begin_send_message is
> replaced by red_channel_client_begin_send_message.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> ---
>  server/sound.c | 659
>  +++++++++++++++++++++------------------------------------
>  1 file changed, 245 insertions(+), 414 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index f27a53d..2bb5688 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -82,8 +82,6 @@ typedef struct AudioFrameContainer AudioFrameContainer;
>  typedef struct SpicePlaybackState PlaybackChannel;
>  typedef struct SpiceRecordState RecordChannel;
>  
> -typedef void (*snd_channel_send_messages_proc)(void *in_channel);
> -typedef int (*snd_channel_handle_message_proc)(SndChannelClient *client,
> size_t size, uint32_t type, void *message);
>  typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
>  typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
>  

....

> @@ -994,6 +672,13 @@ static SndChannelClient *__new_channel(SndChannel
> *channel, int size, uint32_t c
>      if (!client->channel_client) {
>          goto error2;
>      }
> +
> +    /* SndChannelClient is not yet a RedChannelClient, but we still need to
> go from our
> +     * RedChannelClient implementation (DummyChannelClient) to the
> SndChannelClient instance
> +     * in various vfuncs
> +     */
> +    g_object_set_data(G_OBJECT(client->channel_client),
> "sound-channel-client", client);
> +
>      if
>      (!snd_channel_config_socket(RED_CHANNEL_CLIENT(client->channel_client)))
>      {
>          goto error2;
>      }
> @@ -1005,6 +690,135 @@ error2:
>      return NULL;
>  }
>  
> +/* This function is called when the "persistent" item is removed from the
> + * queue. Note that there is not free call as the item is allocated into
> + * SndChannelClient.
> + * This is used to have a simple item in RedChannelClient queue but to send
> + * multiple messages in a row if possible.
> + * During realtime sound transmission you usually don't want to queue too
> + * much data or having retransmission preferring instead loosing some
> + * samples.
> + */
> +

I would remove this extra empty line 

> +static void snd_persistent_pipe_item_free(struct RedPipeItem *item)
> +{
> +    SndChannelClient *client = SPICE_CONTAINEROF(item, SndChannelClient,
> persistent_pipe_item);
> +
> +    red_pipe_item_init_full(item, RED_PIPE_ITEM_PERSISTENT,
> +                            snd_persistent_pipe_item_free);
> +
> +    if (client->on_message_done) {
> +        client->on_message_done(client);
> +    }
> +}
> +
> +static void snd_send(SndChannelClient * client)
> +{
> +    RedChannelClient *rcc = client->channel_client;
> +
> +    if (!client || !red_channel_client_pipe_is_empty(rcc) ||
> !client->command) {

the !client check here is useless after its use

> +        return;
> +    }
> +    // just append a dummy item and push!
> +    red_pipe_item_init_full(&client->persistent_pipe_item,
> RED_PIPE_ITEM_PERSISTENT,
> +                            snd_persistent_pipe_item_free);
> +    red_channel_client_pipe_add_push(rcc, &client->persistent_pipe_item);
> +}
> +
> +static void playback_channel_send_item(RedChannelClient *rcc, G_GNUC_UNUSED
> RedPipeItem *item)
> +{
> +    SndChannelClient *client = snd_channel_client_from_dummy(rcc);
> +    PlaybackChannelClient *playback_client = SPICE_CONTAINEROF(client,
> PlaybackChannelClient, base);
> +
> +    client->command &= SND_PLAYBACK_MODE_MASK|SND_PLAYBACK_PCM_MASK|
> +                       SND_CTRL_MASK|SND_VOLUME_MUTE_MASK|
> +                       SND_MIGRATE_MASK|SND_PLAYBACK_LATENCY_MASK;
> +    while (client->command) {
> +        if (client->command & SND_PLAYBACK_MODE_MASK) {
> +            client->command &= ~SND_PLAYBACK_MODE_MASK;
> +            if (playback_send_mode(playback_client)) {
> +                break;
> +            }
> +        }
> +        if (client->command & SND_PLAYBACK_PCM_MASK) {
> +            spice_assert(!playback_client->in_progress &&
> playback_client->pending_frame);
> +            playback_client->in_progress = playback_client->pending_frame;
> +            playback_client->pending_frame = NULL;
> +            client->command &= ~SND_PLAYBACK_PCM_MASK;
> +            if (snd_playback_send_write(playback_client)) {
> +                break;
> +            }
> +            spice_printerr("snd_send_playback_write failed");
> +        }
> +        if (client->command & SND_CTRL_MASK) {
> +            client->command &= ~SND_CTRL_MASK;
> +            if (snd_playback_send_ctl(playback_client)) {
> +                break;
> +            }
> +        }
> +        if (client->command & SND_VOLUME_MASK) {
> +            client->command &= ~SND_VOLUME_MASK;
> +            if (snd_playback_send_volume(playback_client)) {
> +                break;
> +            }
> +        }
> +        if (client->command & SND_MUTE_MASK) {
> +            client->command &= ~SND_MUTE_MASK;
> +            if (snd_playback_send_mute(playback_client)) {
> +                break;
> +            }
> +        }
> +        if (client->command & SND_MIGRATE_MASK) {
> +            client->command &= ~SND_MIGRATE_MASK;
> +            if (snd_playback_send_migrate(playback_client)) {
> +                break;
> +            }
> +        }
> +        if (client->command & SND_PLAYBACK_LATENCY_MASK) {
> +            client->command &= ~SND_PLAYBACK_LATENCY_MASK;
> +            if (snd_playback_send_latency(playback_client)) {
> +                break;
> +            }
> +        }
> +    }
> +    snd_send(client);
> +}
> +
....

Beside that

Acked-by: Frediano Ziglio <fziglio at redhat.com>

Frediano