Initial rfx progressive integration #2879

Nexarian · 2023-12-16T17:51:37Z

Mostly base functions and utilities necessary to enable RFX Progressive
Add more EGFX work.
Update encoder.
Does not yet include caps determination to enable RFX progressive (yet).

metalefty · 2023-12-18T13:04:26Z

xrdp/xrdp_encoder.c

    if (client_info->mcs_connection_type != CONNECTION_TYPE_LAN)
    {
-        return 0;
+        if ((mm->egfx_flags & 3) == 0)


Where is 3 come from?

You know I was wondering that myself (Most of this is code's lineage comes from Jay's original egfx branch that was started many years ago).

I think it's this: In this simplified version of GFX, this flag can only ever be 1 (For H264) or 2 (For Progressive RFX). Anding with 3 means "It is neither 1 nor 2":

0b00 & 3 = 0 0b01 & 3 = 1 0b10 & 3 = 2 0b11 & 3 = 3

So basically, if it's not a LAN connection, and it's not EGFX, drop the connection because the only other use for the encoder is "non-progressive RFX" which requires LAN per the spec.

There's probably a clearer way to write this. I think egfx_flags needs to become an enum, too. Thoughts?

I agree. Either enum or define as macro would be clearer.

metalefty · 2023-12-19T09:17:40Z

libxrdp/xrdp_sec.c

@@ -2133,7 +2133,15 @@ xrdp_sec_process_mcs_data_CS_CORE(struct xrdp_sec *self, struct stream *s)
    {
        client_info->bpp = 32;
    }
-
+    if (earlyCapabilityFlags & 0x100) /* RNS_UD_CS_SUPPORT_DYNVC_GFX_PROTOCOL */


Let's add this to ms-rdpbcgr.h.
https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/00f1da4a-ee9c-421a-852f-c19f92343d73

metalefty · 2023-12-19T09:19:21Z

Logic looks good to me. Some trivial thoughts.

metalefty · 2023-12-19T09:20:16Z

libxrdp/xrdp_sec.c

-
+    if (earlyCapabilityFlags & 0x100) /* RNS_UD_CS_SUPPORT_DYNVC_GFX_PROTOCOL */
+    {
+        LOG_DEVEL(LOG_LEVEL_INFO, "client supports gfx");


I think this should be LOG instead of LOG_DEVEL.

Fixed. Do you think the "client DOES NOT support gfx" message should also be a LOG?

Either is fine. Let's leave it as-is now.

So... Should I merge this then?

- Mostly base functions and utilities necessary to enable RFX Progressive - Add more EGFX work & mode flags. - Update encoder. - Does not yet include caps determination to enable RFX progressive (yet). - Update protocol constants

metalefty · 2023-12-20T05:39:34Z

@Nexarian I have created gfx_mainline_merge_work branch. Let's merge into the branch now. After everything's done, let's merge gfx_mainline_merge_work into devel.

matt335672

Definitely worth getting this into its own branch

matt335672 · 2023-12-20T10:02:13Z

common/Makefile.am

@@ -71,6 +71,6 @@ libcommon_la_SOURCES = \
  $(PIXMAN_SOURCES)

 libcommon_la_LIBADD = \
-  -lpthread \
+  -lpthread -lrt \


I don't think this is needed - it seems to build OK without it. Am I missing a compile option?

No, again, future use. Trying to get in base/fundamental changes that I know we will need in the future.

I'm fine with that. Thanks for the clarification.

The reason I raised it was there's probably a better way to add this with autotools. I'll investigate further when we've got more dependencies to play with.

matt335672 · 2023-12-20T10:08:13Z

xrdp/xrdp_wm.c

+                {
+                    /* gfx session but have not recieved caps advertise yet,
+                       set flag so we will connect to backend later */
+                    self->mm->gfx_delay_autologin = 1;


This flag appears to be set but not used.

Correct. It's intended for future use, just like the xrdp_egfx.c and xrdp_egfx.h files. It WILL be used once GFX is turned on.

Nexarian · 2023-12-20T14:57:33Z

@Nexarian I have created gfx_mainline_merge_work branch. Let's merge into the branch now. After everything's done, let's merge gfx_mainline_merge_work into devel.

Hmm, I had a slightly different approach. I was thinking that we slowly merge the pieces that we're sure we need into devel in smaller chunks like this. In this way it gives us an opportunity to tune/review smaller pieces over time. We can put the actual enabling of RFX Progressive/GFX under a build flag and call it experimental until we decide to officially release it. It also avoids the entirety of the mainline_merge problem where it became something of a second-devel/release that many were unofficially using.

However, I'm open to doing it this way as well. Given that there are unfinished "for future use" portions in this PR, I'll merge into gfx_mainline_merge_work

metalefty · 2023-12-21T01:06:08Z

It also avoids the entirety of the mainline_merge problem where it became something of a second-devel/release that many were unofficially using.

I think it is not a big problem now because gfx_mainline_merge_work will be merged into devel in a month and we will focus on GFX for a while. So other developments are suspended and not so many other pull requests will be merged into devel.

matt335672 · 2023-12-21T14:29:25Z

I'm happy with pausing other developments for now. #2745 is probably the biggest outstanding change I've got and that can wait.

Nexarian requested review from matt335672, jsorg71 and metalefty December 16, 2023 17:52

Nexarian force-pushed the add_egfx_multimon_common branch from 1598bb1 to 7845283 Compare December 16, 2023 19:30

metalefty reviewed Dec 18, 2023

View reviewed changes

Nexarian force-pushed the add_egfx_multimon_common branch 3 times, most recently from 8d128ca to 80e8268 Compare December 19, 2023 05:16

Nexarian requested a review from metalefty December 19, 2023 05:25

metalefty reviewed Dec 19, 2023

View reviewed changes

Initial rfx progressive integration

1ee3c64

- Mostly base functions and utilities necessary to enable RFX Progressive - Add more EGFX work & mode flags. - Update encoder. - Does not yet include caps determination to enable RFX progressive (yet). - Update protocol constants

Nexarian force-pushed the add_egfx_multimon_common branch from c571251 to 1ee3c64 Compare December 20, 2023 04:17

Nexarian requested a review from metalefty December 20, 2023 04:25

metalefty changed the base branch from devel to gfx_mainline_merge_work December 20, 2023 05:39

metalefty approved these changes Dec 20, 2023

View reviewed changes

matt335672 reviewed Dec 20, 2023

View reviewed changes

Nexarian merged commit 02d329e into neutrinolabs:gfx_mainline_merge_work Dec 20, 2023
13 checks passed

Nexarian deleted the add_egfx_multimon_common branch December 20, 2023 14:58

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

Initial rfx progressive integration #2879

Initial rfx progressive integration #2879

Nexarian commented Dec 16, 2023

metalefty Dec 18, 2023

Nexarian Dec 18, 2023 •

edited

Loading

metalefty Dec 19, 2023

Nexarian Dec 19, 2023

metalefty Dec 19, 2023

Nexarian Dec 20, 2023

metalefty commented Dec 19, 2023

metalefty Dec 19, 2023

Nexarian Dec 20, 2023

metalefty Dec 20, 2023

Nexarian Dec 20, 2023

metalefty commented Dec 20, 2023

matt335672 left a comment

matt335672 Dec 20, 2023

Nexarian Dec 20, 2023

matt335672 Dec 21, 2023

matt335672 Dec 20, 2023

Nexarian Dec 20, 2023

matt335672 Dec 21, 2023

Nexarian commented Dec 20, 2023

metalefty commented Dec 21, 2023

matt335672 commented Dec 21, 2023

Initial rfx progressive integration #2879

Initial rfx progressive integration #2879

Conversation

Nexarian commented Dec 16, 2023

Choose a reason for hiding this comment

Nexarian Dec 18, 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

metalefty commented Dec 19, 2023

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

metalefty commented Dec 20, 2023

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

Nexarian commented Dec 20, 2023

metalefty commented Dec 21, 2023

matt335672 commented Dec 21, 2023

Nexarian Dec 18, 2023 •

edited

Loading