[Spice-devel] [PATCH v6 04/18] sound: Change AudioFrame allocation

[Jonathon Jongsma]

Took me a little while to wrap my brain around why we need to do this,
but I think I understand it pretty well now. I've tried to
rewrite/expand the commit log below in my own words to ensure I
understand it properly. Feel free to use any of the below for the
commit log if it is helpful. Or if it's incorrect, please correct me.

---

When qemu (for example) delivers audio samples to the spice server, it
does so by requesting a buffer from the spice server
(spice_server_playback_get_buffer()), filling them with audio data, and
then calling spice_server_playback_put_samples() to send them to the
client. Between the call to _get_buffer() and the call to
_put_samples(), we need to ensure that the buffer remains valid. Since
this buffer is allocated within PlaybackChannelClient, we did this by
incrementing a ref on the PlaybackChannelClient in _get_buffer(), and
decrementing the ref in _put_samples(). This has the effect of
potentially keeping the PlaybackChannelClient alive after the spice
client has disconnected. 

This was not a problem when PlaybackChannelClient was a simple helper
class. But we intend to change PlaybackChannelClient so that it
inherits from RedChannelClient. In that case, the reference taken in
_get_buffer() would result in the RedChannelClient (and associated
RedChannel, etc) being kept alive longer than expected. To avoid this,
we add an additional ref-counted adapter class (AudioFrameContainer)
that owns the allocated audio frames and can outlive the
RedChannelClient if necessary. When the client is freed, the
AudioFrameContainer is just unreferenced.

---

The code looks good to me.

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>


On Fri, 2016-12-02 at 10:53 +0000, Frediano Ziglio wrote:
> Previously these frames were allocated inside PlaybackChannelClient
> while now they are allocated in a different structure.
> This allows the samples to outlive the PlaybackChannelClient.
> This was possible even before and the client was kept alive.
> However we are moving to have RedChannelClient as a base
> class so this can be an issue as this will cause the entire
> RedChannel and RedChannelClient to stay alive. To avoid this
> potential problem allocate the frames in a different structure
> keeping a reference from PlaybackChannelClient. When the client
> is freed the AudioFrameContainer is just unreferences.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/sound.c | 62 ++++++++++++++++++++++++++++++++++++++++------
> -----
>  1 file changed, 49 insertions(+), 13 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 5e056b6..d76481e 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -77,6 +77,8 @@ typedef struct SndChannelClient SndChannelClient;
>  typedef struct SndChannel SndChannel;
>  typedef struct PlaybackChannelClient PlaybackChannelClient;
>  typedef struct RecordChannelClient RecordChannelClient;
> +typedef struct AudioFrame AudioFrame;
> +typedef struct AudioFrameContainer AudioFrameContainer;
>  typedef struct SpicePlaybackState PlaybackChannel;
>  typedef struct SpiceRecordState RecordChannel;
>  
> @@ -126,11 +128,21 @@ struct AudioFrame {
>      uint32_t samples[SND_CODEC_MAX_FRAME_SIZE];
>      PlaybackChannelClient *client;
>      AudioFrame *next;
> +    AudioFrameContainer *container;
> +    gboolean allocated;
> +};
> +
> +#define NUM_AUDIO_FRAMES 3
> +struct AudioFrameContainer
> +{
> +    int refs;
> +    AudioFrame items[NUM_AUDIO_FRAMES];
>  };
>  
>  struct PlaybackChannelClient {
>      SndChannelClient base;
> -    AudioFrame frames[3];
> +
> +    AudioFrameContainer *frames;
>      AudioFrame *free_frames;
>      AudioFrame *in_progress;   /* Frame being sent to the client */
>      AudioFrame *pending_frame; /* Next frame to send to the client
> */
> @@ -218,12 +230,7 @@ static SndChannel *snd_channels;
>  static void snd_receive(SndChannelClient *client);
>  static void snd_playback_start(SndChannel *channel);
>  static void snd_record_start(SndChannel *channel);
> -
> -static SndChannelClient *snd_channel_ref(SndChannelClient *client)
> -{
> -    client->refs++;
> -    return client;
> -}
> +static void snd_playback_alloc_frames(PlaybackChannelClient
> *playback);
>  
>  static SndChannelClient *snd_channel_unref(SndChannelClient *client)
>  {
> @@ -1191,7 +1198,10 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_get_buffer(SpicePlaybackInstance *
>          return;
>      }
>      spice_assert(playback_client->base.active);
> -    snd_channel_ref(client);
> +    if (!playback_client->free_frames->allocated) {
> +        playback_client->free_frames->allocated = TRUE;
> +        ++playback_client->frames->refs;
> +    }
>  
>      *frame = playback_client->free_frames->samples;
>      playback_client->free_frames = playback_client->free_frames-
> >next;
> @@ -1204,9 +1214,15 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_put_samples(SpicePlaybackInstance 
>      AudioFrame *frame;
>  
>      frame = SPICE_CONTAINEROF(samples, AudioFrame, samples[0]);
> +    if (frame->allocated) {
> +        frame->allocated = FALSE;
> +        if (--frame->container->refs == 0) {
> +            free(frame->container);
> +            return;
> +        }
> +    }
>      playback_client = frame->client;
> -    spice_assert(playback_client);
> -    if (!snd_channel_unref(&playback_client->base) ||
> +    if (!playback_client ||
>          sin->st->channel.connection != &playback_client->base) {
>          /* lost last reference, client has been destroyed previously
> */
>          spice_info("audio samples belong to a disconnected client");
> @@ -1284,6 +1300,15 @@ static void on_new_playback_channel(SndChannel
> *channel, SndChannelClient *snd_c
>  static void snd_playback_cleanup(SndChannelClient *client)
>  {
>      PlaybackChannelClient *playback_client =
> SPICE_CONTAINEROF(client, PlaybackChannelClient, base);
> +    int i;
> +
> +    // free frames, unref them
> +    for (i = 0; i < NUM_AUDIO_FRAMES; ++i) {
> +        playback_client->frames->items[i].client = NULL;
> +    }
> +    if (--playback_client->frames->refs == 0) {
> +        free(playback_client->frames);
> +    }
>  
>      if (playback_client->base.active) {
>          reds_enable_mm_time(snd_channel_get_server(client));
> @@ -1315,9 +1340,8 @@ static void snd_set_playback_peer(RedChannel
> *red_channel, RedClient *client, Re
>                                                                     c
> aps, num_caps))) {
>          return;
>      }
> -    snd_playback_free_frame(playback_client, &playback_client-
> >frames[0]);
> -    snd_playback_free_frame(playback_client, &playback_client-
> >frames[1]);
> -    snd_playback_free_frame(playback_client, &playback_client-
> >frames[2]);
> +
> +    snd_playback_alloc_frames(playback_client);
>  
>      int client_can_celt =
> red_channel_client_test_remote_cap(playback_client-
> >base.channel_client,
>                                            SPICE_PLAYBACK_CAP_CELT_0_
> 5_1);
> @@ -1760,3 +1784,15 @@ void snd_set_playback_compression(int on)
>          }
>      }
>  }
> +
> +static void snd_playback_alloc_frames(PlaybackChannelClient
> *playback)
> +{
> +    int i;
> +
> +    playback->frames = spice_new0(AudioFrameContainer, 1);
> +    playback->frames->refs = 1;
> +    for (i = 0; i < NUM_AUDIO_FRAMES; ++i) {
> +        playback->frames->items[i].container = playback->frames;
> +        snd_playback_free_frame(playback, &playback->frames-
> >items[i]);
> +    }
> +}