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-377: Address performance regression related to migration to guac_display. #1045

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

mike-jumper
Copy link
Contributor

This partly reverts the changes introduced by d6db8fa (#681), restoring the previous client-side frame handling.

The new asynchronous flush mechanism leveraging requestAnimationFrame() does not perform as well as the old synchronous flush. This appears to be due to delays in when the browser actually allows the frame to proceed, which cause the client to lag behind.

A future change that could improve things further might be to process graphical changes in a WebWorker when available.

The asynchronous flush mechanism leveraging requestAnimationFrame() does
not perform as well as the old synchronous flush. This appears to be due
to delays in when the browser actually allows the frame to proceed,
causing the client to lag behind.

The old synchronous flush mechanism does not suffer from such issues.
@mike-jumper mike-jumper marked this pull request as draft January 16, 2025 08:51
@mike-jumper mike-jumper marked this pull request as ready for review January 16, 2025 08:56
@mike-jumper mike-jumper changed the title GUACAMOLE-377: Revert to synchronous flush (asynchronous is slower). GUACAMOLE-377: Address performance regression related to migration to guac_display. Jan 29, 2025
@mike-jumper
Copy link
Contributor Author

mike-jumper commented Jan 29, 2025

I've added some additional changes here that should significantly help with latency. By using the new ImageDecoder object where available (essentially any browser except on iOS), we can decode image streams as they are received without having to wait for the whole image contents to become available.

@DanielMcAssey
Copy link

Additionally, another future improvement is use of OffscreenCanvas with a Worker

Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the two comment issues identified above, and I think this is ready to go.

Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike-jumper - I'm good with it. Not sure if there's anything that @DanielMcAssey mentioned that's worth addressing, or if you want to keep it as-is?

@mike-jumper
Copy link
Contributor Author

I think it's find to keep as-is. I've been experimenting with using a WebWorker for rendering (and, implicitly, OffscreenCanvas), but I don't think there's any need to hold off on these improvements.

If the WebWorker approach does improve things further, I think it's OK for those improvements to be after 1.6.0. The current approach is already significantly faster.

@necouchman necouchman merged commit 38541c6 into apache:staging/1.6.0 Feb 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.

3 participants