Use fourcc value from the guest to set pixel format for VNC.#151
Use fourcc value from the guest to set pixel format for VNC.#151jordanhendricks merged 1 commit intooxidecomputer:masterfrom
Conversation
server/Cargo.toml
Outdated
| propolis = { path = "../propolis", features = ["crucible"], default-features = false } | ||
| propolis-client = { path = "../client" } | ||
| rfb = { git = "https://github.com/oxidecomputer/rfb", branch = "main" } | ||
| rfb = { git = "https://github.com/oxidecomputer/rfb", branch = "fourcc" } |
There was a problem hiding this comment.
Note: this is just here for until oxidecomputer/rfb#6 is merged -- I'll switch it back to the main branch before merging.
| match fourcc { | ||
| // edk2 default - xRGB, 4 bytes per pixels | ||
| // The edk2 default: XR24, little-endian xRGB with 4 bytes per pixel. | ||
| 0x34325258 => Some(4), |
There was a problem hiding this comment.
Can/should this be rfb::pixel_formats::fourcc::FOURCC_XR24?
| let fb_spec = config.get_framebuffer_spec(); | ||
| let fb = RamFb::new(fb_spec); | ||
| self.initialize_framebuffer(fb.clone()).await; | ||
|
|
||
| match fourcc::fourcc_to_pixel_format(fb.fourcc) { | ||
| Ok(pf) => { | ||
| vnc_server.set_pixel_format(pf).await; |
There was a problem hiding this comment.
Small nit: Does this routine require that the caller have exclusive access to vnc_server? By itself and without that requirement, it looks like there's a race here (if two callers supply different configs, the last one to call initialize_framebuffer won't necessarily be the last one to call set_pixel_format, so the two could get out of sync).
The only caller I can see locks the vnc_server it provides before calling this function, so I don't think there's a bug here so much as a small seam that could tear later.
Would it make sense to make vnc_server a &mut reference to clarify that only one caller at a time can call this with the same server?
|
Merging for now because the related rfb PR is merged and will break propolis otherwise. I'll address Greg's feedback in a follow up PR. |
2 participants
Summary
This PR uses the new interfaces added in oxidecomputer/rfb#6 to set the pixel format for the VNC server based on the guest's fourcc. Fourcc is a 4-byte code that maps to a pixel format. Support for understanding these codes has been added into the rfb crate.
Details
Previously, when booting a windows VM and viewing its framebuffer with VNC, one might see orange where the screen should be blue (such as the "Orange Screen of Death"). Here is an image of an orange screen that should be blue:
The problem here was two-fold: first, the VNC server in the rfb crate didn't have a way for consumers to specify what the input pixel format is (which in this case, we get from the fourcc value of the guest). Second, the rfb protocol allows for clients to request an alternate pxiel format, and the rfb crate didn't have support for translating between formats. So the client would display pixels in the format it requested, by the pixels wouldn't actually be in that format. Both of these issues are addressed for our purposes in oxidecomputer/rfb#6.
Testing
Here's an example of the windows lock screen before -- note that the sky is orange (orange where there should be blue):
Now, when we connect to a windows VM, the colors look correct, regardless of the pixel format requested by the client.
Example from noVNC (which requests pixel format little-endian xBGR):
Example from tigervnc (which requests pixel format little-endian xRGB):
