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

[Yonit Halperin]

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