Skip to content

Conversation

@aleitner
Copy link
Contributor

@aleitner aleitner commented Mar 15, 2023

XRPD Connection is noticeably slow. Screen updates take time to occur and appear "laggy".

In this Pull Request we:

  • Remove duplicated include statement.
  • Consolidate frame start and last frame end timestamps when reading from rdp.
  • Change mark frames as 1 (in_frame) when we receive anything but an end frame from RDP (SURFACECMD_FRAMEACTION_END). Otherwise mark frames as 0.

@jmuehlner
Copy link
Contributor

Hey @aleitner, you need a GUACAMOLE issue for any changes you want to submit for review.

@aleitner aleitner changed the title KCM-181: Restructure rdp logic to improve graphical data rendering. GUACAMOLE-XXXX: Restructure rdp logic to improve graphical data rendering. Mar 15, 2023
@mike-jumper
Copy link
Contributor

Please also be sure to update your commit message to cover the nature of the intended changes. I suspect this may be partly due to this PR being a draft and the overall changes not yet fully known, but "Restructure rdp logic" is too vague.

@mike-jumper
Copy link
Contributor

@aleitner As the support for the Graphics Pipeline Extension / RemoteFX isn't yet complete due to the regression you're looking to address here, the relevant JIRA issue would be the issue introducing that WIP support:

https://issues.apache.org/jira/browse/GUACAMOLE-377

@aleitner aleitner force-pushed the KCM-181-RDPGFX-LAG-FIX branch from 696115e to 05557ae Compare March 17, 2023 17:39
@aleitner aleitner changed the title GUACAMOLE-XXXX: Restructure rdp logic to improve graphical data rendering. GUACAMOLE-377: Restructure rdp logic to improve graphical data rendering. Mar 17, 2023
@aleitner aleitner marked this pull request as ready for review March 17, 2023 17:41
@aleitner aleitner force-pushed the KCM-181-RDPGFX-LAG-FIX branch 2 times, most recently from f21bc9e to 65e0240 Compare March 17, 2023 20:47
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.

Looks okay to me - in addition to testing these changes against xrdp, have you also tested against other RDP systems, particularly ones that support RemoteFX, to make sure that it doesn't cause any regressions, there?

@aleitner
Copy link
Contributor Author

Looks okay to me - in addition to testing these changes against xrdp, have you also tested against other RDP systems, particularly ones that support RemoteFX, to make sure that it doesn't cause any regressions, there?

I have run my code to access

  • Windows Server 2016 with RemoteFX enabled.
  • Fedora 30 with xrdp 0.9.13
  • Ubuntu 22.04 with xrdp 0.9.17

@mike-jumper
Copy link
Contributor

@aleitner Regarding:

Update Surface frame marker to always mark frame as 0 to make sure the frames get flushed appropriately.

Can you clarify why/how this addresses the issue? RDP provides different markers for the start and end of a frame, and setting 0 always for the call to guac_rdp_gdi_mark_frame() would mean that guac_rdp_gdi_mark_frame() always believes it's at the end of a frame and never marks things as being in progress.

@aleitner aleitner force-pushed the KCM-181-RDPGFX-LAG-FIX branch from 65e0240 to 9a90bff Compare March 20, 2023 19:07
…ing from rdp. Correct logic when marking surface frames.
@aleitner aleitner force-pushed the KCM-181-RDPGFX-LAG-FIX branch from 9a90bff to 07b9698 Compare March 20, 2023 19:20
@aleitner
Copy link
Contributor Author

aleitner commented Mar 20, 2023

Updated logic by changing to mark frames as 1 (in_frame) when we receive anything but an end frame from RDP (SURFACECMD_FRAMEACTION_END). Otherwise mark frames as 0.

Tested on the following machines

  • Windows Server 2016 with RemoteFX enabled.
  • Fedora 30 with xrdp 0.9.13
  • Ubuntu 22.04 with xrdp 0.9.17

Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. The issues fixed here are definitely logic errors. I'm intrigued to see how this affects performance after merge.

@mike-jumper mike-jumper merged commit 77ec058 into apache:master Mar 22, 2023
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.

4 participants