[Spice-devel] [PATCH v2 2/2] display-channel: Handle timeout for joining drawables

[Jonathon Jongsma]

I'm trying to decide if there's any benefit to splitting this out from
the other patch. I'm leaning toward squashing them.

On Thu, 2017-03-02 at 11:34 +0000, Frediano Ziglio wrote:
> The previous patch join correctly the commands however if there
> are no more commands the command joined is delayed till new
> commands arrive. This patch introduce a timeout (currently 10 ms)
> after the command is executed.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/display-channel-private.h |  4 ++++-
>  server/display-channel.c         | 36 ++++++++++++++++++++++++----
> -----
>  2 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/server/display-channel-private.h b/server/display-
> channel-private.h
> index 62e03b6..e289e45 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -20,6 +20,8 @@
>  
>  #include "display-channel.h"
>  
> +#define JOINABLE_TIMEOUT 10 // ms
> +
>  #define NUM_DRAWABLES 1000
>  typedef struct _Drawable _Drawable;
>  struct _Drawable {
> @@ -69,6 +71,8 @@ struct DisplayChannelPrivate
>  
>      int gl_draw_async_count;
>  
> +    uint32_t joinable_generation;
> +    SpiceTimer *joinable_timeout;
>      RedDrawable *joinable_drawable;
>  
>  /* TODO: some day unify this, make it more runtime.. */
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 37d1703..8993b12 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -78,6 +78,9 @@ display_channel_finalize(GObject *object)
>  {
>      DisplayChannel *self = DISPLAY_CHANNEL(object);
>  
> +    SpiceCoreInterfaceInternal* core =
> red_channel_get_core_interface(RED_CHANNEL(self));
> +    core->timer_remove(core, self->priv->joinable_timeout);
> +
>      display_channel_destroy_surfaces(self);
>      image_cache_reset(&self->priv->image_cache);
>      monitors_config_unref(self->priv->monitors_config);
> @@ -1330,17 +1333,23 @@
> display_channel_process_draw_single(DisplayChannel *display,
> RedDrawable *red_dr
>      drawable_unref(drawable);
>  }
>  
> +static void display_joinable_timeout(void *opaque)
> +{
> +    DisplayChannel *display = opaque;
> +    if (display->priv->joinable_drawable) {
> +        display_channel_process_draw_single(display, display->priv-
> >joinable_drawable,
> +                                            display->priv-
> >joinable_generation);
> +        red_drawable_unref(display->priv->joinable_drawable);
> +        display->priv->joinable_drawable = NULL;
> +    }
> +}
> +
>  void display_channel_process_draw(DisplayChannel *display,
> RedDrawable *red_drawable,
>                                    uint32_t
> process_commands_generation)
>  {
>      if (!red_drawable_joinable(red_drawable)) {
>          // not joinable, process all
> -        if (display->priv->joinable_drawable) {
> -            display_channel_process_draw_single(display, display-
> >priv->joinable_drawable,
> -                                                process_commands_gen
> eration);
> -            red_drawable_unref(display->priv->joinable_drawable);
> -            display->priv->joinable_drawable = NULL;
> -        }
> +        display_joinable_timeout(display);

Here I see that my comment on the previous patch (about
process_commands_generation) is no longer an issue because it's now
saved and used in display_joinable_timeout()

>          display_channel_process_draw_single(display, red_drawable,
> process_commands_generation);
>          return;
>      }
> @@ -1351,12 +1360,15 @@ void
> display_channel_process_draw(DisplayChannel *display, RedDrawable
> *red_draw
>          red_drawable = red_drawable_join(display->priv-
> >joinable_drawable, red_drawable);
>      } else {
>          // they can't be joined
> -        display_channel_process_draw_single(display, display->priv-
> >joinable_drawable,
> -                                            process_commands_generat
> ion);
> -        red_drawable_unref(display->priv->joinable_drawable);
> +        display_joinable_timeout(display);
>      }
> +
>      // try to join with next one
>      red_drawable_ref(red_drawable);
> +
> +    SpiceCoreInterfaceInternal* core =
> red_channel_get_core_interface(RED_CHANNEL(display));
> +    core->timer_start(core, display->priv->joinable_timeout,
> JOINABLE_TIMEOUT);
> +    display->priv->joinable_generation =
> process_commands_generation;
>      display->priv->joinable_drawable = red_drawable;
>  }
>  
> @@ -2227,6 +2239,12 @@ display_channel_constructed(GObject *object)
>      self->priv->stream_video = SPICE_STREAM_VIDEO_OFF;
>      display_channel_init_streams(self);
>  
> +    SpiceCoreInterfaceInternal* core =
> red_channel_get_core_interface(channel);
> +    self->priv->joinable_timeout = core->timer_add(core,
> display_joinable_timeout, self);
> +    if (!self->priv->joinable_timeout) {
> +        spice_error("joinable timer create failed");
> +    }
> +
>      red_channel_set_cap(channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
>      red_channel_set_cap(channel,
> SPICE_DISPLAY_CAP_PREF_COMPRESSION);
>      red_channel_set_cap(channel, SPICE_DISPLAY_CAP_STREAM_REPORT);


Aside from the comment about squashing, I think this one looks fine

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>