Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
    Nexarian
    @Nexarian

    I also don't know the driver code at all, I'm largely just railing against this out of pure passion/tenacity. I think that the lack of overall knowledge as to how xorgxrdp works something the XRDP community could use an improvement on (and I guess I'm trying to improve it by helping out myself)

    But I do like that strategy! I suspect it might be a rather large undertaking as the buffer in question may in fact be aliased to something internally represented in the X Window system, but X itself may also support that. But yeah, all of this is speculation at this point.

    At present I'm having a spot of trouble reproducing the issue consistently. It seems to reproduce most clearly over WAN connections with some latency (which makes sense since this is invoked by timer). On my local desktop, getting it to crash is really hard because I have two VirtualBox Ubuntu VMs connecting to each other, and the latency on that is nearly 0, and the resizing is rock solid. I can't get it to crash in spite of doing lots of crazy resizing.

    My next step is to test out with WAN connections the "suppress" flag and the "resize" flag that Jay added last year and see if either of them help the issue.

    I actually suspect the resize flag that was added last year: neutrinolabs/xorgxrdp@228e909 may have been for a very similar issue. The problem was that multiple callbacks for resize were being invoked at inopportune times, and this flag is essentially a global notification of "hold up, resizing, please wait"
    Nexarian
    @Nexarian
    But the good news is we may already have a video memory copy routine that could work here: https://github.com/neutrinolabs/xorgxrdp/blob/devel/module/rdpCapture.c#L1080
    Nexarian
    @Nexarian

    I think I've got it! The fix was indeed to move suppress around

    https://github.com/Nexarian/xrdp/tree/dynamic_monitor_xrdp_master

    Give this a whirl with FreeRDP /usr/local/bin/xfreerdp /bpp:32 /v:c[hostname]:3389 /network:lan /sound /u:[user] /p:[password] /rfx /dynamic-resolution /w:2560 /h:1396

    Note that the height is calibrated for Mac OS X Catalina on a 1440p display, it takes into account the height of the title and menu bar (assuming you have the dock minimized)

    1 reply
    Nexarian
    @Nexarian
    If I'm right, the fix was stupidly simple: Nexarian/xrdp@a74a0c5
    matt335672
    @matt335672

    Seems like a reasonable approach, but it still leavers us with a fragile xorgxrdp driver.

    A couple of other possibilities:-

    1. Have a global counter in xorgxrdp which is incremented on a resize. The callback can then check that it's working with the same video memory layout, and simply finish if it isn't.
    2. If a resize is detected in the video driver when output isn't suppressed, log a significant warning so we can track back and fix it later on.

    Hope that's useful.

    8 replies
    aquesnel
    @aquesnel
    @metalefty can you please take another look at #1767 since I have updated the pull request since your last comment. Thanks.
    metalefty
    @metalefty
    @aquesnel approved.
    Nexarian
    @Nexarian
    @matt335672 So I got a crash even with my fancy suppress output fix. But after a bit more intense debugging I think I figured out WHY it's crashing. I've posted details in the issue: neutrinolabs/xrdp#448
    Nexarian
    @Nexarian

    @metalefty @matt335672 Here's my first PR! I figured out the issue with xorgxrdp, and this should fix the issue and lock it up tight: neutrinolabs/xorgxrdp#183

    This change will help with overall driver stability regardless of the dynamic resizing, but it should be viewed as a pre-req to checking in the xrdp resizing feature.

    matt335672
    @matt335672
    Sounds very much like you're on the right track with this! I'll get round to a more significant review when I'm able.
    matt335672
    @matt335672

    General question - At the moment we've got a client_info structure in common/xrdp_client_info.h shared between xrdp and xorgxrdp. At the moment if we make an incompatible change to this structure in xrdp, users need to update xorgxrdp as well. If they don't, strange things happen with no real clues.

    Would it be a reasonable idea to add a guard value as the second longword here, so that we can pick up incompatible changes between the two and flag them in the driver? I was thinking of something like this:-

    struct xrdp_client_info
    {
      int size; /* bytes for this structure */
      int version; /* should be CLIENT_INFO_CURRENT_VERSION */
       . . .
    };
    . . .
    #define CLIENT_INFO_CURRENT_VERSION 20210225 /* yyyymmdd of last incompatible change */

    and in xrdp_rdp_create():-

        self->client_info.size = sizeof(self->client_info);
        self->client_info.version = CLIENT_INFO_CURRENT_VERSION;

    then in xorgxrdp:rdpClientConProcessMsgClientInfo()

    if (clientCon->client_info.version != CLIENT_INFO_CURRENT_VERSION)
    {
        LLOGLN(0, ("Incompatible client_info version detected (expected %d, got %d). Xorg is unlikely to work properly", CLIENT_INFO_CURRENT_VERSION, clientCon->client_info.version);
    }

    This is off the top of my head, BTW.

    If we make compatible changes to client_info, there's no need to bump the version number.

    Any thoughts?

    metalefty
    @metalefty
    I think that is a very good idea to detect xrdp/xorgxrdp version matches.
    xrdp.ini also has ini_version parameter to do a similar thing. actually, ini_version has never bumped yet.
    aquesnel
    @aquesnel
    I would encourage failing fast when a version mismatch is detected so that it is obvious that something is wrong.
    If there is a way for xrdp to get the version from xorgxrdp, that would allow sending a message to the user logging in and not just to the log file, which would make debugging version issues even easier.
    3 replies
    aquesnel
    @aquesnel
    As I've been re-doing the logging I've been fixing up the code style with astyle for all the files I've touched..
    I've also created a CI build step for checking the code formatting
    diff: https://github.com/neutrinolabs/xrdp/compare/devel...aquesnel:check_formatting
    example run: https://github.com/aquesnel/xrdp/runs/2000423635
    aquesnel
    @aquesnel
    Does anyone have feedback for eventually adding this code formatting check to the pull request CI builds once all the files have been updated to meet the code formatting style.
    Nexarian
    @Nexarian
    In my opinion, the formatting doesn't matter as long as we take steps to make it consistent.
    Because "good formatting" is arbitrary.
    Anyway, here's how to get dynamic resizing working on VNC! Nexarian/xrdp@f40451f
    matt335672
    @matt335672
    I've just raised neutrinolabs/xrdp#1816 as a placeholder for improving the Xorg backend startup. Comments welcome. Have I got the scope right?
    5 replies
    aquesnel
    @aquesnel
    #1816 looks good to me
    Nexarian
    @Nexarian

    I suspect based on the new PR I just submitted to XRDP regarding dynamic resize that something has broken with the cppcheck infrastructure: neutrinolabs/xrdp#1820

    Comparing the logs from an older commit to the one from this, it looks like something has changed with the libz3 installation.

    metalefty
    @metalefty
    Related to #1818, I wrote down my thougts on platform support policy. Just to be sure that we don't need to take special effort on m68k, s390 and sh processors even if #1818 is merged.
    I think Environment section in README is the only part we mention target platform so far. The purpose of the draft wiki page is make it clearer.
    https://github.com/neutrinolabs/xrdp/blob/f947843c2145b6770250f56155d34b2bcf44855d/README.md#environment
    matt335672
    @matt335672
    @metalefty - that looks like a reasonable statement to me.
    1 reply
    @Nexarian - I'll have a look at the cppcheck issue and figure out what's going on. Today's a little awkward but I'll see how far I can get.
    matt335672
    @matt335672

    cppcheck issue is (I think) caused by ubuntu-latest migrating to focal, but the cached cppcheck is still built with 18.04.

    See Z3Prover/z3#660

    Possible solutions:-

    • Get the OS version in the cache tag name. I haven't found a simple way to do this, but I've not been looking for long.
    • Move the cppcheck VM to a specific OS version rather than 'ubuntu-latest'

    Thoughts anyone?

    matt335672
    @matt335672
    Already raised by others, but no obvious solution yet.
    actions/cache#543
    matt335672
    @matt335672
    Fix for cppcheck - see neutrinolabs/xrdp#1821 for discussion
    1 reply
    Nexarian
    @Nexarian
    I just figured out what the resize bug with FreeRDP and /gfx was!
    Because gfx works with surfaces, each surface has to be manually resized on screen resize, which means I have to run this sequence of commands every time:
    xrdp_egfx_send_delete_surface(wm->mm->egfx, 1);
    xrdp_egfx_send_reset_graphics(wm->mm->egfx, session_width, session_height, wm->client_info->monitorCount, wm->client_info->minfo_wm);
    xrdp_egfx_send_create_surface(wm->mm->egfx, 1, session_width, session_width, XR_PIXEL_FORMAT_XRGB_8888);
    xrdp_egfx_send_map_surface(wm->mm->egfx, 1, 0, 0);
    So at this point, I know of no bugs with /dynamic-resolution anymore
    Nexarian
    @Nexarian

    Spoke too soon. There's still something wrong with my dynamic resize + egfx branch. For some reason, XRDP is sending too many tiles to freerdp using XR_RDPGFX_CMDID_WIRETOSURFACE_2 and I cannot seem to figure out why. A great way to reproduce it is to start at 1000x1000 and then scale to 1000x1250 (drag the height down), and then all of a sudden XRDP sends nearly 50 more tiles. I've tried resetting the encoder and it doesn't help, either.

    I'll take a look at this later, probably busy for the rest of the week.

    p4r4d0xshado
    @p4r4d0xshado:matrix.org
    [m]
    Hello
    I have a kali Linux machine on azure and i try to use xrdp i set it up and then i try to connect to it and it keeps disconnecting as soon as i connect?
    metalefty
    @metalefty
    maybe you should read the channel topic
    p4r4d0xshado
    @p4r4d0xshado:matrix.org
    [m]
    Oh my bad
    Nexarian
    @Nexarian
    The code I posted above has a bug. Can anyone find it? (Fun puzzle!)
    Derek Schrock
    @derekschrock
    Seems a little xorgxrdp-ish but for Wayland and is mutter only.... https://www.phoronix.com/scan.php?page=news_item&px=GNOME-40-Headless-Virtual
    Reading threw the HN article it seems they're doing this since they're not using wlroots. KDE/Gnome no wlroots everyone else uses wlroots.
    Is wlroots the path fro xrdp to have Wayland support?
    Satish Gaikwad
    @satishweb

    Hi Team. I had been an XRDPuser for the last month, and I love it! Thank you so much for making my life easier! I had been using XPRA for years, and it took only a few days for me to switch after getting exposed to xrdp.

    I am unsure if this is the right place to post this. Please do let me know if I should post this message/request somewhere else.

    I want to showcase my container image project for xrdp here for your feedback and suggestions. I hope you all like it. I would appreciate it if you could spend few minutes trying it out.

    Use case:- Recently, I moved all of my VM deployments to Kubernetes by converting everything into container images. For the XRDP server VM, I looked at existing container images available in the public container repositories. However, all of them were pretty old, not maintained, and too complex to understand. I spent some time yesterday making xrdp work inside a container. I have a fully working container image for xrdp on Ubuntu. I think that this image is simple to understand as compared to other available images.

    This container image is for those who do not want to set up a VM with GUI with XRDP, or all of their workloads are in containers, and they require GUI access in the remote network. I intend to continue working on this container image project and maintain it besides other images that I have built.

    Based on the suggestions and feedback, I want to add some more features to it below list:

    1. Build container image as soon as a new stable version of xrdp is released.
    2. Add container images for other popular distros.
    3. Automated tests of newly built container image with CI pipeline (I have free infrastructure resources).
    4. Add more configuration options for the container image.
    5. Link container image tag versions to the XRDP version inside the image.

    Run the container with XRDP

    docker run --rm -itd -e GUEST_PASS='guest' -p 3389:3389 -v ${pwd}/data/home:/home/guest --name xrdp satishweb/xrdp

    Connect to container using remote desktop on localhost:3389 and login with user guest and password as guest

    Note:- This is still a work in progress. There is a lot of scope for optimization for sure.

    5 replies
    Satish Gaikwad
    @satishweb
    Correct command: docker run -d -e GUEST_PASS='guest' -p 3389:3389 -v ${pwd}/data/home:/home/guest --name xrdp satishweb/xrdp
    Nexarian
    @Nexarian

    I'm not sure if this is heresy or awesome. I have XRDP working with all these features at the same time:

    1. Dynamic resolution/resize without reconnection
    2. GFX compression (H264)
    3. The wyhash optimization PR
    4. Nvidia acceleration

    Here are the branches:

    1. https://github.com/Nexarian/xrdp/tree/resize_egfx_xrdp_features
    2. https://github.com/Nexarian/xorgxrdp/tree/resize_wyhash_egfx_nvidia_features

    They're rebased off the devel branches, and I've tried to keep the branches clean such that the individual commits on top of devel correspond to their respective WIP features

    Obviously there's a lot of work and testing and bug fixing before all of that can go into devel or be released, but I wanted to show that all of this can work.

    Going forward I'm going to be daily driving these branches.

    This answers the questions as to whether or not nvidia_hack can work with the gfx optimizations. It can!

    Satish Gaikwad
    @satishweb
    There has been 45+ image pulls so far. Thank you very much for trying it out, hoping for some feedback/suggestions. Happy weekend!
    8 replies
    aquesnel
    @aquesnel
    FYI: the guthub CI builds are broken and I've documented my investigation here: https://github.com/neutrinolabs/xrdp/pull/1826#issuecomment-799077185
    The root cause is an update to the VM image by github and there is already an issue opened: actions/virtual-environments#2917
    1 reply