GUACAMOLE-377: Add libguac "guac_display" API for easy and efficient rendering.

[mike-jumper]

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]

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.

https://github.com/apache/guacamole-server/assets/4632905/b0792ae3-f1a7-4bc6-8ce1-932832c32a21

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.

https://github.com/apache/guacamole-server/assets/4632905/6fb37e05-37c3-4eef-ba42-88f4c89442a7

[mike-jumper]

FINALLY ready for review. :relieved:

[necouchman]

FINALLY ready for review. 😌

Whew, just almost 10K lines of code to review. Shouldn't take more than a few minutes ;-).

[jmuehlner]

Ok, read through everything, and it all looks reasonable except for a few minor formatting / wording issues. I'm looking forward to using this!

[aleitner]

Looks good on my initial pass, and code ran without issues as well

[mike-jumper]

I'm seeing some performance regressions in the terminal emulator, which makes some sense given that guac_display is very pixel-oriented.

I'll dig a bit deeper, but if nothing pops up as being a clear bottleneck specific to the terminal, I think it's probably best to roll the terminal emulator back to using its own change tracking until such future time as guac_display can support text-based displays without having to consider each pixel of the characters in the text.

[mike-jumper]

OK - I think I have a handle on the performance regression. The issue seems to have been mainly a memory bottleneck, with the solution being:

1. Use memcpy() where possible
2. Provide to mechanism to point a guac_display_layer at an external buffer (to avoid having to copy from the framebuffer of a client library)

I'm putting the above in place, plus a partial revert of the terminal emulator changes (migrating the display back to the character-based display).

[jmuehlner]

In testing this, I'm seeing some visual artifacts where windows dragged around in VNC have bits of the window content spread around, e.g.

[mike-jumper]

In testing this, I'm seeing some visual artifacts where windows dragged around in VNC have bits of the window content spread around, e.g.

This should now be resolved - it was caused by the GotCopyRect() function having been overridden. Previously, this was no issue because we used our own buffer. Now that we're reusing libvncclient's buffer, the original GotCopyRect() needs to be called or that buffer won't be updated with respect to received CopyRect updates.

[jmuehlner]

In testing this, I'm seeing some visual artifacts where windows dragged around in VNC have bits of the window content spread around, e.g.

This should now be resolved - it was caused by the GotCopyRect() function having been overridden. Previously, this was no issue because we used our own buffer. Now that we're reusing libvncclient's buffer, the original GotCopyRect() needs to be called or that buffer won't be updated with respect to received CopyRect updates.

Nice - I can confirm that this issue looks to be fixed.

[jmuehlner]

I've fully reviewed this and don't see any issues with the code as is - does anyone object if I merge this? @mike-jumper did you have any more that you wanted to add?

[necouchman]

I've fully reviewed this and don't see any issues with the code as is - does anyone object if I merge this? @mike-jumper did you have any more that you wanted to add?

There are a couple of unresolved comments above - one about a missing document block for a function, and one about a non-thread-safe check. Have these been resolved, even though Github is not marking them as outdated?

[mike-jumper]

@necouchman I'll take a look and make sure I've not missed anything.

@jmuehlner I just wrote a threaded display render loop that looks like it may improve things yet further for RDP. If that checks out, I'll push it in a moment.

[mike-jumper]

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.

[mike-jumper]

@jmuehlner The "easy mode" render loop is ready, working, and seems substantially better than the previous render loops. I've updated both VNC and RDP accordingly. If anyone needs something more complex than the provided loop, they can just invoke guac_display_end_multiple_frames() on their own as they see fit.

@necouchman As of right this moment, I think I've resolved all comments. :partying_face:

[jmuehlner]

Aside from a couple of minor typos in the comments, this looks good to me.

[corentin-soriano]

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.