[Spice-devel] [PATCH spice-server] red-channel-client: Do not push items adding them to tail

[Frediano Ziglio]

> 
> On Wed, Sep 13, 2017 at 10:15:14AM +0100, Frediano Ziglio wrote:
> > Now the push is done automatically when a PipeItem is added
> > (cfr commit 5c460de1a3972b7cf2b9b2944d0b500c3affc363
> > "worker: push data when clients can receive them"),
> > forcing a push cause only network fragmentation and is required only if
> > you are handling data in a polling loop (and thus, you are preventing
> > the default event loop from running).
> 
> I'd reword the shortlog to just "channel-client: Remove
> red_channel_client_pipe_add_tail_push"
> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 
> Should something similar be done with
> red_channel_client_pipe_add_push()?
> 
> Christophe
> 

I have a patch like this for red_channel_client_pipe_add_push.
Is pretty straight forward and easy and seems to work as expected...
except for sound. In sound.c I have a comment
"just append a dummy item and push!" and I remember when I wrote
the code (and the comment) that push was important but I don't
remember 100% why was important. But removing the push from that
point (not that is hard to add a red_channel_client_push after
calling a red_channel_client_pipe_add which is in my current patch)
cause sometimes some tickling on the sound. Not easy to get them,
I have to listen to some music for some minutes and is hard to
tell the difference but I tried different times and it seems to
be a real issue. We (humans) can hear ticking in the sound if they
are in the order of very few (1 or 2) milliseconds (we need more
to see delays in video, in the order of 15 milliseconds).
Not pushing can introduce a small delay due to a possible additional
loop (in my tests this is in the order of 5-10 microseconds) but
is quite far the order of magnitude require to hear the ticking in
the sound.
The way sound.c works is a bit different form other channels.
Instead of appending messages to the pipe some bits are marked
in a field specifying the kind of command to send, then a dummy
item is inserted to trigger the sending and a push is forced.
Another difference is the configuration of the socket which
is set to minimize the latency (snd_channel_client_config_socket).

Frediano

> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/cache-item.tmpl.c    | 2 +-
> >  server/dcc.c                | 2 +-
> >  server/red-channel-client.c | 9 ---------
> >  server/red-channel-client.h | 1 -
> >  4 files changed, 2 insertions(+), 12 deletions(-)
> > 
> > diff --git a/server/cache-item.tmpl.c b/server/cache-item.tmpl.c
> > index 47de423bf..19e6b95f1 100644
> > --- a/server/cache-item.tmpl.c
> > +++ b/server/cache-item.tmpl.c
> > @@ -78,7 +78,7 @@ static void FUNC_NAME(remove)(CHANNELCLIENT
> > *channel_client, RedCacheItem *item)
> >      channel_client->priv->VAR_NAME(available) += item->u.cache_data.size;
> >  
> >      red_pipe_item_init(&item->u.pipe_data, RED_PIPE_ITEM_TYPE_INVAL_ONE);
> > -
> > red_channel_client_pipe_add_tail_and_push(RED_CHANNEL_CLIENT(channel_client),
> > &item->u.pipe_data); // for now
> > +    red_channel_client_pipe_add_tail(RED_CHANNEL_CLIENT(channel_client),
> > &item->u.pipe_data); // for now
> >  }
> >  
> >  static int FUNC_NAME(add)(CHANNELCLIENT *channel_client, uint64_t id,
> >  size_t size)
> > diff --git a/server/dcc.c b/server/dcc.c
> > index 1e0ba790f..3bf75a707 100644
> > --- a/server/dcc.c
> > +++ b/server/dcc.c
> > @@ -470,7 +470,7 @@ void dcc_append_drawable(DisplayChannelClient *dcc,
> > Drawable *drawable)
> >      RedDrawablePipeItem *dpi = red_drawable_pipe_item_new(dcc, drawable);
> >  
> >      add_drawable_surface_images(dcc, drawable);
> > -    red_channel_client_pipe_add_tail_and_push(RED_CHANNEL_CLIENT(dcc),
> > &dpi->dpi_pipe_item);
> > +    red_channel_client_pipe_add_tail(RED_CHANNEL_CLIENT(dcc),
> > &dpi->dpi_pipe_item);
> >  }
> >  
> >  void dcc_add_drawable_after(DisplayChannelClient *dcc, Drawable *drawable,
> >  RedPipeItem *pos)
> > diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> > index 8f7308628..0443d6184 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -1596,15 +1596,6 @@ void
> > red_channel_client_pipe_add_tail(RedChannelClient *rcc,
> >      g_queue_push_tail(&rcc->priv->pipe, item);
> >  }
> >  
> > -void red_channel_client_pipe_add_tail_and_push(RedChannelClient *rcc,
> > RedPipeItem *item)
> > -{
> > -    if (!prepare_pipe_add(rcc, item)) {
> > -        return;
> > -    }
> > -    g_queue_push_tail(&rcc->priv->pipe, item);
> > -    red_channel_client_push(rcc);
> > -}
> > -
> >  void red_channel_client_pipe_add_type(RedChannelClient *rcc, int
> >  pipe_item_type)
> >  {
> >      RedPipeItem *item = spice_new(RedPipeItem, 1);
> > diff --git a/server/red-channel-client.h b/server/red-channel-client.h
> > index 732fbdd59..56503c44b 100644
> > --- a/server/red-channel-client.h
> > +++ b/server/red-channel-client.h
> > @@ -96,7 +96,6 @@ int
> > red_channel_client_pipe_item_is_linked(RedChannelClient *rcc, RedPipeItem
> > *i
> >  void red_channel_client_pipe_remove_and_release(RedChannelClient *rcc,
> >  RedPipeItem *item);
> >  void red_channel_client_pipe_remove_and_release_pos(RedChannelClient *rcc,
> >  GList *item_pos);
> >  void red_channel_client_pipe_add_tail(RedChannelClient *rcc, RedPipeItem
> >  *item);
> > -void red_channel_client_pipe_add_tail_and_push(RedChannelClient *rcc,
> > RedPipeItem *item);
> >  /* for types that use this routine -> the pipe item should be freed */
> >  void red_channel_client_pipe_add_type(RedChannelClient *rcc, int
> >  pipe_item_type);
> >  RedPipeItem *red_channel_client_new_empty_msg(int msg_type);