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

[Yaniv Kaul]

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. ?
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.

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
>