xpra icon
Bug tracker and wiki

Opened 23 months ago

Last modified 3 months ago

#2018 new enhancement

html5 support for rencode

Reported by: Antoine Martin Owned by: Tijs van der Zwaan
Priority: major Milestone: 3.0
Component: html5 Version: 2.4.x
Keywords: Cc:

Description

There is now a javascript version we can use: https://github.com/PolkaJS/rencode (ISC License)

Attachments (3)

rencode-js.patch (11.0 KB) - added by Antoine Martin 23 months ago.
almost works?
pyRencode-js.patch (80.0 KB) - added by Tijs van der Zwaan 4 months ago.
pyrencoder.js (76.1 KB) - added by Tijs van der Zwaan 4 months ago.
Specify encoding with string parameter

Download all attachments as: .zip

Change History (20)

Changed 23 months ago by Antoine Martin

Attachment: rencode-js.patch added

almost works?

comment:1 Changed 19 months ago by Antoine Martin

Milestone: 2.53.0
Status: newassigned

See also #2165

Last edited 19 months ago by Antoine Martin (previous) (diff)

comment:2 Changed 17 months ago by Antoine Martin

Resolution: wontfix
Status: assignedclosed

Code is incomplete and would need a lot of work.

comment:3 Changed 4 months ago by Tijs van der Zwaan

Resolution: wontfix
Status: closedreopened

Hi guys,

I've fixed this issue in my local development environment using the following node module; https://github.com/cinderblock/python-rencode/.

Is anyone still interested in a patch?

comment:4 Changed 4 months ago by Antoine Martin

Owner: changed from Antoine Martin to Tijs van der Zwaan
Status: reopenednew

Absolutely, please send it along.

Changed 4 months ago by Tijs van der Zwaan

Attachment: pyRencode-js.patch added

comment:5 Changed 4 months ago by Tijs van der Zwaan

Owner: changed from Tijs van der Zwaan to Tijs van der Zwaan
Status: newassigned

Here you go.
The patch might need some extra attention, but the basics are there.
This will break internet explorer functionality. Is IE 11 currently supported?

comment:6 Changed 4 months ago by Antoine Martin

Thanks.
Yes, IE 11 is still meant to be supported, so I'll need to find a way to either ignore rencode with IE11 or just fix it.

Last edited 4 months ago by Antoine Martin (previous) (diff)

comment:7 Changed 4 months ago by Antoine Martin

Resolution: fixed
Status: assignedclosed

Merged in r26508, only a minor fix, and the IE11 compatibility workaround.

IE11 already needed build fixes for 4.0: r26507, and other minor fixes: r26509, r26510, etc.
There are still other problems with IE, but those aren't new and at least it runs now.

Thanks for the patch!

comment:8 Changed 4 months ago by Antoine Martin

Caused a regression: #2787

comment:9 Changed 4 months ago by Antoine Martin

Resolution: fixed
Status: closedreopened

comment:10 Changed 4 months ago by Antoine Martin

Owner: changed from Tijs van der Zwaan to Tijs van der Zwaan
Status: reopenednew

This happens on small rgb32 screen updates because the pixel data ends up inlined with the packet data.

This worksaround the issue by not using packet inlined data (ugly):

Index: xpra/net/protocol.py
===================================================================
--- xpra/net/protocol.py	(revision 26521)
+++ xpra/net/protocol.py	(working copy)
@@ -562,7 +562,7 @@
                 #(it may now be a "Compressed" item and be processed further)
             if ti in (Compressed, LevelCompressed):
                 #already compressed data (usually pixels, cursors, etc)
-                if not item.can_inline or l>INLINE_SIZE:
+                if True:
                     il = 0
                     if ti==LevelCompressed:
                         #unlike Compressed (usually pixels, decompressed in the paint thread),

@Tijs van der Zwaan: what's happening with the pixel data? Why is it mangled by the PyRencode decoder? I've tried playing with the utf8 flag of the decode function, the result is different - but still broken..

FYI: the pixel data gets converted to a Uint8Array in Protocol.js near pass to our packet handler.
(IIRC because strings can't be sent from the worker)

comment:11 Changed 4 months ago by Tijs van der Zwaan

Hi Antoine,

I've figured out that it's an encoding problem between Python and javascript. I've implemented a quick and dirty workaround by using base64 encoding.

Protocol.py

                    #data is small enough, inline it:
                    packet[i] = str(base64.b64encode(item.data), "utf-8")

protocol.js

				if (typeof img_data === 'string') {
					const img_data_raw = atob(img_data);
					const uint = new Uint8Array(img_data_raw.length);
					for(i=0,j=img_data_raw.length;i<j;++i) {
						uint[i] = img_data_raw.charCodeAt(i);
					}
					packet[7] = uint;	

I'm going to investigate it further...

comment:12 Changed 4 months ago by Antoine Martin

Thanks for looking into it.
I don't understand what PyRencode is doing to that binary string that we can't just receive it exactly as it is on the wire, just sliced off. (Uint8Array or whatever)

comment:13 Changed 4 months ago by Tijs van der Zwaan

Thanks!!! Your comment '..is doing to that binary string..' pointed me in the right direction :)
PyRencoder got a switch for UTF-8. If we set the switch to false, it uses ascii encodig, but we want binary.
I've changed the python-rencode node package and the PyRencoder wrapper. It now accepts the encoding parameter as string.

Patch:

@@ -333,7 +333,7 @@
        let packet = null;
        try {
                if (proto_flags==1) {
-                packet = PyRencoder.decode(Buffer.from(packet_data), false);
+                packet = PyRencoder.decode(Buffer.from(packet_data), 'binary');
                } else {
                 packet = bdecode(packet_data);
                }

The new PyRencoder.js is in the attachment. Can you test this?
And do you want the ES6 source of the PyRencoder? It's just a wrapper for python-rencode node package, build with webpack/babel.

Last edited 4 months ago by Antoine Martin (previous) (diff)

Changed 4 months ago by Tijs van der Zwaan

Attachment: pyrencoder.js added

Specify encoding with string parameter

comment:14 Changed 4 months ago by Antoine Martin

Thanks, applied in r26562.
It seems to work fine.

Can you generate a non-minified version of pyrencode.js?
All the other libraries are minified during the build, not in the repository.

comment:15 Changed 4 months ago by Antoine Martin

Also, the rencode mode should be optional, even if the option is not necessarily shown on the connect dialog page.

Why: it's useful to switch packet encoders to track down bugs and verify that it works as expected...
Would have saved me time because it breaks AES encryption - this also needs fixing.
Try AES mode from wiki/Clients/HTML5.

comment:16 Changed 3 months ago by Tijs van der Zwaan

Thanks for the comments, I didn't had the time to try AES mode lately. Will fix this soon.

comment:17 Changed 3 months ago by Antoine Martin

I didn't had the time to try AES mode lately. Will fix this soon.

FYI: there are other AES options in #2615.

Note: See TracTickets for help on using tickets.