Bug 250795 - support RFB 3.3 for VNC client interoperability
Summary: support RFB 3.3 for VNC client interoperability
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: bhyve (show other bugs)
Version: 12.1-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-virtualization (Nobody)
Depends on:
Reported: 2020-11-02 08:22 UTC by marko
Modified: 2020-12-18 00:39 UTC (History)
4 users (show)

See Also:

svn diff output of the relevant changes (4.62 KB, patch)
2020-11-02 08:22 UTC, marko
no flags Details | Diff
svn diff output of the relevant changes; ver2 (9.45 KB, patch)
2020-11-06 14:06 UTC, marko
no flags Details | Diff
svn diff output of the relevant changes; ver3 (10.87 KB, patch)
2020-11-08 17:22 UTC, marko
no flags Details | Diff
rfb.c diff (13.56 KB, patch)
2020-12-14 07:15 UTC, Peter Grehan
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description marko 2020-11-02 08:22:17 UTC
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.
Comment 1 Ed Maste freebsd_committer 2020-11-05 02:53:20 UTC
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
Comment 2 Peter Grehan freebsd_committer 2020-11-05 04:35:08 UTC
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.
Comment 3 Henrik Gulbrandsen 2020-11-05 06:48:14 UTC
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.
Comment 4 Henrik Gulbrandsen 2020-11-05 06:56:42 UTC
For some reason, Bugzilla refused to accept the patch as an attachment, so I have uploaded it to my web server for easy access:

Comment 5 marko 2020-11-05 07:50:27 UTC
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.
Comment 6 marko 2020-11-06 14:06:48 UTC
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.
Comment 7 marko 2020-11-08 17:22:44 UTC
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.
Comment 8 Henrik Gulbrandsen 2020-11-08 18:46:28 UTC
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.
Comment 9 Peter Grehan freebsd_committer 2020-11-08 23:35:28 UTC
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.
Comment 10 Peter Grehan freebsd_committer 2020-11-30 04:08:56 UTC
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.
Comment 11 Peter Grehan freebsd_committer 2020-12-14 07:15:45 UTC
Created attachment 220542 [details]
rfb.c diff

Will put up in phab shortly.
Comment 12 commit-hook freebsd_committer 2020-12-18 00:39:15 UTC
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

  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 :
   - tightvnc
   - tigervnc
   - krdc
   - vinagre

  Linux (Ubuntu)
   - krdc
   - vinagre
   - tigervnc
   - xtightvncviewer
   - remmina

   - 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