[Spice-devel] [PATCH v5 7/9] Remove dependency from dcc-encoders to Drawable

[Jonathon Jongsma]

On Wed, 2016-06-15 at 10:37 +0100, Frediano Ziglio wrote:
> Encoding image requires a RedDrawable (where the data is stored) and
> a Ring where to store information to free Glz structures.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/dcc-encoders.c | 17 ++++++++++-------
>  server/dcc-encoders.h |  3 ++-
>  server/dcc.c          |  4 +++-
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> index c690746..fda3b83 100644
> --- a/server/dcc-encoders.c
> +++ b/server/dcc-encoders.c
> @@ -22,7 +22,9 @@
>  #include <glib.h>
>  
>  #include "dcc-encoders.h"
> -#include "display-channel.h"
> +#include "spice-bitmap-utils.h"
> +#include "red-worker.h" // red_drawable_unref
> +#include "pixmap-cache.h" // MAX_CACHE_CLIENTS
>  
>  #define ZLIB_DEFAULT_COMPRESSION_LEVEL 3
>  
> @@ -1145,7 +1147,7 @@ int image_encoders_compress_lz4(ImageEncoders *enc,
> SpiceImage *dest,
>  
>  /* if already exists, returns it. Otherwise allocates and adds it (1) to the
> ring tail
>     in the channel (2) to the Drawable*/
> -static RedGlzDrawable *get_glz_drawable(ImageEncoders *enc, Drawable
> *drawable)
> +static RedGlzDrawable *get_glz_drawable(ImageEncoders *enc, RedDrawable
> *red_drawable, Ring *drawable_ring)

When this function signature changes, it becomes a very odd function. "Here's an
encoder and a list of glzdrawables, if the list I gave you has an element that
matches the encoder, give it to me". 

>  {
>      RedGlzDrawable *ret;
>      RingItem *item, *next;
> @@ -1153,7 +1155,7 @@ static RedGlzDrawable *get_glz_drawable(ImageEncoders
> *enc, Drawable *drawable)
>      // TODO - I don't really understand what's going on here, so doing the
> technical equivalent
>      // now that we have multiple glz_dicts, so the only way to go from dcc to
> drawable glz is to go
>      // over the glz_ring (unless adding some better data structure then a
> ring)
> -    DRAWABLE_FOREACH_GLZ_SAFE(drawable, item, next, ret) {
> +    SAFE_FOREACH(item, next, TRUE, drawable_ring, ret, LINK_TO_GLZ(item)) {
>          if (ret->encoders == enc) {
>              return ret;
>          }
> @@ -1162,7 +1164,7 @@ static RedGlzDrawable *get_glz_drawable(ImageEncoders
> *enc, Drawable *drawable)
>      ret = spice_new(RedGlzDrawable, 1);
>  
>      ret->encoders = enc;
> -    ret->red_drawable = red_drawable_ref(drawable->red_drawable);
> +    ret->red_drawable = red_drawable_ref(red_drawable);
>      ret->has_drawable = TRUE;
>      ret->instances_count = 0;
>      ring_init(&ret->instances);
> @@ -1170,7 +1172,7 @@ static RedGlzDrawable *get_glz_drawable(ImageEncoders
> *enc, Drawable *drawable)
>      ring_item_init(&ret->link);
>      ring_item_init(&ret->drawable_link);
>      ring_add_before(&ret->link, &enc->glz_drawables);
> -    ring_add(&drawable->glz_ring, &ret->drawable_link);
> +    ring_add(drawable_ring, &ret->drawable_link);
>      enc->shared_data->glz_drawable_count++;
>      return ret;
>  }
> @@ -1196,7 +1198,8 @@ static GlzDrawableInstanceItem
> *add_glz_drawable_instance(RedGlzDrawable *glz_dr
>  #define MIN_GLZ_SIZE_FOR_ZLIB 100
>  
>  int image_encoders_compress_glz(ImageEncoders *enc,
> -                                SpiceImage *dest, SpiceBitmap *src, Drawable
> *drawable,
> +                                SpiceImage *dest, SpiceBitmap *src,
> +                                RedDrawable *red_drawable, Ring
> *drawable_ring,
>                                  compress_send_data_t* o_comp_data,
>                                  gboolean enable_zlib_glz_wrap)
>  {

This function also becomes less straightforward and more unwieldy, I think. I
understand that it allows us to encapsulate things better, but I think that it
does it at the expense of readability of the code. At the very minimum,
drawable_ring should be renamed. Due to the fact that we don't have strongly-
typed containers in C, we can't tell what Ring contains. Since there are many
types of "drawable" classes, "drawable_ring" doesn't give us enough information.

> @@ -1228,7 +1231,7 @@ int image_encoders_compress_glz(ImageEncoders *enc,
>  
>      encoder_data_init(&glz_data->data);
>  
> -    glz_drawable = get_glz_drawable(enc, drawable);
> +    glz_drawable = get_glz_drawable(enc, red_drawable, drawable_ring);
>      glz_drawable_instance = add_glz_drawable_instance(glz_drawable);
>  
>      glz_data->data.u.lines_data.chunks = src->data;
> diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> index 66a7daa..03230bb 100644
> --- a/server/dcc-encoders.h
> +++ b/server/dcc-encoders.h
> @@ -195,7 +195,8 @@ int image_encoders_compress_jpeg(ImageEncoders *enc,
> SpiceImage *dest,
>  int image_encoders_compress_lz4(ImageEncoders *enc, SpiceImage *dest,
>                                  SpiceBitmap *src, compress_send_data_t*
> o_comp_data);
>  int image_encoders_compress_glz(ImageEncoders *enc,
> -                                SpiceImage *dest, SpiceBitmap *src, struct
> Drawable *drawable,
> +                                SpiceImage *dest, SpiceBitmap *src,
> +                                RedDrawable *red_drawable, Ring
> *drawable_ring,
>                                  compress_send_data_t* o_comp_data,
>                                  gboolean enable_zlib_glz_wrap);
>  
> diff --git a/server/dcc.c b/server/dcc.c
> index fc601e9..39a88eb 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -735,7 +735,9 @@ int dcc_compress_image(DisplayChannelClient *dcc,
>          success = image_encoders_compress_quic(&dcc->encoders, dest, src,
> o_comp_data);
>          break;
>      case SPICE_IMAGE_COMPRESSION_GLZ:
> -        success = image_encoders_compress_glz(&dcc->encoders, dest, src,
> drawable, o_comp_data,
> +        success = image_encoders_compress_glz(&dcc->encoders, dest, src,
> +                                              drawable->red_drawable,
> &drawable->glz_ring,
> +                                              o_comp_data,
>                                                display_channel-
> >enable_zlib_glz_wrap);
>          if (success) {
>              break;

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