[Spice-devel] [PATCH v4 07/19] Encapsulate some data in dcc-encoders
Jonathon Jongsma
jjongsma at redhat.com
Tue Jun 14 15:12:17 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.
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.
> +
> +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.)
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.
> +
> 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>
More information about the Spice-devel
mailing list