TigerVNC / tigervnc

High performance, multi-platform VNC client and server
https://tigervnc.org
GNU General Public License v2.0
5.24k stars 954 forks source link

Invalid pixel format when bpp = 24 #1867

Closed any1 closed 1 week ago

any1 commented 1 week ago

When the server reports a pixel format with bpp set to 24, vncviewer complains about an "invalid pixel format". Instead of failing, the client could override the server's pixel format preference, but it does not do this.

WayVNC running on NVidia hardware will report a preferred bpp of 24. This is simply because the graphics driver will supply 24 bit buffers.

To Reproduce Compile the following and run it:

// gcc -o 24-bit-test 24-bit-test.c -lneatvnc -laml $(pkg-config --libs --cflags pixman-1) -DAML_UNSTABLE_API=1

#include <neatvnc.h>

#include <stdio.h>
#include <aml.h>
#include <signal.h>
#include <assert.h>
#include <pixman.h>
#include <libdrm/drm_fourcc.h>

static void on_sigint()
{
    aml_exit(aml_get_default());
}

int main(int argc, char* argv[])
{
    struct aml* aml = aml_new();
    aml_set_default(aml);

    struct nvnc* server = nvnc_open("127.0.0.1", 5900);
    assert(server);

    struct nvnc_display* display = nvnc_display_new(0, 0);
    assert(display);

    nvnc_add_display(server, display);
    nvnc_set_name(server, "24-bit-test");

    struct nvnc_fb* fb = nvnc_fb_new(512, 512, DRM_FORMAT_RGB888, 512);
    assert(fb);

    struct pixman_region16 damage;
    pixman_region_init_rect(&damage, 0, 0, nvnc_fb_get_width(fb),
            nvnc_fb_get_height(fb));
    nvnc_display_feed_buffer(display, fb, &damage);
    pixman_region_fini(&damage);

    struct aml_signal* sig = aml_signal_new(SIGINT, on_sigint, NULL, NULL);
    aml_start(aml_get_default(), sig);
    aml_unref(sig);

    aml_run(aml);

    nvnc_close(server);
    nvnc_display_unref(display);
    nvnc_fb_unref(fb);
    aml_unref(aml);
}

Expected behavior The client is free to request any format that it prefers. If it can't cope with what the server suggests, it should pick something that works before requesting the first framebuffer.

Client:

Server:

Additional context The issue was reported by @drigoskalwalker here: https://github.com/any1/wayvnc/issues/335

CendioOssman commented 1 week ago

24 bit bpp is invalid according to the RFB specification. Are you sure you don't mean depth?

We abort on invalid pixel formats as a way to detect that we are likely out of sync with the protocol stream for some reason. So those checks are not something we want to remove as it makes things much harder to debug.

any1 commented 1 week ago

No, I meant bpp, but you are right. I didn't even think to check the spec for this. Thanks for reminding me of this fact!

I wasn't suggesting to remove the checks though, just request a different format is the checks fail. But if the server is out of spec, it's valid to fail.