Adding egfx base functions. #2338

Nexarian · 2022-08-10T05:28:08Z

No description provided.

matt335672

@Nexarian - a few thoughts for you. I thought I'd comment now as next week is looking a bit hairy time-wise.

matt335672 · 2022-08-10T08:47:17Z

xrdp/xrdp_egfx.c

+    int error;
+    int bytes;
+    struct stream *s;
+    char *holdp;


There are some macros to do this stuff in parse.h, namely s_push_layer() and s_pop_layer(). You might want to consider using these for consistency with other code elsewhere.

good point, agreed.

@matt335672 I believe this is resolved.

matt335672 · 2022-08-10T08:49:09Z

xrdp/xrdp_egfx.c

+
+    LOG(LOG_LEVEL_TRACE, "xrdp_egfx_send_fill_surface:");
+    make_stream(s);
+    init_stream(s, 8192);


Is this guaranteed to be big enough, or should you scale it depending on num_rects? Somehing like init_stream(s, 1024 + num_rects * 8);

yes, unlikely but yes.

@matt335672 I believe this is resolved.

matt335672 · 2022-08-10T08:51:11Z

xrdp/xrdp_egfx.c

+    LOG(LOG_LEVEL_TRACE, "xrdp_egfx_send_wire_to_surface1:");
+    make_stream(s);
+    bytes = bitmap_data_length + 8192;
+    bytes += 5 * (bitmap_data_length / 0xFFFF);


USHRT_MAX for consistency?

I guess a define like MAX_PART_SIZE would be best

@matt335672 I believe this is resolved.

xrdp/xrdp_egfx.h

xrdp/xrdp_egfx.c

pnowack · 2022-08-12T17:51:59Z

xrdp/xrdp_egfx.c

+    out_uint32_le(s, 340);
+    out_uint32_le(s, width);
+    out_uint32_le(s, height);
+    out_uint32_le(s, monitor_count == 0 ? 1 : monitor_count);


Since I am not an xrdp member, its your choice. But personally I wouldn't apply workarounds for the own code here.

This is a workaround for Microsoft's Mac OS Client. We can't ever send a monitorcount of 0, even though internally sometimes it's set that way. So, from Microsoft's perspective, this is important to always be one.

What would you recommend I do instead?

We can't ever send a monitorcount of 0

Well, it's RDP, it's about graphical remote desktop. You always have at least one monitor. Sending a monitor config here with 0 monitors is a bug.
If you know, that the monitor config changed, but not know yet the actual monitor layout, then defer sending this PDU until you know the new layout.
Same for the subsequent PDUs, which provide the graphics content (wire_to_surface, etc.).
There is no need to send this PDU until the screen changes or the layout changes are done.

The question would also be: If you have 0 monitors, what size for the graphics output buffer do you use in that situation?
The graphics output buffer only contains user visible surfaces, i.e. surfaces that are mapped to an output (map_surface_to_output).
You can have offscreen surfaces, that are not mapped, that are actually larger than the graphics output buffer itself, in case that is relevant for you.

I agree that 0 monitors is a bug in XRDP, but it's pretty baked in at this point and probably out of scope for this PR to fix it.

In the past "0 monitors with a session_width and a session_height" just meant "multi-mon is not enabled and we have one single large surface." It didn't actually matter until GFX integration, which cares about monitor numbering more than RLE or RemoteFX appeared to.

That is indeed wrong. I'll write up an issue about what's wrong here and that we should fix it (and hopefully if I have time, what needs to be fixed), then reference it in a TODO.

Sound good?

I agree that 0 monitors is a bug in XRDP, but it's pretty baked in at this point and probably out of scope for this PR to fix it.

Sure, xrdp_egfx_send_reset_graphics() is also not used yet.

Personally, I would, in case that bug is unavoidable, apply the workaround at the earliest point possible (i.e. in future commits, where xrdp_egfx_send_reset_graphics is actually used). This way, the area of code, where that bug exists would be reduced.

Sound good?

Ultimately, it is up to you how you handle that, because I am not part of the xrdp project. I can only point out wrong or fishy things here.

I'll write up an issue about what's wrong here and that we should fix it (and hopefully if I have time, what needs to be fixed), then reference it in a TODO.

Creating an issue sounds sane to avoid, that the issue is lost, while the bug still exists.

pnowack · 2022-08-12T17:53:10Z

xrdp/xrdp_egfx.c

+        {
+            return 1;
+        }
+        switch (cmdId)


You must be able to handle the Cache Import Offer PDU (and reply to it), even when you don't want to import anything.

#2502

No, that is not enough or right: Quoting the docs here (1.5.2 Server Implementation Requirements):

Furthermore, servers implementing the Remote Desktop Protocol: Graphics Pipeline Extension must be capable of processing the following messages: - RDPGFX_FRAME_ACKNOWLEDGE_PDU (section 2.2.2.13) - RDPGFX_CACHE_IMPORT_OFFER_PDU (section 2.2.2.16) - RDPGFX_CAPS_ADVERTISE_PDU (section 2.2.2.18)

Also, 3.2.5.16 Processing an RDPGFX_CACHE_IMPORT_OFFER_PDU message:

Once the RDPGFX_CACHE_IMPORT_OFFER_PDU message has been processed, the server MUST respond by sending the RDPGFX_CACHE_IMPORT_REPLY_PDU (section 2.2.2.17) message to the client (section 3.2.5.17).

Got it! I copied your comment into the issue so we don't lose it.

What's the best way to test this? Is there a way to force FreeRDP to send this message so we can test the negotiation? Or is there a mode on the Microsoft clients we can set to trigger it?

mstsc sends that PDU (though not always).

matt335672

I'm not at all up-to-speed on what EGFX does, so I've restricted my review to looking at possible overflows and leaks. From that perspective, I think this is fine - I haven't found any issues.

pnowack · 2023-01-03T12:12:04Z

xrdp/xrdp_egfx.c

+            return 1;
+        }
+        holdp = s->p;
+        if (capsDataLength == 4)


You might want to mention (for others), that caps version 101 is indirectly excluded here, but that's up to you.

pnowack · 2023-01-10T13:09:25Z

xrdp/xrdp_egfx.c

+    /* RDP_SEGMENTED_DATA */
+    out_uint8(s, 0xE0); /* descriptor = SINGLE */
+    /* RDP8_BULK_ENCODED_DATA */
+    out_uint8(s, 0x04); /* header = PACKET_COMPR_TYPE_RDP8 */


Small suggestion (not an issue), you could use a define for the PACKET_COMPR_TYPE_RDP8.

- This isn't hooked up to anything yet. That will come later with further EGFX commits. - There are some TODO items in this code around the way XRDP handles caps negotiation and monitor storage. - This is a great candidate for unit testing in the future.

Nexarian · 2023-01-15T06:59:21Z

It seems this has been reviewed well enough to be merged. I don't want to sit on this any further, as I definitely need to get moving on EGFX merge into devel so the benefits can be enjoyed by more standard users of XRDP.

For further issues and bug fixes, future PRs where I incorporate more of mainline_merge will be a great candidate.

matt335672 reviewed Aug 10, 2022

View reviewed changes

pnowack reviewed Aug 12, 2022

View reviewed changes

Nexarian mentioned this pull request Aug 20, 2022

Implement GFX graphics pipeline in xrdp #1422

Closed

Nexarian marked this pull request as ready for review October 17, 2022 03:33

Nexarian force-pushed the add_egfx_base_functions branch 3 times, most recently from ea1462c to cd3f134 Compare October 20, 2022 03:43

Nexarian force-pushed the add_egfx_base_functions branch 3 times, most recently from 00471d0 to 2f7551e Compare January 2, 2023 06:25

Nexarian requested review from jsorg71, pnowack and matt335672 and removed request for pnowack January 2, 2023 06:31

matt335672 reviewed Jan 3, 2023

View reviewed changes

pnowack reviewed Jan 3, 2023

View reviewed changes

pnowack reviewed Jan 10, 2023

View reviewed changes

Nexarian mentioned this pull request Jan 15, 2023

In xrdp_egfx.c, handle Cache Import PDU #2502

Open

Adding egfx base functions.

af8995e

- This isn't hooked up to anything yet. That will come later with further EGFX commits. - There are some TODO items in this code around the way XRDP handles caps negotiation and monitor storage. - This is a great candidate for unit testing in the future.

Nexarian force-pushed the add_egfx_base_functions branch from ceafa74 to af8995e Compare January 15, 2023 06:47

Nexarian merged commit 8fdb0fa into neutrinolabs:devel Jan 15, 2023

Nexarian deleted the add_egfx_base_functions branch January 15, 2023 06:59

metalefty mentioned this pull request Feb 21, 2024

Win 11 mstsc.exe doesn't send RemoteFX capability #2400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding egfx base functions. #2338

Adding egfx base functions. #2338

Nexarian commented Aug 10, 2022

matt335672 left a comment

matt335672 Aug 10, 2022

jsorg71 Aug 12, 2022

Nexarian Jan 2, 2023

matt335672 Aug 10, 2022

jsorg71 Aug 12, 2022

Nexarian Jan 2, 2023

matt335672 Aug 10, 2022

jsorg71 Aug 12, 2022

Nexarian Jan 2, 2023

pnowack Aug 12, 2022

Nexarian Jan 2, 2023

pnowack Jan 3, 2023 •

edited

Loading

Nexarian Jan 3, 2023

pnowack Jan 10, 2023

pnowack Aug 12, 2022

jsorg71 Aug 12, 2022

Nexarian Jan 15, 2023

pnowack Jan 15, 2023

Nexarian Jan 15, 2023

pnowack Jan 15, 2023

matt335672 left a comment

pnowack Jan 3, 2023

Nexarian Jan 15, 2023

pnowack Jan 10, 2023

Nexarian Jan 15, 2023

Nexarian commented Jan 15, 2023

Adding egfx base functions. #2338

Adding egfx base functions. #2338

Conversation

Nexarian commented Aug 10, 2022

matt335672 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnowack Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt335672 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nexarian commented Jan 15, 2023

pnowack Jan 3, 2023 •

edited

Loading