Re: [Spice-devel] [spice-server v2 09/10] sound: Turn {Playback, Record}ChannelClient into GObjects

[Christophe Fergeau]

Hey,

On Thu, Jan 26, 2017 at 02:55:03AM -0500, Frediano Ziglio wrote:
> > 
> > This is in preparation for making them inherit from RedChannelClient.
> > Doing it in one go would result in a very huge commit, so this commit
> > starts by turning these into GObjects, while still using a
> > DummyChannelClient instance for sending the data.
> > 
> > Based on a patch from Frediano Ziglio 
> > 
> > Signed-off-by: Christophe Fergeau 
> 
> IMO this patch is too artificial, I would merge to 10/10

To be honest, I'm a bit torn on this one. I totally agree that it's very
artificial. On the other hand, it also reduces the per-commit changes in sound.c
significantly:


server/sound.c | 325 
+-
and then
server/sound.c | 262 
++-

VS

server/sound.c | 411 
+--
For the combined patch

and we've got a more or less mechanical patch turning SndChannelClient
into a GObject, and then another one removing
SndChannelClient::channel_client. So not fully sure what is best between
merging the two patches or not :-/

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server v2 09/10] sound: Turn {Playback, Record}ChannelClient into GObjects

[Frediano Ziglio]

> 
> This is in preparation for making them inherit from RedChannelClient.
> Doing it in one go would result in a very huge commit, so this commit
> starts by turning these into GObjects, while still using a
> DummyChannelClient instance for sending the data.
> 
> Based on a patch from Frediano Ziglio 
> 
> Signed-off-by: Christophe Fergeau 

IMO this patch is too artificial, I would merge to 10/10

Frediano

> ---
>  server/sound.c | 326
>  +
>  1 file changed, 210 insertions(+), 116 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index d0cfbf1..7738de4 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -83,14 +83,17 @@ typedef struct SpicePlaybackState PlaybackChannel;
>  typedef struct SpiceRecordState RecordChannel;
>  
>  typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
> -typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
>  
> -#define SND_CHANNEL_CLIENT(obj) (&(obj)->base)
> +#define TYPE_SND_CHANNEL_CLIENT snd_channel_client_get_type()
> +#define SND_CHANNEL_CLIENT(obj) \
> +(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_SND_CHANNEL_CLIENT,
> SndChannelClient))
> +#define IS_SND_CHANNEL_CLIENT(obj) \
> +(G_TYPE_CHECK_INSTANCE_TYPE ((obj), TYPE_SND_CHANNEL_CLIENT))
> +GType snd_channel_client_get_type(void) G_GNUC_CONST;
>  
>  /* Connects an audio client to a Spice client */
>  struct SndChannelClient {
> -int refs;
> -
> +GObject parent;
>  RedChannelClient *channel_client;
>  
>  int active;
> @@ -104,9 +107,14 @@ struct SndChannelClient {
>  RedPipeItem persistent_pipe_item;
>  
>  snd_channel_on_message_done_proc on_message_done;
> -snd_channel_cleanup_channel_proc cleanup;
>  };
>  
> +typedef struct SndChannelClientClass {
> +GObjectClass parent_class;
> +} SndChannelClientClass;
> +
> +G_DEFINE_TYPE(SndChannelClient, snd_channel_client, G_TYPE_OBJECT)
> +
>  
>  enum {
>  RED_PIPE_ITEM_PERSISTENT = RED_PIPE_ITEM_TYPE_CHANNEL_BASE,
> @@ -129,8 +137,13 @@ struct AudioFrameContainer
>  AudioFrame items[NUM_AUDIO_FRAMES];
>  };
>  
> +#define TYPE_PLAYBACK_CHANNEL_CLIENT playback_channel_client_get_type()
> +#define PLAYBACK_CHANNEL_CLIENT(obj) \
> +(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL_CLIENT,
> PlaybackChannelClient))
> +GType playback_channel_client_get_type(void) G_GNUC_CONST;
> +
>  struct PlaybackChannelClient {
> -SndChannelClient base;
> +SndChannelClient parent;
>  
>  AudioFrameContainer *frames;
>  AudioFrame *free_frames;
> @@ -142,6 +155,13 @@ struct PlaybackChannelClient {
>  uint8_t  encode_buf[SND_CODEC_MAX_COMPRESSED_BYTES];
>  };
>  
> +typedef struct PlaybackChannelClientClass {
> +SndChannelClientClass parent_class;
> +} PlaybackChannelClientClass;
> +
> +G_DEFINE_TYPE(PlaybackChannelClient, playback_channel_client,
> TYPE_SND_CHANNEL_CLIENT)
> +
> +
>  typedef struct SpiceVolumeState {
>  uint16_t *volume;
>  uint8_t volume_nchannels;
> @@ -202,8 +222,13 @@ typedef struct RecordChannelClass {
>  G_DEFINE_TYPE(RecordChannel, record_channel, TYPE_SND_CHANNEL)
>  
>  
> +#define TYPE_RECORD_CHANNEL_CLIENT record_channel_client_get_type()
> +#define RECORD_CHANNEL_CLIENT(obj) \
> +(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_RECORD_CHANNEL_CLIENT,
> RecordChannelClient))
> +GType record_channel_client_get_type(void) G_GNUC_CONST;
> +
>  struct RecordChannelClient {
> -SndChannelClient base;
> +SndChannelClient parent;
>  uint32_t samples[RECORD_SAMPLES_SIZE];
>  uint32_t write_pos;
>  uint32_t read_pos;
> @@ -214,22 +239,41 @@ struct RecordChannelClient {
>  uint8_t  decode_buf[SND_CODEC_MAX_FRAME_BYTES];
>  };
>  
> +typedef struct RecordChannelClientClass {
> +SndChannelClientClass parent_class;
> +} RecordChannelClientClass;
> +
> +G_DEFINE_TYPE(RecordChannelClient, record_channel_client,
> TYPE_SND_CHANNEL_CLIENT)
> +
> +
>  /* A list of all Spice{Playback,Record}State objects */
>  static SndChannel *snd_channels;
>  
>  static void snd_playback_start(SndChannel *channel);
>  static void snd_record_start(SndChannel *channel);
> -static void snd_playback_alloc_frames(PlaybackChannelClient *playback);
>  static void snd_send(SndChannelClient * client);
>  
> -static SndChannelClient *snd_channel_unref(SndChannelClient *client)
> +enum {
> +PROP0,
> +PROP_CHANNEL_CLIENT
> +};
> +
> +static void
> +snd_channel_client_set_property(GObject *object,
> +guint property_id,
> +const GValue *value,
> +GParamSpec *pspec)
>  {
> -if (!--client->refs) {
> -spice_printerr("SndChannelClient=%p freed", client);
> -free(client);
> -return NULL;
> +SndChannelClient *self = SND_CHANNEL_CLIENT(object);
> +
> +switch (property_id)
> +{
> +case 

[Spice-devel] [spice-server v2 09/10] sound: Turn {Playback, Record}ChannelClient into GObjects

[Christophe Fergeau]

This is in preparation for making them inherit from RedChannelClient.
Doing it in one go would result in a very huge commit, so this commit
starts by turning these into GObjects, while still using a
DummyChannelClient instance for sending the data.

Based on a patch from Frediano Ziglio 

Signed-off-by: Christophe Fergeau 
---
 server/sound.c | 326 +
 1 file changed, 210 insertions(+), 116 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index d0cfbf1..7738de4 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -83,14 +83,17 @@ typedef struct SpicePlaybackState PlaybackChannel;
 typedef struct SpiceRecordState RecordChannel;
 
 typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
-typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
 
-#define SND_CHANNEL_CLIENT(obj) (&(obj)->base)
+#define TYPE_SND_CHANNEL_CLIENT snd_channel_client_get_type()
+#define SND_CHANNEL_CLIENT(obj) \
+(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_SND_CHANNEL_CLIENT, 
SndChannelClient))
+#define IS_SND_CHANNEL_CLIENT(obj) \
+(G_TYPE_CHECK_INSTANCE_TYPE ((obj), TYPE_SND_CHANNEL_CLIENT))
+GType snd_channel_client_get_type(void) G_GNUC_CONST;
 
 /* Connects an audio client to a Spice client */
 struct SndChannelClient {
-int refs;
-
+GObject parent;
 RedChannelClient *channel_client;
 
 int active;
@@ -104,9 +107,14 @@ struct SndChannelClient {
 RedPipeItem persistent_pipe_item;
 
 snd_channel_on_message_done_proc on_message_done;
-snd_channel_cleanup_channel_proc cleanup;
 };
 
+typedef struct SndChannelClientClass {
+GObjectClass parent_class;
+} SndChannelClientClass;
+
+G_DEFINE_TYPE(SndChannelClient, snd_channel_client, G_TYPE_OBJECT)
+
 
 enum {
 RED_PIPE_ITEM_PERSISTENT = RED_PIPE_ITEM_TYPE_CHANNEL_BASE,
@@ -129,8 +137,13 @@ struct AudioFrameContainer
 AudioFrame items[NUM_AUDIO_FRAMES];
 };
 
+#define TYPE_PLAYBACK_CHANNEL_CLIENT playback_channel_client_get_type()
+#define PLAYBACK_CHANNEL_CLIENT(obj) \
+(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL_CLIENT, 
PlaybackChannelClient))
+GType playback_channel_client_get_type(void) G_GNUC_CONST;
+
 struct PlaybackChannelClient {
-SndChannelClient base;
+SndChannelClient parent;
 
 AudioFrameContainer *frames;
 AudioFrame *free_frames;
@@ -142,6 +155,13 @@ struct PlaybackChannelClient {
 uint8_t  encode_buf[SND_CODEC_MAX_COMPRESSED_BYTES];
 };
 
+typedef struct PlaybackChannelClientClass {
+SndChannelClientClass parent_class;
+} PlaybackChannelClientClass;
+
+G_DEFINE_TYPE(PlaybackChannelClient, playback_channel_client, 
TYPE_SND_CHANNEL_CLIENT)
+
+
 typedef struct SpiceVolumeState {
 uint16_t *volume;
 uint8_t volume_nchannels;
@@ -202,8 +222,13 @@ typedef struct RecordChannelClass {
 G_DEFINE_TYPE(RecordChannel, record_channel, TYPE_SND_CHANNEL)
 
 
+#define TYPE_RECORD_CHANNEL_CLIENT record_channel_client_get_type()
+#define RECORD_CHANNEL_CLIENT(obj) \
+(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_RECORD_CHANNEL_CLIENT, 
RecordChannelClient))
+GType record_channel_client_get_type(void) G_GNUC_CONST;
+
 struct RecordChannelClient {
-SndChannelClient base;
+SndChannelClient parent;
 uint32_t samples[RECORD_SAMPLES_SIZE];
 uint32_t write_pos;
 uint32_t read_pos;
@@ -214,22 +239,41 @@ struct RecordChannelClient {
 uint8_t  decode_buf[SND_CODEC_MAX_FRAME_BYTES];
 };
 
+typedef struct RecordChannelClientClass {
+SndChannelClientClass parent_class;
+} RecordChannelClientClass;
+
+G_DEFINE_TYPE(RecordChannelClient, record_channel_client, 
TYPE_SND_CHANNEL_CLIENT)
+
+
 /* A list of all Spice{Playback,Record}State objects */
 static SndChannel *snd_channels;
 
 static void snd_playback_start(SndChannel *channel);
 static void snd_record_start(SndChannel *channel);
-static void snd_playback_alloc_frames(PlaybackChannelClient *playback);
 static void snd_send(SndChannelClient * client);
 
-static SndChannelClient *snd_channel_unref(SndChannelClient *client)
+enum {
+PROP0,
+PROP_CHANNEL_CLIENT
+};
+
+static void
+snd_channel_client_set_property(GObject *object,
+guint property_id,
+const GValue *value,
+GParamSpec *pspec)
 {
-if (!--client->refs) {
-spice_printerr("SndChannelClient=%p freed", client);
-free(client);
-return NULL;
+SndChannelClient *self = SND_CHANNEL_CLIENT(object);
+
+switch (property_id)
+{
+case PROP_CHANNEL_CLIENT:
+self->channel_client = g_value_dup_object(value);
+break;
+default:
+G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
 }
-return client;
 }
 
 static SndChannelClient *snd_channel_client_from_dummy(RedChannelClient *dummy)
@@ -238,6 +282,7 @@ static SndChannelClient