Xpra-org / xpra-html5

HTML5 client for Xpra
Mozilla Public License 2.0
211 stars 55 forks source link

html5 support for rencode #18

Closed totaam closed 3 years ago

totaam commented 5 years ago

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

totaam commented 5 years ago

2018-10-29 16:06:18: antoine uploaded file rencode-js.patch (11.0 KiB)

almost works?

totaam commented 5 years ago

See also Xpra-org/xpra-html5#16

totaam commented 5 years ago

Code is incomplete and would need a lot of work.

totaam commented 4 years ago

2020-05-26 22:34:20: TijZwa commented


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?

totaam commented 4 years ago

Absolutely, please send it along.

totaam commented 4 years ago

2020-05-27 15:29:05: TijZwa uploaded file pyRencode-js.patch (80.0 KiB)

totaam commented 4 years ago

2020-05-27 15:30:31: TijZwa commented


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?

totaam commented 4 years ago

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.

totaam commented 4 years ago

Merged in Xpra-org/xpra@7d90a83d8e16a04e01a82ab36d9fa0507e01bbf4, only a minor fix, and the IE11 compatibility workaround.

IE11 already needed build fixes for 4.0: Xpra-org/xpra@85493a2eaa630aea388578a2d9f01cfc5f252711, and other minor fixes: Xpra-org/xpra@d2bdeab47ae81d00caef30bca8e0d2acbf27e28f, Xpra-org/xpra@024a2f212f0f5fada47d978962f74ba91a1f13f6, etc. There are still other problems with IE, but those aren't new and at least it runs now.

Thanks for the patch!

totaam commented 4 years ago

Caused a regression: Xpra-org/xpra#2787

totaam commented 4 years ago

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)

totaam commented 4 years ago

2020-05-31 12:23:49: TijZwa commented


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

totaam commented 4 years ago

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)

totaam commented 4 years ago

2020-05-31 19:09:24: TijZwa commented


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.

totaam commented 4 years ago

2020-05-31 19:10:51: TijZwa uploaded file pyrencoder.js (76.1 KiB)

Specify encoding with string parameter

totaam commented 4 years ago

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.

totaam commented 4 years ago

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 Clients HTML5.

totaam commented 4 years ago

2020-07-03 10:29:16: TijZwa commented


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

totaam commented 4 years ago

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

FYI: there are other AES options in Xpra-org/xpra#2615.

totaam commented 3 years ago

This has caused a regression with file transfers: Xpra-org/xpra-html5#6.

totaam commented 3 years ago

@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:

totaam commented 3 years ago

2021-01-05 15:47:02: antoine uploaded file html5-send-binary.patch (1.8 KiB)

patch to allow the bencoder to handle binary buffers directly

totaam commented 3 years ago

2021-01-14 08:34:38: TijZwa commented


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

totaam commented 3 years ago

2021-01-14 08:35:38: TijZwa uploaded file rencode.zip (1.1 KiB)

totaam commented 3 years ago
$ 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.

totaam commented 3 years ago

2021-01-14 16:19:35: TijZwa commented


Replying to [comment:21 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.

totaam commented 3 years ago

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.

totaam commented 3 years ago

Because of #6, I thought I would try to write a simple rencode version that can reliably handle Uint8Array without needing to use strings. Then I remembered why I hate Javascript:

Object.keys({1:2})
["1"]

Use a number, get a string.

totaam commented 3 years ago

Done. It wasn't that hard.

It does fix #6 too. (I have not tried AES yet)