[Spice-devel] [PATCH v4 07/19] Encapsulate some data in dcc-encoders
Frediano Ziglio
fziglio at redhat.com
Wed Jun 15 09:43:44 UTC 2016
>
> On Tue, 2016-06-14 at 10:32 +0100, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> > server/dcc-encoders.c | 60
> > ++++++++++++++++++++++++++++++++++++++++++++++-
> > -
> > server/dcc-encoders.h | 33 ++------------------------
> > server/display-channel.c | 13 +++--------
> > server/display-channel.h | 5 ----
> > 4 files changed, 63 insertions(+), 48 deletions(-)
> >
> > diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> > index 4de780d..13dbff5 100644
> > --- a/server/dcc-encoders.c
> > +++ b/server/dcc-encoders.c
> > @@ -26,8 +26,45 @@
> >
> > #define ZLIB_DEFAULT_COMPRESSION_LEVEL 3
> >
> > +#define MAX_GLZ_DRAWABLE_INSTANCES 2
> > +
> > +typedef struct GlzDrawableInstanceItem GlzDrawableInstanceItem;
> > +
> > +/* for each qxl drawable, there may be several instances of lz drawables
> > */
> > +/* TODO - reuse this stuff for the top level. I just added a second level
> > of
> > multiplicity
> > + * at the Drawable by keeping a ring, so:
> > + * Drawable -> (ring of) RedGlzDrawable -> (up to 2)
> > GlzDrawableInstanceItem
> > + * and it should probably (but need to be sure...) be
> > + * Drawable -> ring of GlzDrawableInstanceItem.
> > + */
> > +struct GlzDrawableInstanceItem {
> > + RingItem glz_link;
> > + RingItem free_link;
> > + GlzEncDictImageContext *context;
> > + RedGlzDrawable *glz_drawable;
> > +};
> > +
> > +struct RedGlzDrawable {
> > + RingItem link; // ordered by the time it was encoded
> > + RingItem drawable_link;
> > + RedDrawable *red_drawable;
> > + struct Drawable *drawable;
> > + GlzDrawableInstanceItem instances_pool[MAX_GLZ_DRAWABLE_INSTANCES];
> > + Ring instances;
> > + uint8_t instances_count;
> > + ImageEncoders *encoders;
> > +};
> > +
> > +#define LINK_TO_GLZ(ptr) SPICE_CONTAINEROF((ptr), RedGlzDrawable, \
> > + drawable_link)
> > +#define DRAWABLE_FOREACH_GLZ_SAFE(drawable, link, next, glz) \
> > + SAFE_FOREACH(link, next, drawable, &(drawable)->glz_ring, glz,
> > LINK_TO_GLZ(link))
> > +
> > static void image_encoders_free_glz_drawable_instance(ImageEncoders *enc,
> > GlzDrawableInstanceItem
> > *instance);
> > +static void encoder_data_init(EncoderData *data);
> > +static void encoder_data_reset(EncoderData *data);
> > +
> >
> > static SPICE_GNUC_NORETURN SPICE_GNUC_PRINTF(2, 3) void
> > quic_usr_error(QuicUsrContext *usr, const char *fmt, ...)
> > @@ -139,14 +176,14 @@ static void glz_usr_free(GlzEncoderUsrContext *usr,
> > void
> > *ptr)
> > free(ptr);
> > }
> >
> > -void encoder_data_init(EncoderData *data)
> > +static void encoder_data_init(EncoderData *data)
> > {
> > data->bufs_tail = g_new(RedCompressBuf, 1);
> > data->bufs_head = data->bufs_tail;
> > data->bufs_head->send_next = NULL;
> > }
> >
> > -void encoder_data_reset(EncoderData *data)
> > +static void encoder_data_reset(EncoderData *data)
> > {
> > RedCompressBuf *buf = data->bufs_head;
> > while (buf) {
> > @@ -577,6 +614,25 @@ void image_encoders_free_glz_drawables(ImageEncoders
> > *enc)
> > pthread_rwlock_unlock(&glz_dict->encode_lock);
> > }
> >
> > +void image_encoders_glz_free_from_drawable(struct Drawable *drawable)
> > +{
> > + RingItem *glz_item, *next_item;
> > + RedGlzDrawable *glz;
> > + DRAWABLE_FOREACH_GLZ_SAFE(drawable, glz_item, next_item, glz) {
> > + image_encoders_free_glz_drawable(glz->encoders, glz);
> > + }
> > +}
>
> I find that the name of this function is not very clear. Basically, it is
> freeing and removing all of the RedGlzDrawables attached to the given
> Drawable.
> In my opinion, the fact that ImageEncoders is used internally should not
> necessarily be relevant to the name of the function. I think something like
> drawable_free_glz_drawables(Drawable*) would be a lot easier to understand.
>
Renamed.
> Digging a bit deeper, it seems that the only reason that
> image_encoders_free_glz_drawable needs ImageEncoders here is to update the
> glz
> dictionary and decrement glz_drawable_count. I'll send a couple additional
> patches that refactor this to remove the ImageEncoders parameter from this
> function.
>
Yes, glz by the way could be possible optimized. Having all code/declarations
in a single .c file should help.
> > +
> > +void image_encoders_glz_detach_from_drawable(struct Drawable *drawable)
> > +{
> > + RingItem *item, *next;
> > +
> > + RING_FOREACH_SAFE(item, next, &drawable->glz_ring) {
> > + SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link)->drawable =
> > NULL;
> > + ring_remove(item);
> > + }
> > +}
>
> Here again, I think that the image_encoders_ function prefix confuses
> things slightly. The ImageEncoders structure is not used at all in this
> function, even internally. So, I think a more fitting name might be something
> like drawable_detach_glz_drawables(Drawable*)...
>
> (Long-term, I think the 'Drawable' type badly needs to be renamed as there's
> just way too much confusion between Drawable, QXLDrawable, RedGlzDrawable,
> RedDrawable, etc. It's impossible for me to keep straight which struct is for
> what.)
>
If I remember you proposed to rename RedDrawable to RedDrawableCmd and
Drawable to RedDrawable. Would make sense.
About RedGlzDrawable as said before will be contained in a single c file
so the confusion will be less.
> Also, it's not clear why this function uses RING_FOREACH_SAFE() when the
> previous one uses DRAWABLE_FOREACH_GLZ_SAFE(). I realize that this was just
> existing code that you moved, but it's still a bit odd.
>
Don't know...
> > +
> > void image_encoders_freeze_glz(ImageEncoders *enc)
> > {
> > pthread_rwlock_wrlock(&enc->glz_dict->encode_lock);
> > diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> > index 655c1b3..68fd5a5 100644
> > --- a/server/dcc-encoders.h
> > +++ b/server/dcc-encoders.h
> > @@ -32,7 +32,6 @@
> > #include "zlib-encoder.h"
> >
> > typedef struct RedCompressBuf RedCompressBuf;
> > -typedef struct GlzDrawableInstanceItem GlzDrawableInstanceItem;
> > typedef struct RedGlzDrawable RedGlzDrawable;
> > typedef struct ImageEncoders ImageEncoders;
> > typedef struct ImageEncoderSharedData ImageEncoderSharedData;
> > @@ -50,6 +49,8 @@ void
> > image_encoders_free_glz_drawables_to_free(ImageEncoders* enc);
> > gboolean image_encoders_glz_create(ImageEncoders *enc, uint8_t id);
> > void image_encoders_freeze_glz(ImageEncoders *enc);
> > void image_encoders_release_glz(ImageEncoders *enc);
> > +void image_encoders_glz_free_from_drawable(struct Drawable *drawable);
> > +void image_encoders_glz_detach_from_drawable(struct Drawable *drawable);
> >
> > #define RED_COMPRESS_BUF_SIZE (1024 * 64)
> > struct RedCompressBuf {
> > @@ -106,9 +107,6 @@ typedef struct {
> > char message_buf[512];
> > } EncoderData;
> >
> > -void encoder_data_init(EncoderData *data);
> > -void encoder_data_reset(EncoderData *data);
> > -
> > typedef struct {
> > QuicUsrContext usr;
> > EncoderData data;
> > @@ -141,33 +139,6 @@ typedef struct {
> > EncoderData data;
> > } GlzData;
> >
> > -#define MAX_GLZ_DRAWABLE_INSTANCES 2
> > -
> > -/* for each qxl drawable, there may be several instances of lz drawables
> > */
> > -/* TODO - reuse this stuff for the top level. I just added a second level
> > of
> > multiplicity
> > - * at the Drawable by keeping a ring, so:
> > - * Drawable -> (ring of) RedGlzDrawable -> (up to 2)
> > GlzDrawableInstanceItem
> > - * and it should probably (but need to be sure...) be
> > - * Drawable -> ring of GlzDrawableInstanceItem.
> > - */
> > -struct GlzDrawableInstanceItem {
> > - RingItem glz_link;
> > - RingItem free_link;
> > - GlzEncDictImageContext *context;
> > - RedGlzDrawable *glz_drawable;
> > -};
> > -
> > -struct RedGlzDrawable {
> > - RingItem link; // ordered by the time it was encoded
> > - RingItem drawable_link;
> > - RedDrawable *red_drawable;
> > - Drawable *drawable;
> > - GlzDrawableInstanceItem instances_pool[MAX_GLZ_DRAWABLE_INSTANCES];
> > - Ring instances;
> > - uint8_t instances_count;
> > - ImageEncoders *encoders;
> > -};
> > -
> > struct ImageEncoderSharedData {
> > uint32_t glz_drawable_count;
> >
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index f1e8ba1..f923be8 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1171,11 +1171,7 @@ static bool free_one_drawable(DisplayChannel
> > *display,
> > int force_glz_free)
> >
> > drawable = SPICE_CONTAINEROF(ring_item, Drawable, list_link);
> > if (force_glz_free) {
> > - RingItem *glz_item, *next_item;
> > - RedGlzDrawable *glz;
> > - DRAWABLE_FOREACH_GLZ_SAFE(drawable, glz_item, next_item, glz) {
> > - image_encoders_free_glz_drawable(glz->encoders, glz);
> > - }
> > + image_encoders_glz_free_from_drawable(drawable);
> > }
> > drawable_draw(display, drawable);
> > container = drawable->tree_item.base.container;
> > @@ -1328,7 +1324,6 @@ static void
> > drawable_unref_surface_deps(DisplayChannel
> > *display, Drawable *drawa
> > void drawable_unref(Drawable *drawable)
> > {
> > DisplayChannel *display = drawable->display;
> > - RingItem *item, *next;
> >
> > if (--drawable->refs != 0)
> > return;
> > @@ -1345,10 +1340,8 @@ void drawable_unref(Drawable *drawable)
> > drawable_unref_surface_deps(display, drawable);
> > display_channel_surface_unref(display, drawable->surface_id);
> >
> > - RING_FOREACH_SAFE(item, next, &drawable->glz_ring) {
> > - SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link)->drawable =
> > NULL;
> > - ring_remove(item);
> > - }
> > + image_encoders_glz_detach_from_drawable(drawable);
> > +
> > if (drawable->red_drawable) {
> > red_drawable_unref(drawable->red_drawable);
> > }
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 8588583..318a793 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -88,11 +88,6 @@ void drawable_unref (Drawable *drawable);
> > #define DRAWABLE_FOREACH_DPI_SAFE(drawable, link, next, dpi) \
> > SAFE_FOREACH(link, next, drawable, &(drawable)->pipes, dpi,
> > LINK_TO_DPI(link))
> >
> > -#define LINK_TO_GLZ(ptr) SPICE_CONTAINEROF((ptr), RedGlzDrawable, \
> > - drawable_link)
> > -#define DRAWABLE_FOREACH_GLZ_SAFE(drawable, link, next, glz) \
> > - SAFE_FOREACH(link, next, drawable, &(drawable)->glz_ring, glz,
> > LINK_TO_GLZ(link))
> > -
> > enum {
> > RED_PIPE_ITEM_TYPE_DRAW = RED_PIPE_ITEM_TYPE_COMMON_LAST,
> > RED_PIPE_ITEM_TYPE_IMAGE,
>
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
>
Frediano
More information about the Spice-devel
mailing list