[Spice-devel] [PATCH v5 7/9] Remove dependency from dcc-encoders to Drawable
Frediano Ziglio
fziglio at redhat.com
Thu Jun 16 09:26:22 UTC 2016
>
> 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".
>
Yes, I added a new "GlzImageRetention" abstraction.
Glz retention is really complicated... I started a branch time ago trying
to understand how it works and make it simpler. I though I had understand
and I get much easier code however at the end crashed very reliably and
I understood I didn't understand all the various aspects.
However I didn't manage to finish and there patches are not working
(the branch is quite obsolete now).
> > {
> > 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.
>
Yes, now is going to be a "glz_retention".
> > @@ -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>
>
Hope that with everything encapsulated will be easier to make Glz
easier and possibly even faster.
Glz is currently potentially one of the most lossless efficient
compression algorithms as can compress images based on a large
dictionary with images from all cards.
Frediano
More information about the Spice-devel
mailing list