Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GUACAMOLE-2043: Fix playback of recordings which have multibyte character instructions. #1066

Merged

Conversation

eugen-keeper
Copy link
Contributor

@eugen-keeper eugen-keeper commented Mar 4, 2025

At first I considered to unite and refactor Guacamole.SessionRecording.loadInstruction and function handleInstruction(opcode, args). My idea was to get rid of Guacamole.SessionRecording.getElementSize. Unfortunately, most likely it will not allow us to fix the issue isolated because the recording can be received as a blob or via a tunnel. loadInstruction/getElementSize are used for both cases and through Gucamole.Parser. There would be too many changes with a chance of new bugs affecting not just the playback.

So to fix the issue isolated, I simply calculate UTF8 string byte size for every instruction content. Performance benchmarks show that a custom implementation of such a function works better than TextEncoder and Blob API. For example, a benchmark. Also, a StackOverflow topic there one of the answers provides benchmarks.

In my tests, a 10Mb recording is preprocessed for about 3 seconds. Byte size calculation of instructions (UTF8 strings) takes about 1-2% of the time. Overall, I did not see significant time difference in preprocessing with or without that byte size calculation. On the other hand, the preprocessing time includes everything including delays to download the content, etc. I could not measure pure parsing time with/without the byte size calculation due to the async function workflow. A single instruction processing takes less than 1 ms. So you cannot measure all instructions separately and then just sum. It will still be 0.

Another option would be to limit byte size calculation for some instructions only. For example,
var getElementSize = function getElementSize(value, unicode) {
var valueLength = value.length;
var protocolSize = (unicode ? getUtf8StringByteSize(value) : valueLength) + 3;
...
and
frameEnd += getElementSize(args[i], opcode === 'name');

@eugen-keeper eugen-keeper marked this pull request as draft March 4, 2025 20:29
@eugen-keeper eugen-keeper marked this pull request as ready for review March 4, 2025 21:44
@eugen-keeper eugen-keeper force-pushed the unicode-instruction-playback branch from 43b844e to 73bd6fa Compare March 7, 2025 17:33
@eugen-keeper eugen-keeper requested a review from mike-jumper March 7, 2025 18:40
@eugen-keeper eugen-keeper force-pushed the unicode-instruction-playback branch from 73bd6fa to ddcc335 Compare March 7, 2025 18:42
@mike-jumper mike-jumper merged commit 4dfeab7 into apache:patch Mar 7, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants