xpra icon
Bug tracker and wiki

This bug tracker and wiki are being discontinued
please use https://github.com/Xpra-org/xpra instead.


Opened 3 years ago

Last modified 9 months ago

#2018 assigned enhancement

html5 support for rencode

Reported by: Antoine Martin Owned by: Antoine Martin
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 (5)

rencode-js.patch (11.0 KB) - added by Antoine Martin 3 years ago.
almost works?
pyRencode-js.patch (80.0 KB) - added by Tijs van der Zwaan 17 months ago.
pyrencoder.js (76.1 KB) - added by Tijs van der Zwaan 17 months ago.
Specify encoding with string parameter
html5-send-binary.patch (1.8 KB) - added by Antoine Martin 9 months ago.
patch to allow the bencoder to handle binary buffers directly
rencode.zip (1.1 KB) - added by Tijs van der Zwaan 9 months ago.

Download all attachments as: .zip

Change History (29)

Changed 3 years ago by Antoine Martin

Attachment: rencode-js.patch added

almost works?

comment:1 Changed 3 years ago by Antoine Martin

Milestone: 2.53.0
Status: newassigned

See also #2165

Last edited 3 years ago by Antoine Martin (previous) (diff)

comment:2 Changed 2 years ago by Antoine Martin

Resolution: wontfix
Status: assignedclosed

Code is incomplete and would need a lot of work.

comment:3 Changed 17 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 17 months ago by Antoine Martin

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

Absolutely, please send it along.

Changed 17 months ago by Tijs van der Zwaan

Attachment: pyRencode-js.patch added

comment:5 Changed 17 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 17 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 17 months ago by Antoine Martin (previous) (diff)

comment:7 Changed 17 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 17 months ago by Antoine Martin

Caused a regression: #2787

comment:9 Changed 17 months ago by Antoine Martin

Resolution: fixed
Status: closedreopened

comment:10 Changed 17 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 17 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 17 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 17 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 17 months ago by Antoine Martin (previous) (diff)

Changed 17 months ago by Tijs van der Zwaan

Attachment: pyrencoder.js added

Specify encoding with string parameter

comment:14 Changed 17 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 17 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 16 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 16 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.

comment:18 Changed 9 months ago by Antoine Martin

This has caused a regression with file transfers: #2993.

comment:19 Changed 9 months ago by Antoine Martin

@Tijs, as per comment:14:

Can you generate a non-minified version of pyrencode.js?

(preferably with steps so that I can re-generate it when there are new versions)
It would make it a easier to debug.
My guess is that it comes from here: https://github.com/cinderblock/python-rencode and the code does this with strings:

function encodeStr(buffs, str) {
    // JS strings are always utf8
    var buff = Buffer.from(str, 'utf8');
    if (buff.length < STR_FIXED_COUNT) {
        writeBufferChar(buffs, STR_FIXED_START + buff.length);
        writeBuffer(buffs, buff);
    }
    else {
        writeBuffer(buffs, Buffer.from(buff.length + ':', 'ascii'));
        writeBuffer(buffs, buff);
    }
}

OTOH, some potential solutions:

  • disable UTF8 completely in rencode: we just encode / decode strings as needed instead, not in the packet encoding layer
  • teach rencode and bencode to handle Uint8Array directly, saves having to convert the file data to a string (patch for bencode attached)

Changed 9 months ago by Antoine Martin

Attachment: html5-send-binary.patch added

patch to allow the bencoder to handle binary buffers directly

comment:20 Changed 9 months ago by Tijs van der Zwaan

I'm very sorry for the delay. I've included the code used to build this. Yes, I used python-rencode.

The project is very simple:
1) Unpack
2) npm install
3) npm run build-prod -> this build the library to ./builds with Babel

Changed 9 months ago by Tijs van der Zwaan

Attachment: rencode.zip added

comment:21 Changed 9 months ago by Antoine Martin

$ git clone https://github.com/cinderblock/python-rencode
$ cd python-rencode
$ npm install
$ npm run build-prod
npm ERR! missing script: build-prod

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/antoine/.npm/_logs/2021-01-14T15_04_41_545Z-debug.log

Can you build the non-minified version and attach it to this ticket?

The more I look at this thing and the more I think it should be re-written in plain Uint8Array vanilla javascript.
The code is really not very efficient either.

comment:22 in reply to:  21 Changed 9 months ago by Tijs van der Zwaan

Replying to Antoine Martin:

$ git clone https://github.com/cinderblock/python-rencode
$ cd python-rencode
$ npm install
$ npm run build-prod
npm ERR! missing script: build-prod

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/antoine/.npm/_logs/2021-01-14T15_04_41_545Z-debug.log

Can you build the non-minified version and attach it to this ticket?

The more I look at this thing and the more I think it should be re-written in plain Uint8Array vanilla javascript.
The code is really not very efficient either.

Hi Antoine,

You need to run the npm commands on the source files i've included in the zip file, not the python-rencode repository.
The python-rencode libs will be auto-installed by npm.

The non-minified is still kind of unreadable because of all the polyfills.
I will try to create a human-readable version.

comment:23 Changed 9 months ago by Antoine Martin

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

You need to run the npm commands on the source files i've included in the zip file, not the python-rencode repository

Doh! I had missed the zip file..
That works fine, but it's impossible to edit the resulting library file.

I might just re-write it in plain JS tomorrow.

comment:24 Changed 9 months ago by migration script

this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/2018

Note: See TracTickets for help on using tickets.