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

Submitted by Frediano Ziglio on Dec. 2, 2016, 10:53 a.m.

Details

Message ID c929632247b30a8112331329bf380cbb0ba03daa.1480676032.git-series.fziglio@redhat.com
State Accepted
Commit 852202a0497bd42e238fe338a93d8cf073f9c8d1
Headers show
Series "Remove DummyChannel* objects" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Dec. 2, 2016, 10:53 a.m.
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@redhat.com>
---
 server/sound.c | 62 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

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
                                                                    caps, 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]);
+    }
+}

Comments

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@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@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]);
> +    }
> +}