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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel