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 primarily affecting RDP. #573

Open
wants to merge 4 commits into
base: staging/1.6.0
Choose a base branch
from

Conversation

mike-jumper
Copy link
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
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
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
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.
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