Provided x11 example - glitchy screen in client

[nicmorais]

Describing the bug When running the x11.c example under examples/server, the client's screen receives a glitchy image. The glitch changes when draging windows in the server's desktop.

To Reproduce Downloaded the library from source. Built it with GCC, and executed the x11 example without arguments. Connected to the server with AVNC, also tried with xtightvncviewer. Client and server were both in the same network.

Expected Behavior I expected to watch to the desktop in the client end with no major glitches.

Logs/Backtraces

16/01/2023 14:52:03 Listening for VNC connections on TCP port 5900
16/01/2023 14:52:03 Listening for VNC connections on TCP6 port 5900
16/01/2023 14:52:06   0 other clients
16/01/2023 14:52:06 Normal socket connection
16/01/2023 14:52:06 Client Protocol Version 3.8
16/01/2023 14:52:06 Protocol version sent 3.8, using 3.8
16/01/2023 14:52:06 rfbProcessClientSecurityType: executing handler for type 1
16/01/2023 14:52:06 rfbProcessClientSecurityType: returning securityResult for client rfb version >= 3.8
16/01/2023 14:52:06 Pixel format for client 192.168.99.242:
16/01/2023 14:52:06   32 bpp, depth 24, little endian
16/01/2023 14:52:06   true colour: max r 255 g 255 b 255, shift r 0 g 8 b 16
16/01/2023 14:52:06 rfbProcessClientNormalMessage: ignoring unsupported encoding type ultraZip
16/01/2023 14:52:06 Using compression level 3 for client 192.168.99.242
16/01/2023 14:52:06 Using image quality level 5 for client 192.168.99.242
16/01/2023 14:52:06 Using JPEG subsampling 2, Q77 for client 192.168.99.242
16/01/2023 14:52:06 Enabling X-style cursor updates for client 192.168.99.242
16/01/2023 14:52:06 Enabling full-color cursor updates for client 192.168.99.242
16/01/2023 14:52:06 Enabling cursor position updates for client 192.168.99.242
16/01/2023 14:52:06 Enabling KeyboardLedState protocol extension for client 192.168.99.242
16/01/2023 14:52:06 Enabling NewFBSize protocol extension for client 192.168.99.242
16/01/2023 14:52:06 Enabling ExtDesktopSize protocol extension for client 192.168.99.242
16/01/2023 14:52:06 Enabling LastRect protocol extension for client 192.168.99.242
16/01/2023 14:52:06 Enabling SupportedMessages protocol extension for client 192.168.99.242
16/01/2023 14:52:06 Enabling SupportedEncodings protocol extension for client 192.168.99.242
16/01/2023 14:52:06 Enabling ServerIdentity protocol extension for client 192.168.99.242
16/01/2023 14:52:06 rfbProcessClientNormalMessage: ignoring unsupported encoding type Enc(0xFFFFFEFE)
16/01/2023 14:52:06 Using tight encoding for client 192.168.99.242
16/01/2023 14:52:06 Sending rfbEncodingExtDesktopSize for size (1920x1080) 

Environment: Server:

• Ubuntu 22
• KDE Plasma 5.24.7 (X11)

Client (tried with):

• Debian 11 (xtightvncviewer);
• Android 8 (AVNC)

Additional Information When client and server are running in the same machine, there are no glitches.

[bk138]

Yeah this is a known bug, see https://github.com/LibVNC/libvncserver/pull/503#issuecomment-1064472566 which is also mentioned in the source code.

If you can work it out together with @hubenchang0515 that'd be awesome.

[nicmorais]

Hello, @bk138 Sorry, I searched for the mentions of the word "glitch" but did not find any issues. I'm currently trying to figure out what's wrong, but it seems like @hubenchang0515 knows XCB/X11 better than me.

Also, KRFB seems to rely on the same principle as x11.c. Such example did not use XDamage to map the modified regions, I think that would be useful (and maybe faster).

Since the glitch effect does not occur in KRFB, we could look on what they did...

But I'll keep trying my best and will post here my progress. Thanks in advance!

[bk138]

Thanks! You might also want to take a look at x11vnc, but this uses Xlib afaict.

[hubenchang0515]

I can't reproduce this error.

You can change the convert_bgrx_to_rgb as this to save pixels as picture file(/tmp/debug.ppm).

Then you can check if the code read x11 and convert color correctly.

void convert_bgrx_to_rgb(const uint8_t* in, uint16_t width, uint16_t height, uint8_t* buff)
{
    for (uint16_t y = 0; y < height; y++)
    {
        for(uint16_t x = 0; x < width; x++)
        {
            buff[(y*width+x)*3] = in[(y*width+x)*4 + 2];
            buff[(y*width+x)*3 + 1] = in[(y*width+x)*4 + 1];
            buff[(y*width+x)*3 + 2] = in[(y*width+x)*4];
        }
    }

    FILE* fp = fopen("/tmp/debug.ppm", "wb");
    fprintf(fp, "P6\n%u %u 255\n", width, height);
    fwrite(buff, width * height * 3, 1, fp);
    fclose(fp);
    exit(1);
}  

If the file is correct, this error most likely happened in dirty_copy.

My Test

RealVNC on iPad:

AVNC on Android:

My environment

[bk138]

I did some tests using MultiVNC which allows settings the preferred encoding: only getting glitches when using Tight, which is in line with your findings that it does not happen on localhost @nicmorais. Can you reproduce @nicmorais @hubenchang0515 ?

[nicmorais]

Hello. Thanks a lot for this project and the attention given to my issue.

With the code snippet @hubenchang0515 has provided, I can see the picture at /tmp/debug.ppm without any glitches. The actual issue seems to be related with Tight encoding, when I connected to the session with x11.c example and xtightvncviewer, passing the -encodings "tight" parameter, it gets all glitchy. Even with both client and server running on the same machine (connecting to localhost).

When connecting to the VNC session of x11.c with -encoding "copyrect", no glitches appear, but I've only tried this with localhost connection. I'll try it with a remote connection tomorrow, but I would like to be able to use tight encoding because it has showed the lowest delay so far, and is also compatible with AVNC "out of the box".

[bk138]

Well the thing is @nicmorais: Tight works well with other servers. It's complicated...

[VelorumS]

Doesn't memcpy to the frameBuffer need some locking? How this can't be glitchy?

(sorry if I don't understand something, it's my first look at the library)

[VelorumS]

The actual problem is that there are 24 bits per pixel while the current tight implementation supports only 8, 16 and 32.

So put 4 as bytesPerPixel here (and fix the image copy code):

rfbScreenInfoPtr rfbScreen = rfbGetScreen(&argc, argv, (int)width, (int)height, 8, 3, 3);

I think that there should be something like the framebuffer mutexes locks while clients are encoding.

[bk138]

Always happy about a good pull request @ChipmunkV :-)

[nicmorais]

@ChipmunkV . Thanks, that fixed, @josealvim figured out:

int bytesPerPixel = 4;
rfbGetScreen(&argc, argv, (int)width, (int)height, 8, 3, bytesPerPixel);
rfbScreen->frameBuffer = (char*)malloc(bytesPerPixel * width * height);

And:

void convert_bgrx_to_rgb(const uint8_t* in, uint16_t width, uint16_t height, uint8_t* buff)
{
    for (uint16_t y = 0; y < height; y++)
    {
        for(uint16_t x = 0; x < width; x++)
        {
            buff[(y*width+x)*4] = in[(y*width+x)*4 + 2];
            buff[(y*width+x)*4 + 1] = in[(y*width+x)*4 + 1];
            buff[(y*width+x)*4 + 2] = in[(y*width+x)*4];
        }
    }
}

That fixes the screen for tight encoding. I'll be busy in the next days, so not enough time to make a pull request...

@bk138 feel free to close this issue. Thanks!

[bk138]

@nicmorais I can commit this, but who's the author? Shall we credit @josealvim?

[josealvim]

Good morning! I haven't tested it personally, I was merely attempting to avoid my work with Java by helping @nicmorais

From what I could tell, it solved his issue, though. If you can commit it and feel like it, you may credit me lol. Thank you for your time!

[bk138]

@nicmorais @josealvim getting a segfault with these changes, here's the patch based on https://github.com/LibVNC/libvncserver/issues/546#issuecomment-1431457016:

diff --git a/examples/server/x11.c b/examples/server/x11.c
index e4915817..bcc44287 100644
--- a/examples/server/x11.c
+++ b/examples/server/x11.c
@@ -2,7 +2,6 @@
 // Need CMake 3.24.0 to find these libraries. see https://cmake.org/cmake/help/v3.24/module/FindX11.html
 // XWayland not support to read screen, because wayland not allow it.
 // Read screen in wayland need use XDG desktop portals' interface `org.freedesktop.portal.Screenshot` and `org.freedesktop.portal.ScreenCast`
-// Under some environment, this code not work well, see https://github.com/LibVNC/libvncserver/pull/503#issuecomment-1064472566

 #include <rfb/rfb.h>
 #include <xcb/xcb.h>
@@ -65,9 +64,9 @@ int main(int argc, char* argv[])
     get_window_size(conn, root, &width, &height);
     void* frameBuffer = malloc(3UL * width * height);

-    rfbScreenInfoPtr rfbScreen = rfbGetScreen(&argc, argv, (int)width, (int)height, 8, 3, 3);
+    rfbScreenInfoPtr rfbScreen = rfbGetScreen(&argc, argv, (int)width, (int)height, 8, 3, 4);
     rfbScreen->desktopName = "LibVNCServer X11 Example";
-    rfbScreen->frameBuffer = (char*)malloc(3UL * width * height);
+    rfbScreen->frameBuffer = (char*)malloc(4 * width * height);
     rfbScreen->alwaysShared = TRUE;
     rfbScreen->kbdAddEvent = keyCallback;
     rfbScreen->ptrAddEvent = mouseCallback;
@@ -117,9 +116,9 @@ void convert_bgrx_to_rgb(const uint8_t* in, uint16_t width, uint16_t height, uin
     {
         for(uint16_t x = 0; x < width; x++)
         {
-            buff[(y*width+x)*3] = in[(y*width+x)*4 + 2];
-            buff[(y*width+x)*3 + 1] = in[(y*width+x)*4 + 1];
-            buff[(y*width+x)*3 + 2] = in[(y*width+x)*4];
+            buff[(y*width+x)*4] = in[(y*width+x)*4 + 2];
+            buff[(y*width+x)*4 + 1] = in[(y*width+x)*4 + 1];
+            buff[(y*width+x)*4 + 2] = in[(y*width+x)*4];
         }
     }
 }

[josealvim]

I'll check with Nicolas and see if he changed something else that made it work; because I saw it working on his end.

[nicmorais]

@bk138 Hello again. Sorry, I forgot to say about some other lines I need to change to fix it. #559 has it all, I've just tested and it works as expected.

Thanks in advance!