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: Add libguac "guac_display" API for easy and efficient rendering. #525

Merged
merged 53 commits into from
Sep 29, 2024

Conversation

mike-jumper
Copy link
Contributor

This change builds off the internal guac_common_display and guac_common_surface structures to create a new public guac_display structure and surrounding API. The approach within guac_display improves on that of guac_common_surface in several ways:

  • It makes use of a pool of worker threads to parallelize the encoding process. The appropriate number of worker threads is detected automatically when the guac_display is created.
  • Changes due to scrolling, etc. are automatically detected in real time using a combination of a hash table, a hash function designed to avoid inefficient memory traversal, and a 2D variant of Rabin-Karp. Such drawing operations are automatically transformed into copy operations.
  • Changes that can be represented as a simple rect followed by cfill are automatically recognized and sent out as such.
  • Dirty rectangles covering the changes made to the display are automatically broken up and tightened to more efficiently represent the actual changes made, regardless of how large the dirty rectangle submitted to guac_display may have been.
  • Adjustment for client-side processing lag is handled automatically.
  • Encoding of images for a previous frame happens asynchronously while the next frame is being assembled, reducing latency.
  • Any number of additional frames may be assembled while waiting for the previous frame to encode. If a previous frame is still being encoded, further frames are combined together into a pending frame that will be flushed when encoding is complete.

With these changes, you can now throw virtually anything at the display and it will magically get decomposed into an efficient combination of copies, draws, and rectangles, even if the information regarding the nature of those updates is unavailable (ie: RDPGFX and SPICE).

Metrics covering rendering and optimization performance of guac_display are logged at the "trace" level.

I'm opening this as a draft for now, as these changes are currently incomplete:

  • Only VNC has been migrated to the new API, and then only partially.
  • I suspect that the internals of the terminal emulator can be simplified, given that scrolling is now automatically detected.
  • I have not yet added a mechanism for callers to "hint" that a change is due to a copy (this will be necessary for handling RDP's bitmap cache).

Once the above is finished, I'll delete the old guac_common_display, guac_common_surface, etc. and this will be ready.

@mike-jumper
Copy link
Contributor Author

I've been using the following patch to observe the impact these changes:

https://gist.github.com/mike-jumper/0afd41c9fbcbc764f719890fd0dd4e3b

With the above patch in place, you can set Guacamole.Display.DEBUG to true to enable a debug mode in which draw operations to the Guacamole display are highlighted in colored rectangles, where the color varies by the type of operation:

  • RED: Draw of compressed image data (the most expensive operation)
  • BLUE: Copy of existing image data (ie: an optimized scroll, restoring data from a cache - much cheaper than encoding, sending, and decoding image data)
  • GREEN: Solid-color rectangular draw (a combination of rect and cfill - very cheap).

Behavior of guac_common_surface

Here, everything is red. Dirty rects are generally nicely split and tightened around the regions actually changed, but that's about as deep as the processing goes. Without information from the remote desktop server explicitly stating that a particular update is a copy, we can't detect that a copy would be better.

rendering-before-guac-display.mp4

Behavior of guac_display

Now, things get much more colorful. Things that can be represented more efficiently as copies or rectangles are automatically optimized. This includes cases when the nature of those updates would prevent even the remote desktop server from knowing that a copy has occurred.

rendering-after-guac-display.mp4

.gitignore Show resolved Hide resolved
…h NULL if necessary for external cleanup tasks.
It is otherwise difficult to guarantee that all operations touching the
pending frame buffer will occur while holding an open raw context,
resulting in unstable behavior.
No longer necessary now that the last and pending frame buffers are not
interleaved.
… lock.

Acquiring the read lock first and then reentrantly acquiring the write
lock may result in other existing readers getting promoted to writers.
…ges match exactly.

Doing otherwise can result in display operations overlapping. This is
because combining two vertically adjacent operations that have different
widths causes additional cells to be included that are not covered by
either original operation. If other operations are within those
additional cells, then the operations flushed for the frame will
overlap, consuming unnecessary additional processing and bandwidth.
…fill" pairs.

Other instructions that occur between "rect" and "cfill" may disrupt the
path set by "rect", resulting in "cfill" having no effect, resulting in
graphical artifacts.

This also has the side effect of reducing thread contention by flushing
the simple operations early (it is now less likely that multiple worker
threads will be free for further tasks at nearly exactly the same time).
Testing doesn't seem to support an increase in throughput or
responsiveness from doubling up on worker threads.
@corentin-soriano
Copy link
Contributor

I haven't had any problems with RDP/VNC. In terminal, cat a large file is very slow and can in some cases disconnect me from guacamole and the display is not restored when exiting the alternate buffer.

@corentin-soriano Thanks for checking this. The remaining performance trouble with the terminal seems rooted in GUACAMOLE-1256, so if there isn't an obvious fix, I think we'll just end up rolling back those changes and revisiting next release. The changes here should no longer affect that.

I misunderstood your comment in the ticket, I thought you had completely replaced the display_flush and that the problem was solved.
In this case, what seems best is to remove it in the scroll_up function while keeping it in scroll_down. The latencies would no longer be present, and the scrolling bug in vi would also be gone.
I will create the PR on Monday.
Thanks for the clarification.

…ng index to pool.

Doing otherwise results in a race condition where the index of a valid
stream is changed to -1 by a different thread, breaking assertions and
causing the connection to disconnect.
Copy link
Contributor

@jmuehlner jmuehlner left a comment

Choose a reason for hiding this comment

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

Everything looks good to me! Let's light this candle.

@jmuehlner jmuehlner merged commit b754d3f into apache:staging/1.6.0 Sep 29, 2024
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.

5 participants