[Spice-devel] [PATCH 01/30] Do not release too much drawables

[Frediano Ziglio]

> 
> I have to admit that I'm not very familiar with this bit of the code. But
> it's
> clearly a behavior change and it seems like one that could have non-obvious
> or
> hard-to-test consequences. Can you give a bit more justification for the
> change?
> Does it fix an observed issue? It seems that these glz drawables are specific
> to
> each client, so it doesn't seem "wrong" to free the same number for each
> client.
> 
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> 

I'm more confident but after some consideration I think the code is
wrong with and without the patch, just not problem happens as there
is only one possible client so the result does not change.

The limit counter is expressed in number of Drawables while the
function I patched uses number of RedGlzDrawable!

I should write some document about these Glz stuff, I think I'll
move the patch, not really specific to this patchset.

Frediano

> 
> On Tue, 2016-06-07 at 11:17 +0100, Frediano Ziglio wrote:
> > Accumulate counter, do not reset for each client.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/dcc-encoders.c    | 5 ++---
> >  server/dcc-encoders.h    | 3 ++-
> >  server/display-channel.c | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> > index 5570798..cc235fa 100644
> > --- a/server/dcc-encoders.c
> > +++ b/server/dcc-encoders.c
> > @@ -532,13 +532,12 @@ void dcc_free_glz_drawable(DisplayChannelClient *dcc,
> > RedGlzDrawable *drawable)
> >   * Drawable (their qxl drawables are released too).
> >   * NOTE - the caller should prevent encoding using the dictionary during
> >   the
> > operation
> >   */
> > -int dcc_free_some_independent_glz_drawables(DisplayChannelClient *dcc)
> > +int dcc_free_some_independent_glz_drawables(DisplayChannelClient *dcc, int
> > n)
> >  {
> >      RingItem *ring_link;
> > -    int n = 0;
> >  
> >      if (!dcc) {
> > -        return 0;
> > +        return n;
> >      }
> >      ring_link = ring_get_head(&dcc->glz_drawables);
> >      while ((n < RED_RELEASE_BUNCH_SIZE) && (ring_link != NULL)) {
> > diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> > index 0d3e96a9..fed8d58 100644
> > --- a/server/dcc-encoders.h
> > +++ b/server/dcc-encoders.h
> > @@ -40,7 +40,8 @@
> > void             dcc_encoders_init
> >                            (DisplayChannelCl
> > ie
> >  void             dcc_encoders_free
> >                             (DisplayChannelC
> > lient *dcc);
> >  void             dcc_free_glz_drawable
> >                         (DisplayChannelC
> > lient *dcc,
> >                                                                RedGlzDrawable
> > *drawable);
> > -int              dcc_free_some_independent_glz_drawables
> >      (DisplayChannelC
> > lient *dcc);
> > +int              dcc_free_some_independent_glz_drawables
> >      (DisplayChannelC
> > lient *dcc,
> > +                                                              int
> > release_count);
> >  void             dcc_free_glz_drawables
> >                        (DisplayChannelC
> > lient *dcc);
> >  void             dcc_free_glz_drawables_to_free
> >                (DisplayChannelC
> > lient* dcc);
> >  void             dcc_freeze_glz
> >                                (DisplayChannelC
> > lient *dcc);
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 2888cad..db059b4 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1267,7 +1267,7 @@ void display_channel_free_some(DisplayChannel
> > *display)
> >              // encoding using the dictionary is prevented since the
> >              following
> > operations might
> >              // change the dictionary
> >              pthread_rwlock_wrlock(&glz_dict->encode_lock);
> > -            n = dcc_free_some_independent_glz_drawables(dcc);
> > +            n = dcc_free_some_independent_glz_drawables(dcc, n);
> >          }
> >      }
> >  
>