GFX tidy-ups

[matt335672]

A number of tidy-ups to the GFX code:-

Edit 2024-1-25 : NeutrinoRDP is now working with GFX. Comment updated

Fixes

• Don't enable GFX if the client doesn't support 32 bpp.

  mstsc.exe sets 'GFX available' in the early capability flags for bpp < 32, but when you try to use it, mstsc.exe fails.

• Add error checking to the xrdp_mm_egfx_send_planar_bitmap() codepaths

  Without this, you can end up with xrdp processes which don't exit when the client exits.

• Improved codec logging when creating an encoder thread.

• Fixed behaviour if a GFX codec cannot be chosen.

Changes

• If using GFX, always wait for the egfx virtual channel to come up before starting the Window Manager state machine.

  This keeps the codepath for autologin the same as the non-autologin case, and makes sure the channel is up before we draw anything.

• Only start the encoder thread for xorgxrdp as other session types don't need it.

  This is a fairly significant change and encompasses 4 commits in this PR.

• Update NeutrinoRDP to not pass the "drdynvc" virtual channel through to the target.

  Dynamic virtual channels for neutrinordp have not been working since v0.9.9. This doesn't fix that, but it does prevent a target which attempts to use drdynvc from blowing up GFX.

What's working

• GFX/RFX/no-codec session types tested with mstsc.exe and xorg and xvnc backends
• neutrinordp

What's not working

• neutrinordp mouse buttons. This appears to be a regression in devel, so not related to GFX.

[metalefty]

Thanks for the tidy-up! All the changes you made are significant for stability.

[matt335672]

I've worked out what's happening with NeutrinoRDP.

The neutrinordp target is sending a DVC Capabilities Request PDU [MS-RDPEDYC] through xrdp to the other end. This is responding with a DVC Capabilities Response PDU which we are unpacking and then not forwarding to the target (via neutrinordp) but processing it ourselves. This results in a second call to xrdp_mm_drdynvc_up() and hence egfx_initialize().

As fas as I can tell, this won't have worked properly since #1224 was merged for v0.9.9 in Oct 2018. So the simplest fix for now is probably:-

1. to not pass DRDYNVC through to the target as it won't get a capabilities response PDU since v0.9.9
2. Raise an issue on this. No users have yet done so.

[Nexarian]

I'll take a look at this today. There's A LOT of code here that make what look to be significant changes to the workflow. That being said, all of this looks super important for stability and thanks for doing so!

[Nexarian]

Also update the comment on the PR about "what's not working" :)

[matt335672]

Header comment updated!

The only significant change to the workflow is the encoder I think.

• If GFX is available we always start a GFX surface and use that. If xup isn't loaded, xrdp_mm_egfx_send_planar_bitmap() is used for all updates (login screen, vnc, neutrinordp). This doesn't need an encoder.
• When xup is loaded, it starts the encoder as soon as the connection is confirmed. It can then make the module calls which use it.

[Nexarian]

Something in this breaks resize-on-the-fly with RFX Progressive with the Mac OS client. There are multiple really good fixes here. I will cherry-pick and merge the ones that I've verified don't break anything.

To test resize-on-the-fly if you don't have a MacBook you can use FreeRDP. Unfortunately Microsoft put DRM protection on the .RDP files for the Azure Windows RDP client (yes, DRM for an RDP client...) so there is still no easy-to-access Windows client (other than FreeRDP) that supports resize-on-the-fly for testing.

[matt335672]

That sounds good - thanks. I'll wait for you.

Once you've done the cherry-picking I'll slim these commits down, rebase them and figure out what the problem is (if it happens on FreeRDP).

[Nexarian]

cf7bcf5 works but if you switch between 16 bit and 32 bit the session will still crash. Still, it's a good fix and I'll merge it in. We need to fix that bug though (it might also be in devel/release as of today though)

[Nexarian]

bab04ed seems to cause issues with resize, skipping that one.

[Nexarian]

c17fb9c worked great. Merged.

[Nexarian]

3ddf526 broke resizing. Skipping.

[Nexarian]

I couldn't get 9725c79 and f246658 to work, it simply created a dead session.

[Nexarian]

45e7412 also breaks resizing.

At this point I think it's probably appropriate to rebase and get a copy of FreeRDP to validate the rest of these changes work with resizing.

That's all I'll do for now.

[matt335672]

You comment bout switching between 16 and 32 bit is interesting - this means switching between GFX and non-GFX sessions. Possibly there's some state info hanging around in xorgxrdp which causes it.

I'll rebase and reproduce the problems you mention above.

[matt335672]

I'm having problems getting dynamic resizing working at all with FreeRDP.

xrdp: 7277860
xorgxrdp : neutrinolabs/xorgxrdp@b93ad93

Command line I'm using is:-

xfreerdp /gfx /v:xrdp-test.test.lan /u:testuser /d: /p: /dynamic-resolution

With FreeRDP 3.2, when I try to resize the client I get a blank screen and this in the log:-

[10:44:21:341] [23017:000059ea] [WARN][com.freerdp.core.rdp] - [rdp_handle_sc_flags][0x55560861a230]: [CONNECTION_STATE_FINALIZATION_CLIENT_SYNC] unexpected server message, expected flag FINALIZE_SC_SYNCHRONIZE_PDU [0x00000001] [have NO_FLAG_SET [0x00000000]]
[10:44:21:342] [23017:000059ea] [WARN][com.freerdp.core.rdp] - [rdp_handle_sc_flags][0x55560861a230]: [CONNECTION_STATE_FINALIZATION_CLIENT_SYNC] unexpected server message, expected flag FINALIZE_SC_SYNCHRONIZE_PDU [0x00000001] [have NO_FLAG_SET [0x00000000]]
[10:44:21:442] [23017:000059ea] [WARN][com.freerdp.core.rdp] - [rdp_handle_sc_flags][0x55560861a230]: [CONNECTION_STATE_FINALIZATION_CLIENT_SYNC] unexpected server message, expected flag FINALIZE_SC_SYNCHRONIZE_PDU [0x00000001] [have NO_FLAG_SET [0x00000000]]

with FreeRDP 2.90 I don't get the messages, but I get a blank screen.

@Nexarian - I'll carry on investigating, but if you could post what version of FreeRDP works for you (and what the command line is) that might be useful.

[matt335672]

See #2929 - I'll come back to the above when I've got a better handle on how resize is supposed to work - at the moment I'm not convinced I'm on top of the spec.

[matt335672]

Rebased, but please ignore for now. I'm going to look into whether I can optimise the GFX path through the resize state machine first.

[matt335672]

I'm going to close this now. Given the way resize is going to work, I think we're going to need the encoder for all GFX codepaths.

We're working a lot on client-initiated dynamic resize at the moment. The server-initiated resizes (i.e. via xrandr) also need some looking at, possibly after our first GFX drop.

If we keep the GFX instructions in xorgxrdp rather than moving them into xrdp we'll need to add corresponding code to the vnc module. This doesn't feel quite right to me, but we can look into that later.

[Nexarian]

we'll need to add corresponding code to the vnc module. This doesn't feel quite right to me, but we can look into that later.

Agreed.