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@xxxxxxxxxx>
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@xxxxxxxxxx>
> ---
> 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]);
> + }
> +}
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel