Created attachment 219296 [details] svn diff output of the relevant changes Apple's screen sharing app uses RFB 3.3 for authentication. Luckily this is not very different from currently supported RFB 3.8. Including a patch which adds support for RFB 3.3. Another thing which had to be changed was the removal of first unsolicited TX of screen contents right after connection gets established. After this I can use Apple's tool as my VNC client.
Applying the patch to head I get: Patching file usr.sbin/bhyve/rfb.c using Plan A... Hunk #1 succeeded at 223 (offset 2 lines). Hunk #2 succeeded at 628 (offset 2 lines). Hunk #3 succeeded at 643 (offset 2 lines). Hunk #4 succeeded at 760 (offset 2 lines). Hunk #5 failed at 774. Hunk #6 succeeded at 867 (offset 2 lines). Hunk #7 succeeded at 878 (offset 2 lines). Hunk #8 succeeded at 918 (offset 2 lines). Hunk #9 succeeded at 938 (offset 2 lines). 1 out of 9 hunks failed--saving rejects to usr.sbin/bhyve/rfb.c.rej
I was able to apply the failed portion of the patch manually. Amazing to see VNC display in the macos client :) There do appear to be some issues though: a disconnect and reconnect (and sometimes the first connect) will fail maybe 40% of the time. Perhaps a race with the first thread and the update message ? There are also some issues with VNC viewer - every now and then it won't refresh on connect. This wasn't the case prior to the patch - perhaps another interaction with the update message.
This is just a reminder that I had to make big changes in the RFB code to make it work with TigerVNC while I was working on the Video BIOS. Unsolicited updates and occasional refresh problems were among the issues I fixed. From my notes: "While debugging, I noticed that the display was not correctly updated when I filled it with black, although it worked well for other fill colors. This was because the CRC values had been cleared in the recent resize, so rfb.c thought the display was already black. I fixed the problem by adding an invalid variable in rfb.c, to make sure that all data is sent in the update following the resize." My patch for rfb.c still applies cleanly to 12.2-STABLE, but the patch in this bug report will almost certainly give conflicts, so I thought I should mention my changes again before they become hopelessly obsolete.
For some reason, Bugzilla refused to accept the patch as an attachment, so I have uploaded it to my web server for easy access: https://www.gulbra.net/freebsd-bhyve/RFB_changes_for_Video_BIOS.diff
Thanks for the review, and comments. I'll switch over to head of trunk to do follow-up dev on this code. My bad for not checking the patch against it. I agree, TX of screen updates needs a bit of followup work. We should honor the order of client request vs. first TX of server screen data, which hopefully should address that first refresh. I'll look into that. I tested the code with TigerVNC also, rudimentary tests showed it behaving ok. Admittedly, didn't test it very thoroughly. Interesting observation regarding combination of all black screen block & CRC. It seemed to me that the CRC variation here would produce non-zero results for all black data, but I haven't validated that.
Created attachment 219393 [details] svn diff output of the relevant changes; ver2 Here's an updated version of the patch, which includes additional performance fixes. I incorporated Henrik's changes from the pointed diff; the most important performance improvement from there was the fact that the server would not send an update until client requests one. I also reduced the amount of memory allocated for CRC buffers. It was computed slightly incorrectly, but ended up allocating 32 times too much memory. Behaviour changes are now that a) support RFB protocol versions 3.3 and 3.7 for authentication, b) don't send first screen until client asks for it c) don't send updates (at all) unless client has asked for it AND we have data to send. I've tested this against tightVNC, realVNC, apple's screen sharing, tigerVNC, krdc and vinagre.
Created attachment 219467 [details] svn diff output of the relevant changes; ver3 Modified to make the server more responsive. Reception of client update request wakes writer thread immediately, instead of using timer only for the wakeup.
As far as I can tell, the entire point of the writing thread is to limit the VNC frame rate to ~25Hz, so we don't use 100% of the CPU for display updates. Otherwise, you could just call rfb_send_screen() directly from rfb_recv_update_msg(), like I did in "#ifdef USE_MAXIMUM_FRAME_RATE" in my patch. With an immediate wakeup, you rely on the client to limit the frame rate.
ver2 works well with a wide range of VNC clients. Yet to try ver3 but am interested to see if clients are pacing the requests.
v3 works great and has very snappy mouse-tracking, but the downside is it uses 8.5% host CPU time when the guest is idle, compared to 2.0% for the v2 patch. I have a modified version of v2 that double the update frequency, but only updates every 2nd iteration unless there is keyboard/mouse movement detected from the VNC input stream. This appears to preserve the mouse-tracking improvements in the v3 patch, but is now down to 2.2% host CPU with an idle guest. Will post an updated diff shortly.
Created attachment 220542 [details] rfb.c diff Will put up in phab shortly.
A commit references this bug: Author: grehan Date: Fri Dec 18 00:38:48 UTC 2020 New revision: 368747 URL: https://svnweb.freebsd.org/changeset/base/368747 Log: Fix issues with various VNC clients. - support VNC version 3.3 (macos "Screen Sharing" builtin client) - wait until client has requested an update prior to sending framebuffer data - don't send an update if no framebuffer updates detected - increase framebuffer poll frequency to 30Hz, and double that when kbd/mouse input detected - zero uninitialized array elements in rfb_send_server_init_msg() - fix overly large allocation in rfb_init() - use atomics for flags shared between input and output threads - use #defines for constants This work was contributed by Marko Kiiskila, with reuse of some earlier work by Henrik Gulbrandsen. Clients tested : FreeBSD-current - tightvnc - tigervnc - krdc - vinagre Linux (Ubuntu) - krdc - vinagre - tigervnc - xtightvncviewer - remmina MacOS - VNC Viewer - TigerVNC - Screen Sharing (builtin client) Windows 10 - Tiger VNC - VNC Viewer (cursor lag) - UltraVNC (cursor lag) o/s independent - noVNC (browser) using websockify relay PR: 250795 Submitted by: Marko Kiiskila <marko@apache.org> Reviewed by: jhb (bhyve) MFC after: 3 weeks Relnotes: yes Differential Revision: https://reviews.freebsd.org/D27605 Changes: head/usr.sbin/bhyve/rfb.c