[Spice-devel] [PATCH spice-gtk v2 1/2] Change the setting of the images cache size and the glz window size
Hans de Goede
hdegoede at redhat.com
Mon Jan 23 02:36:31 PST 2012
Hi,
I've various remarks, see my comments inline.
On 01/22/2012 09:12 AM, Yonit Halperin wrote:
> Set the default sizes to be the same as in the old linux spice client.
> cache_size=20M pixels (instead of 32M), window_size=8M pixels for a 64MB
> dev ram (instead of 16M pixels).
> ---
> gtk/channel-display-priv.h | 2 -
> gtk/channel-display.c | 22 +++++++---
> gtk/channel-main.c | 1 +
> gtk/spice-session-priv.h | 12 ++++++
> gtk/spice-session.c | 90 +++++++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 117 insertions(+), 10 deletions(-)
>
> diff --git a/gtk/channel-display-priv.h b/gtk/channel-display-priv.h
> index 332d271..eca1787 100644
> --- a/gtk/channel-display-priv.h
> +++ b/gtk/channel-display-priv.h
> @@ -36,8 +36,6 @@
>
> G_BEGIN_DECLS
>
> -#define DISPLAY_PIXMAP_CACHE (1024 * 1024 * 32)
> -#define GLZ_WINDOW_SIZE (1024 * 1024 * 16)
>
> typedef struct display_surface {
> RingItem link;
> diff --git a/gtk/channel-display.c b/gtk/channel-display.c
> index ec42829..2473ac6 100644
> --- a/gtk/channel-display.c
> +++ b/gtk/channel-display.c
> @@ -766,13 +766,21 @@ static void emit_invalidate(SpiceChannel *channel, SpiceRect *bbox)
> static void spice_display_channel_up(SpiceChannel *channel)
> {
> SpiceMsgOut *out;
> - SpiceMsgcDisplayInit init = {
> - .pixmap_cache_id = 1,
> - .pixmap_cache_size = DISPLAY_PIXMAP_CACHE,
> - .glz_dictionary_id = 1,
> - .glz_dictionary_window_size = GLZ_WINDOW_SIZE,
> - };
> -
> + SpiceSession *s = spice_channel_get_session(channel);
> + SpiceMsgcDisplayInit init;
> + int cache_size;
> + int glz_window_size;
> +
> + g_object_get(s,
> + "cache-size",&cache_size,
> + "glz-window-size",&glz_window_size,
> + NULL);
> + SPICE_DEBUG("%s: cache_size %d, glz_window_size %d (bytes)", __FUNCTION__,
> + cache_size, glz_window_size);
> + init.pixmap_cache_id = 1;
> + init.glz_dictionary_id = 1;
> + init.pixmap_cache_size = cache_size / 4; /* pixels */
> + init.glz_dictionary_window_size = glz_window_size / 4; /* pixels */
> out = spice_msg_out_new(channel, SPICE_MSGC_DISPLAY_INIT);
> out->marshallers->msgc_display_init(out->marshaller,&init);
> spice_msg_out_send_internal(out);
> diff --git a/gtk/channel-main.c b/gtk/channel-main.c
> index ebf660f..b2df547 100644
> --- a/gtk/channel-main.c
> +++ b/gtk/channel-main.c
> @@ -1128,6 +1128,7 @@ static void main_handle_init(SpiceChannel *channel, SpiceMsgIn *in)
> init->current_mouse_mode);
>
> spice_session_set_mm_time(session, init->multi_media_time);
> + spice_session_set_caches_hints(session, init->ram_hint, init->display_channels_hint);
>
> c->agent_tokens = init->agent_tokens;
> if (init->agent_connected)
> diff --git a/gtk/spice-session-priv.h b/gtk/spice-session-priv.h
> index 5df1182..a814cfe 100644
> --- a/gtk/spice-session-priv.h
> +++ b/gtk/spice-session-priv.h
> @@ -27,6 +27,10 @@
>
> G_BEGIN_DECLS
>
> +#define IMAGES_CACHE_SIZE_DEFAULT (1024 * 1024 * 80)
> +#define MIN_GLZ_WINDOW_SIZE_DEFAULT (1024 * 1024 * 12)
> +#define MAX_GLZ_WINDOW_SIZE_DEFAULT MIN((LZ_MAX_WINDOW_SIZE * 4), 1024 * 1024 * 64)
> +
Are these glz window size min and max values only for the default calculation, or
should the same min / max also be applied in spice_session_set_property() ? If they
also should be applied in spice_session_set_property, I suggest dropping the
_DEFAULT, and adding checks for them there. If they should not be applied in
spice_session_set_property(), I would like to see some additional MIN/MAX defines
which are used to clamp the input value in spice_session_set_property(), likewise I
would like to see some min / max defines for the image cache size and the
PROP_CACHE_SIZE input value clamped to these in spice_session_set_property().
> struct _SpiceSessionPrivate {
> char *host;
> char *port;
> @@ -84,6 +88,11 @@ struct _SpiceSessionPrivate {
> display_cache images;
> display_cache palettes;
> SpiceGlzDecoderWindow *glz_window;
> + int images_cache_size;
> + int glz_window_size;
> + uint32_t pci_ram_size;
> + uint32_t display_channels_count;
> +
>
> /* associated objects */
> SpiceAudio *audio_manager;
> @@ -120,6 +129,9 @@ const gchar* spice_session_get_cert_subject(SpiceSession *session);
> const gchar* spice_session_get_ciphers(SpiceSession *session);
> const gchar* spice_session_get_ca_file(SpiceSession *session);
>
> +void spice_session_set_caches_hints(SpiceSession *session,
> + uint32_t pci_ram_size,
> + uint32_t display_channels_count);
> void spice_session_get_caches(SpiceSession *session,
> display_cache **images,
> display_cache **palettes,
> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
> index 79ba1b6..d36709e 100644
> --- a/gtk/spice-session.c
> +++ b/gtk/spice-session.c
> @@ -101,6 +101,8 @@ enum {
> PROP_DISABLE_EFFECTS,
> PROP_COLOR_DEPTH,
> PROP_READ_ONLY,
> + PROP_CACHE_SIZE,
> + PROP_GLZ_WINDOW_SIZE,
> };
>
> /* signals */
> @@ -407,7 +409,13 @@ static void spice_session_get_property(GObject *gobject,
> break;
> case PROP_READ_ONLY:
> g_value_set_boolean(value, s->read_only);
> - break;
> + break;
> + case PROP_CACHE_SIZE:
> + g_value_set_int(value, s->images_cache_size);
> + break;
> + case PROP_GLZ_WINDOW_SIZE:
> + g_value_set_int(value, s->glz_window_size);
> + break;
> default:
> G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
> break;
> @@ -508,6 +516,12 @@ static void spice_session_set_property(GObject *gobject,
> s->read_only = g_value_get_boolean(value);
> g_object_notify(gobject, "read-only");
> break;
> + case PROP_CACHE_SIZE:
> + s->images_cache_size = g_value_get_int(value);
> + break;
> + case PROP_GLZ_WINDOW_SIZE:
> + s->glz_window_size = g_value_get_int(value);
> + break;
> default:
> G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
> break;
> @@ -913,6 +927,36 @@ static void spice_session_class_init(SpiceSessionClass *klass)
> G_PARAM_CONSTRUCT |
> G_PARAM_STATIC_STRINGS));
>
> + /**
> + * SpiceSession:cache-size:
> + *
> + * Images cache size. If 0, don't set.
> + *
> + **/
> + g_object_class_install_property
> + (gobject_class, PROP_CACHE_SIZE,
> + g_param_spec_int("cache-size",
> + "Cache size",
> + "Images cache size (bytes)",
> + 0, G_MAXINT, 0,
> + G_PARAM_READWRITE |
> + G_PARAM_STATIC_STRINGS));
> +
> + /**
> + * SpiceSession:glz-window-size:
> + *
> + * Glz window size. If 0, don't set.
> + *
> + **/
> + g_object_class_install_property
> + (gobject_class, PROP_GLZ_WINDOW_SIZE,
> + g_param_spec_int("glz-window-size",
> + "Glz window size",
> + "Glz window size (bytes)",
> + 0, LZ_MAX_WINDOW_SIZE * 4, 0,
> + G_PARAM_READWRITE |
> + G_PARAM_STATIC_STRINGS));
> +
> g_type_class_add_private(klass, sizeof(SpiceSessionPrivate));
> }
>
> @@ -1664,3 +1708,47 @@ void spice_session_get_caches(SpiceSession *session,
> if (glz_window)
> *glz_window = s->glz_window;
> }
> +
> +G_GNUC_INTERNAL
> +void spice_session_set_caches_hints(SpiceSession *session,
> + uint32_t pci_ram_size,
> + uint32_t display_channels_count)
> +{
> + SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
> +
> + g_return_if_fail(s != NULL);
> +
> + s->pci_ram_size = pci_ram_size;
> + s->display_channels_count = display_channels_count;
> +
> + /* TODO: when setting cache and window size, we should consider the client's
> + * available memory and the number of display channels */
> + if (s->images_cache_size == 0) {
> + s->images_cache_size = IMAGES_CACHE_SIZE_DEFAULT;
> + }
> +
> + if (s->glz_window_size == 0) {
> + s->glz_window_size = MIN(MAX_GLZ_WINDOW_SIZE_DEFAULT, pci_ram_size / 2);
> + s->glz_window_size = MAX(MIN_GLZ_WINDOW_SIZE_DEFAULT, s->glz_window_size);
> + }
> +}
> +
You're not using the 2 functions below, nor are you adding them to any
header file, instead you're using g_object_get above, which I believe is
the better way to handle this. Please drop these 2 functions.
> +G_GNUC_INTERNAL
> +int spice_session_get_images_cache_size(SpiceSession *session)
> +{
> + SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
> +
> + g_return_val_if_fail(s != NULL, 0);
> + g_return_val_if_fail(s->images_cache_size != -1, 0);
> + return s->images_cache_size;
> +}
> +
> +G_GNUC_INTERNAL
> +int spice_session_get_glz_window_size(SpiceSession *session)
> +{
> + SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
> +
> + g_return_val_if_fail(s != NULL, 0);
> + g_return_val_if_fail(s->glz_window_size != -1, 0);
> + return s->glz_window_size;
> +}
Regards,
Hans
More information about the Spice-devel
mailing list