[Spice-devel] [PATCH spice-gtk v2 1/2] Change the setting of the images cache size and the glz window size

[Hans de Goede]

Hi,

On 01/24/2012 09:01 AM, Yonit Halperin wrote:
> Hi,
>
> On 01/23/2012 12:36 PM, Hans de Goede wrote:
>> 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().
>>
> The min/max default values are used only for the default calculation.
> In g_object_class_install_property I set the min/max value for both the cache and dictionary. Their minimal size is 0. The maximal size for the cache is G_MAXINT and for the dictionary it is the maximal value that can be processed by the lz code: LZ_MAX_WINDOW_SIZE * 4 ( LZ_MAX_WINDOW_SIZE is in pixels). These limits will allow us to test a variety of values. Once we have conclusion for the optimal sizes (depending on different conditions), we can set stricter limits.
> Do you want me to add explicit defines for these limits?

No, my bad, I forgot about the setting of min / max on the g_param_spec, so
wrt min/max things are fine, if you drop the 2 unnecessary get functions
this patch is good to go.

Regards,

Hans