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

[Jonathon Jongsma]

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>


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);
>          }
>      }
>