[Spice-devel] [PATCH spice-gtk v2 9/8] main: don't update display timer for unchanged config

[Jonathon Jongsma]

Yeah, as I mentioned in an email just a few minutes ago, I do still have the
same concern, but it's a minor one and arguably a very rare corner case. So
probably it shouldn't hold up this patch. 

On Wed, 2016-03-23 at 20:39 +0100, Pavel Grunt wrote:
> Hi,
> 
> So the timer is the main problem ?
> 
> It also solves/avoids problems with a guest running on wayland when the
> "resize-guest" property is TRUE.
> See bug https://bugzilla.redhat.com/show_bug.cgi?id=1266484 ;
> It avoids destroying the primary surface when the display configuration
> has not changed

hmm.  so, the suggestion from my previous email (only compare with previous
config and return early from update_display() if there's currently a pending
config message scheduled) would probably break this case again (if I understand
the situation correctly). Do you know *why* we're repeatedly updating to the
same monitor config under wayland?

> 
> I would like to hear from Jonathon what he thinks about it. There were
> some concerns / discussion whether it should be compared to the "real"
> state of displays in the guest.
> https://lists.freedesktop.org/archives/spice-devel/2015-October/022785.
> html
> 
> Maybe some check should be done on the server side because it is
> "closer" to the guest.

I was not advocating doing anything on the server side. What I had in mind was
to simply have spice-gtk cache the last received monitor config from the server.
And use this cached value to determine whether the new config is the same as the
current setting.


> 
> Pavel
> 
> On Wed, 2016-03-23 at 18:02 +0100, Marc-André Lureau wrote:
> > With virgl, set_monitor_ready() may be called each time the scanout
> > is
> > updated to set the monitor area. This will call
> > spice_main_update_display(), and keep the timer postponed even if the
> > monitor configuration didn't change. Treat unchanged configuration
> > has a
> > no-op and keep configuration timer unchanged. This fixes monitor
> > autoconfig with virgl (when the display is regularly updated).
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> > ---
> >  src/channel-main.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 1c19de1..b4875f6 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -121,6 +121,14 @@ typedef enum {
> >      DISPLAY_ENABLED,
> >  } SpiceDisplayState;
> >  
> > +typedef struct {
> > +    int                     x;
> > +    int                     y;
> > +    int                     width;
> > +    int                     height;
> > +    SpiceDisplayState       display_state;
> > +} SpiceDisplayConfig;
> > +
> >  struct _SpiceMainChannelPrivate  {
> >      enum SpiceMouseMode         mouse_mode;
> >      bool                        agent_connected;
> > @@ -140,13 +148,7 @@ struct _SpiceMainChannelPrivate  {
> >      guint                       agent_msg_pos;
> >      uint8_t                     agent_msg_size;
> >      uint32_t                    agent_caps[VD_AGENT_CAPS_SIZE];
> > -    struct {
> > -        int                     x;
> > -        int                     y;
> > -        int                     width;
> > -        int                     height;
> > -        SpiceDisplayState       display_state;
> > -    } display[MAX_DISPLAY];
> > +    SpiceDisplayConfig          display[MAX_DISPLAY];
> >      gint                        timer_id;
> >      GQueue                      *agent_msg_queue;
> >      GHashTable                  *file_xfer_tasks;
> > @@ -2686,10 +2688,15 @@ void
> > spice_main_update_display(SpiceMainChannel *channel, int id,
> >  
> >      g_return_if_fail(id < SPICE_N_ELEMENTS(c->display));
> >  
> > -    c->display[id].x      = x;
> > -    c->display[id].y      = y;
> > -    c->display[id].width  = width;
> > -    c->display[id].height = height;
> > +    SpiceDisplayConfig display = {
> > +        .x = x, .y = y, .width = width, .height = height,
> > +        .display_state = c->display[id].display_state
> > +    };
> > +
> > +    if (memcmp(&display, &c->display[id],
> > sizeof(SpiceDisplayConfig)) == 0)
> > +        return;
> > +
> > +    c->display[id] = display;
> >  
> >      if (update)
> >          update_display_timer(channel, 1);