Resize state machine: A fix and a question

[matt335672]

The commit is the easy bit.

This commit allows non-GFX sessions to resize.

At present the EGFX state is destroyed by states WMRZ_EGFX_DELETE_SURFACE through WRMZ_EGFX_DELETE. This means that at WMRZ_EGFX_INITIALIZE we cannot distinguish between EGFX not being ever used, and EGFX having been torn down. Consequently, when running non-GFX, we don't correctly recover the session.

Now for the question.

With FreeRDP 3.2.0 the non-GFX codepath is working, but I get a lot of messages of this form:-

[15:25:21:766] [49323:0000c0ac] [WARN][com.freerdp.core.rdp] - [rdp_handle_sc_flags][0x5568f25ac230]: [CONNECTION_STATE_FINALIZATION_CLIENT_SYNC] unexpected server message, expected flag FINALIZE_SC_SYNCHRONIZE_PDU [0x00000001] [have NO_FLAG_SET [0x00000000]]

These are generated after we start a Deactivation-Reactivation sequence with libxrdp_reset() and before we call xrdp_rdp_send_synchronise() There seem to be a couple of problems with this area of the state machine:-

1. for GFX we don't need to do libxrdp_reset(). From [MS-RDPEDISP] 1.3 :-

   Changes in the server-side display configuration occur out of band to the Remote Desktop Protocol:Display Control Virtual Channel Extension. If the requested graphics configuration is valid and can be configured on the server, then the server will either:

   • Initiate a Deactivation-Reactivation Sequence (as specified in [MS-RDPBCGR] section 1.3.1.3) if the Remote Desktop Protocol: Graphics Pipeline Extension is not being used to remote sessiongraphics.
   • Restart the graphics pipeline using the surface management commands (specified in [MS-RDPEGFX] section 1.3) if the Remote Desktop Protocol: Graphics Pipeline Extension is being used to remote session graphics.

2. If we enter the Deactivation-Reactivation sequence, we aren't waiting for it to finish before we start sending graphics commands. From [MS-RDPBCGR] 1.3.11 [emphasis mine]

Once the client has sent the Confirm Active PDU, it can start sending mouse and keyboard input to the server, and upon receipt of the Font List PDU the server can start sending graphics output to the client.

So it seems to me that both the GFX and non-GFX resize codepaths need some work here to be conformant to the specification.

@Nexarian - I'd very much appreciate your comments on the above.

[matt335672]

PS: I've got pretty much a full day available on Wednesday to look at this. I imagine you'll be rather busy doing other things.

[matt335672]

Small update - it's looking like we can make use of the suppress_output mechanism to prevent us sending screen updates to the client during deactivation-reactivation. That's removed a lot of the warnings from FreeRDP, but I've only get a prototype in place so far.

[Nexarian]

Sorry for the delayed response. In reference to the concern about not knowing whether not to create the encoder, I thought I solved this by making use of the state machine itself.. If GFX isn't initialized, we just skip the entirety of the GFX shutdown/restart workflow... But maybe this isn't quite good enough?

As to the FreeRDP error. Yes, it's been known for some time but I hadn't invested in fixing it because as best I could tell it was only a warning and didn't break anything. It was a low-priority "to fix" but if we are fixing spec conformance, yes, great idea.

Yes, according to the spec you don't need to reset everything in order for GFX resizing to work. You're supposed to be able to simply delete surfaces and re-create them at the new size. I was never able to get this to work with the Mac OS client, which seemed to require that the entire thing be shutdown and rebuilt. Microsoft insists that this is not true. So we have an issue where my tested experience conflicts directly with the spec, which is why I've always asserted that "I need someone else to sanity check this."

Also yes to suppress_output. I was never able to get it work to utilize it to prevent errant signals from being sent when we didn't want them. If you notice there are a lot of checks to make sure things are "up" so an errant video frame doesn't slip through before everything is re-initialized.

Also to installing FreeRDP -- Any variation of V2 or V3 should work for our purposes here, we really only need GFX/RFX Pro and resizing, which has been stable in FreeRDP for a long time. Building FreeRDP from scratch is always difficult for me, but installing any production version from the last few years is likely fine for our purposes.

[pnowack]

I was never able to get this to work with the Mac OS client, which seemed to require that the entire thing be shutdown and rebuilt.

Would be funny, if you run here into the pitfall of 3.3.5.19 ([MS-RDPEGFX]), where the client tries to reset the whole [MS-RDPEGFX] protocol by sending the CapsAdvertise PDU again (Caps version 103 or later), after [MS-RDPEGFX] was already initialized, because it is a section that is easily missed, but it has a high impact, if it happens. It therefore wouldn't surprise me, if xrdp doesn't handle that. FreeRDPs shadow server doesn't handle that case too (g-r-d does though).

[Nexarian]

Link for reference. Yep I'm pretty sure we don't handle that. Thoughts @jsorg71 ?

[matt335672]

Thanks @Nexarian.

In reference to the state machine line you reference, that bit's fine. The problem is later on. This if test is always true, no matter what codepath is taken to get there, so we end up re-initialising EGFX when it wasn't started in the first place.

As I said, I've got a bit of time tomorrow (I'm on UTC+0 at the mo'), so I'll try to get the latest gfx_mainline_merge_work working cleanly with FreeRDP 3.2.0. At the moment on my rig I'm getting glitches with devel, let alone gfx_mainline_merge_work so there's some stuff that need getting to the bottom of.

The reason I want to use 3.2.0 is that the FreeRDP team have been pushing to get their stuff as standards compliant as it can be. That's been good for us too - see #2839.

@pnowack - that's a great call. I'll look into it tomorrow. For reference, here's the section:-

If the capability set received in the RDPGFX_CAPS_CONFIRM_PDU message is RDPGFX_CAPSET_VERSION103, RDPGFX_CAPSET_VERSION104, RDPGFX_CAPSET_VERSION105, RDPGFX_CAPSET_VERSION106, or RDPGFX_CAPSET_VERSION107 then the client can resend the RDPGFX_CAPS_ADVERTISE_PDU message during the connection to reset the protocol. The client MUST reset the channel state after sending the RDPGFX_CAPS_ADVERTISE_PDU message and MUST ignore any messages sent by the server until RDPGFX_CAPS_CONFIRM_PDU message is received.

[Nexarian]

I'm embarrassed that I missed that. There was a time where I was SURE I'd tested this case. Ah well. In any case, let's get this merged and then we can continue to test.

[matt335672]

Thanks.

I suggest you do the merge since you're in charge of the branch!

[matt335672]

I've added a bunch of commits around the dynamic resize state machine.

With these commits dynamic resize works with GFX and non-GFX clients and Xorg and VNC backends.

I've tested with FreeRDP 3.2.0. This is now warning-free for the dynamic resize codepaths.

Some points:-

• Major change is adding a function xrdp_rdp_suppress_output(). This calls into the module suppress_output method to get the module to bunch changes up rather than sending them immediately. The top level function is provided with a reason why output is being suppressed, so that output suppression can be nested.
• I've not looked at optimising the EGFX state machine yet. I'll try to get a hackingtosh together so I can test with that, but I'm not hopeful it will be good enough.
• I'm still getting a few corrupted resizes for both GFX and non-GFX where the graphics output hasn't been fully updated. A further resize fixes things.
• The server-side resize paths work, but I'm still getting a few warnings on FreeRDP 3.2.0

[matt335672]

On the corrupted screens, the problem seems to be down to a corrupted framebuffer in the X server. This happens for both Xvnc and Xorg backends. I've no idea why this should be happening.

[Nexarian]

Do you want me to hold off on merging this until you look into it further? Or is this still a major improvement from where things were?

[matt335672]

It's a big step forward, certainly. Probably worth merging despite the wrinkles.

[matt335672]

PS: Please check with MacOS if you get time. There should be no regressions here though.

[Nexarian]

If you're not moving backward, you're moving fowards, will do :)

[Nexarian]

Validated that it doesn't break resizing on Mac OS under basic stress tests (Resizing whilst playing a YouTube video, multiple resizes queued up at once, switching from the smallest possible to the largest possible size multiple times).

Great work, I'll merge it and we can continue testing/iterating!