GUACAMOLE-1026: Add support for FreeRDP3. #517

aleitner · 2024-05-09T04:50:49Z

Refactored to support both FreeRDP 2 and 3.

Added version checking for FreeRDP 3.x and fallback to 2 if 3 doesn't exist
Additional checks introduced for various FreeRDP 3.x feature support
- _aligned_free and _aligned_malloc renamed to winpr_aligned_free and winpr_aligned_malloc
- ReadColor and WriteColor renamed to FreeRDPReadColor and FreeRDPWriteColor
- Various CLIPRDR properties moved to CLIPRDR_HEADER common struct for CLIPRDR structs.
- Required use of getter/setter functions interacting with rdpSettings
  - freerdp_settings_set_pointer, freerdp_settings_set_bool, freerdp_settings_set_string, freerdp_settings_set_uint32, freerdp_settings_get_pointer_writable, freerdp_settings_get_bool, freerdp_settings_get_string, freerdp_settings_get_uint32
  - Note: freerdp_settings_set_pointer and freerdp_settings_get_pointer_writable were only added for 3.X where as the rest were available within 2.X.
  - getter/setter functions within 3.X now use updated enum types for keys prefixed with FreeRDP_.
- FreeRDP 3.X moved context-related pointers (rdpSettings* settings, rdpInput* input) to a context structure in the freerdp* instance.
- freerdp_shall_disconnect renamed to freerdp_shall_disconnect_context.
- IDRDYNVC_ENTRY_POINTS.GetPluginData() return now requires const.
- glyph.New and pointer.Set no longer require arguments to be const.

necouchman

A couple of initial comments - overall looks pretty clean!

necouchman · 2024-05-09T15:19:02Z

configure.ac

 then
    AC_CHECK_MEMBERS([freerdp.VerifyCertificateEx],,,
-                     [[#include <freerdp/freerdp.h>]])
+            [[#include <freerdp/freerdp.h>]])


There seem to be some alignment issues throughout the configure.ac file, now. If we're sticking to how things are generally done throughout the file, the start of this should be aligned with the [freerdp.VerifyCertificateEx] line above, but it seems to be less indented, now.. There are several of these occurrences throughout the changes in the file.

~~Updated formatting~~

Nevermind looks like my editor is rendering differently than github

Should be cleaner now

src/libguac/string.c

jmuehlner · 2024-05-09T17:44:52Z

src/protocols/rdp/settings.c

+
+    }
+
+    /* Set individual flags - some FreeRDP versions overwrite the above */


Overwrite which things above?

The guac_rdp_get_performance_flags function is called prior to this and that has some of the same settings.

guacamole-server/src/protocols/rdp/settings.c

Line 1493 in 32d232a

freerdp_settings_set_uint32(rdp_settings, FreeRDP_PerformanceFlags, guac_rdp_get_performance_flags(guac_settings));

guacamole-server/src/protocols/rdp/settings.c

Lines 1410 to 1441 in 32d232a

static int guac_rdp_get_performance_flags(guac_rdp_settings* guac_settings) {

/* No performance flags initially */

int flags = PERF_FLAG_NONE;

/* Desktop wallpaper */

if (!guac_settings->wallpaper_enabled)

flags |= PERF_DISABLE_WALLPAPER;

/* Theming of desktop/windows */

if (!guac_settings->theming_enabled)

flags |= PERF_DISABLE_THEMING;

/* Font smoothing (ClearType) */

if (guac_settings->font_smoothing_enabled)

flags |= PERF_ENABLE_FONT_SMOOTHING;

/* Full-window drag */

if (!guac_settings->full_window_drag_enabled)

flags |= PERF_DISABLE_FULLWINDOWDRAG;

/* Desktop composition (Aero) */

if (guac_settings->desktop_composition_enabled)

flags |= PERF_ENABLE_DESKTOP_COMPOSITION;

/* Menu animations */

if (!guac_settings->menu_animations_enabled)

flags |= PERF_DISABLE_MENUANIMATIONS;

return flags;

}

So "the above" means "flags set by guac_rdp_get_performance_flags() above"?

If so, mind updating the comment to be a bit more specific?

src/protocols/rdp/settings.c

necouchman · 2024-05-10T20:07:36Z

I need to pull the code down and actually run it through some testing before I'm good with it, I'll try to get that done this weekend.

Currently this is targeted at the main branch which would put it in the 1.6.0 release. In the past I think it was mentioned that this might go into a 2.0 release, so wanted to check and see if @mike-jumper or @jmuehlner had any thoughts on that? I'm fine with it going into 1.6.0 - it seems like it's a pretty minimal and compatible set of changes, just want to make sure we're all in agreement.

jmuehlner · 2024-05-10T20:33:10Z

I need to pull the code down and actually run it through some testing before I'm good with it, I'll try to get that done this weekend.

Currently this is targeted at the main branch which would put it in the 1.6.0 release. In the past I think it was mentioned that this might go into a 2.0 release, so wanted to check and see if @mike-jumper or @jmuehlner had any thoughts on that? I'm fine with it going into 1.6.0 - it seems like it's a pretty minimal and compatible set of changes, just want to make sure we're all in agreement.

It doesn't drop compatibility with FreeRDP 2.x so I'm fine putting it in main, and in 1.6.0.

mike-jumper · 2024-05-22T16:38:58Z

No objection from me for inclusion in 1.6.0 either as long as we maintain compatibility with FreeRDP 2.x.

mike-jumper · 2024-05-22T16:59:15Z

I've added this temporary build to help verify these changes against various 2.x and 3.x versions of FreeRDP:

https://ci-builds.apache.org/job/Guacamole/job/guacamole-server-wip-freerdp3/

The above is identical to our existing build except that it builds against this RDP-3 branch (not main) and includes FreeRDP's main and latest 3.x release as additional versions to be tested.

jmuehlner · 2024-05-23T16:27:12Z

I've added this temporary build to help verify these changes against various 2.x and 3.x versions of FreeRDP:

https://ci-builds.apache.org/job/Guacamole/job/guacamole-server-wip-freerdp3/

The above is identical to our existing build except that it builds against this RDP-3 branch (not main) and includes FreeRDP's main and latest 3.x release as additional versions to be tested.

Looks like the FreeRDP 3 test build failed: https://ci-builds.apache.org/job/Guacamole/job/guacamole-server-wip-freerdp3/FREERDP_BRANCH=3.5.1,JENKINS_LABEL_EXPRESSION=ubuntu/lastBuild/console

aleitner · 2024-05-23T16:28:44Z

I've added this temporary build to help verify these changes against various 2.x and 3.x versions of FreeRDP:
https://ci-builds.apache.org/job/Guacamole/job/guacamole-server-wip-freerdp3/
The above is identical to our existing build except that it builds against this RDP-3 branch (not main) and includes FreeRDP's main and latest 3.x release as additional versions to be tested.

Looks like the FreeRDP 3 test build failed: https://ci-builds.apache.org/job/Guacamole/job/guacamole-server-wip-freerdp3/FREERDP_BRANCH=3.5.1,JENKINS_LABEL_EXPRESSION=ubuntu/lastBuild/console

Huh that shouldn't happen. I just pushed changes a little bit ago to resolve that

jmuehlner · 2024-05-23T16:33:52Z

Huh that shouldn't happen. I just pushed changes a little bit ago to resolve that

Ah ok - looks like a new build did kick off a bit ago - maybe that'll fix it.
https://ci-builds.apache.org/job/Guacamole/job/guacamole-server-wip-freerdp3/2/

jmuehlner · 2024-05-23T22:32:32Z

Huh that shouldn't happen. I just pushed changes a little bit ago to resolve that

Ah ok - looks like a new build did kick off a bit ago - maybe that'll fix it. https://ci-builds.apache.org/job/Guacamole/job/guacamole-server-wip-freerdp3/2/

Looks like it failed again... https://ci-builds.apache.org/job/Guacamole/job/guacamole-server-wip-freerdp3/2/FREERDP_BRANCH=3.5.1,JENKINS_LABEL_EXPRESSION=ubuntu/

aleitner force-pushed the RDP-3 branch from 89f42d4 to 32d232a Compare May 9, 2024 05:11

aleitner marked this pull request as ready for review May 9, 2024 05:39

necouchman requested changes May 9, 2024

View reviewed changes

jmuehlner reviewed May 9, 2024

View reviewed changes

src/protocols/rdp/settings.c Show resolved Hide resolved

aleitner force-pushed the RDP-3 branch 4 times, most recently from 6d1b313 to daceeaf Compare May 10, 2024 01:17

aleitner mentioned this pull request May 13, 2024

Restoration of Public API Caching Functions and Headers in FreeRDP 3.x FreeRDP/FreeRDP#10140

Closed

GUACAMOLE-1026: Add support for FreeRDP3.

37a5368

aleitner force-pushed the RDP-3 branch from daceeaf to 7fc64a6 Compare May 22, 2024 13:52

GUACAMOLE-1026: Use freerdp3 within Dockerfile.

530fe17

aleitner force-pushed the RDP-3 branch from 7fc64a6 to 530fe17 Compare May 23, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GUACAMOLE-1026: Add support for FreeRDP3. #517

GUACAMOLE-1026: Add support for FreeRDP3. #517

aleitner commented May 9, 2024 •

edited

necouchman left a comment

necouchman May 9, 2024

aleitner May 10, 2024 •

edited

aleitner May 10, 2024

jmuehlner May 9, 2024

aleitner May 10, 2024

jmuehlner May 21, 2024

necouchman commented May 10, 2024

jmuehlner commented May 10, 2024

mike-jumper commented May 22, 2024

mike-jumper commented May 22, 2024 •

edited

jmuehlner commented May 23, 2024

aleitner commented May 23, 2024

jmuehlner commented May 23, 2024

jmuehlner commented May 23, 2024


		}

		/* Set individual flags - some FreeRDP versions overwrite the above */

	static int guac_rdp_get_performance_flags(guac_rdp_settings* guac_settings) {

	/* No performance flags initially */
	int flags = PERF_FLAG_NONE;

	/* Desktop wallpaper */
	if (!guac_settings->wallpaper_enabled)
	flags \|= PERF_DISABLE_WALLPAPER;

	/* Theming of desktop/windows */
	if (!guac_settings->theming_enabled)
	flags \|= PERF_DISABLE_THEMING;

	/* Font smoothing (ClearType) */
	if (guac_settings->font_smoothing_enabled)
	flags \|= PERF_ENABLE_FONT_SMOOTHING;

	/* Full-window drag */
	if (!guac_settings->full_window_drag_enabled)
	flags \|= PERF_DISABLE_FULLWINDOWDRAG;

	/* Desktop composition (Aero) */
	if (guac_settings->desktop_composition_enabled)
	flags \|= PERF_ENABLE_DESKTOP_COMPOSITION;

	/* Menu animations */
	if (!guac_settings->menu_animations_enabled)
	flags \|= PERF_DISABLE_MENUANIMATIONS;

	return flags;

	}

GUACAMOLE-1026: Add support for FreeRDP3. #517

Are you sure you want to change the base?

GUACAMOLE-1026: Add support for FreeRDP3. #517

Conversation

aleitner commented May 9, 2024 • edited

necouchman left a comment

Choose a reason for hiding this comment

necouchman May 9, 2024

Choose a reason for hiding this comment

aleitner May 10, 2024 • edited

Choose a reason for hiding this comment

aleitner May 10, 2024

Choose a reason for hiding this comment

jmuehlner May 9, 2024

Choose a reason for hiding this comment

aleitner May 10, 2024

Choose a reason for hiding this comment

jmuehlner May 21, 2024

Choose a reason for hiding this comment

necouchman commented May 10, 2024

jmuehlner commented May 10, 2024

mike-jumper commented May 22, 2024

mike-jumper commented May 22, 2024 • edited

jmuehlner commented May 23, 2024

aleitner commented May 23, 2024

jmuehlner commented May 23, 2024

jmuehlner commented May 23, 2024

aleitner commented May 9, 2024 •

edited

aleitner May 10, 2024 •

edited

mike-jumper commented May 22, 2024 •

edited