Skip to content

GUACAMOLE-377: Address performance regression primarily affecting RDP.#573

Merged
necouchman merged 4 commits intoapache:staging/1.6.0from
mike-jumper:fix-perf
Apr 21, 2025
Merged

GUACAMOLE-377: Address performance regression primarily affecting RDP.#573
necouchman merged 4 commits intoapache:staging/1.6.0from
mike-jumper:fix-perf

Conversation

@mike-jumper
Copy link
Copy Markdown
Contributor

@mike-jumper mike-jumper commented Jan 13, 2025

These changes address two issues that were affecting the performance of Guacamole after the introduction of guac_display, particularly RDP:

  • The dirty rects of the last frame were not being updated when the corresponding dirty rects of the pending frame were empty.

    It's correct to not flush updated image data if the dirty rects are empty, but the rects tracking the state of the last frame need to be correctly updated to match the state of the pending frame, even if that state is empty. Doing otherwise results in unnecessary updates being transmitted to the client, since guac_display incorrectly believes that things were changed in the last frame.

  • Sending a new sync for each movement of the mouse can at least cause client-side slowdowns. There may be things that can be improved in the client.

Additional changes related to this are part of apache/guacamole-client#1045.

@mike-jumper mike-jumper marked this pull request as ready for review January 13, 2025 23:04
@corentin-soriano
Copy link
Copy Markdown
Member

I notice a significant improvement in performance; however, when moving a window (especially during fast movements), there can be some "mixing" that I cannot reproduce without the fix. Here is a preview:
image
image

I find it harder to reproduce on a Linux VM (GNOME RDP) than on a Windows with the same configuration.
image

Freerdp3 with this configuration:
image

@mike-jumper
Copy link
Copy Markdown
Contributor Author

@corentin-soriano I remember seeing those artifacts before. I think what you're seeing is due to a timing-related bug that's present regardless of these changes, and it's just that these changes make that bug more likely to appear.

@corentin-soriano
Copy link
Copy Markdown
Member

@mike-jumper I suspected it was linked to the speed gain but I preferred to have confirmation.
Thanks for the clarification!

…forward.

This seems to induce latency and possibly negatively affects tracking of
client-side processing lag.
@necouchman necouchman merged commit 79843f5 into apache:staging/1.6.0 Apr 21, 2025
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