[Spice-devel] RFC: remove Adler checksum and zlib header/trailer from zlib compressed images

[Yaniv Kaul]

On 02/26/2012 01:46 PM, Yonit Halperin wrote:
> On 02/23/2012 08:46 PM, Yaniv Kaul wrote:
>> On 02/06/2012 09:43 AM, Yonit Halperin wrote:
>>> On 02/06/2012 09:02 AM, Yaniv Kaul wrote:
>>>> On 01/29/2012 11:29 AM, Alon Levy wrote:
>>>>> On Sun, Jan 29, 2012 at 09:28:34AM +0200, Yaniv Kaul wrote:
>>>>>> These small changes to server and (gtk) client seem to work, and
>>>>>> remove both the zlib header/trailer from the deflate stream as well
>>>>>> as the Adler checksum, which removes several bytes from the stream
>>>>>> as well as speedup (unnoticeable) of the compression/decompression
>>>>>> due to the removal of the checksum calculation.
>>>>>>
>>>>>> Problem is - not sure how to test it - don't know how to get an
>>>>>> image once with and once without the patch - and ensure it's the
>>>>>> same image.
>>>>>> The fact that the first image is never zlib compressed complicates
>>>>>> things. I could not reliably get a 2nd image sent to the client
>>>>>> which looks the same as one in a different connection.
>>>>>>
>>>>>> Nevertheless it does no harm - and images pass well - verified by
>>>>>> connecting with a modified Spice GTK client to a modified Spice
>>>>>> server, as well as looking at the network capture via wireshark.
>>>>>>
>>>>>> It's of course missing the display channel specific capability
>>>>>> negotiation for this change.
>>>>
>>>> I'm afraid I could not really find where to perform the display 
>>>> channel
>>>> specific capability negotiation. :(
>>>> Anyone?
>>>>
>>> Hi,
>>>
>>> red_dispatcher.c/red_dispatcher_set_display_peer receives the client's
>>> caps.
>>> They should be sent to red_worker.c/handle_dev_display_connect (need
>>> to change RedWorkerMessageDisplayConnect payload to contain them).
>>> caps are tested via red_channel_client_test_remote_cap.
>>> The negotiation should be in red_worker.c/handle_new_display_channel.
>>>
>>> The display channel server caps should be set in
>>> red_worker.c/display_channel_create via red_channel.set_cap.
>>>
>>> Cheers,
>>> Yonit.
>>
>> 1. Client: I could not find how to do it in spice-gtk. I assume it's in
>> gtk/channel-display.c , under spice_display_channel_init(), and I assume
>> via spice_channel_set_capability(), but I could not get a hold of a
>> SpiceChannel object to pass to that function. ?
> You should use the macro SPIC 
> spice_channel_set_capability(SPICE_CHANNEL(channel), <cap>)
> You can see  an example in spice_main_channel_init

Exactly what I've tried to do, but couldn't get SPICE_CHANNEL() the 
right parameter. Probably overlooked something.

>> 2. Server: looks like zlib is init'ed via red_worker.c/red_init_zlib(),
>> which is called from red_worker.c/red_worker_main() , so anything after
>> that is a bit too late. Unless I re-init zlib (which looks like a light
>> operation) on ever connect.
>>
> The zlib encoder can be moved from the worker to display channel 
> client. With multi clients, different clients may have different caps. 
> Btw, for now we encode each message for each client, instead of 
> reusing the already encoded msg. We plan to change this, and than the 
> encoder will be moved to groups of channel client that share the same 
> encoders.
> The initialization and release of the encoder can be in the 
> connect/disconnect of the display channel client ( 
> handle_new_display_channel/display_channel_client_on_disconnect).

When a client disconnects, I assume the cache is cleaned, no? What 
happens with multiple clients?
Y.

>> Thanks,
>> Y.
>>
>>> TIA,
>>>> Y.
>>>>
>>>>>> Comments are welcome.
>>>>> Other then that adding a link to the docs page of
>>>>> inlateInit2/deflateInit2 in the commit message would be nice for 
>>>>> those
>>>>> seeking to find the explanation for the magic numbers, no comment,
>>>>> thanks for doing this!
>>>>>
>>>>> If you want a repeatable test you can write one or use one of the
>>>>> existing artificial tests in server/tests.
>>>>>
>>>>>> Client change:
>>>>>> diff --git a/gtk/decode-zlib.c b/gtk/decode-zlib.c
>>>>>> index a692020..997f350 100644
>>>>>> --- a/gtk/decode-zlib.c
>>>>>> +++ b/gtk/decode-zlib.c
>>>>>> @@ -63,7 +63,7 @@ SpiceZlibDecoder *zlib_decoder_new(void)
>>>>>> d->_z_strm.opaque = Z_NULL;
>>>>>> d->_z_strm.next_in = Z_NULL;
>>>>>> d->_z_strm.avail_in = 0;
>>>>>> - z_ret = inflateInit(&d->_z_strm);
>>>>>> + z_ret = inflateInit2(&d->_z_strm, -15);
>>>>>> if (z_ret != Z_OK) {
>>>>>> g_warning("zlib decoder init failed, error %d", z_ret);
>>>>>> goto fail;
>>>>>>
>>>>>>
>>>>>> server change:
>>>>>> diff --git a/server/zlib_encoder.c b/server/zlib_encoder.c
>>>>>> index c51466b..66a039a 100644
>>>>>> --- a/server/zlib_encoder.c
>>>>>> +++ b/server/zlib_encoder.c
>>>>>> @@ -47,7 +47,7 @@ ZlibEncoder*
>>>>>> zlib_encoder_create(ZlibEncoderUsrContext *usr, int level)
>>>>>> enc->strm.zfree = Z_NULL;
>>>>>> enc->strm.opaque = Z_NULL;
>>>>>>
>>>>>> - z_ret = deflateInit(&enc->strm, level);
>>>>>> + z_ret = deflateInit2(&enc->strm, level, Z_DEFLATED, -15, 8,
>>>>>> Z_DEFAULT_STRATEGY);
>>>>>> enc->last_level = level;
>>>>>> if (z_ret != Z_OK) {
>>>>>> red_printf("zlib error");
>>>>>> _______________________________________________
>>>>>> Spice-devel mailing list
>>>>>> Spice-devel at lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>
>>>> _______________________________________________
>>>> Spice-devel mailing list
>>>> Spice-devel at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>
>>
>