Re: [Spice-devel] [client v6 2/3] streaming: Stop streaming if frames cannot be decoded

[Victor Toso]

Hi,

On Tue, Nov 22, 2016 at 06:01:38PM +0100, Francois Gouget wrote:
> Report the stream as invalid if the frames cannot be decoded. This will
> force the server to send regular screen updates instead.
>
> Signed-off-by: Francois Gouget 
> ---
> 
> Uses report_invalid_stream() now.
> 
>  src/channel-display-gst.c   | 41 -
>  src/channel-display-mjpeg.c |  8 +---
>  src/channel-display-priv.h  |  4 +++-
>  src/channel-display.c   |  6 +-
>  4 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index f52299f..5fb0b77 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -280,6 +280,28 @@ static void free_pipeline(SpiceGstDecoder *decoder)
>  decoder->pipeline = NULL;
>  }
>  
> +static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, 
> gpointer video_decoder)
> +{
> +SpiceGstDecoder *decoder = video_decoder;
> +
> +if (GST_MESSAGE_TYPE(msg) == GST_MESSAGE_ERROR) {
> +GError *err = NULL;
> +gchar *debug_info = NULL;
> +gst_message_parse_error(msg, , _info);
> +spice_warning("GStreamer error from element %s: %s",
> +  GST_OBJECT_NAME(msg->src), err->message);
> +if (debug_info) {
> +SPICE_DEBUG("debug information: %s", debug_info);
> +g_free(debug_info);
> +}
> +g_clear_error();
> +
> +/* We won't be able to process any more frame anyway */
> +free_pipeline(decoder);
> +}
> +return TRUE;
> +}
> +
>  static gboolean create_pipeline(SpiceGstDecoder *decoder)
>  {
>  gchar *desc;
> @@ -324,6 +346,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>  
>  appsink_cbs.new_sample = new_sample;
>  gst_app_sink_set_callbacks(decoder->appsink, _cbs, decoder, 
> NULL);
> +gst_bus_add_watch(gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline)),
> +  handle_pipeline_message, decoder);

You are leaking GstBus here. I'll fix it before pushing.

>  
>  decoder->clock = gst_pipeline_get_clock(GST_PIPELINE(decoder->pipeline));
>  
> @@ -390,9 +414,9 @@ static void release_buffer_data(gpointer data)
>  spice_msg_in_unref(frame_msg);
>  }
>  
> -static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> -  SpiceMsgIn *frame_msg,
> -  int32_t latency)
> +static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> +  SpiceMsgIn *frame_msg,
> +  int32_t latency)
>  {
>  SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
>  
> @@ -400,7 +424,7 @@ static void spice_gst_decoder_queue_frame(VideoDecoder 
> *video_decoder,
>  uint32_t size = spice_msg_in_frame_data(frame_msg, );
>  if (size == 0) {
>  SPICE_DEBUG("got an empty frame buffer!");
> -return;
> +return TRUE;
>  }
>  
>  SpiceStreamDataHeader *frame_op = spice_msg_in_parsed(frame_msg);
> @@ -419,7 +443,13 @@ static void spice_gst_decoder_queue_frame(VideoDecoder 
> *video_decoder,
>   * saves CPU so do it.
>   */
>  SPICE_DEBUG("dropping a late MJPEG frame");
> -return;
> +return TRUE;
> +}
> +
> +if (decoder->pipeline == NULL) {
> +/* An error occurred, causing the GStreamer pipeline to be freed */
> +spice_warning("An error occurred, stopping the video stream");
> +return FALSE;
>  }
>  
>  /* ref() the frame_msg for the buffer */
> @@ -440,6 +470,7 @@ static void spice_gst_decoder_queue_frame(VideoDecoder 
> *video_decoder,
>  SPICE_DEBUG("GStreamer error: unable to push frame of size %u", 
> size);
>  stream_dropped_frame_on_playback(decoder->base.stream);
>  }
> +return TRUE;
>  }
>  
>  static gboolean gstvideo_init(void)
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 4976d53..67746c3 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -235,8 +235,9 @@ static void mjpeg_decoder_drop_queue(MJpegDecoder 
> *decoder)
>  
>  /* -- VideoDecoder's public API -- */
>  
> -static void mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
> -  SpiceMsgIn *frame_msg, int32_t latency)
> +static gboolean mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
> +  SpiceMsgIn *frame_msg,
> +  int32_t latency)
>  {
>  MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
>  SpiceMsgIn *last_msg;
> @@ -262,12 +263,13 @@ static void mjpeg_decoder_queue_frame(VideoDecoder 
> *video_decoder,
>   * So drop late frames as early as possible to save on 

Re: [Spice-devel] [spice v6 3/3] streaming: Stop streaming if the client reports a streaming error

[Victor Toso]

Hi!

On Tue, Nov 22, 2016 at 06:01:48PM +0100, Francois Gouget wrote:
> By removing the stream's video encoder we force the stream to send
> future frames using the fallback code, that is as regular screen
> updates.
> However note that we keep the stream object: we have to. Otherwise
> future frames would trigger the creation of a new stream object with a
> new video encoder which would again try to stream the video and fail
> again and again.
>
> Signed-off-by: Francois Gouget 

So, I postpone this for some time because it was reviewed by Christophe
and I was waiting to see if he would come back to it.

The thing is, he acked! For the future, I recommend you to include the
Acked-by metadata in acked patches, which always help a lot.

The server side patch was my main worry and I'm happy to see this was
acked.

> ---
>
> v5 was acked so no change.

Yep!

I intend to push this one soon today.

>
>  server/dcc.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/server/dcc.c b/server/dcc.c
> index e9b438f..cf5f44c 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -1059,6 +1059,15 @@ static int 
> dcc_handle_stream_report(DisplayChannelClient *dcc,
>  return TRUE;
>  }
>  
> +if (report->num_frames == 0 && report->num_drops == UINT_MAX) {
> +spice_warning("stream_report: the client does not support stream %u",
> +  report->stream_id);
> +/* Stop streaming the video so the client can see it */
> +agent->video_encoder->destroy(agent->video_encoder);
> +agent->video_encoder = NULL;
> +return TRUE;
> +}
> +
>  if (report->unique_id != agent->report_id) {
>  spice_warning("stream_report: unique id mismatch: local (%u) != msg 
> (%u) "
>"The old stream was probably replaced by a new one",
> -- 
> 2.10.1
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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] [PATCH v4 07/17] sound: Convert SndChannelClient to GObject

[Jonathon Jongsma]

On Thu, 2016-12-01 at 17:21 +0100, Christophe Fergeau wrote:
> On Thu, Dec 01, 2016 at 11:24:29AM +, Frediano Ziglio wrote:
> > 
> > This patch is quite huge but have some benefits:
> > - reduce dependency (DummyChannel* and some RedChannelClient
> > internals);
> > - reuse RedChannelClient instead of reading from the RedsStream
> >   directly.
> > 
> > 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.
> > 
> > Another difference is the AudioFrame allocation. 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 having RedChannelClient as
> > as base class 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.
> 
> No luck in adding a few more introductory patches to split this one
> more?
> 
> Christophe


I agree with Christophe. This is quite difficult to review properly.

Jonathon

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

Re: [Spice-devel] [PATCH v4 06/17] sound: Implements config_socket RedChannel callback

[Jonathon Jongsma]

On Thu, 2016-12-01 at 11:24 +, Frediano Ziglio wrote:
> This code is the same inside __new_channel but will set the
> RedsStream from RedChannel.

Shouldn't this code be removed from __new_channel() then?



> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 46 +-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 011cff2..bf59789 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -1001,7 +1001,51 @@ error1:
>  
>  static int snd_channel_config_socket(RedChannelClient *rcc)
>  {
> -g_assert_not_reached();
> +int delay_val;
> +int flags;
> +#ifdef SO_PRIORITY
> +int priority;
> +#endif
> +int tos;
> +RedsStream *stream = red_channel_client_get_stream(rcc);
> +RedClient *red_client = red_channel_client_get_client(rcc);
> +MainChannelClient *mcc = red_client_get_main(red_client);
> +
> +if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
> +spice_printerr("accept failed, %s", strerror(errno));
> +return FALSE;
> +}
> +
> +#ifdef SO_PRIORITY
> +priority = 6;
> +if (setsockopt(stream->socket, SOL_SOCKET, SO_PRIORITY,
> (void*),
> +   sizeof(priority)) == -1) {
> +if (errno != ENOTSUP) {
> +spice_printerr("setsockopt failed, %s",
> strerror(errno));
> +}
> +}
> +#endif
> +
> +tos = IPTOS_LOWDELAY;
> +if (setsockopt(stream->socket, IPPROTO_IP, IP_TOS, (void*),
> sizeof(tos)) == -1) {
> +if (errno != ENOTSUP) {
> +spice_printerr("setsockopt failed, %s",
> strerror(errno));
> +}
> +}
> +
> +delay_val = main_channel_client_is_low_bandwidth(mcc) ? 0 : 1;
> +if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
> _val, sizeof(delay_val)) == -1) {
> +if (errno != ENOTSUP) {
> +spice_printerr("setsockopt failed, %s",
> strerror(errno));
> +}
> +}
> +
> +if (fcntl(stream->socket, F_SETFL, flags | O_NONBLOCK) == -1) {
> +spice_printerr("accept failed, %s", strerror(errno));
> +return FALSE;
> +}
> +
> +return TRUE;
>  }
>  
>  static void snd_channel_on_disconnect(RedChannelClient *rcc)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v4 05/17] sound: Convert SndChannel to GObject

[Jonathon Jongsma]

On Thu, 2016-12-01 at 11:24 +, Frediano Ziglio wrote:
> Stops using the dummy channel.
> Data handling still goes through DummyChannelClient which is why
> empty implementation of some required vfuncs is working.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 224 +++--
> -
>  1 file changed, 159 insertions(+), 65 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 5b66f3a..011cff2 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -144,9 +144,14 @@ typedef struct SpiceVolumeState {
>  int mute;
>  } SpiceVolumeState;
>  
> +#define TYPE_SND_CHANNEL snd_channel_get_type()
> +#define SND_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),
> TYPE_SND_CHANNEL, SndChannel))
> +GType snd_channel_get_type(void) G_GNUC_CONST;
> +
>  /* Base class for SpicePlaybackState and SpiceRecordState */
>  struct SndChannel {
> -RedChannel *base_channel;
> +RedChannel parent;
> +
>  SndChannelClient *connection; /* Only one client is supported */
>  SndChannel *next; /* For the global SndChannel list */
>  
> @@ -155,14 +160,40 @@ struct SndChannel {
>  uint32_t frequency;
>  };
>  
> +typedef RedChannelClass SndChannelClass;

I guess this works, but I'd prefer the more straightforward 

struct SndChannelClass {
  RedChannelClass parent_class;
};



> +
> +G_DEFINE_TYPE(SndChannel, snd_channel, RED_TYPE_CHANNEL)
> +
> +
> +typedef struct SpicePlaybackState PlaybackChannel;
> +typedef SndChannelClass PlaybackChannelClass;

as above

> +
> +#define TYPE_PLAYBACK_CHANNEL playback_channel_get_type()
> +#define PLAYBACK_CHANNEL(obj) \
> +(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL,
> PlaybackChannel))
> +GType playback_channel_get_type(void) G_GNUC_CONST;
> +
>  struct SpicePlaybackState {
> -struct SndChannel channel;
> +SndChannel channel;
>  };
>  
> +G_DEFINE_TYPE(PlaybackChannel, playback_channel, TYPE_SND_CHANNEL)
> +
> +
> +typedef struct SpiceRecordState RecordChannel;
> +typedef SndChannelClass RecordChannelClass;

same

> +
> +#define TYPE_RECORD_CHANNEL record_channel_get_type()
> +#define RECORD_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),
> TYPE_RECORD_CHANNEL, RecordChannel))
> +GType record_channel_get_type(void) G_GNUC_CONST;
> +
>  struct SpiceRecordState {
> -struct SndChannel channel;
> +SndChannel channel;
>  };
>  
> +G_DEFINE_TYPE(RecordChannel, record_channel, TYPE_SND_CHANNEL)
> +
> +
>  typedef struct RecordChannelClient {
>  SndChannelClient base;
>  uint32_t samples[RECORD_SAMPLES_SIZE];
> @@ -201,7 +232,7 @@ static SndChannelClient
> *snd_channel_unref(SndChannelClient *client)
>  static RedsState* snd_channel_get_server(SndChannelClient *client)
>  {
>  g_return_val_if_fail(client != NULL, NULL);
> -return red_channel_get_server(client->channel->base_channel);
> +return red_channel_get_server(RED_CHANNEL(client->channel));
>  }
>  
>  static void snd_disconnect_channel(SndChannelClient *client)
> @@ -892,7 +923,7 @@ static SndChannelClient *__new_channel(SndChannel
> *channel, int size, uint32_t c
>  #endif
>  int tos;
>  MainChannelClient *mcc = red_client_get_main(red_client);
> -RedsState *reds = red_channel_get_server(channel->base_channel);
> +RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
>  
>  spice_assert(stream);
>  if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
> @@ -953,7 +984,7 @@ static SndChannelClient *__new_channel(SndChannel
> *channel, int size, uint32_t c
>  client->cleanup = cleanup;
>  
>  client->channel_client =
> -dummy_channel_client_create(channel->base_channel,
> red_client,
> +dummy_channel_client_create(RED_CHANNEL(channel),
> red_client,
>  num_common_caps, common_caps,
> num_caps, caps);
>  if (!client->channel_client) {
>  goto error2;
> @@ -968,6 +999,29 @@ error1:
>  return NULL;
>  }
>  
> +static int snd_channel_config_socket(RedChannelClient *rcc)
> +{
> +g_assert_not_reached();
> +}
> +
> +static void snd_channel_on_disconnect(RedChannelClient *rcc)
> +{
> +g_assert_not_reached();
> +}
> +
> +static uint8_t*
> +snd_channel_client_alloc_recv_buf(RedChannelClient *rcc, uint16_t
> type, uint32_t size)
> +{
> +g_assert_not_reached();
> +}
> +
> +static void
> +snd_channel_client_release_recv_buf(RedChannelClient *rcc, uint16_t
> type, uint32_t size,
> +uint8_t *msg)
> +{
> +g_assert_not_reached();
> +}
> +
>  static void snd_disconnect_channel_client(RedChannelClient *rcc)
>  {
>  SndChannel *channel;
> @@ -975,7 +1029,7 @@ static void
> snd_disconnect_channel_client(RedChannelClient *rcc)
>  uint32_t type;
>  
>  spice_assert(red_channel);
> -channel = (SndChannel *)g_object_get_data(G_OBJECT(red_channel),
> "sound-channel");
> +channel = SND_CHANNEL(red_channel);
>  

Re: [Spice-devel] [PATCH v4 04/17] sound: Rename SndWorker to SndChannel

[Jonathon Jongsma]

On Thu, 2016-12-01 at 11:24 +, Frediano Ziglio wrote:
> SndWorker has been historically based on RedChannel, initial git
> commit
> has:
> struct SndWorker {
>  Channel base;
>  ...
> };


I'd add a short note to the commit log explaining that this is only
possible since we just renamed SndChannel to SndChannelClient.

On an semi-unrelated note: Should we take this opportunity to use the
full word "Sound" for these types instead of "Snd"? SoundChannel

I find unnecessary shortening to be a bit annoying. It would also avoid
the confusion that could be introduced by bisecting old versions of the
code and not realizing that the name SndChannel refers to completely
different objects in different versions of the repository.


Jonathon



> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 312 +
> -
>  1 file changed, 156 insertions(+), 156 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index bafdd1e..5b66f3a 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -79,12 +79,12 @@ typedef int
> (*snd_channel_handle_message_proc)(SndChannelClient *client, size_t 
>  typedef void (*snd_channel_on_message_done_proc)(SndChannelClient
> *client);
>  typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient
> *client);
>  
> -typedef struct SndWorker SndWorker;
> +typedef struct SndChannel SndChannel;
>  
>  /* Connects an audio client to a Spice client */
>  struct SndChannelClient {
>  RedsStream *stream;
> -SndWorker *worker;
> +SndChannel *channel;
>  spice_parse_channel_func_t parser;
>  int refs;
>  
> @@ -145,10 +145,10 @@ typedef struct SpiceVolumeState {
>  } SpiceVolumeState;
>  
>  /* Base class for SpicePlaybackState and SpiceRecordState */
> -struct SndWorker {
> +struct SndChannel {
>  RedChannel *base_channel;
>  SndChannelClient *connection; /* Only one client is supported */
> -SndWorker *next; /* For the global SndWorker list */
> +SndChannel *next; /* For the global SndChannel list */
>  
>  int active;
>  SpiceVolumeState volume;
> @@ -156,11 +156,11 @@ struct SndWorker {
>  };
>  
>  struct SpicePlaybackState {
> -struct SndWorker worker;
> +struct SndChannel channel;
>  };
>  
>  struct SpiceRecordState {
> -struct SndWorker worker;
> +struct SndChannel channel;
>  };
>  
>  typedef struct RecordChannelClient {
> @@ -176,11 +176,11 @@ typedef struct RecordChannelClient {
>  } RecordChannelClient;
>  
>  /* A list of all Spice{Playback,Record}State objects */
> -static SndWorker *workers;
> +static SndChannel *snd_channels;
>  
>  static void snd_receive(SndChannelClient *client);
> -static void snd_playback_start(SndWorker *worker);
> -static void snd_record_start(SndWorker *worker);
> +static void snd_playback_start(SndChannel *channel);
> +static void snd_record_start(SndChannel *channel);
>  
>  static SndChannelClient *snd_channel_ref(SndChannelClient *client)
>  {
> @@ -201,12 +201,12 @@ static SndChannelClient
> *snd_channel_unref(SndChannelClient *client)
>  static RedsState* snd_channel_get_server(SndChannelClient *client)
>  {
>  g_return_val_if_fail(client != NULL, NULL);
> -return red_channel_get_server(client->worker->base_channel);
> +return red_channel_get_server(client->channel->base_channel);
>  }
>  
>  static void snd_disconnect_channel(SndChannelClient *client)
>  {
> -SndWorker *worker;
> +SndChannel *channel;
>  RedsState *reds;
>  RedChannel *red_channel;
>  uint32_t type;
> @@ -220,17 +220,17 @@ static void
> snd_disconnect_channel(SndChannelClient *client)
>  g_object_get(red_channel, "channel-type", , NULL);
>  spice_debug("SndChannelClient=%p rcc=%p type=%d",
>   client, client->channel_client, type);
> -worker = client->worker;
> +channel = client->channel;
>  client->cleanup(client);
> -red_channel_client_disconnect(worker->connection-
> >channel_client);
> -worker->connection->channel_client = NULL;
> +red_channel_client_disconnect(channel->connection-
> >channel_client);
> +channel->connection->channel_client = NULL;
>  reds_core_watch_remove(reds, client->stream->watch);
>  client->stream->watch = NULL;
>  reds_stream_free(client->stream);
>  client->stream = NULL;
>  spice_marshaller_destroy(client->send_data.marshaller);
>  snd_channel_unref(client);
> -worker->connection = NULL;
> +channel->connection = NULL;
>  }
>  
>  static void snd_playback_free_frame(PlaybackChannelClient
> *playback_client, AudioFrame *frame)
> @@ -384,11 +384,11 @@ static int
> snd_record_handle_message(SndChannelClient *client, size_t size, uint
>  return snd_record_handle_write((RecordChannelClient
> *)client, size, message);
>  case SPICE_MSGC_RECORD_MODE: {
>  SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
> -SndWorker *worker = client->worker;
> +   

Re: [Spice-devel] [PATCH v4 00/17] Remove DummyChannel* objects

[Jonathon Jongsma]

I'm a little bit confused. This appears to be the third patch series
that is designated "v4". That means this should actually be v6, no?
(although the previous series was sent exactly 1 minute before this
one, so maybe they're the same?)


On Thu, 2016-12-01 at 11:24 +, Frediano Ziglio wrote:
> These objects were used by the sound channel as
> this channel read/write to/from client directly.
> This make the code of this channel quite different
> from the other ones.
> Also this reduce code duplication from RedChannelClient
> and increase encapsulation.
> First set of patches attempt to rename fields/structures
> to prepare for the conversion to RedChannel/RedChannelClient
> and GObject.
> Than other patches basically do the conversion and
> some cleanup.
> 
> Changes since v4:
> - merged some patches;
> - make sure all commit are working;
> - some comment updates;
> - added some additional cleanup patches.
> 
> Changes since v3:
> - updated some commit message;
> - rename structures and related fields in the same commit.
> - reverted wrong field rename (RedChannel *client).
> 
> Changes since v2:
> - send playback data as soon as possible;
> - split renames putting client renames first then worker ones;
> - some commit message changes.
> 
> Changes since v1:
> - split some long lines;
> - do not attempt to free in progress frame before is full sent.
>   This could cause raw frames to be overridden.
> 
> Frediano Ziglio (17):
>   sound: Use worker directly
>   sound: Rename SndChannel to SndChannelClient
>   sound: Rename {Record,Playback}Channel to *ChannelClient
>   sound: Rename SndWorker to SndChannel
>   sound: Convert SndChannel to GObject
>   sound: Implements config_socket RedChannel callback
>   sound: Convert SndChannelClient to GObject
>   Remove DummyChannel* objects
>   Make RedChannelClient::incoming private
>   sound: Free more on SndChannel finalize
>   sound: Use default disconnect for client channels
>   sound: Reuse code for snd_set_{playback,record}_peer
>   sound: Reuse code for migrating client channels
>   sound: Reuse code to set volume and mute
>   sound: Use default message handler if possible
>   sound: Make clear active and client_active are boolean
>   sound: Use memcpy instead of manually copy volume array
> 
>  server/Makefile.am  |4 +-
>  server/dummy-channel-client.c   |  138 +--
>  server/dummy-channel-client.h   |   64 +-
>  server/dummy-channel.c  |   94 +--
>  server/dummy-channel.h  |   60 +-
>  server/red-channel-client-private.h |   11 +-
>  server/red-channel-client.c |   12 +-
>  server/red-channel-client.h |   13 +-
>  server/sound.c  | 1737 +--
> -
>  9 files changed, 850 insertions(+), 1283 deletions(-)
>  delete mode 100644 server/dummy-channel-client.c
>  delete mode 100644 server/dummy-channel-client.h
>  delete mode 100644 server/dummy-channel.c
>  delete mode 100644 server/dummy-channel.h
> 
> base-commit: 1f800090ef671e9fccb49ed64c01a85b6eb0890a
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server] Add a comment to red_channel_client_init_send_data

[Jonathon Jongsma]

On Thu, 2016-11-24 at 08:57 -0500, Frediano Ziglio wrote:
> > 
> > 
> > On Thu, Nov 24, 2016 at 10:48:24AM +, Frediano Ziglio wrote:
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/red-channel-client.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/server/red-channel-client.h b/server/red-channel-
> > > client.h
> > > index 94b4f58..93d0315 100644
> > > --- a/server/red-channel-client.h
> > > +++ b/server/red-channel-client.h
> > > @@ -89,6 +89,9 @@ void
> > > red_channel_client_shutdown(RedChannelClient *rcc);
> > >  int red_channel_client_handle_message(RedChannelClient *rcc,
> > > uint32_t
> > >  size,
> > >    uint16_t type, void
> > > *message);
> > >  /* when preparing send_data: should call init and then use
> > > marshaller */
> > > +/* item is retained as long as the message is sent to the
> > > client,
> > 
> > "The item is retained", then I don't understand when the item stops
> > being retained. "The item is only released after the message has
> > been
> > sent to the client" ?
> > 
> > > 
> > > + * this is used for instance to make sure an attached data
> > > referenced
> > 
> > "This is used for instance to make sure attached data ..."
> > 
> > > 
> > > + * by the marshaller is still valid when data are used */
> > 
> > and I would say "when it's used"
> > 
> > Maybe better to wait for Jonathon's input on the phrasing?
> > 
> > Christophe
> > 
> 
> I'll wait, in the meantime I updated the commit log.
> 
> What's not clear with this API is when to pass the item and when not.
> Basically when there is a spice_marshaller_add_ref in the code that
> send the item potentially it's required to pass the item to
> red_channel_client_init_send_data, otherwise passing NULL will free
> the item when *_send_item returns (if not references elsewhere).
> 
> Frediano



Hmm, this is a weird function. I'm tempted to re-design the whole thing
rather than just adding a comment. 

I'm still a bit confused about when you're supposed to pass an item to
this function, even after your description. As far as I can tell, it's
only required when a marshaller function doesn't copy all of the data
from the PipeItem but instead references some data from that PipeItem
by pointer? In that case, the PipeItem needs to stay alive until we
actually send the message so that it can read that data to send it over
the network.

As far as I can tell, what you say about spice_marshaller_add_ref() is
sort of correct, but only if the data that we're adding is owned by the
PipeItem. (by the way, spice_marshaller_add_ref() is a weird name,
since it doesn't seem to have anythign to do with references?? Perhaps
spice_marshaller_add_data_buf() would be more accurate?)

Here's a very rough proof-of-concept that I think is a bit nicer.
It only tackles one single occurence of this pattern, but would perhaps
allow us to remove this third parameter from _init_send_data()
completely if it was done for all other cases. Note that it has
undergone almost no testing; I post it only for discussion. A nicer
solution would involve improving/redesigning the add_buf_from_info()
function since the pipe_item argument appears unrelated and tacked on.
Comments appreciated.



diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index e421bf7..49e5229 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -128,10 +128,20 @@ typedef struct {
 uint32_t size;
 } AddBufInfo;
 
-static void add_buf_from_info(SpiceMarshaller *m, AddBufInfo *info)
+static void unref_buf(uint8_t* data, void *opaque)
+{
+/* the data is owned by the pipe item, so just unref the pipe item
*/
+RedPipeItem *item = opaque;
+red_pipe_item_unref(item);
+}
+
+static void add_buf_from_info(SpiceMarshaller *m, AddBufInfo *info,
RedPipeItem *item)
 {
 if (info->data) {
-spice_marshaller_add_ref(m, info->data, info->size);
+/* the pipe item ultimately owns the data, so keep it alive
until the
+ * data is no longer needed */
+red_pipe_item_ref(item);
+spice_marshaller_add_ref_full(m, info->data, info->size,
unref_buf, item);
 }
 }
 
@@ -205,7 +215,7 @@ static void
red_marshall_cursor_init(CursorChannelClient *ccc, SpiceMarshaller *
 
 cursor_fill(ccc, , cursor_channel->item, );
 spice_marshall_msg_cursor_init(base_marshaller, );
-add_buf_from_info(base_marshaller, );
+add_buf_from_info(base_marshaller, , pipe_item);
 }
 
 static void cursor_marshall(CursorChannelClient *ccc,
@@ -235,13 +245,13 @@ static void cursor_marshall(CursorChannelClient
*ccc,
 SpiceMsgCursorSet cursor_set;
 AddBufInfo info;
 
-red_channel_client_init_send_data(rcc,
SPICE_MSG_CURSOR_SET, pipe_item);
+red_channel_client_init_send_data(rcc,
SPICE_MSG_CURSOR_SET, NULL);
 cursor_set.position = cmd->u.set.position;
 cursor_set.visible = 

Re: [Spice-devel] [spice-gtk v2 1/2] channel-usbredir: Fix crash on channel-up

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 02:20:29PM +0100, Victor Toso wrote:
> Hi,
> 
> On Thu, Dec 01, 2016 at 01:29:50PM +0100, Christophe Fergeau wrote:
> > On Wed, Nov 30, 2016 at 06:36:32PM +0100, Victor Toso wrote:
> > > From: Victor Toso 
> > > 
> > > SpiceSession does not initialize its SpiceUsbDeviceManager object on
> > > startup that could lead to a race condition where channel-usbredir is
> > > requested to flush data while it is uninitialized.
> > > 
> > > In a few places, spice_usb_device_manager_get() is called as in
> > > usb-device-widget.c and spice-gtk-session.c but not used in
> > > spicy-stats, making the tool to crash on startup.
> > 
> > Just running spicy-stats when there is a usbredir channel is going to
> > cause a crash?
> 
> Yes

Ok, I'd mention this explicitly "making the tool crash on startup if
it encounters an usbredir channel"

> 
> > Isn't this avoided by your next patch as well
> 
> Yes
> 
> > which makes sure host is not NULL before trying to flush?
> 
> I see this as two different problems, really.
> 
> 1-) chanel-usbredir should take in consideration that it might not be
> initialized (second patch)
> 2-) SpiceSession should initialize SpiceUsbDeviceManager (this patch)
> otherwise, situations like (1) could happen;
> 
> spice_usb_device_manager_initable_init() could fail here so, the
> situation in (1) might as well happen.
> 
> > 
> > Christophe
> > 
> > > 
> > >  #0 in usbredirhost_write_guest_data (host=0x0) at 
> > > usbredir/usbredirhost/usbredirhost.c:876
> > >  #1 in spice_usbredir_channel_up (c=0x643830) at channel-usbredir.c:821
> > >  #2 in spice_channel_up (channel=0x643830) at spice-channel.c:1238
> > >  #3 in spice_channel_recv_auth (channel=0x643830) at spice-channel.c:1225
> > >  #4 in spice_channel_coroutine (data=0x643830) at spice-channel.c:2580
> > >  #5 in coroutine_trampoline (cc=0x642ec0) at coroutine_ucontext.c:63
> > >  #6 in continuation_trampoline (i0=6565568, i1=0) at continuation.c:55
> > > 
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838
> > > 
> > > Signed-off-by: Victor Toso 
> > > Reported-by: Michael Cullen 
> > > ---
> > >  src/spice-session.c | 7 +++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/src/spice-session.c b/src/spice-session.c
> > > index f900bd1..91e4f97 100644
> > > --- a/src/spice-session.c
> > > +++ b/src/spice-session.c
> > > @@ -281,6 +281,7 @@ static void spice_session_init(SpiceSession *session)
> > >  {
> > >  SpiceSessionPrivate *s;
> > >  gchar *channels;
> > > +GError *err = NULL;
> > >  
> > >  SPICE_DEBUG("New session (compiled from package " PACKAGE_STRING 
> > > ")");
> > >  s = session->priv = SPICE_SESSION_GET_PRIVATE(session);
> > > @@ -293,6 +294,12 @@ static void spice_session_init(SpiceSession *session)
> > >  s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
> > >  s->glz_window = glz_decoder_window_new();
> > >  update_proxy(session, NULL);
> > > +
> > > +spice_usb_device_manager_get(session, );

I know this is going to be redundant, but
session->priv->usb_manager = spice_usb_device_manager_get(session, );
would look a bit better.

Either way,
Acked-by: Christophe Fergeau 


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] [PATCH v4 04/17] sound: Rename SndWorker to SndChannel

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 11:47:15AM -0500, Frediano Ziglio wrote:
> > 
> > On Thu, Dec 01, 2016 at 11:24:26AM +, Frediano Ziglio wrote:
> > > @@ -1506,73 +1506,73 @@ static void remove_worker(SndWorker *worker)
> > >  
> > >  void snd_attach_playback(RedsState *reds, SpicePlaybackInstance *sin)
> > >  {
> > > -SndWorker *playback_worker;
> > > -RedChannel *channel;
> > > +SndChannel *playback;
> > > +RedChannel *red_channel;
> > 
> > Here s/channel/red_channel was not really necessary as the other
> > variable is named "playback" and not channel, ditto in snd_attach_record
> > below.
> > 
> 
> Should I revert them?

I'm fine with both options, so as you prefer :)

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] [PATCH v4 04/17] sound: Rename SndWorker to SndChannel

[Frediano Ziglio]

> 
> On Thu, Dec 01, 2016 at 11:24:26AM +, Frediano Ziglio wrote:
> > @@ -1506,73 +1506,73 @@ static void remove_worker(SndWorker *worker)
> >  
> >  void snd_attach_playback(RedsState *reds, SpicePlaybackInstance *sin)
> >  {
> > -SndWorker *playback_worker;
> > -RedChannel *channel;
> > +SndChannel *playback;
> > +RedChannel *red_channel;
> 
> Here s/channel/red_channel was not really necessary as the other
> variable is named "playback" and not channel, ditto in snd_attach_record
> below.
> 

Should I revert them?

> Acked-by: Christophe Fergeau 
> 
> Christophe

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1] channel-main: demote log level from warning to debug

[Victor Toso]

Hi,

On Thu, Dec 01, 2016 at 06:17:41PM +0200, Uri Lublin wrote:
> On 11/30/2016 07:50 PM, Victor Toso wrote:
> > From: Victor Toso 
> >
> > Failing to get playback or record volume async on startup is very
> > common making this warning too worrisome for users.
> >
> > If the audio back-end does not cache the last volume used or if this
> > is the first time the application is launched, this message will be
> > seen.
> >
> > Signed-off-by: Victor Toso 
>
> Hi Victor,
>
> OK, but (see below)
>
> > ---
> >  src/channel-main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 72ca712..e632c8e 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -1171,7 +1171,7 @@ static void audio_playback_volume_info_cb(GObject 
> > *object, GAsyncResult *res, gp
> >, );
> >  if (ret == FALSE || volume == NULL || nchannels == 0) {
> >  if (error != NULL) {
> > -g_warning("Failed to get playback async volume info: %s", 
> > error->message);
> > +spice_debug("Failed to get playback async volume info: %s", 
> > error->message);
>
> I see SPICE_DEBUG is used in lines above/below.
>
> Uri.

Nowadays they should be very similar.

SPICE_DEBUG: src/spice-util.h, uses g_debug()
spice_debug: spice-common/common/log.h uses g_log()

I think I removed all SPICE_DEBUG in the past while trying to improve
the logs...

For now, to keep coherence I can change to the uppercase one before
pushing (if acked)

Cheers,

>
> >  g_error_free(error);
> >  } else {
> >  SPICE_DEBUG("Failed to get playback async volume info");
> > @@ -1227,7 +1227,7 @@ static void audio_record_volume_info_cb(GObject 
> > *object, GAsyncResult *res, gpoi
> >  ret = spice_audio_get_record_volume_info_finish(audio, res, , 
> > , , );
> >  if (ret == FALSE || volume == NULL || nchannels == 0) {
> >  if (error != NULL) {
> > -g_warning("Failed to get record async volume info: %s", 
> > error->message);
> > +spice_debug("Failed to get record async volume info: %s", 
> > error->message);
> >  g_error_free(error);
> >  } else {
> >  SPICE_DEBUG("Failed to get record async volume info");
> > 
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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-gtk v1] channel-main: demote log level from warning to debug

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 05:06:42PM +0100, Victor Toso wrote:
> Hi,
> 
> On Thu, Dec 01, 2016 at 04:33:36PM +0100, Christophe Fergeau wrote:
> > On Thu, Dec 01, 2016 at 02:15:49PM +0100, Victor Toso wrote:
> > > On Thu, Dec 01, 2016 at 11:37:32AM +0100, Christophe Fergeau wrote:
> > > > On Wed, Nov 30, 2016 at 06:50:39PM +0100, Victor Toso wrote:
> > > > > From: Victor Toso 
> > > > >
> > > > > Failing to get playback or record volume async on startup is very
> > > > > common making this warning too worrisome for users.
> > > > >
> > > >
> > > > Looked quickly at the code, and this seems to be getting the volume info
> > > > from pulseaudio. What is special at startup that makes this not work?
> > >
> > > As mentioned in the commit log below:
> > >
> > > > > If the audio back-end does not cache the last volume used or if this
> > > > > is the first time the application is launched, this message will be
> > > > > seen.
> > >
> > > With pulseaudio back-end, the playback/record volume is cached in user
> > > file with the application name as reference. The volume info should
> > > fail:
> > >
> > > 1-) For playback, in the first time a spice client is launched
> > > 2-) For record, always till the first it is used and, furthermore,
> > > cached.
> >
> > It seems obvious to you that in this case retrieving the volume is going
> > to fail, and that it's normal, but it's not obvious at all to me.
> > The application is running, so it should be able to retrieve some kind
> > of volume?
> 
> Ah, I thought that how to get the volume was understood and the problem
> was understanding when the warning is showed, sorry.

Thanks for the detailed explanation below, the big thing that I
was missing is:
> Now, the volume-sync happens when agent announce its capabilities. It is
> common that we don't have a playback stream being played at this time,
> and it is even harder for a record stream.

With that in mind, it makes sense that our only fallback is the cached
info, which might be missing in the cases that you outline, which
triggers the warning. If you can mention this bit in the commit log,

Acked-by: Christophe Fergeau 

Thanks,

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] [PATCH v4 07/17] sound: Convert SndChannelClient to GObject

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 11:24:29AM +, Frediano Ziglio wrote:
> This patch is quite huge but have some benefits:
> - reduce dependency (DummyChannel* and some RedChannelClient internals);
> - reuse RedChannelClient instead of reading from the RedsStream
>   directly.
> 
> 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.
> 
> Another difference is the AudioFrame allocation. 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 having RedChannelClient as
> as base class 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.

No luck in adding a few more introductory patches to split this one
more?

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-gtk v1] channel-main: demote log level from warning to debug

[Uri Lublin]

On 11/30/2016 07:50 PM, Victor Toso wrote:

From: Victor Toso 

Failing to get playback or record volume async on startup is very
common making this warning too worrisome for users.

If the audio back-end does not cache the last volume used or if this
is the first time the application is launched, this message will be
seen.

Signed-off-by: Victor Toso 


Hi Victor,

OK, but (see below)


---
 src/channel-main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/channel-main.c b/src/channel-main.c
index 72ca712..e632c8e 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1171,7 +1171,7 @@ static void audio_playback_volume_info_cb(GObject 
*object, GAsyncResult *res, gp
   , );
 if (ret == FALSE || volume == NULL || nchannels == 0) {
 if (error != NULL) {
-g_warning("Failed to get playback async volume info: %s", 
error->message);
+spice_debug("Failed to get playback async volume info: %s", 
error->message);


I see SPICE_DEBUG is used in lines above/below.

Uri.


 g_error_free(error);
 } else {
 SPICE_DEBUG("Failed to get playback async volume info");
@@ -1227,7 +1227,7 @@ static void audio_record_volume_info_cb(GObject *object, 
GAsyncResult *res, gpoi
 ret = spice_audio_get_record_volume_info_finish(audio, res, , , 
, );
 if (ret == FALSE || volume == NULL || nchannels == 0) {
 if (error != NULL) {
-g_warning("Failed to get record async volume info: %s", 
error->message);
+spice_debug("Failed to get record async volume info: %s", 
error->message);
 g_error_free(error);
 } else {
 SPICE_DEBUG("Failed to get record async volume info");



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

Re: [Spice-devel] [PATCH v4 04/17] sound: Rename SndWorker to SndChannel

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 11:24:26AM +, Frediano Ziglio wrote:
> @@ -1506,73 +1506,73 @@ static void remove_worker(SndWorker *worker)
>  
>  void snd_attach_playback(RedsState *reds, SpicePlaybackInstance *sin)
>  {
> -SndWorker *playback_worker;
> -RedChannel *channel;
> +SndChannel *playback;
> +RedChannel *red_channel;

Here s/channel/red_channel was not really necessary as the other
variable is named "playback" and not channel, ditto in snd_attach_record
below.

Acked-by: Christophe Fergeau 

Christophe

>  ClientCbs client_cbs = { NULL, };
>  
>  sin->st = spice_new0(SpicePlaybackState, 1);
> -playback_worker = >st->worker;
> -playback_worker->frequency = SND_CODEC_CELT_PLAYBACK_FREQ; /* Default to 
> the legacy rate */
> +playback = >st->channel;
> +playback->frequency = SND_CODEC_CELT_PLAYBACK_FREQ; /* Default to the 
> legacy rate */
>  
> -// TODO: Make RedChannel base of worker? instead of assigning it to 
> channel->data
> -channel = dummy_channel_new(reds, SPICE_CHANNEL_PLAYBACK, 0);
> +// TODO: Make RedChannel base of channel? instead of assigning it to 
> channel->data
> +red_channel = dummy_channel_new(reds, SPICE_CHANNEL_PLAYBACK, 0);
>  
> -g_object_set_data(G_OBJECT(channel), "sound-worker", playback_worker);
> +g_object_set_data(G_OBJECT(red_channel), "sound-channel", playback);
>  client_cbs.connect = snd_set_playback_peer;
>  client_cbs.disconnect = snd_disconnect_channel_client;
>  client_cbs.migrate = snd_playback_migrate_channel_client;
> -red_channel_register_client_cbs(channel, _cbs, playback_worker);
> +red_channel_register_client_cbs(red_channel, _cbs, playback);
>  
>  if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1, 
> SND_CODEC_ANY_FREQUENCY))
> -red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_CELT_0_5_1);
> +red_channel_set_cap(red_channel, SPICE_PLAYBACK_CAP_CELT_0_5_1);
>  
> -red_channel_set_cap(channel, SPICE_PLAYBACK_CAP_VOLUME);
> +red_channel_set_cap(red_channel, SPICE_PLAYBACK_CAP_VOLUME);
>  
> -playback_worker->base_channel = channel;
> -add_worker(playback_worker);
> -reds_register_channel(reds, channel);
> +playback->base_channel = red_channel;
> +add_channel(playback);
> +reds_register_channel(reds, red_channel);
>  }
>  
>  void snd_attach_record(RedsState *reds, SpiceRecordInstance *sin)
>  {
> -SndWorker *record_worker;
> -RedChannel *channel;
> +SndChannel *record;
> +RedChannel *red_channel;
>  ClientCbs client_cbs = { NULL, };
>  
>  sin->st = spice_new0(SpiceRecordState, 1);
> -record_worker = >st->worker;
> -record_worker->frequency = SND_CODEC_CELT_PLAYBACK_FREQ; /* Default to 
> the legacy rate */
> +record = >st->channel;
> +record->frequency = SND_CODEC_CELT_PLAYBACK_FREQ; /* Default to the 
> legacy rate */
>  
> -// TODO: Make RedChannel base of worker? instead of assigning it to 
> channel->data
> -channel = dummy_channel_new(reds, SPICE_CHANNEL_RECORD, 0);
> +// TODO: Make RedChannel base of channel? instead of assigning it to 
> channel->data
> +red_channel = dummy_channel_new(reds, SPICE_CHANNEL_RECORD, 0);
>  
> -g_object_set_data(G_OBJECT(channel), "sound-worker", record_worker);
> +g_object_set_data(G_OBJECT(red_channel), "sound-channel", record);
>  client_cbs.connect = snd_set_record_peer;
>  client_cbs.disconnect = snd_disconnect_channel_client;
>  client_cbs.migrate = snd_record_migrate_channel_client;
> -red_channel_register_client_cbs(channel, _cbs, record_worker);
> +red_channel_register_client_cbs(red_channel, _cbs, record);
>  if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1, 
> SND_CODEC_ANY_FREQUENCY))
> -red_channel_set_cap(channel, SPICE_RECORD_CAP_CELT_0_5_1);
> -red_channel_set_cap(channel, SPICE_RECORD_CAP_VOLUME);
> +red_channel_set_cap(red_channel, SPICE_RECORD_CAP_CELT_0_5_1);
> +red_channel_set_cap(red_channel, SPICE_RECORD_CAP_VOLUME);
>  
> -record_worker->base_channel = channel;
> -add_worker(record_worker);
> -reds_register_channel(reds, channel);
> +record->base_channel = red_channel;
> +add_channel(record);
> +reds_register_channel(reds, red_channel);
>  }
>  
> -static void snd_detach_common(SndWorker *worker)
> +static void snd_detach_common(SndChannel *channel)
>  {
> -if (!worker) {
> +if (!channel) {
>  return;
>  }
> -RedsState *reds = red_channel_get_server(worker->base_channel);
> +RedsState *reds = red_channel_get_server(channel->base_channel);
>  
> -remove_worker(worker);
> -snd_disconnect_channel(worker->connection);
> -reds_unregister_channel(reds, worker->base_channel);
> -red_channel_destroy(worker->base_channel);
> -free(worker->volume.volume);
> -worker->volume.volume = NULL;
> +remove_channel(channel);
> +

Re: [Spice-devel] [spice-gtk v1] channel-main: demote log level from warning to debug

[Victor Toso]

Hi,

On Thu, Dec 01, 2016 at 04:33:36PM +0100, Christophe Fergeau wrote:
> On Thu, Dec 01, 2016 at 02:15:49PM +0100, Victor Toso wrote:
> > On Thu, Dec 01, 2016 at 11:37:32AM +0100, Christophe Fergeau wrote:
> > > On Wed, Nov 30, 2016 at 06:50:39PM +0100, Victor Toso wrote:
> > > > From: Victor Toso 
> > > >
> > > > Failing to get playback or record volume async on startup is very
> > > > common making this warning too worrisome for users.
> > > >
> > >
> > > Looked quickly at the code, and this seems to be getting the volume info
> > > from pulseaudio. What is special at startup that makes this not work?
> >
> > As mentioned in the commit log below:
> >
> > > > If the audio back-end does not cache the last volume used or if this
> > > > is the first time the application is launched, this message will be
> > > > seen.
> >
> > With pulseaudio back-end, the playback/record volume is cached in user
> > file with the application name as reference. The volume info should
> > fail:
> >
> > 1-) For playback, in the first time a spice client is launched
> > 2-) For record, always till the first it is used and, furthermore,
> > cached.
>
> It seems obvious to you that in this case retrieving the volume is going
> to fail, and that it's normal, but it's not obvious at all to me.
> The application is running, so it should be able to retrieve some kind
> of volume?

Ah, I thought that how to get the volume was understood and the problem
was understanding when the warning is showed, sorry.

We can retrieve the volume in two ways: from an ongoing stream
and from cache, in spice-pulse.c see the following comment on
pulse_stream_restore_info_async()

 /* If Playback/Record stream is created we use pulse API to get volume-info
  * from those streams directly. If the stream is not created, retrieve last
  * volume/mute values from Pulse database using the application name;
  * If we already have retrieved volume-info from Pulse database then it is
  * safe to return the volume-info we already have in info */

However, when we call either
[playback] pa_context_set_sink_input_volume() or
[record] pa_context_set_source_output_volume(), PulseAudio should cache
the stream volume in a local cache file, here it is $HOME/.config/pulse/

Now, the volume-sync happens when agent announce its capabilities. It is
common that we don't have a playback stream being played at this time,
and it is even harder for a record stream.

> Or is it trying to retrieve the volume that was set in the previous
> session, which I understand can fail?

Yes, I think the failure is always when retrieving the volume from cache
as it should be super simple when there is an ongoing stream.

> But in this case, why are the next calls fine with retrieving the
> volume?

Is it? I think we will keep seeing the warning unless PulseAudio caches
the stream volume.

Well, all above was true when I implemented this. I've removed the cache
file from PulseAudio and it does not seem to be creating a new one? :(

>
> Christophe
>
> >
> > For gstreamer back-end, even with pulseaudio src/sink, it might not fail
> > but it will be wrong, see:
> > https://bugzilla.gnome.org/show_bug.cgi?id=766020
> >
> > > >
> > > > Signed-off-by: Victor Toso 
> > > > ---
> > > >  src/channel-main.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > > index 72ca712..e632c8e 100644
> > > > --- a/src/channel-main.c
> > > > +++ b/src/channel-main.c
> > > > @@ -1171,7 +1171,7 @@ static void audio_playback_volume_info_cb(GObject 
> > > > *object, GAsyncResult *res, gp
> > > >, );
> > > >  if (ret == FALSE || volume == NULL || nchannels == 0) {
> > > >  if (error != NULL) {
> > > > -g_warning("Failed to get playback async volume info: %s", 
> > > > error->message);
> > > > +spice_debug("Failed to get playback async volume info: 
> > > > %s", error->message);
> > > >  g_error_free(error);
> > > >  } else {
> > > >  SPICE_DEBUG("Failed to get playback async volume info");
> > > > @@ -1227,7 +1227,7 @@ static void audio_record_volume_info_cb(GObject 
> > > > *object, GAsyncResult *res, gpoi
> > > >  ret = spice_audio_get_record_volume_info_finish(audio, res, , 
> > > > , , );
> > > >  if (ret == FALSE || volume == NULL || nchannels == 0) {
> > > >  if (error != NULL) {
> > > > -g_warning("Failed to get record async volume info: %s", 
> > > > error->message);
> > > > +spice_debug("Failed to get record async volume info: %s", 
> > > > error->message);
> > > >  g_error_free(error);
> > > >  } else {
> > > >  SPICE_DEBUG("Failed to get record async volume info");
> > > > -- 
> > > > 2.9.3
> > > > 
> > > > ___
> > > > Spice-devel 

Re: [Spice-devel] [PATCH v4 03/17] sound: Rename {Record, Playback}Channel to *ChannelClient

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 11:24:25AM +, Frediano Ziglio wrote:
> Make easier to understand that they refer to client and not
> all channel.
> 
> Specifically:
> - RecordChannel -> RecordChannelClient
> - PlaybackChannel -> PlaybackChannelClient
> - playback_channel -> playback_client
> - record_channel -> record_client
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 368 +-
>  1 file changed, 184 insertions(+), 184 deletions(-)

> @@ -1194,12 +1194,12 @@ static void snd_set_playback_peer(RedChannel 
> *channel, RedClient *client, RedsSt
>int num_caps, uint32_t *caps)
>  {
>  SndWorker *worker = g_object_get_data(G_OBJECT(channel), "sound-worker");
> -PlaybackChannel *playback_channel;
> +PlaybackChannelClient *playback_client;
>  
>  snd_disconnect_channel(worker->connection);
>  
> -if (!(playback_channel = (PlaybackChannel *)__new_channel(worker,
> -  
> sizeof(*playback_channel),
> +if (!(playback_client = (PlaybackChannelClient *)__new_channel(worker,
> +  
> sizeof(*playback_client),
>
> SPICE_CHANNEL_PLAYBACK,
>client,
>stream,

Alignment

> @@ -1440,12 +1440,12 @@ static void snd_set_record_peer(RedChannel *channel, 
> RedClient *client, RedsStre
>  int num_caps, uint32_t *caps)
>  {
>  SndWorker *worker = g_object_get_data(G_OBJECT(channel), "sound-worker");
> -RecordChannel *record_channel;
> +RecordChannelClient *record_client;
>  
>  snd_disconnect_channel(worker->connection);
>  
> -if (!(record_channel = (RecordChannel *)__new_channel(worker,
> -  
> sizeof(*record_channel),
> +if (!(record_client = (RecordChannelClient *)__new_channel(worker,
> +  
> sizeof(*record_client),
>
> SPICE_CHANNEL_RECORD,
>client,
>stream,

Alignment too

Acked-by: Christophe Fergeau 


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] [PATCH v4 02/17] sound: Rename SndChannel to SndChannelClient

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 11:24:24AM +, Frediano Ziglio wrote:
> SndWorker has been historically based on RedChannel, initial git commit
> has:
> struct SndWorker {
>  Channel base;
>  ...
> };
> 
> SndChannel, contrary to what its name may imply is more focused on
> marshalling/sending of sound data, which is the responsibility of
> RedChannelClient for the other SPICE channels.
> 
> This commit and following ones make the naming of these 2 types more
> consistent with how the rest of the code is structured.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 522 +-
>  1 file changed, 261 insertions(+), 261 deletions(-)

Basing most of my review on the count of line changed ;)

> -static SndChannel *__new_channel(SndWorker *worker, int size, uint32_t 
> channel_id,
> - RedClient *client,
> +static SndChannelClient *__new_channel(SndWorker *worker, int size, uint32_t 
> channel_id,
> + RedClient *red_client,
>   RedsStream *stream,
>   int migrate,
>   snd_channel_send_messages_proc 
> send_messages,

Alignment got broken here


Acked-by: Christophe Fergeau 


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] [PATCH v4 01/17] sound: Use worker directly

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Thu, Dec 01, 2016 at 11:24:23AM +, Frediano Ziglio wrote:
> SpicePlaybackState and SpiceRecordState have same structures
> changing only slightly the behaviour.
> Using SndWorker instead allows some minor simplification and
> more code reuse.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 26 --
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 8eddd2b..beab473 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -179,8 +179,8 @@ typedef struct RecordChannel {
>  static SndWorker *workers;
>  
>  static void snd_receive(SndChannel *channel);
> -static void snd_playback_start(SpicePlaybackState *st);
> -static void snd_record_start(SpiceRecordState *st);
> +static void snd_playback_start(SndWorker *worker);
> +static void snd_record_start(SndWorker *worker);
>  
>  static SndChannel *snd_channel_ref(SndChannel *channel)
>  {
> @@ -1026,11 +1026,11 @@ SPICE_GNUC_VISIBLE void 
> spice_server_playback_set_mute(SpicePlaybackInstance *si
>  snd_playback_send_mute(playback_channel);
>  }
>  
> -static void snd_playback_start(SpicePlaybackState *st)
> +static void snd_playback_start(SndWorker *worker)
>  {
> -SndChannel *channel = st->worker.connection;
> +SndChannel *channel = worker->connection;
>  
> -st->worker.active = 1;
> +worker->active = 1;
>  if (!channel)
>  return;
>  spice_assert(!channel->active);
> @@ -1046,7 +1046,7 @@ static void snd_playback_start(SpicePlaybackState *st)
>  
>  SPICE_GNUC_VISIBLE void spice_server_playback_start(SpicePlaybackInstance 
> *sin)
>  {
> -return snd_playback_start(sin->st);
> +return snd_playback_start(>st->worker);
>  }
>  
>  SPICE_GNUC_VISIBLE void spice_server_playback_stop(SpicePlaybackInstance 
> *sin)
> @@ -1195,7 +1195,6 @@ static void snd_set_playback_peer(RedChannel *channel, 
> RedClient *client, RedsSt
>  {
>  SndWorker *worker = g_object_get_data(G_OBJECT(channel), "sound-worker");
>  PlaybackChannel *playback_channel;
> -SpicePlaybackState *st = SPICE_CONTAINEROF(worker, SpicePlaybackState, 
> worker);
>  
>  snd_disconnect_channel(worker->connection);
>  
> @@ -1240,7 +1239,7 @@ static void snd_set_playback_peer(RedChannel *channel, 
> RedClient *client, RedsSt
>  }
>  
>  if (worker->active) {
> -snd_playback_start(st);
> +snd_playback_start(worker);
>  }
>  snd_playback_send(worker->connection);
>  }
> @@ -1294,12 +1293,12 @@ SPICE_GNUC_VISIBLE void 
> spice_server_record_set_mute(SpiceRecordInstance *sin, u
>  snd_record_send_mute(record_channel);
>  }
>  
> -static void snd_record_start(SpiceRecordState *st)
> +static void snd_record_start(SndWorker *worker)
>  {
> -SndChannel *channel = st->worker.connection;
> +SndChannel *channel = worker->connection;
>  RecordChannel *record_channel = SPICE_CONTAINEROF(channel, 
> RecordChannel, base);
>  
> -st->worker.active = 1;
> +worker->active = 1;
>  if (!channel)
>  return;
>  spice_assert(!channel->active);
> @@ -1316,7 +1315,7 @@ static void snd_record_start(SpiceRecordState *st)
>  
>  SPICE_GNUC_VISIBLE void spice_server_record_start(SpiceRecordInstance *sin)
>  {
> -snd_record_start(sin->st);
> +snd_record_start(>st->worker);
>  }
>  
>  SPICE_GNUC_VISIBLE void spice_server_record_stop(SpiceRecordInstance *sin)
> @@ -1442,7 +1441,6 @@ static void snd_set_record_peer(RedChannel *channel, 
> RedClient *client, RedsStre
>  {
>  SndWorker *worker = g_object_get_data(G_OBJECT(channel), "sound-worker");
>  RecordChannel *record_channel;
> -SpiceRecordState *st = SPICE_CONTAINEROF(worker, SpiceRecordState, 
> worker);
>  
>  snd_disconnect_channel(worker->connection);
>  
> @@ -1465,7 +1463,7 @@ static void snd_set_record_peer(RedChannel *channel, 
> RedClient *client, RedsStre
>  
>  on_new_record_channel(worker, _channel->base);
>  if (worker->active) {
> -snd_record_start(st);
> +snd_record_start(worker);
>  }
>  snd_record_send(worker->connection);
>  }
> -- 
> git-series 0.9.1
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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] udscs: Fix a potential NULL pointer dereference

[Francois Gouget]

On Thu, 1 Dec 2016, Christophe Fergeau wrote:

> On Thu, Dec 01, 2016 at 05:19:33AM +0100, Francois Gouget wrote:
> > udscs_server_fill_fds() should accept being passed a NULL pointer.
> 
> I would reword the commit log a bit, like "udscs_server_fill_fds() is
> dereferencing the 'server' pointer, and then checks if it's NULL. This
> commit makes sure the NULL check happens first"
> I'll amend and push, thanks for the patch!

Works for me.


> Acked-by: Christophe Fergeau 
> 
> 
> > 
> > Signed-off-by: Francois Gouget 
> > ---
> >  src/udscs.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/udscs.c b/src/udscs.c
> > index 414dce5..fdd75a4 100644
> > --- a/src/udscs.c
> > +++ b/src/udscs.c
> > @@ -495,11 +495,12 @@ int udscs_server_fill_fds(struct udscs_server 
> > *server, fd_set *readfds,
> >  fd_set *writefds)
> >  {
> >  struct udscs_connection *conn;
> > -int nfds = server->fd + 1;
> > +int nfds;
> >  
> >  if (!server)
> >  return -1;
> >  
> > +nfds = server->fd + 1;
> >  FD_SET(server->fd, readfds);
> >  
> >  conn = server->connections_head.next;
> > -- 
> > 2.10.2
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 


-- 
Francois Gouget 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server] gstreamer: Correctly don't allow too limited bit rates

[Francois Gouget]

On Thu, 1 Dec 2016, Frediano Ziglio wrote:

> The check to limit too low bit rates was setting encoder->bit_rate
> instead of bit_rate. However after some lines bit_rate was used
> to set encoder->bit_rate basically removing the lower threshold.
> 
> Signed-off-by: Frediano Ziglio 
> ---

Acked-by: Francois Gouget 


>  server/gstreamer-encoder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index b9b1a56..e28ab00 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -595,7 +595,7 @@ static void set_bit_rate(SpiceGstEncoder *encoder, 
> uint64_t bit_rate)
>  }
>  if (bit_rate < SPICE_GST_MIN_BITRATE) {
>  /* Don't let the bit rate go too low... */
> -encoder->bit_rate = SPICE_GST_MIN_BITRATE;
> +bit_rate = SPICE_GST_MIN_BITRATE;
>  } else if (bit_rate > encoder->bit_rate) {
>  /* or too high */
>  bit_rate = MIN(bit_rate, get_bit_rate_cap(encoder));
> 


-- 
Francois Gouget 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1] channel-main: demote log level from warning to debug

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 02:15:49PM +0100, Victor Toso wrote:
> On Thu, Dec 01, 2016 at 11:37:32AM +0100, Christophe Fergeau wrote:
> > On Wed, Nov 30, 2016 at 06:50:39PM +0100, Victor Toso wrote:
> > > From: Victor Toso 
> > >
> > > Failing to get playback or record volume async on startup is very
> > > common making this warning too worrisome for users.
> > >
> >
> > Looked quickly at the code, and this seems to be getting the volume info
> > from pulseaudio. What is special at startup that makes this not work?
> 
> As mentioned in the commit log below:
> 
> > > If the audio back-end does not cache the last volume used or if this
> > > is the first time the application is launched, this message will be
> > > seen.
> 
> With pulseaudio back-end, the playback/record volume is cached in user
> file with the application name as reference. The volume info should
> fail:
> 
> 1-) For playback, in the first time a spice client is launched
> 2-) For record, always till the first it is used and, furthermore,
> cached.

It seems obvious to you that in this case retrieving the volume is going
to fail, and that it's normal, but it's not obvious at all to me. The
application is running, so it should be able to retrieve some kind of
volume? Or is it trying to retrieve the volume that was set in the
previous session, which I understand can fail? But in this case, why
are the next calls fine with retrieving the volume?

Christophe

> 
> For gstreamer back-end, even with pulseaudio src/sink, it might not fail
> but it will be wrong, see:
> https://bugzilla.gnome.org/show_bug.cgi?id=766020
> 
> > >
> > > Signed-off-by: Victor Toso 
> > > ---
> > >  src/channel-main.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > index 72ca712..e632c8e 100644
> > > --- a/src/channel-main.c
> > > +++ b/src/channel-main.c
> > > @@ -1171,7 +1171,7 @@ static void audio_playback_volume_info_cb(GObject 
> > > *object, GAsyncResult *res, gp
> > >, );
> > >  if (ret == FALSE || volume == NULL || nchannels == 0) {
> > >  if (error != NULL) {
> > > -g_warning("Failed to get playback async volume info: %s", 
> > > error->message);
> > > +spice_debug("Failed to get playback async volume info: %s", 
> > > error->message);
> > >  g_error_free(error);
> > >  } else {
> > >  SPICE_DEBUG("Failed to get playback async volume info");
> > > @@ -1227,7 +1227,7 @@ static void audio_record_volume_info_cb(GObject 
> > > *object, GAsyncResult *res, gpoi
> > >  ret = spice_audio_get_record_volume_info_finish(audio, res, , 
> > > , , );
> > >  if (ret == FALSE || volume == NULL || nchannels == 0) {
> > >  if (error != NULL) {
> > > -g_warning("Failed to get record async volume info: %s", 
> > > error->message);
> > > +spice_debug("Failed to get record async volume info: %s", 
> > > error->message);
> > >  g_error_free(error);
> > >  } else {
> > >  SPICE_DEBUG("Failed to get record async volume info");
> > > -- 
> > > 2.9.3
> > > 
> > > ___
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 



> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



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

[Spice-devel] [spice-gtk v3] channel-usbredir: prevent crash when calling without host

[Victor Toso]

From: Victor Toso 

As spice_usbredir_channel_up() can be called when channel-usbredir is
not yet initialized, priv->host can be NULL leading to a crash in
usbredir.

See previous patch that does initialize SpiceUsbDeviceManager on
SpiceSession init and also the comment in 5b252b0f499601bcf387c02a4
which fix similar behavior.

We should simply log a critical message instead of crashing the
application.

Signed-off-by: Victor Toso 
---
 src/channel-usbredir.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 4837d68..392a35e 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -817,6 +817,8 @@ static void spice_usbredir_channel_up(SpiceChannel *c)
 SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
 SpiceUsbredirChannelPrivate *priv = channel->priv;
 
+g_return_if_fail(priv->host != NULL);
+
 /* Flush any pending writes */
 usbredirhost_write_guest_data(priv->host);
 }
-- 
2.9.3

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

Re: [Spice-devel] [spice-gtk v2 2/2] channel-usbredir: prevent crash when calling without host

[Victor Toso]

Hi,

On Thu, Dec 01, 2016 at 01:33:56PM +0100, Christophe Fergeau wrote:
> On Wed, Nov 30, 2016 at 06:57:12PM +0100, Victor Toso wrote:
> > Hi,
> > 
> > On Wed, Nov 30, 2016 at 06:45:45PM +0100, Pavel Grunt wrote:
> > > imho this should be in usbredir
> > >
> > > It is not nice when library crashes your program
> > 
> > I don't disagree. Libraries should use guards accordingly.
> > 
> > But it doesn't make this correct. We should have initialized
> > usbredirhost before calling this API, making this a spice-gtk mistake.
> > 
> > Either way is fine to me... maybe both :)
> 
> I guess an usbredir patch adding such guards to public entry points
> would be accepted.
>
> Christophe

Okay, I'll look into the public APIs in usbredir and set some guards
there too.

  Cheers




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-gtk v2 1/2] channel-usbredir: Fix crash on channel-up

[Victor Toso]

Hi,

On Thu, Dec 01, 2016 at 01:29:50PM +0100, Christophe Fergeau wrote:
> On Wed, Nov 30, 2016 at 06:36:32PM +0100, Victor Toso wrote:
> > From: Victor Toso 
> > 
> > SpiceSession does not initialize its SpiceUsbDeviceManager object on
> > startup that could lead to a race condition where channel-usbredir is
> > requested to flush data while it is uninitialized.
> > 
> > In a few places, spice_usb_device_manager_get() is called as in
> > usb-device-widget.c and spice-gtk-session.c but not used in
> > spicy-stats, making the tool to crash on startup.
> 
> Just running spicy-stats when there is a usbredir channel is going to
> cause a crash?

Yes

> Isn't this avoided by your next patch as well

Yes

> which makes sure host is not NULL before trying to flush?

I see this as two different problems, really.

1-) chanel-usbredir should take in consideration that it might not be
initialized (second patch)
2-) SpiceSession should initialize SpiceUsbDeviceManager (this patch)
otherwise, situations like (1) could happen;

spice_usb_device_manager_initable_init() could fail here so, the
situation in (1) might as well happen.

> 
> Christophe
> 
> > 
> >  #0 in usbredirhost_write_guest_data (host=0x0) at 
> > usbredir/usbredirhost/usbredirhost.c:876
> >  #1 in spice_usbredir_channel_up (c=0x643830) at channel-usbredir.c:821
> >  #2 in spice_channel_up (channel=0x643830) at spice-channel.c:1238
> >  #3 in spice_channel_recv_auth (channel=0x643830) at spice-channel.c:1225
> >  #4 in spice_channel_coroutine (data=0x643830) at spice-channel.c:2580
> >  #5 in coroutine_trampoline (cc=0x642ec0) at coroutine_ucontext.c:63
> >  #6 in continuation_trampoline (i0=6565568, i1=0) at continuation.c:55
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838
> > 
> > Signed-off-by: Victor Toso 
> > Reported-by: Michael Cullen 
> > ---
> >  src/spice-session.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index f900bd1..91e4f97 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -281,6 +281,7 @@ static void spice_session_init(SpiceSession *session)
> >  {
> >  SpiceSessionPrivate *s;
> >  gchar *channels;
> > +GError *err = NULL;
> >  
> >  SPICE_DEBUG("New session (compiled from package " PACKAGE_STRING ")");
> >  s = session->priv = SPICE_SESSION_GET_PRIVATE(session);
> > @@ -293,6 +294,12 @@ static void spice_session_init(SpiceSession *session)
> >  s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
> >  s->glz_window = glz_decoder_window_new();
> >  update_proxy(session, NULL);
> > +
> > +spice_usb_device_manager_get(session, );
> > +if (err != NULL) {
> > +SPICE_DEBUG("Could not initialize SpiceUsbDeviceManager - %s", 
> > err->message);
> > +g_clear_error();
> > +}
> >  }
> >  
> >  static void
> > -- 
> > 2.9.3
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel




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] [PATCH v4 08/14] sound: Implement on_disconnect RedChannel callback

[Frediano Ziglio]

> 
> On Wed, Nov 30, 2016 at 12:34:52PM +, Frediano Ziglio wrote:
> > Avoid having dandling pointer to a client.
> 
> 'dangling'
> 

I though I merged this patch in "sound: Convert SndChannelClient to GObject".
I did some mistake probably, but will be merged to that patch anyway.

> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/sound.c |  9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/server/sound.c b/server/sound.c
> > index a5b960b..00eab67 100644
> > --- a/server/sound.c
> > +++ b/server/sound.c
> > @@ -773,6 +773,14 @@ static int snd_channel_config_socket(RedChannelClient
> > *rcc)
> >  return TRUE;
> >  }
> >  
> > +static void snd_channel_on_disconnect(RedChannelClient *rcc)
> > +{
> > +SndChannel *channel =
> > SND_CHANNEL(red_channel_client_get_channel(rcc));
> > +if (channel->connection && rcc ==
> > RED_CHANNEL_CLIENT(channel->connection)) {
> 
> Is it expected that channel->connection will sometimes not be equal to rcc
> here? Or
> should it issue some kind of runtime warning if this happens?
> 

I think is quite paranoid check. There is currently only one client
so channel->connection should be the same.
Maybe could happen that a second client connect basically kicking out
the old one so possibly this could be the onle reason having
channel->connection && rcc != channel->connection.
I was thinking to remove the connection field and use client list from 
RedChannel
instead.
I was thinking also to add support for multiple clients to the code as
an improvement and as a demonstration that current code is better
than what it used to be but I don't know what to do for the recording!
Mix all recordings? Keep a main client channel and discard the others?

> > +channel->connection = NULL;
> 
> Christophe
> 

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v4 08/14] sound: Implement on_disconnect RedChannel callback

[Christophe Fergeau]

On Wed, Nov 30, 2016 at 12:34:52PM +, Frediano Ziglio wrote:
> Avoid having dandling pointer to a client.

'dangling'

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c |  9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/server/sound.c b/server/sound.c
> index a5b960b..00eab67 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -773,6 +773,14 @@ static int snd_channel_config_socket(RedChannelClient 
> *rcc)
>  return TRUE;
>  }
>  
> +static void snd_channel_on_disconnect(RedChannelClient *rcc)
> +{
> +SndChannel *channel = SND_CHANNEL(red_channel_client_get_channel(rcc));
> +if (channel->connection && rcc == 
> RED_CHANNEL_CLIENT(channel->connection)) {

Is it expected that channel->connection will sometimes not be equal to rcc 
here? Or
should it issue some kind of runtime warning if this happens?

> +channel->connection = NULL;

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-gtk v2 2/2] channel-usbredir: prevent crash when calling without host

[Christophe Fergeau]

On Wed, Nov 30, 2016 at 06:57:12PM +0100, Victor Toso wrote:
> Hi,
> 
> On Wed, Nov 30, 2016 at 06:45:45PM +0100, Pavel Grunt wrote:
> > imho this should be in usbredir
> >
> > It is not nice when library crashes your program
> 
> I don't disagree. Libraries should use guards accordingly.
> 
> But it doesn't make this correct. We should have initialized
> usbredirhost before calling this API, making this a spice-gtk mistake.
> 
> Either way is fine to me... maybe both :)

I guess an usbredir patch adding such guards to public entry points
would be accepted.

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-gtk v2 2/2] channel-usbredir: prevent crash when calling without host

[Christophe Fergeau]

On Wed, Nov 30, 2016 at 06:36:33PM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> Similar fix was done in 5b252b0f499601bcf387c02a4dd35d27ed34c
> 
> We should log a critical message instead of crashing the application.

I think the log should be more detailed than in the previous iteration,
not less. It's much better to explain why this is a correct fix, and not
papering over another bug (priv->host being NULL when it really should
not). This could be just quoting some parts of
5b252b0f499601bcf387c02a4dd35d27ed34c if relevant, or describing a
scenario where the crash could occur on startup.

The way it is now, it looks more like "ok, NULL pointer here for
unknown reasons, let's avoid the crash"

Christophe

> Signed-off-by: Victor Toso 
> ---
>  src/channel-usbredir.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 4837d68..392a35e 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -817,6 +817,8 @@ static void spice_usbredir_channel_up(SpiceChannel *c)
>  SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
>  SpiceUsbredirChannelPrivate *priv = channel->priv;
>  
> +g_return_if_fail(priv->host != NULL);
> +
>  /* Flush any pending writes */
>  usbredirhost_write_guest_data(priv->host);
>  }
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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-gtk v2 1/2] channel-usbredir: Fix crash on channel-up

[Christophe Fergeau]

On Wed, Nov 30, 2016 at 06:36:32PM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> SpiceSession does not initialize its SpiceUsbDeviceManager object on
> startup that could lead to a race condition where channel-usbredir is
> requested to flush data while it is uninitialized.
> 
> In a few places, spice_usb_device_manager_get() is called as in
> usb-device-widget.c and spice-gtk-session.c but not used in
> spicy-stats, making the tool to crash on startup.

Just running spicy-stats when there is a usbredir channel is going to
cause a crash? Isn't this avoided by your next patch as well which makes
sure host is not NULL before trying to flush?

Christophe

> 
>  #0 in usbredirhost_write_guest_data (host=0x0) at 
> usbredir/usbredirhost/usbredirhost.c:876
>  #1 in spice_usbredir_channel_up (c=0x643830) at channel-usbredir.c:821
>  #2 in spice_channel_up (channel=0x643830) at spice-channel.c:1238
>  #3 in spice_channel_recv_auth (channel=0x643830) at spice-channel.c:1225
>  #4 in spice_channel_coroutine (data=0x643830) at spice-channel.c:2580
>  #5 in coroutine_trampoline (cc=0x642ec0) at coroutine_ucontext.c:63
>  #6 in continuation_trampoline (i0=6565568, i1=0) at continuation.c:55
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838
> 
> Signed-off-by: Victor Toso 
> Reported-by: Michael Cullen 
> ---
>  src/spice-session.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/spice-session.c b/src/spice-session.c
> index f900bd1..91e4f97 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -281,6 +281,7 @@ static void spice_session_init(SpiceSession *session)
>  {
>  SpiceSessionPrivate *s;
>  gchar *channels;
> +GError *err = NULL;
>  
>  SPICE_DEBUG("New session (compiled from package " PACKAGE_STRING ")");
>  s = session->priv = SPICE_SESSION_GET_PRIVATE(session);
> @@ -293,6 +294,12 @@ static void spice_session_init(SpiceSession *session)
>  s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
>  s->glz_window = glz_decoder_window_new();
>  update_proxy(session, NULL);
> +
> +spice_usb_device_manager_get(session, );
> +if (err != NULL) {
> +SPICE_DEBUG("Could not initialize SpiceUsbDeviceManager - %s", 
> err->message);
> +g_clear_error();
> +}
>  }
>  
>  static void
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

[Spice-devel] [PATCH spice-server] gstreamer: Correctly don't allow too limited bit rates

[Frediano Ziglio]

The check to limit too low bit rates was setting encoder->bit_rate
instead of bit_rate. However after some lines bit_rate was used
to set encoder->bit_rate basically removing the lower threshold.

Signed-off-by: Frediano Ziglio 
---
 server/gstreamer-encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index b9b1a56..e28ab00 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -595,7 +595,7 @@ static void set_bit_rate(SpiceGstEncoder *encoder, uint64_t 
bit_rate)
 }
 if (bit_rate < SPICE_GST_MIN_BITRATE) {
 /* Don't let the bit rate go too low... */
-encoder->bit_rate = SPICE_GST_MIN_BITRATE;
+bit_rate = SPICE_GST_MIN_BITRATE;
 } else if (bit_rate > encoder->bit_rate) {
 /* or too high */
 bit_rate = MIN(bit_rate, get_bit_rate_cap(encoder));
-- 
2.9.3

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

Re: [Spice-devel] [client v2 2/3] spicy: Temporarily ignore deprecation warnings

[Christophe Fergeau]

I think I'd handle this one similarly to the vala warnings:

diff --git a/src/Makefile.am b/src/Makefile.am
index 3f81866..66e3c64 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -402,10 +402,12 @@ spicy_LDADD = 
\
$(LIBM) \
$(NULL)

+# FIXME: GtkAction and lots of GtkUI APIs are deprecated
 spicy_CPPFLAGS =   \
$(AM_CPPFLAGS)  \
$(GTHREAD_CFLAGS)   \
-DSPICE_DISABLE_DEPRECATED  \
+   -Wno-deprecated-declarations\
$(NULL)



On Wed, Nov 23, 2016 at 07:09:20AM +0100, Francois Gouget wrote:
> GtkAction and lots of GtkUI APIs are deprecated.
> 
> Signed-off-by: Francois Gouget 
> ---
>  src/spicy.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/spicy.c b/src/spicy.c
> index c502428..c4a6c7d 100644
> --- a/src/spicy.c
> +++ b/src/spicy.c
> @@ -38,6 +38,9 @@
>  
>  #include "spicy-connect.h"
>  
> +/* FIXME: GtkAction and lots of GtkUI APIs are deprecated */
> +G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> +
>  typedef struct spice_connection spice_connection;
>  
>  enum {
> -- 
> 2.10.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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] [client v2 3/3] build-sys: Enable deprecation warnings instead of ignoring them entirely

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 04:27:36AM +0100, Francois Gouget wrote:
>  
> 
> 
> Btw, what's the status of this patchset?

Fine with me, I'll add a closing G_END_DEPRECATION_xxx to the end of
spicy.c and push. This adds a few more warnings in various places, but
they can be fixed afterwards (though I think you had sent patches for
these too).

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] [client v2 3/3] build-sys: Enable deprecation warnings instead of ignoring them entirely

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 04:27:36AM +0100, Francois Gouget wrote:
> On Wed, 23 Nov 2016, Christophe Fergeau wrote:
> 
> > On Wed, Nov 23, 2016 at 07:09:28AM +0100, Francois Gouget wrote:
> > > For Spice-gtk most deprecation issues come from changes outside Spice
> > > (GLib) and thus should not be treated as errors to not break
> > > compilation for users who have newer third-party libraries.
> > > However they must be visible otherwise Spice developers will not be
> > > aware of them and thus will not fix them before breakage happens.
> > 
> > Also iirc -DXXX_VERSION_MIN_REQUIRED=yyy need deprecation warnings to be
> > functional
> 
> Hmmm, I applied the patch below and I'm getting errors with or without 
> this patchset.
> 
> diff --git a/configure.ac b/configure.ac
> index f3e7f8d..4661e9f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -136,7 +136,7 @@ AS_IF([test "x$with_gtk" != "xno"],
>[AS_IF([test "x$os_win32" = "xyes"],
>   [PKG_CHECK_MODULES(GTK, gtk+-3.0 >= $GTK_REQUIRED)],
>   [PKG_CHECK_MODULES(GTK, gtk+-3.0 >= $GTK_REQUIRED epoxy)])]
> -  [GTK_CFLAGS="$GTK_CFLAGS 
> -DGDK_VERSION_MIN_REQUIRED=$GTK_ENCODED_VERSION \
> +  [GTK_CFLAGS="$GTK_CFLAGS -DGDK_VERSION_MIN_REQUIRED=4 \
> 
> -DGDK_VERSION_MAX_ALLOWED=$GTK_ENCODED_VERSION"])
>  SPICE_GTK_REQUIRES="${SPICE_GTK_REQUIRES} gtk+-3.0 >= $GTK_REQUIRED"

Dunno, with your patch applied, I'm getting
channel-webdav.c:317:5: attention : ‘g_output_stream_write_all_finish’
is deprecated: Not available before 2.44 [-Wdeprecated-declarations]

so the warning implies it's related to deprecation warnings (and it is
good to iee it as before I think this was silent)

Christophe


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

[Spice-devel] [PATCH v4 12/17] sound: Reuse code for snd_set_{playback, record}_peer

[Frediano Ziglio]

Almost identical beside the type.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 53 +++
 1 file changed, 16 insertions(+), 37 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 486fc95..0304fec 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1071,9 +1071,9 @@ playback_channel_client_constructed(GObject *object)
 snd_send(SND_CHANNEL_CLIENT(playback_client));
 }
 
-static void snd_set_playback_peer(RedChannel *red_channel, RedClient *client, 
RedsStream *stream,
-  int migration, int num_common_caps, uint32_t 
*common_caps,
-  int num_caps, uint32_t *caps)
+static void snd_set_peer(RedChannel *red_channel, RedClient *client, 
RedsStream *stream,
+ int migration, int num_common_caps, uint32_t 
*common_caps,
+ int num_caps, uint32_t *caps, GType type)
 {
 SndChannel *channel = SND_CHANNEL(red_channel);
 GArray *common_caps_array = NULL, *caps_array = NULL;
@@ -1093,7 +1093,7 @@ static void snd_set_playback_peer(RedChannel 
*red_channel, RedClient *client, Re
 g_array_append_vals(caps_array, caps, num_caps);
 }
 
-g_initable_new(TYPE_PLAYBACK_CHANNEL_CLIENT,
+g_initable_new(type,
NULL, NULL,
"channel", channel,
"client", client,
@@ -1110,6 +1110,15 @@ static void snd_set_playback_peer(RedChannel 
*red_channel, RedClient *client, Re
 }
 }
 
+static void snd_set_playback_peer(RedChannel *red_channel, RedClient *client, 
RedsStream *stream,
+  int migration, int num_common_caps, uint32_t 
*common_caps,
+  int num_caps, uint32_t *caps)
+{
+snd_set_peer(red_channel, client, stream, migration,
+ num_common_caps, common_caps, num_caps, caps,
+ TYPE_PLAYBACK_CHANNEL_CLIENT);
+}
+
 static void snd_record_migrate_channel_client(RedChannelClient *rcc)
 {
 SndChannel *channel;
@@ -1328,39 +1337,9 @@ static void snd_set_record_peer(RedChannel *red_channel, 
RedClient *client, Reds
 int migration, int num_common_caps, uint32_t 
*common_caps,
 int num_caps, uint32_t *caps)
 {
-SndChannel *channel = SND_CHANNEL(red_channel);
-GArray *common_caps_array = NULL, *caps_array = NULL;
-
-if (channel->connection) {
-red_channel_client_disconnect(RED_CHANNEL_CLIENT(channel->connection));
-channel->connection = NULL;
-}
-
-if (common_caps) {
-common_caps_array = g_array_sized_new(FALSE, FALSE, sizeof 
(*common_caps),
-  num_common_caps);
-g_array_append_vals(common_caps_array, common_caps, num_common_caps);
-}
-if (caps) {
-caps_array = g_array_sized_new(FALSE, FALSE, sizeof (*caps), num_caps);
-g_array_append_vals(caps_array, caps, num_caps);
-}
-
-g_initable_new(TYPE_RECORD_CHANNEL_CLIENT,
-   NULL, NULL,
-   "channel", channel,
-   "client", client,
-   "stream", stream,
-   "caps", caps_array,
-   "common-caps", common_caps_array,
-   NULL);
-
-if (caps_array) {
-g_array_unref(caps_array);
-}
-if (common_caps_array) {
-g_array_unref(common_caps_array);
-}
+snd_set_peer(red_channel, client, stream, migration,
+ num_common_caps, common_caps, num_caps, caps,
+ TYPE_RECORD_CHANNEL_CLIENT);
 }
 
 static void snd_playback_migrate_channel_client(RedChannelClient *rcc)
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 14/17] sound: Reuse code to set volume and mute

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 51 +--
 1 file changed, 21 insertions(+), 30 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 30a0c20..4c4b533 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -812,12 +812,11 @@ static void snd_set_command(SndChannelClient *client, 
uint32_t command)
 client->command |= command;
 }
 
-SPICE_GNUC_VISIBLE void spice_server_playback_set_volume(SpicePlaybackInstance 
*sin,
-  uint8_t nchannels,
-  uint16_t *volume)
+static void snd_channel_set_volume(SndChannel *channel,
+   uint8_t nchannels, uint16_t *volume)
 {
-SpiceVolumeState *st = >st->channel.volume;
-SndChannelClient *client = sin->st->channel.connection;
+SpiceVolumeState *st = >volume;
+SndChannelClient *client = channel->connection;
 
 st->volume_nchannels = nchannels;
 free(st->volume);
@@ -830,10 +829,17 @@ SPICE_GNUC_VISIBLE void 
spice_server_playback_set_volume(SpicePlaybackInstance *
 snd_send(client);
 }
 
-SPICE_GNUC_VISIBLE void spice_server_playback_set_mute(SpicePlaybackInstance 
*sin, uint8_t mute)
+SPICE_GNUC_VISIBLE void spice_server_playback_set_volume(SpicePlaybackInstance 
*sin,
+  uint8_t nchannels,
+  uint16_t *volume)
 {
-SpiceVolumeState *st = >st->channel.volume;
-SndChannelClient *client = sin->st->channel.connection;
+snd_channel_set_volume(>st->channel, nchannels, volume);
+}
+
+static void snd_channel_set_mute(SndChannel *channel, uint8_t mute)
+{
+SpiceVolumeState *st = >volume;
+SndChannelClient *client = channel->connection;
 
 st->mute = mute;
 
@@ -844,6 +850,11 @@ SPICE_GNUC_VISIBLE void 
spice_server_playback_set_mute(SpicePlaybackInstance *si
 snd_send(client);
 }
 
+SPICE_GNUC_VISIBLE void spice_server_playback_set_mute(SpicePlaybackInstance 
*sin, uint8_t mute)
+{
+snd_channel_set_mute(>st->channel, mute);
+}
+
 static void snd_playback_start(SndChannel *channel)
 {
 SndChannelClient *client = channel->connection;
@@ -1129,32 +1140,12 @@ SPICE_GNUC_VISIBLE void 
spice_server_record_set_volume(SpiceRecordInstance *sin,
 uint8_t nchannels,
 uint16_t *volume)
 {
-SpiceVolumeState *st = >st->channel.volume;
-SndChannelClient *client = sin->st->channel.connection;
-
-st->volume_nchannels = nchannels;
-free(st->volume);
-st->volume = spice_memdup(volume, sizeof(uint16_t) * nchannels);
-
-if (!client || nchannels == 0)
-return;
-
-snd_set_command(client, SND_VOLUME_MUTE_MASK);
-snd_send(client);
+snd_channel_set_volume(>st->channel, nchannels, volume);
 }
 
 SPICE_GNUC_VISIBLE void spice_server_record_set_mute(SpiceRecordInstance *sin, 
uint8_t mute)
 {
-SpiceVolumeState *st = >st->channel.volume;
-SndChannelClient *client = sin->st->channel.connection;
-
-st->mute = mute;
-
-if (!client)
-return;
-
-snd_set_command(client, SND_VOLUME_MUTE_MASK);
-snd_send(client);
+snd_channel_set_mute(>st->channel, mute);
 }
 
 static void snd_record_start(SndChannel *channel)
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 02/17] sound: Rename SndChannel to SndChannelClient

[Frediano Ziglio]

SndWorker has been historically based on RedChannel, initial git commit
has:
struct SndWorker {
 Channel base;
 ...
};

SndChannel, contrary to what its name may imply is more focused on
marshalling/sending of sound data, which is the responsibility of
RedChannelClient for the other SPICE channels.

This commit and following ones make the naming of these 2 types more
consistent with how the rest of the code is structured.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 522 +-
 1 file changed, 261 insertions(+), 261 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index beab473..02adf79 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -73,16 +73,16 @@ enum PlaybackCommand {
 #define SND_PLAYBACK_PCM_MASK (1 << SND_PLAYBACK_PCM)
 #define SND_PLAYBACK_LATENCY_MASK ( 1 << SND_PLAYBACK_LATENCY)
 
-typedef struct SndChannel SndChannel;
+typedef struct SndChannelClient SndChannelClient;
 typedef void (*snd_channel_send_messages_proc)(void *in_channel);
-typedef int (*snd_channel_handle_message_proc)(SndChannel *channel, size_t 
size, uint32_t type, void *message);
-typedef void (*snd_channel_on_message_done_proc)(SndChannel *channel);
-typedef void (*snd_channel_cleanup_channel_proc)(SndChannel *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);
 
 typedef struct SndWorker SndWorker;
 
-/* Connects an audio channel to a Spice client */
-struct SndChannel {
+/* Connects an audio client to a Spice client */
+struct SndChannelClient {
 RedsStream *stream;
 SndWorker *worker;
 spice_parse_channel_func_t parser;
@@ -127,7 +127,7 @@ struct AudioFrame {
 };
 
 struct PlaybackChannel {
-SndChannel base;
+SndChannelClient base;
 AudioFrame frames[3];
 AudioFrame *free_frames;
 AudioFrame *in_progress;   /* Frame being sent to the client */
@@ -147,7 +147,7 @@ typedef struct SpiceVolumeState {
 /* Base class for SpicePlaybackState and SpiceRecordState */
 struct SndWorker {
 RedChannel *base_channel;
-SndChannel *connection; /* Only one client is supported */
+SndChannelClient *connection; /* Only one client is supported */
 SndWorker *next; /* For the global SndWorker list */
 
 int active;
@@ -164,7 +164,7 @@ struct SpiceRecordState {
 };
 
 typedef struct RecordChannel {
-SndChannel base;
+SndChannelClient base;
 uint32_t samples[RECORD_SAMPLES_SIZE];
 uint32_t write_pos;
 uint32_t read_pos;
@@ -178,58 +178,58 @@ typedef struct RecordChannel {
 /* A list of all Spice{Playback,Record}State objects */
 static SndWorker *workers;
 
-static void snd_receive(SndChannel *channel);
+static void snd_receive(SndChannelClient *client);
 static void snd_playback_start(SndWorker *worker);
 static void snd_record_start(SndWorker *worker);
 
-static SndChannel *snd_channel_ref(SndChannel *channel)
+static SndChannelClient *snd_channel_ref(SndChannelClient *client)
 {
-channel->refs++;
-return channel;
+client->refs++;
+return client;
 }
 
-static SndChannel *snd_channel_unref(SndChannel *channel)
+static SndChannelClient *snd_channel_unref(SndChannelClient *client)
 {
-if (!--channel->refs) {
-spice_printerr("SndChannel=%p freed", channel);
-free(channel);
+if (!--client->refs) {
+spice_printerr("SndChannelClient=%p freed", client);
+free(client);
 return NULL;
 }
-return channel;
+return client;
 }
 
-static RedsState* snd_channel_get_server(SndChannel *channel)
+static RedsState* snd_channel_get_server(SndChannelClient *client)
 {
-g_return_val_if_fail(channel != NULL, NULL);
-return red_channel_get_server(channel->worker->base_channel);
+g_return_val_if_fail(client != NULL, NULL);
+return red_channel_get_server(client->worker->base_channel);
 }
 
-static void snd_disconnect_channel(SndChannel *channel)
+static void snd_disconnect_channel(SndChannelClient *client)
 {
 SndWorker *worker;
 RedsState *reds;
 RedChannel *red_channel;
 uint32_t type;
 
-if (!channel || !channel->stream) {
+if (!client || !client->stream) {
 spice_debug("not connected");
 return;
 }
-red_channel = red_channel_client_get_channel(channel->channel_client);
-reds = snd_channel_get_server(channel);
+red_channel = red_channel_client_get_channel(client->channel_client);
+reds = snd_channel_get_server(client);
 g_object_get(red_channel, "channel-type", , NULL);
-spice_debug("SndChannel=%p rcc=%p type=%d",
- channel, channel->channel_client, type);
-worker = channel->worker;
-channel->cleanup(channel);
+spice_debug("SndChannelClient=%p rcc=%p type=%d",
+ client, 

[Spice-devel] [PATCH v4 01/17] sound: Use worker directly

[Frediano Ziglio]

SpicePlaybackState and SpiceRecordState have same structures
changing only slightly the behaviour.
Using SndWorker instead allows some minor simplification and
more code reuse.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 26 --
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 8eddd2b..beab473 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -179,8 +179,8 @@ typedef struct RecordChannel {
 static SndWorker *workers;
 
 static void snd_receive(SndChannel *channel);
-static void snd_playback_start(SpicePlaybackState *st);
-static void snd_record_start(SpiceRecordState *st);
+static void snd_playback_start(SndWorker *worker);
+static void snd_record_start(SndWorker *worker);
 
 static SndChannel *snd_channel_ref(SndChannel *channel)
 {
@@ -1026,11 +1026,11 @@ SPICE_GNUC_VISIBLE void 
spice_server_playback_set_mute(SpicePlaybackInstance *si
 snd_playback_send_mute(playback_channel);
 }
 
-static void snd_playback_start(SpicePlaybackState *st)
+static void snd_playback_start(SndWorker *worker)
 {
-SndChannel *channel = st->worker.connection;
+SndChannel *channel = worker->connection;
 
-st->worker.active = 1;
+worker->active = 1;
 if (!channel)
 return;
 spice_assert(!channel->active);
@@ -1046,7 +1046,7 @@ static void snd_playback_start(SpicePlaybackState *st)
 
 SPICE_GNUC_VISIBLE void spice_server_playback_start(SpicePlaybackInstance *sin)
 {
-return snd_playback_start(sin->st);
+return snd_playback_start(>st->worker);
 }
 
 SPICE_GNUC_VISIBLE void spice_server_playback_stop(SpicePlaybackInstance *sin)
@@ -1195,7 +1195,6 @@ static void snd_set_playback_peer(RedChannel *channel, 
RedClient *client, RedsSt
 {
 SndWorker *worker = g_object_get_data(G_OBJECT(channel), "sound-worker");
 PlaybackChannel *playback_channel;
-SpicePlaybackState *st = SPICE_CONTAINEROF(worker, SpicePlaybackState, 
worker);
 
 snd_disconnect_channel(worker->connection);
 
@@ -1240,7 +1239,7 @@ static void snd_set_playback_peer(RedChannel *channel, 
RedClient *client, RedsSt
 }
 
 if (worker->active) {
-snd_playback_start(st);
+snd_playback_start(worker);
 }
 snd_playback_send(worker->connection);
 }
@@ -1294,12 +1293,12 @@ SPICE_GNUC_VISIBLE void 
spice_server_record_set_mute(SpiceRecordInstance *sin, u
 snd_record_send_mute(record_channel);
 }
 
-static void snd_record_start(SpiceRecordState *st)
+static void snd_record_start(SndWorker *worker)
 {
-SndChannel *channel = st->worker.connection;
+SndChannel *channel = worker->connection;
 RecordChannel *record_channel = SPICE_CONTAINEROF(channel, RecordChannel, 
base);
 
-st->worker.active = 1;
+worker->active = 1;
 if (!channel)
 return;
 spice_assert(!channel->active);
@@ -1316,7 +1315,7 @@ static void snd_record_start(SpiceRecordState *st)
 
 SPICE_GNUC_VISIBLE void spice_server_record_start(SpiceRecordInstance *sin)
 {
-snd_record_start(sin->st);
+snd_record_start(>st->worker);
 }
 
 SPICE_GNUC_VISIBLE void spice_server_record_stop(SpiceRecordInstance *sin)
@@ -1442,7 +1441,6 @@ static void snd_set_record_peer(RedChannel *channel, 
RedClient *client, RedsStre
 {
 SndWorker *worker = g_object_get_data(G_OBJECT(channel), "sound-worker");
 RecordChannel *record_channel;
-SpiceRecordState *st = SPICE_CONTAINEROF(worker, SpiceRecordState, worker);
 
 snd_disconnect_channel(worker->connection);
 
@@ -1465,7 +1463,7 @@ static void snd_set_record_peer(RedChannel *channel, 
RedClient *client, RedsStre
 
 on_new_record_channel(worker, _channel->base);
 if (worker->active) {
-snd_record_start(st);
+snd_record_start(worker);
 }
 snd_record_send(worker->connection);
 }
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 03/17] sound: Rename {Record, Playback}Channel to *ChannelClient

[Frediano Ziglio]

Make easier to understand that they refer to client and not
all channel.

Specifically:
- RecordChannel -> RecordChannelClient
- PlaybackChannel -> PlaybackChannelClient
- playback_channel -> playback_client
- record_channel -> record_client

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 368 +-
 1 file changed, 184 insertions(+), 184 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 02adf79..bafdd1e 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -116,17 +116,17 @@ struct SndChannelClient {
 snd_channel_cleanup_channel_proc cleanup;
 };
 
-typedef struct PlaybackChannel PlaybackChannel;
+typedef struct PlaybackChannelClient PlaybackChannelClient;
 
 typedef struct AudioFrame AudioFrame;
 struct AudioFrame {
 uint32_t time;
 uint32_t samples[SND_CODEC_MAX_FRAME_SIZE];
-PlaybackChannel *channel;
+PlaybackChannelClient *client;
 AudioFrame *next;
 };
 
-struct PlaybackChannel {
+struct PlaybackChannelClient {
 SndChannelClient base;
 AudioFrame frames[3];
 AudioFrame *free_frames;
@@ -163,7 +163,7 @@ struct SpiceRecordState {
 struct SndWorker worker;
 };
 
-typedef struct RecordChannel {
+typedef struct RecordChannelClient {
 SndChannelClient base;
 uint32_t samples[RECORD_SAMPLES_SIZE];
 uint32_t write_pos;
@@ -173,7 +173,7 @@ typedef struct RecordChannel {
 uint32_t start_time;
 SndCodec codec;
 uint8_t  decode_buf[SND_CODEC_MAX_FRAME_BYTES];
-} RecordChannel;
+} RecordChannelClient;
 
 /* A list of all Spice{Playback,Record}State objects */
 static SndWorker *workers;
@@ -233,20 +233,20 @@ static void snd_disconnect_channel(SndChannelClient 
*client)
 worker->connection = NULL;
 }
 
-static void snd_playback_free_frame(PlaybackChannel *playback_channel, 
AudioFrame *frame)
+static void snd_playback_free_frame(PlaybackChannelClient *playback_client, 
AudioFrame *frame)
 {
-frame->channel = playback_channel;
-frame->next = playback_channel->free_frames;
-playback_channel->free_frames = frame;
+frame->client = playback_client;
+frame->next = playback_client->free_frames;
+playback_client->free_frames = frame;
 }
 
 static void snd_playback_on_message_done(SndChannelClient *client)
 {
-PlaybackChannel *playback_channel = (PlaybackChannel *)client;
-if (playback_channel->in_progress) {
-snd_playback_free_frame(playback_channel, 
playback_channel->in_progress);
-playback_channel->in_progress = NULL;
-if (playback_channel->pending_frame) {
+PlaybackChannelClient *playback_client = (PlaybackChannelClient *)client;
+if (playback_client->in_progress) {
+snd_playback_free_frame(playback_client, playback_client->in_progress);
+playback_client->in_progress = NULL;
+if (playback_client->pending_frame) {
 client->command |= SND_PLAYBACK_PCM_MASK;
 }
 }
@@ -311,7 +311,7 @@ static int snd_send_data(SndChannelClient *client)
 return TRUE;
 }
 
-static int snd_record_handle_write(RecordChannel *record_channel, size_t size, 
void *message)
+static int snd_record_handle_write(RecordChannelClient *record_client, size_t 
size, void *message)
 {
 SpiceMsgcRecordPacket *packet;
 uint32_t write_pos;
@@ -319,39 +319,39 @@ static int snd_record_handle_write(RecordChannel 
*record_channel, size_t size, v
 uint32_t len;
 uint32_t now;
 
-if (!record_channel) {
+if (!record_client) {
 return FALSE;
 }
 
 packet = (SpiceMsgcRecordPacket *)message;
 
-if (record_channel->mode == SPICE_AUDIO_DATA_MODE_RAW) {
+if (record_client->mode == SPICE_AUDIO_DATA_MODE_RAW) {
 data = (uint32_t *)packet->data;
 size = packet->data_size >> 2;
 size = MIN(size, RECORD_SAMPLES_SIZE);
  } else {
 int decode_size;
-decode_size = sizeof(record_channel->decode_buf);
-if (snd_codec_decode(record_channel->codec, packet->data, 
packet->data_size,
-record_channel->decode_buf, _size) != SND_CODEC_OK)
+decode_size = sizeof(record_client->decode_buf);
+if (snd_codec_decode(record_client->codec, packet->data, 
packet->data_size,
+record_client->decode_buf, _size) != SND_CODEC_OK)
 return FALSE;
-data = (uint32_t *) record_channel->decode_buf;
+data = (uint32_t *) record_client->decode_buf;
 size = decode_size >> 2;
 }
 
-write_pos = record_channel->write_pos % RECORD_SAMPLES_SIZE;
-record_channel->write_pos += size;
+write_pos = record_client->write_pos % RECORD_SAMPLES_SIZE;
+record_client->write_pos += size;
 len = RECORD_SAMPLES_SIZE - write_pos;
 now = MIN(len, size);
 size -= now;
-memcpy(record_channel->samples + write_pos, data, now << 2);
+memcpy(record_client->samples + write_pos, data, now << 2);
 
 if (size) {
-

[Spice-devel] [PATCH v4 13/17] sound: Reuse code for migrating client channels

[Frediano Ziglio]

We support only a single client so don't waste code just
to check this.
The worst stuff can happen is that we'll migrate multiple
connections.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 37 +
 1 file changed, 5 insertions(+), 32 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 0304fec..30a0c20 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1119,20 +1119,10 @@ static void snd_set_playback_peer(RedChannel 
*red_channel, RedClient *client, Re
  TYPE_PLAYBACK_CHANNEL_CLIENT);
 }
 
-static void snd_record_migrate_channel_client(RedChannelClient *rcc)
+static void snd_migrate_channel_client(RedChannelClient *rcc)
 {
-SndChannel *channel;
-RedChannel *red_channel = red_channel_client_get_channel(rcc);
-
-spice_assert(red_channel);
-channel = SND_CHANNEL(red_channel);
-spice_assert(channel);
-
-if (channel->connection) {
-spice_assert(RED_CHANNEL_CLIENT(channel->connection) == rcc);
-snd_set_command(channel->connection, SND_MIGRATE_MASK);
-snd_send(channel->connection);
-}
+snd_set_command(SND_CHANNEL_CLIENT(rcc), SND_MIGRATE_MASK);
+snd_send(SND_CHANNEL_CLIENT(rcc));
 }
 
 SPICE_GNUC_VISIBLE void spice_server_record_set_volume(SpiceRecordInstance 
*sin,
@@ -1342,23 +1332,6 @@ static void snd_set_record_peer(RedChannel *red_channel, 
RedClient *client, Reds
  TYPE_RECORD_CHANNEL_CLIENT);
 }
 
-static void snd_playback_migrate_channel_client(RedChannelClient *rcc)
-{
-SndChannel *channel;
-RedChannel *red_channel = red_channel_client_get_channel(rcc);
-
-spice_assert(red_channel);
-channel = SND_CHANNEL(red_channel);
-spice_assert(channel);
-spice_debug(NULL);
-
-if (channel->connection) {
-spice_assert(RED_CHANNEL_CLIENT(channel->connection) == rcc);
-snd_set_command(channel->connection, SND_MIGRATE_MASK);
-snd_send(channel->connection);
-}
-}
-
 static void add_channel(SndChannel *channel)
 {
 channel->next = snd_channels;
@@ -1426,7 +1399,7 @@ playback_channel_constructed(GObject *object)
 G_OBJECT_CLASS(playback_channel_parent_class)->constructed(object);
 
 client_cbs.connect = snd_set_playback_peer;
-client_cbs.migrate = snd_playback_migrate_channel_client;
+client_cbs.migrate = snd_migrate_channel_client;
 red_channel_register_client_cbs(RED_CHANNEL(self), _cbs, self);
 
 if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1, 
SND_CODEC_ANY_FREQUENCY)) {
@@ -1476,7 +1449,7 @@ record_channel_constructed(GObject *object)
 G_OBJECT_CLASS(record_channel_parent_class)->constructed(object);
 
 client_cbs.connect = snd_set_record_peer;
-client_cbs.migrate = snd_record_migrate_channel_client;
+client_cbs.migrate = snd_migrate_channel_client;
 red_channel_register_client_cbs(RED_CHANNEL(self), _cbs, self);
 
 if (snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_CELT_0_5_1, 
SND_CODEC_ANY_FREQUENCY)) {
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 15/17] sound: Use default message handler if possible

[Frediano Ziglio]

red_channel_client_handle_message can handle base messages
so reuse it.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 16 +---
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 4c4b533..1832e3b 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -310,18 +310,6 @@ static int snd_record_handle_write(RecordChannelClient 
*record_client, size_t si
 }
 
 static int
-playback_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, uint16_t 
type, void *message)
-{
-switch (type) {
-case SPICE_MSGC_DISCONNECTING:
-break;
-default:
-return red_channel_client_handle_message(rcc, size, type, message);
-}
-return TRUE;
-}
-
-static int
 record_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, uint16_t 
type, void *message)
 {
 RecordChannelClient *record_client = (RecordChannelClient *)rcc; // FIXME
@@ -358,8 +346,6 @@ record_channel_handle_parsed(RedChannelClient *rcc, 
uint32_t size, uint16_t type
 record_client->start_time = mark->time;
 break;
 }
-case SPICE_MSGC_DISCONNECTING:
-break;
 default:
 return red_channel_client_handle_message(rcc, size, type, message);
 }
@@ -1411,7 +1397,7 @@ playback_channel_class_init(SndChannelClass *klass)
 object_class->constructed = playback_channel_constructed;
 
 channel_class->parser = 
spice_get_client_channel_parser(SPICE_CHANNEL_PLAYBACK, NULL);
-channel_class->handle_parsed = playback_channel_handle_parsed;
+channel_class->handle_parsed = red_channel_client_handle_message;
 channel_class->send_item = playback_channel_send_item;
 }
 
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 01/17] sound: Use worker directly

[Frediano Ziglio]

SpicePlaybackState and SpiceRecordState have same structures
changing only slightly the behaviour.
Using SndWorker instead allows some minor simplification and
more code reuse.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 26 --
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 8eddd2b..beab473 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -179,8 +179,8 @@ typedef struct RecordChannel {
 static SndWorker *workers;
 
 static void snd_receive(SndChannel *channel);
-static void snd_playback_start(SpicePlaybackState *st);
-static void snd_record_start(SpiceRecordState *st);
+static void snd_playback_start(SndWorker *worker);
+static void snd_record_start(SndWorker *worker);
 
 static SndChannel *snd_channel_ref(SndChannel *channel)
 {
@@ -1026,11 +1026,11 @@ SPICE_GNUC_VISIBLE void 
spice_server_playback_set_mute(SpicePlaybackInstance *si
 snd_playback_send_mute(playback_channel);
 }
 
-static void snd_playback_start(SpicePlaybackState *st)
+static void snd_playback_start(SndWorker *worker)
 {
-SndChannel *channel = st->worker.connection;
+SndChannel *channel = worker->connection;
 
-st->worker.active = 1;
+worker->active = 1;
 if (!channel)
 return;
 spice_assert(!channel->active);
@@ -1046,7 +1046,7 @@ static void snd_playback_start(SpicePlaybackState *st)
 
 SPICE_GNUC_VISIBLE void spice_server_playback_start(SpicePlaybackInstance *sin)
 {
-return snd_playback_start(sin->st);
+return snd_playback_start(>st->worker);
 }
 
 SPICE_GNUC_VISIBLE void spice_server_playback_stop(SpicePlaybackInstance *sin)
@@ -1195,7 +1195,6 @@ static void snd_set_playback_peer(RedChannel *channel, 
RedClient *client, RedsSt
 {
 SndWorker *worker = g_object_get_data(G_OBJECT(channel), "sound-worker");
 PlaybackChannel *playback_channel;
-SpicePlaybackState *st = SPICE_CONTAINEROF(worker, SpicePlaybackState, 
worker);
 
 snd_disconnect_channel(worker->connection);
 
@@ -1240,7 +1239,7 @@ static void snd_set_playback_peer(RedChannel *channel, 
RedClient *client, RedsSt
 }
 
 if (worker->active) {
-snd_playback_start(st);
+snd_playback_start(worker);
 }
 snd_playback_send(worker->connection);
 }
@@ -1294,12 +1293,12 @@ SPICE_GNUC_VISIBLE void 
spice_server_record_set_mute(SpiceRecordInstance *sin, u
 snd_record_send_mute(record_channel);
 }
 
-static void snd_record_start(SpiceRecordState *st)
+static void snd_record_start(SndWorker *worker)
 {
-SndChannel *channel = st->worker.connection;
+SndChannel *channel = worker->connection;
 RecordChannel *record_channel = SPICE_CONTAINEROF(channel, RecordChannel, 
base);
 
-st->worker.active = 1;
+worker->active = 1;
 if (!channel)
 return;
 spice_assert(!channel->active);
@@ -1316,7 +1315,7 @@ static void snd_record_start(SpiceRecordState *st)
 
 SPICE_GNUC_VISIBLE void spice_server_record_start(SpiceRecordInstance *sin)
 {
-snd_record_start(sin->st);
+snd_record_start(>st->worker);
 }
 
 SPICE_GNUC_VISIBLE void spice_server_record_stop(SpiceRecordInstance *sin)
@@ -1442,7 +1441,6 @@ static void snd_set_record_peer(RedChannel *channel, 
RedClient *client, RedsStre
 {
 SndWorker *worker = g_object_get_data(G_OBJECT(channel), "sound-worker");
 RecordChannel *record_channel;
-SpiceRecordState *st = SPICE_CONTAINEROF(worker, SpiceRecordState, worker);
 
 snd_disconnect_channel(worker->connection);
 
@@ -1465,7 +1463,7 @@ static void snd_set_record_peer(RedChannel *channel, 
RedClient *client, RedsStre
 
 on_new_record_channel(worker, _channel->base);
 if (worker->active) {
-snd_record_start(st);
+snd_record_start(worker);
 }
 snd_record_send(worker->connection);
 }
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 05/17] sound: Convert SndChannel to GObject

[Frediano Ziglio]

Stops using the dummy channel.
Data handling still goes through DummyChannelClient which is why
empty implementation of some required vfuncs is working.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 224 +++---
 1 file changed, 159 insertions(+), 65 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 5b66f3a..011cff2 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -144,9 +144,14 @@ typedef struct SpiceVolumeState {
 int mute;
 } SpiceVolumeState;
 
+#define TYPE_SND_CHANNEL snd_channel_get_type()
+#define SND_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_SND_CHANNEL, 
SndChannel))
+GType snd_channel_get_type(void) G_GNUC_CONST;
+
 /* Base class for SpicePlaybackState and SpiceRecordState */
 struct SndChannel {
-RedChannel *base_channel;
+RedChannel parent;
+
 SndChannelClient *connection; /* Only one client is supported */
 SndChannel *next; /* For the global SndChannel list */
 
@@ -155,14 +160,40 @@ struct SndChannel {
 uint32_t frequency;
 };
 
+typedef RedChannelClass SndChannelClass;
+
+G_DEFINE_TYPE(SndChannel, snd_channel, RED_TYPE_CHANNEL)
+
+
+typedef struct SpicePlaybackState PlaybackChannel;
+typedef SndChannelClass PlaybackChannelClass;
+
+#define TYPE_PLAYBACK_CHANNEL playback_channel_get_type()
+#define PLAYBACK_CHANNEL(obj) \
+(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL, PlaybackChannel))
+GType playback_channel_get_type(void) G_GNUC_CONST;
+
 struct SpicePlaybackState {
-struct SndChannel channel;
+SndChannel channel;
 };
 
+G_DEFINE_TYPE(PlaybackChannel, playback_channel, TYPE_SND_CHANNEL)
+
+
+typedef struct SpiceRecordState RecordChannel;
+typedef SndChannelClass RecordChannelClass;
+
+#define TYPE_RECORD_CHANNEL record_channel_get_type()
+#define RECORD_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), 
TYPE_RECORD_CHANNEL, RecordChannel))
+GType record_channel_get_type(void) G_GNUC_CONST;
+
 struct SpiceRecordState {
-struct SndChannel channel;
+SndChannel channel;
 };
 
+G_DEFINE_TYPE(RecordChannel, record_channel, TYPE_SND_CHANNEL)
+
+
 typedef struct RecordChannelClient {
 SndChannelClient base;
 uint32_t samples[RECORD_SAMPLES_SIZE];
@@ -201,7 +232,7 @@ static SndChannelClient *snd_channel_unref(SndChannelClient 
*client)
 static RedsState* snd_channel_get_server(SndChannelClient *client)
 {
 g_return_val_if_fail(client != NULL, NULL);
-return red_channel_get_server(client->channel->base_channel);
+return red_channel_get_server(RED_CHANNEL(client->channel));
 }
 
 static void snd_disconnect_channel(SndChannelClient *client)
@@ -892,7 +923,7 @@ static SndChannelClient *__new_channel(SndChannel *channel, 
int size, uint32_t c
 #endif
 int tos;
 MainChannelClient *mcc = red_client_get_main(red_client);
-RedsState *reds = red_channel_get_server(channel->base_channel);
+RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
 
 spice_assert(stream);
 if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
@@ -953,7 +984,7 @@ static SndChannelClient *__new_channel(SndChannel *channel, 
int size, uint32_t c
 client->cleanup = cleanup;
 
 client->channel_client =
-dummy_channel_client_create(channel->base_channel, red_client,
+dummy_channel_client_create(RED_CHANNEL(channel), red_client,
 num_common_caps, common_caps, num_caps, 
caps);
 if (!client->channel_client) {
 goto error2;
@@ -968,6 +999,29 @@ error1:
 return NULL;
 }
 
+static int snd_channel_config_socket(RedChannelClient *rcc)
+{
+g_assert_not_reached();
+}
+
+static void snd_channel_on_disconnect(RedChannelClient *rcc)
+{
+g_assert_not_reached();
+}
+
+static uint8_t*
+snd_channel_client_alloc_recv_buf(RedChannelClient *rcc, uint16_t type, 
uint32_t size)
+{
+g_assert_not_reached();
+}
+
+static void
+snd_channel_client_release_recv_buf(RedChannelClient *rcc, uint16_t type, 
uint32_t size,
+uint8_t *msg)
+{
+g_assert_not_reached();
+}
+
 static void snd_disconnect_channel_client(RedChannelClient *rcc)
 {
 SndChannel *channel;
@@ -975,7 +1029,7 @@ static void snd_disconnect_channel_client(RedChannelClient 
*rcc)
 uint32_t type;
 
 spice_assert(red_channel);
-channel = (SndChannel *)g_object_get_data(G_OBJECT(red_channel), 
"sound-channel");
+channel = SND_CHANNEL(red_channel);
 spice_assert(channel);
 g_object_get(red_channel, "channel-type", , NULL);
 
@@ -1126,7 +1180,7 @@ void snd_set_playback_latency(RedClient *client, uint32_t 
latency)
 
 for (; now; now = now->next) {
 uint32_t type;
-g_object_get(now->base_channel, "channel-type", , NULL);
+g_object_get(RED_CHANNEL(now), "channel-type", , NULL);
 if (type == SPICE_CHANNEL_PLAYBACK && now->connection &&
 red_channel_client_get_client(now->connection->channel_client) == 

[Spice-devel] [PATCH v4 09/17] Make RedChannelClient::incoming private

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/red-channel-client-private.h | 11 +++
 server/red-channel-client.c | 12 ++--
 server/red-channel-client.h | 13 -
 3 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/server/red-channel-client-private.h 
b/server/red-channel-client-private.h
index 38759ff..5f31ed2 100644
--- a/server/red-channel-client-private.h
+++ b/server/red-channel-client-private.h
@@ -50,6 +50,16 @@ typedef struct OutgoingHandler {
 int size;
 } OutgoingHandler;
 
+typedef struct IncomingHandler {
+IncomingHandlerInterface *cb;
+void *opaque;
+uint8_t header_buf[MAX_HEADER_SIZE];
+SpiceDataHeaderOpaque header;
+uint32_t header_pos;
+uint8_t *msg; // data of the msg following the header. allocated by 
alloc_msg_buf.
+uint32_t msg_pos;
+} IncomingHandler;
+
 struct RedChannelClientPrivate
 {
 RedChannel *channel;
@@ -99,6 +109,7 @@ struct RedChannelClientPrivate
 RedChannelClientLatencyMonitor latency_monitor;
 RedChannelClientConnectivityMonitor connectivity_monitor;
 
+IncomingHandler incoming;
 OutgoingHandler outgoing;
 };
 
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 24b2fb0..2b1843a 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -267,8 +267,8 @@ static void red_channel_client_constructed(GObject *object)
 {
 RedChannelClient *self =  RED_CHANNEL_CLIENT(object);
 
-self->incoming.opaque = self;
-self->incoming.cb = red_channel_get_incoming_handler(self->priv->channel);
+self->priv->incoming.opaque = self;
+self->priv->incoming.cb = 
red_channel_get_incoming_handler(self->priv->channel);
 
 self->priv->outgoing.opaque = self;
 self->priv->outgoing.cb = 
red_channel_get_outgoing_handler(self->priv->channel);
@@ -276,15 +276,15 @@ static void red_channel_client_constructed(GObject 
*object)
 self->priv->outgoing.size = 0;
 
 if (red_channel_client_test_remote_common_cap(self, 
SPICE_COMMON_CAP_MINI_HEADER)) {
-self->incoming.header = mini_header_wrapper;
+self->priv->incoming.header = mini_header_wrapper;
 self->priv->send_data.header = mini_header_wrapper;
 self->priv->is_mini_header = TRUE;
 } else {
-self->incoming.header = full_header_wrapper;
+self->priv->incoming.header = full_header_wrapper;
 self->priv->send_data.header = full_header_wrapper;
 self->priv->is_mini_header = FALSE;
 }
-self->incoming.header.data = self->incoming.header_buf;
+self->priv->incoming.header.data = self->priv->incoming.header_buf;
 }
 
 static void red_channel_client_class_init(RedChannelClientClass *klass)
@@ -1189,7 +1189,7 @@ static void red_peer_handle_incoming(RedsStream *stream, 
IncomingHandler *handle
 void red_channel_client_receive(RedChannelClient *rcc)
 {
 g_object_ref(rcc);
-red_peer_handle_incoming(rcc->priv->stream, >incoming);
+red_peer_handle_incoming(rcc->priv->stream, >priv->incoming);
 g_object_unref(rcc);
 }
 
diff --git a/server/red-channel-client.h b/server/red-channel-client.h
index 94b4f58..13ddf30 100644
--- a/server/red-channel-client.h
+++ b/server/red-channel-client.h
@@ -190,23 +190,10 @@ gboolean 
red_channel_client_set_migration_seamless(RedChannelClient *rcc);
 void red_channel_client_set_destroying(RedChannelClient *rcc);
 gboolean red_channel_client_is_destroying(RedChannelClient *rcc);
 
-typedef struct IncomingHandler {
-IncomingHandlerInterface *cb;
-void *opaque;
-uint8_t header_buf[MAX_HEADER_SIZE];
-SpiceDataHeaderOpaque header;
-uint32_t header_pos;
-uint8_t *msg; // data of the msg following the header. allocated by 
alloc_msg_buf.
-uint32_t msg_pos;
-} IncomingHandler;
-
 struct RedChannelClient
 {
 GObject parent;
 
-/* protected */
-IncomingHandler incoming;
-
 RedChannelClientPrivate *priv;
 };
 
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 06/17] sound: Implements config_socket RedChannel callback

[Frediano Ziglio]

This code is the same inside __new_channel but will set the
RedsStream from RedChannel.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 46 +-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/server/sound.c b/server/sound.c
index 011cff2..bf59789 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1001,7 +1001,51 @@ error1:
 
 static int snd_channel_config_socket(RedChannelClient *rcc)
 {
-g_assert_not_reached();
+int delay_val;
+int flags;
+#ifdef SO_PRIORITY
+int priority;
+#endif
+int tos;
+RedsStream *stream = red_channel_client_get_stream(rcc);
+RedClient *red_client = red_channel_client_get_client(rcc);
+MainChannelClient *mcc = red_client_get_main(red_client);
+
+if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
+spice_printerr("accept failed, %s", strerror(errno));
+return FALSE;
+}
+
+#ifdef SO_PRIORITY
+priority = 6;
+if (setsockopt(stream->socket, SOL_SOCKET, SO_PRIORITY, (void*),
+   sizeof(priority)) == -1) {
+if (errno != ENOTSUP) {
+spice_printerr("setsockopt failed, %s", strerror(errno));
+}
+}
+#endif
+
+tos = IPTOS_LOWDELAY;
+if (setsockopt(stream->socket, IPPROTO_IP, IP_TOS, (void*), 
sizeof(tos)) == -1) {
+if (errno != ENOTSUP) {
+spice_printerr("setsockopt failed, %s", strerror(errno));
+}
+}
+
+delay_val = main_channel_client_is_low_bandwidth(mcc) ? 0 : 1;
+if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, _val, 
sizeof(delay_val)) == -1) {
+if (errno != ENOTSUP) {
+spice_printerr("setsockopt failed, %s", strerror(errno));
+}
+}
+
+if (fcntl(stream->socket, F_SETFL, flags | O_NONBLOCK) == -1) {
+spice_printerr("accept failed, %s", strerror(errno));
+return FALSE;
+}
+
+return TRUE;
 }
 
 static void snd_channel_on_disconnect(RedChannelClient *rcc)
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 00/17] Remove DummyChannel* objects

[Frediano Ziglio]

These objects were used by the sound channel as
this channel read/write to/from client directly.
This make the code of this channel quite different
from the other ones.
Also this reduce code duplication from RedChannelClient
and increase encapsulation.
First set of patches attempt to rename fields/structures
to prepare for the conversion to RedChannel/RedChannelClient
and GObject.
Than other patches basically do the conversion and
some cleanup.

Changes since v4:
- merged some patches;
- make sure all commit are working;
- some comment updates;
- added some additional cleanup patches.

Changes since v3:
- updated some commit message;
- rename structures and related fields in the same commit.
- reverted wrong field rename (RedChannel *client).

Changes since v2:
- send playback data as soon as possible;
- split renames putting client renames first then worker ones;
- some commit message changes.

Changes since v1:
- split some long lines;
- do not attempt to free in progress frame before is full sent.
  This could cause raw frames to be overridden.

Frediano Ziglio (17):
  sound: Use worker directly
  sound: Rename SndChannel to SndChannelClient
  sound: Rename {Record,Playback}Channel to *ChannelClient
  sound: Rename SndWorker to SndChannel
  sound: Convert SndChannel to GObject
  sound: Implements config_socket RedChannel callback
  sound: Convert SndChannelClient to GObject
  Remove DummyChannel* objects
  Make RedChannelClient::incoming private
  sound: Free more on SndChannel finalize
  sound: Use default disconnect for client channels
  sound: Reuse code for snd_set_{playback,record}_peer
  sound: Reuse code for migrating client channels
  sound: Reuse code to set volume and mute
  sound: Use default message handler if possible
  sound: Make clear active and client_active are boolean
  sound: Use memcpy instead of manually copy volume array

 server/Makefile.am  |4 +-
 server/dummy-channel-client.c   |  138 +--
 server/dummy-channel-client.h   |   64 +-
 server/dummy-channel.c  |   94 +--
 server/dummy-channel.h  |   60 +-
 server/red-channel-client-private.h |   11 +-
 server/red-channel-client.c |   12 +-
 server/red-channel-client.h |   13 +-
 server/sound.c  | 1737 +---
 9 files changed, 850 insertions(+), 1283 deletions(-)
 delete mode 100644 server/dummy-channel-client.c
 delete mode 100644 server/dummy-channel-client.h
 delete mode 100644 server/dummy-channel.c
 delete mode 100644 server/dummy-channel.h

base-commit: 1f800090ef671e9fccb49ed64c01a85b6eb0890a
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 04/17] sound: Rename SndWorker to SndChannel

[Frediano Ziglio]

SndWorker has been historically based on RedChannel, initial git commit
has:
struct SndWorker {
 Channel base;
 ...
};

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 312 +-
 1 file changed, 156 insertions(+), 156 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index bafdd1e..5b66f3a 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -79,12 +79,12 @@ typedef int 
(*snd_channel_handle_message_proc)(SndChannelClient *client, size_t 
 typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
 typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
 
-typedef struct SndWorker SndWorker;
+typedef struct SndChannel SndChannel;
 
 /* Connects an audio client to a Spice client */
 struct SndChannelClient {
 RedsStream *stream;
-SndWorker *worker;
+SndChannel *channel;
 spice_parse_channel_func_t parser;
 int refs;
 
@@ -145,10 +145,10 @@ typedef struct SpiceVolumeState {
 } SpiceVolumeState;
 
 /* Base class for SpicePlaybackState and SpiceRecordState */
-struct SndWorker {
+struct SndChannel {
 RedChannel *base_channel;
 SndChannelClient *connection; /* Only one client is supported */
-SndWorker *next; /* For the global SndWorker list */
+SndChannel *next; /* For the global SndChannel list */
 
 int active;
 SpiceVolumeState volume;
@@ -156,11 +156,11 @@ struct SndWorker {
 };
 
 struct SpicePlaybackState {
-struct SndWorker worker;
+struct SndChannel channel;
 };
 
 struct SpiceRecordState {
-struct SndWorker worker;
+struct SndChannel channel;
 };
 
 typedef struct RecordChannelClient {
@@ -176,11 +176,11 @@ typedef struct RecordChannelClient {
 } RecordChannelClient;
 
 /* A list of all Spice{Playback,Record}State objects */
-static SndWorker *workers;
+static SndChannel *snd_channels;
 
 static void snd_receive(SndChannelClient *client);
-static void snd_playback_start(SndWorker *worker);
-static void snd_record_start(SndWorker *worker);
+static void snd_playback_start(SndChannel *channel);
+static void snd_record_start(SndChannel *channel);
 
 static SndChannelClient *snd_channel_ref(SndChannelClient *client)
 {
@@ -201,12 +201,12 @@ static SndChannelClient 
*snd_channel_unref(SndChannelClient *client)
 static RedsState* snd_channel_get_server(SndChannelClient *client)
 {
 g_return_val_if_fail(client != NULL, NULL);
-return red_channel_get_server(client->worker->base_channel);
+return red_channel_get_server(client->channel->base_channel);
 }
 
 static void snd_disconnect_channel(SndChannelClient *client)
 {
-SndWorker *worker;
+SndChannel *channel;
 RedsState *reds;
 RedChannel *red_channel;
 uint32_t type;
@@ -220,17 +220,17 @@ static void snd_disconnect_channel(SndChannelClient 
*client)
 g_object_get(red_channel, "channel-type", , NULL);
 spice_debug("SndChannelClient=%p rcc=%p type=%d",
  client, client->channel_client, type);
-worker = client->worker;
+channel = client->channel;
 client->cleanup(client);
-red_channel_client_disconnect(worker->connection->channel_client);
-worker->connection->channel_client = NULL;
+red_channel_client_disconnect(channel->connection->channel_client);
+channel->connection->channel_client = NULL;
 reds_core_watch_remove(reds, client->stream->watch);
 client->stream->watch = NULL;
 reds_stream_free(client->stream);
 client->stream = NULL;
 spice_marshaller_destroy(client->send_data.marshaller);
 snd_channel_unref(client);
-worker->connection = NULL;
+channel->connection = NULL;
 }
 
 static void snd_playback_free_frame(PlaybackChannelClient *playback_client, 
AudioFrame *frame)
@@ -384,11 +384,11 @@ static int snd_record_handle_message(SndChannelClient 
*client, size_t size, uint
 return snd_record_handle_write((RecordChannelClient *)client, size, 
message);
 case SPICE_MSGC_RECORD_MODE: {
 SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
-SndWorker *worker = client->worker;
+SndChannel *channel = client->channel;
 record_client->mode_time = mode->time;
 if (mode->mode != SPICE_AUDIO_DATA_MODE_RAW) {
-if (snd_codec_is_capable(mode->mode, worker->frequency)) {
-if (snd_codec_create(_client->codec, mode->mode, 
worker->frequency,
+if (snd_codec_is_capable(mode->mode, channel->frequency)) {
+if (snd_codec_create(_client->codec, mode->mode, 
channel->frequency,
  SND_CODEC_DECODE) == SND_CODEC_OK) {
 record_client->mode = mode->mode;
 } else {
@@ -571,7 +571,7 @@ static int snd_send_volume(SndChannelClient *client, 
uint32_t cap, int msg)
 {
 SpiceMsgAudioVolume *vol;
 uint8_t c;
-SpiceVolumeState *st = >worker->volume;
+SpiceVolumeState *st = >channel->volume;
 
 if 

[Spice-devel] [PATCH v4 02/17] sound: Rename SndChannel to SndChannelClient

[Frediano Ziglio]

SndWorker has been historically based on RedChannel, initial git commit
has:
struct SndWorker {
 Channel base;
 ...
};

SndChannel, contrary to what its name may imply is more focused on
marshalling/sending of sound data, which is the responsibility of
RedChannelClient for the other SPICE channels.

This commit and following ones make the naming of these 2 types more
consistent with how the rest of the code is structured.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 522 +-
 1 file changed, 261 insertions(+), 261 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index beab473..02adf79 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -73,16 +73,16 @@ enum PlaybackCommand {
 #define SND_PLAYBACK_PCM_MASK (1 << SND_PLAYBACK_PCM)
 #define SND_PLAYBACK_LATENCY_MASK ( 1 << SND_PLAYBACK_LATENCY)
 
-typedef struct SndChannel SndChannel;
+typedef struct SndChannelClient SndChannelClient;
 typedef void (*snd_channel_send_messages_proc)(void *in_channel);
-typedef int (*snd_channel_handle_message_proc)(SndChannel *channel, size_t 
size, uint32_t type, void *message);
-typedef void (*snd_channel_on_message_done_proc)(SndChannel *channel);
-typedef void (*snd_channel_cleanup_channel_proc)(SndChannel *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);
 
 typedef struct SndWorker SndWorker;
 
-/* Connects an audio channel to a Spice client */
-struct SndChannel {
+/* Connects an audio client to a Spice client */
+struct SndChannelClient {
 RedsStream *stream;
 SndWorker *worker;
 spice_parse_channel_func_t parser;
@@ -127,7 +127,7 @@ struct AudioFrame {
 };
 
 struct PlaybackChannel {
-SndChannel base;
+SndChannelClient base;
 AudioFrame frames[3];
 AudioFrame *free_frames;
 AudioFrame *in_progress;   /* Frame being sent to the client */
@@ -147,7 +147,7 @@ typedef struct SpiceVolumeState {
 /* Base class for SpicePlaybackState and SpiceRecordState */
 struct SndWorker {
 RedChannel *base_channel;
-SndChannel *connection; /* Only one client is supported */
+SndChannelClient *connection; /* Only one client is supported */
 SndWorker *next; /* For the global SndWorker list */
 
 int active;
@@ -164,7 +164,7 @@ struct SpiceRecordState {
 };
 
 typedef struct RecordChannel {
-SndChannel base;
+SndChannelClient base;
 uint32_t samples[RECORD_SAMPLES_SIZE];
 uint32_t write_pos;
 uint32_t read_pos;
@@ -178,58 +178,58 @@ typedef struct RecordChannel {
 /* A list of all Spice{Playback,Record}State objects */
 static SndWorker *workers;
 
-static void snd_receive(SndChannel *channel);
+static void snd_receive(SndChannelClient *client);
 static void snd_playback_start(SndWorker *worker);
 static void snd_record_start(SndWorker *worker);
 
-static SndChannel *snd_channel_ref(SndChannel *channel)
+static SndChannelClient *snd_channel_ref(SndChannelClient *client)
 {
-channel->refs++;
-return channel;
+client->refs++;
+return client;
 }
 
-static SndChannel *snd_channel_unref(SndChannel *channel)
+static SndChannelClient *snd_channel_unref(SndChannelClient *client)
 {
-if (!--channel->refs) {
-spice_printerr("SndChannel=%p freed", channel);
-free(channel);
+if (!--client->refs) {
+spice_printerr("SndChannelClient=%p freed", client);
+free(client);
 return NULL;
 }
-return channel;
+return client;
 }
 
-static RedsState* snd_channel_get_server(SndChannel *channel)
+static RedsState* snd_channel_get_server(SndChannelClient *client)
 {
-g_return_val_if_fail(channel != NULL, NULL);
-return red_channel_get_server(channel->worker->base_channel);
+g_return_val_if_fail(client != NULL, NULL);
+return red_channel_get_server(client->worker->base_channel);
 }
 
-static void snd_disconnect_channel(SndChannel *channel)
+static void snd_disconnect_channel(SndChannelClient *client)
 {
 SndWorker *worker;
 RedsState *reds;
 RedChannel *red_channel;
 uint32_t type;
 
-if (!channel || !channel->stream) {
+if (!client || !client->stream) {
 spice_debug("not connected");
 return;
 }
-red_channel = red_channel_client_get_channel(channel->channel_client);
-reds = snd_channel_get_server(channel);
+red_channel = red_channel_client_get_channel(client->channel_client);
+reds = snd_channel_get_server(client);
 g_object_get(red_channel, "channel-type", , NULL);
-spice_debug("SndChannel=%p rcc=%p type=%d",
- channel, channel->channel_client, type);
-worker = channel->worker;
-channel->cleanup(channel);
+spice_debug("SndChannelClient=%p rcc=%p type=%d",
+ client, 

[Spice-devel] [PATCH v4 07/17] sound: Convert SndChannelClient to GObject

[Frediano Ziglio]

This patch is quite huge but have some benefits:
- reduce dependency (DummyChannel* and some RedChannelClient internals);
- reuse RedChannelClient instead of reading from the RedsStream
  directly.

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.

Another difference is the AudioFrame allocation. 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 having RedChannelClient as
as base class 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 
---
 server/sound.c | 1106 +
 1 file changed, 490 insertions(+), 616 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index bf59789..14a6997 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -32,14 +32,10 @@
 
 #include "spice.h"
 #include "red-common.h"
-#include "dummy-channel.h"
-#include "dummy-channel-client.h"
 #include "main-channel.h"
 #include "reds.h"
 #include "red-qxl.h"
 #include "red-channel-client.h"
-/* FIXME: for now, allow sound channel access to private RedChannelClient data 
*/
-#include "red-channel-client-private.h"
 #include "red-client.h"
 #include "sound.h"
 #include "demarshallers.h"
@@ -56,6 +52,7 @@ enum SndCommand {
 SND_MIGRATE,
 SND_CTRL,
 SND_VOLUME,
+SND_MUTE,
 SND_END_COMMAND,
 };
 
@@ -68,67 +65,82 @@ enum PlaybackCommand {
 #define SND_MIGRATE_MASK (1 << SND_MIGRATE)
 #define SND_CTRL_MASK (1 << SND_CTRL)
 #define SND_VOLUME_MASK (1 << SND_VOLUME)
+#define SND_MUTE_MASK (1 << SND_MUTE)
+#define SND_VOLUME_MUTE_MASK (SND_VOLUME_MASK|SND_MUTE_MASK)
 
 #define SND_PLAYBACK_MODE_MASK (1 << SND_PLAYBACK_MODE)
 #define SND_PLAYBACK_PCM_MASK (1 << SND_PLAYBACK_PCM)
 #define SND_PLAYBACK_LATENCY_MASK ( 1 << SND_PLAYBACK_LATENCY)
 
 typedef struct SndChannelClient SndChannelClient;
-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 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;
+
 typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
-typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
 
-typedef struct SndChannel SndChannel;
+
+#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))
+GType snd_channel_client_get_type(void) G_GNUC_CONST;
 
 /* Connects an audio client to a Spice client */
 struct SndChannelClient {
-RedsStream *stream;
-SndChannel *channel;
-spice_parse_channel_func_t parser;
-int refs;
-
-RedChannelClient *channel_client;
+RedChannelClient parent;
 
 int active;
 int client_active;
-int blocked;
 
 uint32_t command;
 
-struct {
-uint64_t serial;
-SpiceMarshaller *marshaller;
-uint32_t size;
-uint32_t pos;
-} send_data;
-
-struct {
-uint8_t buf[SND_RECEIVE_BUF_SIZE];
-uint8_t *message_start;
-uint8_t *now;
-uint8_t *end;
-} receive_data;
-
-snd_channel_send_messages_proc send_messages;
-snd_channel_handle_message_proc handle_message;
+/* we don't expect very big messages so don't allocate too much
+ * bytes, data will be cached in RecordChannelClient::samples */
+uint8_t 

[Spice-devel] [PATCH v4 10/17] sound: Free more on SndChannel finalize

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 14a6997..ebbe821 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1425,10 +1425,26 @@ snd_channel_init(SndChannel *self)
 }
 
 static void
+snd_channel_finalize(GObject *object)
+{
+SndChannel *channel = SND_CHANNEL(object);
+
+remove_channel(channel);
+
+free(channel->volume.volume);
+channel->volume.volume = NULL;
+
+G_OBJECT_CLASS(snd_channel_parent_class)->finalize(object);
+}
+
+static void
 snd_channel_class_init(SndChannelClass *klass)
 {
+GObjectClass *object_class = G_OBJECT_CLASS(klass);
 RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
 
+object_class->finalize = snd_channel_finalize;
+
 channel_class->config_socket = snd_channel_config_socket;
 channel_class->alloc_recv_buf = snd_channel_client_alloc_recv_buf;
 channel_class->release_recv_buf = snd_channel_client_release_recv_buf;
@@ -1544,10 +1560,7 @@ static void snd_detach_common(SndChannel *channel)
 }
 RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
 
-remove_channel(channel);
 reds_unregister_channel(reds, RED_CHANNEL(channel));
-free(channel->volume.volume);
-channel->volume.volume = NULL;
 red_channel_destroy(RED_CHANNEL(channel));
 }
 
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 04/17] sound: Rename SndWorker to SndChannel

[Frediano Ziglio]

SndWorker has been historically based on RedChannel, initial git commit
has:
struct SndWorker {
 Channel base;
 ...
};

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 312 +-
 1 file changed, 156 insertions(+), 156 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index bafdd1e..5b66f3a 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -79,12 +79,12 @@ typedef int 
(*snd_channel_handle_message_proc)(SndChannelClient *client, size_t 
 typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
 typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
 
-typedef struct SndWorker SndWorker;
+typedef struct SndChannel SndChannel;
 
 /* Connects an audio client to a Spice client */
 struct SndChannelClient {
 RedsStream *stream;
-SndWorker *worker;
+SndChannel *channel;
 spice_parse_channel_func_t parser;
 int refs;
 
@@ -145,10 +145,10 @@ typedef struct SpiceVolumeState {
 } SpiceVolumeState;
 
 /* Base class for SpicePlaybackState and SpiceRecordState */
-struct SndWorker {
+struct SndChannel {
 RedChannel *base_channel;
 SndChannelClient *connection; /* Only one client is supported */
-SndWorker *next; /* For the global SndWorker list */
+SndChannel *next; /* For the global SndChannel list */
 
 int active;
 SpiceVolumeState volume;
@@ -156,11 +156,11 @@ struct SndWorker {
 };
 
 struct SpicePlaybackState {
-struct SndWorker worker;
+struct SndChannel channel;
 };
 
 struct SpiceRecordState {
-struct SndWorker worker;
+struct SndChannel channel;
 };
 
 typedef struct RecordChannelClient {
@@ -176,11 +176,11 @@ typedef struct RecordChannelClient {
 } RecordChannelClient;
 
 /* A list of all Spice{Playback,Record}State objects */
-static SndWorker *workers;
+static SndChannel *snd_channels;
 
 static void snd_receive(SndChannelClient *client);
-static void snd_playback_start(SndWorker *worker);
-static void snd_record_start(SndWorker *worker);
+static void snd_playback_start(SndChannel *channel);
+static void snd_record_start(SndChannel *channel);
 
 static SndChannelClient *snd_channel_ref(SndChannelClient *client)
 {
@@ -201,12 +201,12 @@ static SndChannelClient 
*snd_channel_unref(SndChannelClient *client)
 static RedsState* snd_channel_get_server(SndChannelClient *client)
 {
 g_return_val_if_fail(client != NULL, NULL);
-return red_channel_get_server(client->worker->base_channel);
+return red_channel_get_server(client->channel->base_channel);
 }
 
 static void snd_disconnect_channel(SndChannelClient *client)
 {
-SndWorker *worker;
+SndChannel *channel;
 RedsState *reds;
 RedChannel *red_channel;
 uint32_t type;
@@ -220,17 +220,17 @@ static void snd_disconnect_channel(SndChannelClient 
*client)
 g_object_get(red_channel, "channel-type", , NULL);
 spice_debug("SndChannelClient=%p rcc=%p type=%d",
  client, client->channel_client, type);
-worker = client->worker;
+channel = client->channel;
 client->cleanup(client);
-red_channel_client_disconnect(worker->connection->channel_client);
-worker->connection->channel_client = NULL;
+red_channel_client_disconnect(channel->connection->channel_client);
+channel->connection->channel_client = NULL;
 reds_core_watch_remove(reds, client->stream->watch);
 client->stream->watch = NULL;
 reds_stream_free(client->stream);
 client->stream = NULL;
 spice_marshaller_destroy(client->send_data.marshaller);
 snd_channel_unref(client);
-worker->connection = NULL;
+channel->connection = NULL;
 }
 
 static void snd_playback_free_frame(PlaybackChannelClient *playback_client, 
AudioFrame *frame)
@@ -384,11 +384,11 @@ static int snd_record_handle_message(SndChannelClient 
*client, size_t size, uint
 return snd_record_handle_write((RecordChannelClient *)client, size, 
message);
 case SPICE_MSGC_RECORD_MODE: {
 SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
-SndWorker *worker = client->worker;
+SndChannel *channel = client->channel;
 record_client->mode_time = mode->time;
 if (mode->mode != SPICE_AUDIO_DATA_MODE_RAW) {
-if (snd_codec_is_capable(mode->mode, worker->frequency)) {
-if (snd_codec_create(_client->codec, mode->mode, 
worker->frequency,
+if (snd_codec_is_capable(mode->mode, channel->frequency)) {
+if (snd_codec_create(_client->codec, mode->mode, 
channel->frequency,
  SND_CODEC_DECODE) == SND_CODEC_OK) {
 record_client->mode = mode->mode;
 } else {
@@ -571,7 +571,7 @@ static int snd_send_volume(SndChannelClient *client, 
uint32_t cap, int msg)
 {
 SpiceMsgAudioVolume *vol;
 uint8_t c;
-SpiceVolumeState *st = >worker->volume;
+SpiceVolumeState *st = >channel->volume;
 
 if 

[Spice-devel] [PATCH v4 08/17] Remove DummyChannel* objects

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/Makefile.am|   4 +-
 server/dummy-channel-client.c | 138 +---
 server/dummy-channel-client.h |  64 +
 server/dummy-channel.c|  94 +
 server/dummy-channel.h|  60 +---
 5 files changed, 360 deletions(-)
 delete mode 100644 server/dummy-channel-client.c
 delete mode 100644 server/dummy-channel-client.h
 delete mode 100644 server/dummy-channel.c
 delete mode 100644 server/dummy-channel.h

diff --git a/server/Makefile.am b/server/Makefile.am
index ae79ed7..1442bf1 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -104,10 +104,6 @@ libserver_la_SOURCES = \
red-channel-client-private.h\
red-client.c\
red-client.h\
-   dummy-channel.c \
-   dummy-channel.h \
-   dummy-channel-client.c  \
-   dummy-channel-client.h  \
red-common.h\
dispatcher.c\
dispatcher.h\
diff --git a/server/dummy-channel-client.c b/server/dummy-channel-client.c
deleted file mode 100644
index 61d5683..000
--- a/server/dummy-channel-client.c
+++ /dev/null
@@ -1,138 +0,0 @@
-/*
-   Copyright (C) 2009-2015 Red Hat, Inc.
-
-   This library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   This library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with this library; if not, see .
-*/
-#ifdef HAVE_CONFIG_H
-#include 
-#endif
-
-#include "dummy-channel-client.h"
-#include "red-channel.h"
-#include "red-client.h"
-
-static void dummy_channel_client_initable_interface_init(GInitableIface 
*iface);
-
-G_DEFINE_TYPE_WITH_CODE(DummyChannelClient, dummy_channel_client, 
RED_TYPE_CHANNEL_CLIENT,
-G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE,
-  
dummy_channel_client_initable_interface_init))
-
-#define DUMMY_CHANNEL_CLIENT_PRIVATE(o) \
-(G_TYPE_INSTANCE_GET_PRIVATE((o), TYPE_DUMMY_CHANNEL_CLIENT, 
DummyChannelClientPrivate))
-
-struct DummyChannelClientPrivate
-{
-gboolean connected;
-};
-
-static gboolean dummy_channel_client_initable_init(GInitable *initable,
-   GCancellable *cancellable,
-   GError **error)
-{
-GError *local_error = NULL;
-RedChannelClient *rcc = RED_CHANNEL_CLIENT(initable);
-RedClient *client = red_channel_client_get_client(rcc);
-RedChannel *channel = red_channel_client_get_channel(rcc);
-
-red_channel_add_client(channel, rcc);
-if (!red_client_add_channel(client, rcc, _error)) {
-red_channel_remove_client(channel, rcc);
-}
-
-if (local_error) {
-g_warning("Failed to create channel client: %s", local_error->message);
-g_propagate_error(error, local_error);
-}
-return local_error == NULL;
-}
-
-static void dummy_channel_client_initable_interface_init(GInitableIface *iface)
-{
-iface->init = dummy_channel_client_initable_init;
-}
-
-static gboolean dummy_channel_client_is_connected(RedChannelClient *rcc)
-{
-return DUMMY_CHANNEL_CLIENT(rcc)->priv->connected;
-}
-
-static void dummy_channel_client_disconnect(RedChannelClient *rcc)
-{
-DummyChannelClient *self = DUMMY_CHANNEL_CLIENT(rcc);
-RedChannel *channel = red_channel_client_get_channel(rcc);
-GList *link;
-uint32_t type, id;
-
-if (channel && (link = g_list_find(red_channel_get_clients(channel), 
rcc))) {
-g_object_get(channel, "channel-type", , "id", , NULL);
-spice_printerr("rcc=%p (channel=%p type=%d id=%d)", rcc, channel,
-   type, id);
-red_channel_remove_client(channel, link->data);
-}
-self->priv->connected = FALSE;
-}
-
-static void
-dummy_channel_client_class_init(DummyChannelClientClass *klass)
-{
-RedChannelClientClass *cc_class = RED_CHANNEL_CLIENT_CLASS(klass);
-
-g_type_class_add_private(klass, sizeof(DummyChannelClientPrivate));
-
-cc_class->is_connected = dummy_channel_client_is_connected;
-cc_class->disconnect = dummy_channel_client_disconnect;
-}
-
-static void
-dummy_channel_client_init(DummyChannelClient *self)
-{
-self->priv = 

[Spice-devel] [PATCH v4 03/17] sound: Rename {Record, Playback}Channel to *ChannelClient

[Frediano Ziglio]

Make easier to understand that they refer to client and not
all channel.

Specifically:
- RecordChannel -> RecordChannelClient
- PlaybackChannel -> PlaybackChannelClient
- playback_channel -> playback_client
- record_channel -> record_client

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 368 +-
 1 file changed, 184 insertions(+), 184 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 02adf79..bafdd1e 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -116,17 +116,17 @@ struct SndChannelClient {
 snd_channel_cleanup_channel_proc cleanup;
 };
 
-typedef struct PlaybackChannel PlaybackChannel;
+typedef struct PlaybackChannelClient PlaybackChannelClient;
 
 typedef struct AudioFrame AudioFrame;
 struct AudioFrame {
 uint32_t time;
 uint32_t samples[SND_CODEC_MAX_FRAME_SIZE];
-PlaybackChannel *channel;
+PlaybackChannelClient *client;
 AudioFrame *next;
 };
 
-struct PlaybackChannel {
+struct PlaybackChannelClient {
 SndChannelClient base;
 AudioFrame frames[3];
 AudioFrame *free_frames;
@@ -163,7 +163,7 @@ struct SpiceRecordState {
 struct SndWorker worker;
 };
 
-typedef struct RecordChannel {
+typedef struct RecordChannelClient {
 SndChannelClient base;
 uint32_t samples[RECORD_SAMPLES_SIZE];
 uint32_t write_pos;
@@ -173,7 +173,7 @@ typedef struct RecordChannel {
 uint32_t start_time;
 SndCodec codec;
 uint8_t  decode_buf[SND_CODEC_MAX_FRAME_BYTES];
-} RecordChannel;
+} RecordChannelClient;
 
 /* A list of all Spice{Playback,Record}State objects */
 static SndWorker *workers;
@@ -233,20 +233,20 @@ static void snd_disconnect_channel(SndChannelClient 
*client)
 worker->connection = NULL;
 }
 
-static void snd_playback_free_frame(PlaybackChannel *playback_channel, 
AudioFrame *frame)
+static void snd_playback_free_frame(PlaybackChannelClient *playback_client, 
AudioFrame *frame)
 {
-frame->channel = playback_channel;
-frame->next = playback_channel->free_frames;
-playback_channel->free_frames = frame;
+frame->client = playback_client;
+frame->next = playback_client->free_frames;
+playback_client->free_frames = frame;
 }
 
 static void snd_playback_on_message_done(SndChannelClient *client)
 {
-PlaybackChannel *playback_channel = (PlaybackChannel *)client;
-if (playback_channel->in_progress) {
-snd_playback_free_frame(playback_channel, 
playback_channel->in_progress);
-playback_channel->in_progress = NULL;
-if (playback_channel->pending_frame) {
+PlaybackChannelClient *playback_client = (PlaybackChannelClient *)client;
+if (playback_client->in_progress) {
+snd_playback_free_frame(playback_client, playback_client->in_progress);
+playback_client->in_progress = NULL;
+if (playback_client->pending_frame) {
 client->command |= SND_PLAYBACK_PCM_MASK;
 }
 }
@@ -311,7 +311,7 @@ static int snd_send_data(SndChannelClient *client)
 return TRUE;
 }
 
-static int snd_record_handle_write(RecordChannel *record_channel, size_t size, 
void *message)
+static int snd_record_handle_write(RecordChannelClient *record_client, size_t 
size, void *message)
 {
 SpiceMsgcRecordPacket *packet;
 uint32_t write_pos;
@@ -319,39 +319,39 @@ static int snd_record_handle_write(RecordChannel 
*record_channel, size_t size, v
 uint32_t len;
 uint32_t now;
 
-if (!record_channel) {
+if (!record_client) {
 return FALSE;
 }
 
 packet = (SpiceMsgcRecordPacket *)message;
 
-if (record_channel->mode == SPICE_AUDIO_DATA_MODE_RAW) {
+if (record_client->mode == SPICE_AUDIO_DATA_MODE_RAW) {
 data = (uint32_t *)packet->data;
 size = packet->data_size >> 2;
 size = MIN(size, RECORD_SAMPLES_SIZE);
  } else {
 int decode_size;
-decode_size = sizeof(record_channel->decode_buf);
-if (snd_codec_decode(record_channel->codec, packet->data, 
packet->data_size,
-record_channel->decode_buf, _size) != SND_CODEC_OK)
+decode_size = sizeof(record_client->decode_buf);
+if (snd_codec_decode(record_client->codec, packet->data, 
packet->data_size,
+record_client->decode_buf, _size) != SND_CODEC_OK)
 return FALSE;
-data = (uint32_t *) record_channel->decode_buf;
+data = (uint32_t *) record_client->decode_buf;
 size = decode_size >> 2;
 }
 
-write_pos = record_channel->write_pos % RECORD_SAMPLES_SIZE;
-record_channel->write_pos += size;
+write_pos = record_client->write_pos % RECORD_SAMPLES_SIZE;
+record_client->write_pos += size;
 len = RECORD_SAMPLES_SIZE - write_pos;
 now = MIN(len, size);
 size -= now;
-memcpy(record_channel->samples + write_pos, data, now << 2);
+memcpy(record_client->samples + write_pos, data, now << 2);
 
 if (size) {
-

[Spice-devel] [PATCH v4 11/17] sound: Use default disconnect for client channels

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 21 -
 1 file changed, 21 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index ebbe821..486fc95 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -804,25 +804,6 @@ snd_channel_client_release_recv_buf(RedChannelClient *rcc, 
uint16_t type, uint32
 }
 }
 
-// TODO remove and just use red_channel_client_disconnect ??
-static void snd_disconnect_channel_client(RedChannelClient *rcc)
-{
-SndChannel *channel;
-RedChannel *red_channel = red_channel_client_get_channel(rcc);
-uint32_t type;
-
-spice_assert(red_channel);
-channel = SND_CHANNEL(red_channel);
-spice_assert(channel);
-g_object_get(red_channel, "channel-type", , NULL);
-
-spice_debug("channel-type=%d", type);
-if (channel->connection) {
-spice_assert(RED_CHANNEL_CLIENT(channel->connection) == rcc);
-red_channel_client_disconnect(rcc);
-}
-}
-
 static void snd_set_command(SndChannelClient *client, uint32_t command)
 {
 if (!client) {
@@ -1466,7 +1447,6 @@ playback_channel_constructed(GObject *object)
 G_OBJECT_CLASS(playback_channel_parent_class)->constructed(object);
 
 client_cbs.connect = snd_set_playback_peer;
-client_cbs.disconnect = snd_disconnect_channel_client;
 client_cbs.migrate = snd_playback_migrate_channel_client;
 red_channel_register_client_cbs(RED_CHANNEL(self), _cbs, self);
 
@@ -1517,7 +1497,6 @@ record_channel_constructed(GObject *object)
 G_OBJECT_CLASS(record_channel_parent_class)->constructed(object);
 
 client_cbs.connect = snd_set_record_peer;
-client_cbs.disconnect = snd_disconnect_channel_client;
 client_cbs.migrate = snd_record_migrate_channel_client;
 red_channel_register_client_cbs(RED_CHANNEL(self), _cbs, self);
 
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 00/17] Remove DummyChannel* objects

[Frediano Ziglio]

These objects were used by the sound channel as
this channel read/write to/from client directly.
This make the code of this channel quite different
from the other ones.
Also this reduce code duplication from RedChannelClient
and increase encapsulation.
First set of patches attempt to rename fields/structures
to prepare for the conversion to RedChannel/RedChannelClient
and GObject.
Than other patches basically do the conversion and
some cleanup.

Changes since v4:
- merged some patches;
- make sure all commit are working;
- some comment updates;
- added some additional cleanup patches.

Changes since v3:
- updated some commit message;
- rename structures and related fields in the same commit.
- reverted wrong field rename (RedChannel *client).

Changes since v2:
- send playback data as soon as possible;
- split renames putting client renames first then worker ones;
- some commit message changes.

Changes since v1:
- split some long lines;
- do not attempt to free in progress frame before is full sent.
  This could cause raw frames to be overridden.

Frediano Ziglio (17):
  sound: Use worker directly
  sound: Rename SndChannel to SndChannelClient
  sound: Rename {Record,Playback}Channel to *ChannelClient
  sound: Rename SndWorker to SndChannel
  sound: Convert SndChannel to GObject
  sound: Implements config_socket RedChannel callback
  sound: Convert SndChannelClient to GObject
  Remove DummyChannel* objects
  Make RedChannelClient::incoming private
  sound: Free more on SndChannel finalize
  sound: Use default disconnect for client channels
  sound: Reuse code for snd_set_{playback,record}_peer
  sound: Reuse code for migrating client channels
  sound: Reuse code to set volume and mute
  sound: Use default message handler if possible
  sound: Make clear active and client_active are boolean
  sound: Use memcpy instead of manually copy volume array

 server/Makefile.am  |4 +-
 server/dummy-channel-client.c   |  138 +--
 server/dummy-channel-client.h   |   64 +-
 server/dummy-channel.c  |   94 +--
 server/dummy-channel.h  |   60 +-
 server/red-channel-client-private.h |   11 +-
 server/red-channel-client.c |   12 +-
 server/red-channel-client.h |   13 +-
 server/sound.c  | 1737 +---
 9 files changed, 850 insertions(+), 1283 deletions(-)
 delete mode 100644 server/dummy-channel-client.c
 delete mode 100644 server/dummy-channel-client.h
 delete mode 100644 server/dummy-channel.c
 delete mode 100644 server/dummy-channel.h

base-commit: 1f800090ef671e9fccb49ed64c01a85b6eb0890a
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 09/17] Make RedChannelClient::incoming private

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/red-channel-client-private.h | 11 +++
 server/red-channel-client.c | 12 ++--
 server/red-channel-client.h | 13 -
 3 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/server/red-channel-client-private.h 
b/server/red-channel-client-private.h
index 38759ff..5f31ed2 100644
--- a/server/red-channel-client-private.h
+++ b/server/red-channel-client-private.h
@@ -50,6 +50,16 @@ typedef struct OutgoingHandler {
 int size;
 } OutgoingHandler;
 
+typedef struct IncomingHandler {
+IncomingHandlerInterface *cb;
+void *opaque;
+uint8_t header_buf[MAX_HEADER_SIZE];
+SpiceDataHeaderOpaque header;
+uint32_t header_pos;
+uint8_t *msg; // data of the msg following the header. allocated by 
alloc_msg_buf.
+uint32_t msg_pos;
+} IncomingHandler;
+
 struct RedChannelClientPrivate
 {
 RedChannel *channel;
@@ -99,6 +109,7 @@ struct RedChannelClientPrivate
 RedChannelClientLatencyMonitor latency_monitor;
 RedChannelClientConnectivityMonitor connectivity_monitor;
 
+IncomingHandler incoming;
 OutgoingHandler outgoing;
 };
 
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 24b2fb0..2b1843a 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -267,8 +267,8 @@ static void red_channel_client_constructed(GObject *object)
 {
 RedChannelClient *self =  RED_CHANNEL_CLIENT(object);
 
-self->incoming.opaque = self;
-self->incoming.cb = red_channel_get_incoming_handler(self->priv->channel);
+self->priv->incoming.opaque = self;
+self->priv->incoming.cb = 
red_channel_get_incoming_handler(self->priv->channel);
 
 self->priv->outgoing.opaque = self;
 self->priv->outgoing.cb = 
red_channel_get_outgoing_handler(self->priv->channel);
@@ -276,15 +276,15 @@ static void red_channel_client_constructed(GObject 
*object)
 self->priv->outgoing.size = 0;
 
 if (red_channel_client_test_remote_common_cap(self, 
SPICE_COMMON_CAP_MINI_HEADER)) {
-self->incoming.header = mini_header_wrapper;
+self->priv->incoming.header = mini_header_wrapper;
 self->priv->send_data.header = mini_header_wrapper;
 self->priv->is_mini_header = TRUE;
 } else {
-self->incoming.header = full_header_wrapper;
+self->priv->incoming.header = full_header_wrapper;
 self->priv->send_data.header = full_header_wrapper;
 self->priv->is_mini_header = FALSE;
 }
-self->incoming.header.data = self->incoming.header_buf;
+self->priv->incoming.header.data = self->priv->incoming.header_buf;
 }
 
 static void red_channel_client_class_init(RedChannelClientClass *klass)
@@ -1189,7 +1189,7 @@ static void red_peer_handle_incoming(RedsStream *stream, 
IncomingHandler *handle
 void red_channel_client_receive(RedChannelClient *rcc)
 {
 g_object_ref(rcc);
-red_peer_handle_incoming(rcc->priv->stream, >incoming);
+red_peer_handle_incoming(rcc->priv->stream, >priv->incoming);
 g_object_unref(rcc);
 }
 
diff --git a/server/red-channel-client.h b/server/red-channel-client.h
index 94b4f58..13ddf30 100644
--- a/server/red-channel-client.h
+++ b/server/red-channel-client.h
@@ -190,23 +190,10 @@ gboolean 
red_channel_client_set_migration_seamless(RedChannelClient *rcc);
 void red_channel_client_set_destroying(RedChannelClient *rcc);
 gboolean red_channel_client_is_destroying(RedChannelClient *rcc);
 
-typedef struct IncomingHandler {
-IncomingHandlerInterface *cb;
-void *opaque;
-uint8_t header_buf[MAX_HEADER_SIZE];
-SpiceDataHeaderOpaque header;
-uint32_t header_pos;
-uint8_t *msg; // data of the msg following the header. allocated by 
alloc_msg_buf.
-uint32_t msg_pos;
-} IncomingHandler;
-
 struct RedChannelClient
 {
 GObject parent;
 
-/* protected */
-IncomingHandler incoming;
-
 RedChannelClientPrivate *priv;
 };
 
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 07/17] sound: Convert SndChannelClient to GObject

[Frediano Ziglio]

This patch is quite huge but have some benefits:
- reduce dependency (DummyChannel* and some RedChannelClient internals);
- reuse RedChannelClient instead of reading from the RedsStream
  directly.

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.

Another difference is the AudioFrame allocation. 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 having RedChannelClient as
as base class 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 
---
 server/sound.c | 1106 +
 1 file changed, 490 insertions(+), 616 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index bf59789..14a6997 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -32,14 +32,10 @@
 
 #include "spice.h"
 #include "red-common.h"
-#include "dummy-channel.h"
-#include "dummy-channel-client.h"
 #include "main-channel.h"
 #include "reds.h"
 #include "red-qxl.h"
 #include "red-channel-client.h"
-/* FIXME: for now, allow sound channel access to private RedChannelClient data 
*/
-#include "red-channel-client-private.h"
 #include "red-client.h"
 #include "sound.h"
 #include "demarshallers.h"
@@ -56,6 +52,7 @@ enum SndCommand {
 SND_MIGRATE,
 SND_CTRL,
 SND_VOLUME,
+SND_MUTE,
 SND_END_COMMAND,
 };
 
@@ -68,67 +65,82 @@ enum PlaybackCommand {
 #define SND_MIGRATE_MASK (1 << SND_MIGRATE)
 #define SND_CTRL_MASK (1 << SND_CTRL)
 #define SND_VOLUME_MASK (1 << SND_VOLUME)
+#define SND_MUTE_MASK (1 << SND_MUTE)
+#define SND_VOLUME_MUTE_MASK (SND_VOLUME_MASK|SND_MUTE_MASK)
 
 #define SND_PLAYBACK_MODE_MASK (1 << SND_PLAYBACK_MODE)
 #define SND_PLAYBACK_PCM_MASK (1 << SND_PLAYBACK_PCM)
 #define SND_PLAYBACK_LATENCY_MASK ( 1 << SND_PLAYBACK_LATENCY)
 
 typedef struct SndChannelClient SndChannelClient;
-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 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;
+
 typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
-typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
 
-typedef struct SndChannel SndChannel;
+
+#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))
+GType snd_channel_client_get_type(void) G_GNUC_CONST;
 
 /* Connects an audio client to a Spice client */
 struct SndChannelClient {
-RedsStream *stream;
-SndChannel *channel;
-spice_parse_channel_func_t parser;
-int refs;
-
-RedChannelClient *channel_client;
+RedChannelClient parent;
 
 int active;
 int client_active;
-int blocked;
 
 uint32_t command;
 
-struct {
-uint64_t serial;
-SpiceMarshaller *marshaller;
-uint32_t size;
-uint32_t pos;
-} send_data;
-
-struct {
-uint8_t buf[SND_RECEIVE_BUF_SIZE];
-uint8_t *message_start;
-uint8_t *now;
-uint8_t *end;
-} receive_data;
-
-snd_channel_send_messages_proc send_messages;
-snd_channel_handle_message_proc handle_message;
+/* we don't expect very big messages so don't allocate too much
+ * bytes, data will be cached in RecordChannelClient::samples */
+uint8_t 

[Spice-devel] [PATCH v4 10/17] sound: Free more on SndChannel finalize

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 14a6997..ebbe821 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1425,10 +1425,26 @@ snd_channel_init(SndChannel *self)
 }
 
 static void
+snd_channel_finalize(GObject *object)
+{
+SndChannel *channel = SND_CHANNEL(object);
+
+remove_channel(channel);
+
+free(channel->volume.volume);
+channel->volume.volume = NULL;
+
+G_OBJECT_CLASS(snd_channel_parent_class)->finalize(object);
+}
+
+static void
 snd_channel_class_init(SndChannelClass *klass)
 {
+GObjectClass *object_class = G_OBJECT_CLASS(klass);
 RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
 
+object_class->finalize = snd_channel_finalize;
+
 channel_class->config_socket = snd_channel_config_socket;
 channel_class->alloc_recv_buf = snd_channel_client_alloc_recv_buf;
 channel_class->release_recv_buf = snd_channel_client_release_recv_buf;
@@ -1544,10 +1560,7 @@ static void snd_detach_common(SndChannel *channel)
 }
 RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
 
-remove_channel(channel);
 reds_unregister_channel(reds, RED_CHANNEL(channel));
-free(channel->volume.volume);
-channel->volume.volume = NULL;
 red_channel_destroy(RED_CHANNEL(channel));
 }
 
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 06/17] sound: Implements config_socket RedChannel callback

[Frediano Ziglio]

This code is the same inside __new_channel but will set the
RedsStream from RedChannel.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 46 +-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/server/sound.c b/server/sound.c
index 011cff2..bf59789 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1001,7 +1001,51 @@ error1:
 
 static int snd_channel_config_socket(RedChannelClient *rcc)
 {
-g_assert_not_reached();
+int delay_val;
+int flags;
+#ifdef SO_PRIORITY
+int priority;
+#endif
+int tos;
+RedsStream *stream = red_channel_client_get_stream(rcc);
+RedClient *red_client = red_channel_client_get_client(rcc);
+MainChannelClient *mcc = red_client_get_main(red_client);
+
+if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
+spice_printerr("accept failed, %s", strerror(errno));
+return FALSE;
+}
+
+#ifdef SO_PRIORITY
+priority = 6;
+if (setsockopt(stream->socket, SOL_SOCKET, SO_PRIORITY, (void*),
+   sizeof(priority)) == -1) {
+if (errno != ENOTSUP) {
+spice_printerr("setsockopt failed, %s", strerror(errno));
+}
+}
+#endif
+
+tos = IPTOS_LOWDELAY;
+if (setsockopt(stream->socket, IPPROTO_IP, IP_TOS, (void*), 
sizeof(tos)) == -1) {
+if (errno != ENOTSUP) {
+spice_printerr("setsockopt failed, %s", strerror(errno));
+}
+}
+
+delay_val = main_channel_client_is_low_bandwidth(mcc) ? 0 : 1;
+if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, _val, 
sizeof(delay_val)) == -1) {
+if (errno != ENOTSUP) {
+spice_printerr("setsockopt failed, %s", strerror(errno));
+}
+}
+
+if (fcntl(stream->socket, F_SETFL, flags | O_NONBLOCK) == -1) {
+spice_printerr("accept failed, %s", strerror(errno));
+return FALSE;
+}
+
+return TRUE;
 }
 
 static void snd_channel_on_disconnect(RedChannelClient *rcc)
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 7/7] Support QXL remove on spice_server_remove_interface

[Frediano Ziglio]

Allow to dynamically remove QXL interfaces. This could be used to
support hot swapping of QXL cards.
This code is actually not used in any way.
QXL interfaces are destroyed by spice_server_destroy automatically.

Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/server/reds.c b/server/reds.c
index d350331..6064a6d 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3321,6 +3321,13 @@ SPICE_GNUC_VISIBLE int 
spice_server_remove_interface(SpiceBaseInstance *sin)
 SpiceCharDeviceInstance *char_device = SPICE_CONTAINEROF(sin, 
SpiceCharDeviceInstance, base);
 reds = red_char_device_get_server(char_device->st);
 spice_server_char_device_remove_interface(reds, sin);
+} else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
+QXLInstance *qxl;
+
+qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
+reds = red_qxl_get_server(qxl->st);
+reds->qxl_instances = g_list_remove(reds->qxl_instances, qxl);
+red_qxl_destroy(qxl);
 } else {
 spice_warning("VD_INTERFACE_REMOVING unsupported");
 return -1;
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 6/7] Free properly primary surface during replay

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
---
 server/red-replay-qxl.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 2176068..aeaa545 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -48,6 +48,7 @@ struct SpiceReplay {
 GArray *id_map; // record id -> replay id
 GArray *id_map_inv; // replay id -> record id
 GArray *id_free; // free list
+uint8_t *primary_mem;
 int nsurfaces;
 int end_pos;
 
@@ -1254,6 +1255,8 @@ static void replay_handle_create_primary(QXLWorker 
*worker, SpiceReplay *replay)
 }
 read_binary(replay, "data", , , 0);
 surface.group_id = 0;
+free(replay->primary_mem);
+replay->primary_mem = mem;
 surface.mem = QXLPHYSICAL_FROM_PTR(mem);
 worker->create_primary_surface(worker, 0, );
 }
@@ -1269,6 +1272,8 @@ static void replay_handle_dev_input(QXLWorker *worker, 
SpiceReplay *replay,
 case RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE:
 replay->created_primary = FALSE;
 worker->destroy_primary_surface(worker, 0);
+free(replay->primary_mem);
+replay->primary_mem = NULL;
 break;
 case RED_WORKER_MESSAGE_DESTROY_SURFACES:
 replay->created_primary = FALSE;
@@ -1454,6 +1459,7 @@ SPICE_GNUC_VISIBLE void spice_replay_free(SpiceReplay 
*replay)
 g_array_free(replay->id_map, TRUE);
 g_array_free(replay->id_map_inv, TRUE);
 g_array_free(replay->id_free, TRUE);
+free(replay->primary_mem);
 fclose(replay->fd);
 free(replay);
 }
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 2/7] Rearrange object destruction

[Frediano Ziglio]

Try to arrange destruction in the opposite order of the creation

Signed-off-by: Frediano Ziglio 
---
 server/display-channel.c |  4 ++--
 server/reds.c| 24 ++--
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index 52f0a3d..cd1334d 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -77,10 +77,10 @@ display_channel_finalize(GObject *object)
 {
 DisplayChannel *self = DISPLAY_CHANNEL(object);
 
-G_OBJECT_CLASS(display_channel_parent_class)->finalize(object);
-
 g_array_unref(self->priv->video_codecs);
 g_free(self->priv);
+
+G_OBJECT_CLASS(display_channel_parent_class)->finalize(object);
 }
 
 static void drawable_draw(DisplayChannel *display, Drawable *drawable);
diff --git a/server/reds.c b/server/reds.c
index 75f90c5..17e5ada 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3586,25 +3586,29 @@ SPICE_GNUC_VISIBLE int spice_server_init(SpiceServer 
*reds, SpiceCoreInterface *
 
 SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer *reds)
 {
-g_array_unref(reds->config->renderers);
-g_array_unref(reds->config->video_codecs);
-free(reds->config);
-if (reds->main_channel) {
-red_channel_destroy(RED_CHANNEL(reds->main_channel));
-}
-reds_cleanup(reds);
-
-reds_core_timer_remove(reds, reds->mig_timer);
-
 /* remove the server from the list of servers so that we don't attempt to
  * free it again at exit */
 pthread_mutex_lock(_reds_lock);
 servers = g_list_remove(servers, reds);
 pthread_mutex_unlock(_reds_lock);
 
+if (reds->inputs_channel) {
+reds_unregister_channel(reds, RED_CHANNEL(reds->inputs_channel));
+red_channel_destroy(RED_CHANNEL(reds->inputs_channel));
+}
+if (reds->main_channel) {
+red_channel_destroy(RED_CHANNEL(reds->main_channel));
+}
+reds_core_timer_remove(reds, reds->mig_timer);
+reds_cleanup(reds);
 #ifdef RED_STATISTICS
 stat_file_free(reds->stat_file);
 #endif
+
+g_array_unref(reds->config->renderers);
+g_array_unref(reds->config->video_codecs);
+free(reds->config);
+
 free(reds);
 }
 
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 0/7] Start cleaning objects on destruction

[Frediano Ziglio]

These patches start destroying objects upon destruction.
Lot of the objects are just allocated but never freed.
This however make hardier to understand automatically leaks
as objects are still linked together.
Currently spice_server_destroy is not called so objects
are freed only when program ends by the operating system.
These patches allows using automated testing and some memory
leak detectors (Valgrind, address sanitizer or others) to be
used to discover possible leaks.

Changes since v2:
- merged different patches;
- move "Support QXL remove on spice_server_remove_interface"
  at the top;
- added some comments.

Changes since v1:
- removed a merged patch;
- more comments on "red_worker: add RED_WORKER_MESSAGE_CLOSE_WORKER
  message";
- avoid a crash if main_dispatcher is not initialized.

Frediano Ziglio (7):
  Add red_qxl_destroy function
  Rearrange object destruction
  Free QXL instances in spice_server_destroy
  replay: Free spice server to detect leaks
  Free replay queues
  Free properly primary surface during replay
  Support QXL remove on spice_server_remove_interface

 server/display-channel.c |  4 ++--
 server/red-qxl.c | 25 ++---
 server/red-qxl.h |  1 +
 server/red-replay-qxl.c  |  6 ++
 server/red-worker.c  | 35 ++-
 server/red-worker.h  |  1 +
 server/reds.c| 38 --
 server/tests/replay.c| 21 ++---
 8 files changed, 112 insertions(+), 19 deletions(-)

base-commit: 1f800090ef671e9fccb49ed64c01a85b6eb0890a
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 3/7] Free QXL instances in spice_server_destroy

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/server/reds.c b/server/reds.c
index 17e5ada..d350331 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3592,6 +3592,8 @@ SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer 
*reds)
 servers = g_list_remove(servers, reds);
 pthread_mutex_unlock(_reds_lock);
 
+g_list_free_full(reds->qxl_instances, (GDestroyNotify)red_qxl_destroy);
+
 if (reds->inputs_channel) {
 reds_unregister_channel(reds, RED_CHANNEL(reds->inputs_channel));
 red_channel_destroy(RED_CHANNEL(reds->inputs_channel));
@@ -3600,6 +3602,11 @@ SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer 
*reds)
 red_channel_destroy(RED_CHANNEL(reds->main_channel));
 }
 reds_core_timer_remove(reds, reds->mig_timer);
+
+if (reds->main_dispatcher) {
+g_object_unref(reds->main_dispatcher);
+}
+
 reds_cleanup(reds);
 #ifdef RED_STATISTICS
 stat_file_free(reds->stat_file);
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 5/7] Free replay queues

[Frediano Ziglio]

There could be still some data pending.

Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
---
 server/tests/replay.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/server/tests/replay.c b/server/tests/replay.c
index 23d4125..8ec65d6 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -302,6 +302,21 @@ static gboolean progress_timer(gpointer user_data)
 return TRUE;
 }
 
+static void free_queue(GAsyncQueue *queue)
+{
+for (;;) {
+QXLCommandExt *cmd = g_async_queue_try_pop(queue);
+if (cmd == GINT_TO_POINTER(-1)) {
+continue;
+}
+if (!cmd) {
+break;
+}
+spice_replay_free_cmd(replay, cmd);
+}
+g_async_queue_unref(queue);
+}
+
 int main(int argc, char **argv)
 {
 GError *error = NULL;
@@ -440,9 +455,9 @@ int main(int argc, char **argv)
 g_print("Counted %d commands\n", ncommands);
 
 spice_server_destroy(server);
+free_queue(display_queue);
+free_queue(cursor_queue);
 end_replay();
-g_async_queue_unref(display_queue);
-g_async_queue_unref(cursor_queue);
 
 /* FIXME: there should be a way to join server threads before:
  * g_main_loop_unref(loop);
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 4/7] replay: Free spice server to detect leaks

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
---
 server/tests/replay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/tests/replay.c b/server/tests/replay.c
index f32fa2f..23d4125 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -221,7 +221,6 @@ static void end_replay(void)
 kill(client_pid, SIGINT);
 waitpid(client_pid, _status, 0);
 }
-exit(0);
 }
 
 static void release_resource(QXLInstance *qin, struct QXLReleaseInfoExt 
release_info)
@@ -440,6 +439,7 @@ int main(int argc, char **argv)
 if (print_count)
 g_print("Counted %d commands\n", ncommands);
 
+spice_server_destroy(server);
 end_replay();
 g_async_queue_unref(display_queue);
 g_async_queue_unref(cursor_queue);
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 12/19] Support QXL remove on spice_server_remove_interface

[Frediano Ziglio]

Taking into account that in a following patch the object is destroyed
automatically with spice_server_destroy this patch get less important.

Currently QXL interfaces are added when physical card are added and
QXL cards don't support (in Qemu) hot swap so there is no reason to
have this feature. Unless we want to support hot swap. I'll add some
comments and move the commit.
This can't cause unexpected side effects as is currently dead code.

Frediano

> 
> This most likely deserves a longer commit log explaining why we want
> that (and maybe why it's not going to cause unexpected side effects in
> existing code).
> 
> Christophe
> 
> On Fri, Nov 25, 2016 at 02:52:42PM +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/reds.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index bc0cc01..05afb7c 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3321,6 +3321,13 @@ SPICE_GNUC_VISIBLE int
> > spice_server_remove_interface(SpiceBaseInstance *sin)
> >  SpiceCharDeviceInstance *char_device = SPICE_CONTAINEROF(sin,
> >  SpiceCharDeviceInstance, base);
> >  reds = red_char_device_get_server(char_device->st);
> >  spice_server_char_device_remove_interface(reds, sin);
> > +} else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
> > +QXLInstance *qxl;
> > +
> > +qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
> > +reds = red_qxl_get_server(qxl->st);
> > +reds->qxl_instances = g_list_remove(reds->qxl_instances, qxl);
> > +red_qxl_destroy(qxl);
> >  } else {
> >  spice_warning("VD_INTERFACE_REMOVING unsupported");
> >  return -1;
> > --
> > git-series 0.9.1
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 12/19] Support QXL remove on spice_server_remove_interface

[Frediano Ziglio]

> 
> Could be called in replay ? Or is it called indirectly ?
> 
> Pavel
> 

Yes and not. While I was writing this series spice-server-replay
was using it. However now this is done automatically by
"Free QXL instances in spice_server_destroy" so replay utility
has no reason to free the interface.

Frediano

> On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> > ---
> >  server/reds.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index bc0cc01..05afb7c 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3321,6 +3321,13 @@ SPICE_GNUC_VISIBLE int
> > spice_server_remove_interface(SpiceBaseInstance *sin)
> >  SpiceCharDeviceInstance *char_device =
> > SPICE_CONTAINEROF(sin, SpiceCharDeviceInstance, base);
> >  reds = red_char_device_get_server(char_device->st);
> >  spice_server_char_device_remove_interface(reds, sin);
> > +} else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
> > +QXLInstance *qxl;
> > +
> > +qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
> > +reds = red_qxl_get_server(qxl->st);
> > +reds->qxl_instances = g_list_remove(reds->qxl_instances,
> > qxl);
> > +red_qxl_destroy(qxl);
> >  } else {
> >  spice_warning("VD_INTERFACE_REMOVING unsupported");
> >  return -1;
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [Question] How to evaluate the spice performance?

[Frediano Ziglio]

> 
> Hi guys
> 
> Are there some tools to evaluate the spice client performance.
> 
> 

There are so many stuff to take into account that there aren't
any official tool available. It depends on what you want to
measure really. CPU ? Memory ? Network bandwidth ?

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 14/19] Free QXL instances in spice_server_destroy

[Christophe Fergeau]

On Wed, Nov 30, 2016 at 06:40:26PM +0100, Pavel Grunt wrote:
> On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/reds.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 701aed6..553e7a8 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3599,6 +3599,8 @@ SPICE_GNUC_VISIBLE void
> > spice_server_destroy(SpiceServer *reds)
> >  servers = g_list_remove(servers, reds);
> >  pthread_mutex_unlock(_reds_lock);
> >  
> > +g_list_free_full(reds->qxl_instances,
> > (GDestroyNotify)red_qxl_destroy);
> 
> Ah g_list_free_full is GLIB 2.28, we require 2.22
> 
> There is a few of them already, imo is ok to bump the required version
> to 2.28 since it is in el6 

Yes, definitely.

Christophe

> > +
> >  if (reds->inputs_channel) {
> >  reds_unregister_channel(reds, RED_CHANNEL(reds-
> > >inputs_channel));
> >  red_channel_destroy(RED_CHANNEL(reds->inputs_channel));
> > @@ -3609,6 +3611,11 @@ SPICE_GNUC_VISIBLE void
> > spice_server_destroy(SpiceServer *reds)
> >  if (reds->mig_timer) {
> >  reds_core_timer_remove(reds, reds->mig_timer);
> >  }
> > +
> > +if (reds->main_dispatcher) {
> > +g_object_unref(reds->main_dispatcher);
> > +}
> > +
> >  reds_cleanup(reds);
> >  #ifdef RED_STATISTICS
> >  stat_file_free(reds->stat_file);
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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] udscs: Fix a potential NULL pointer dereference

[Christophe Fergeau]

On Thu, Dec 01, 2016 at 05:19:33AM +0100, Francois Gouget wrote:
> udscs_server_fill_fds() should accept being passed a NULL pointer.

I would reword the commit log a bit, like "udscs_server_fill_fds() is
dereferencing the 'server' pointer, and then checks if it's NULL. This
commit makes sure the NULL check happens first"
I'll amend and push, thanks for the patch!

Acked-by: Christophe Fergeau 


> 
> Signed-off-by: Francois Gouget 
> ---
>  src/udscs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/udscs.c b/src/udscs.c
> index 414dce5..fdd75a4 100644
> --- a/src/udscs.c
> +++ b/src/udscs.c
> @@ -495,11 +495,12 @@ int udscs_server_fill_fds(struct udscs_server *server, 
> fd_set *readfds,
>  fd_set *writefds)
>  {
>  struct udscs_connection *conn;
> -int nfds = server->fd + 1;
> +int nfds;
>  
>  if (!server)
>  return -1;
>  
> +nfds = server->fd + 1;
>  FD_SET(server->fd, readfds);
>  
>  conn = server->connections_head.next;
> -- 
> 2.10.2
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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