From 07e4e23c7b3e8b8c436c6e61d3c1e1e25943fec1 Mon Sep 17 00:00:00 2001 From: Koichiro Iwao Date: Fri, 23 Aug 2024 16:53:30 +0900 Subject: [PATCH 1/5] tconfig: add config which to prefer H264 vs RFX --- tests/xrdp/Makefile.am | 10 ++- tests/xrdp/gfx/gfx.toml | 40 +++++++++++ tests/xrdp/gfx/gfx_codec_h264_only.toml | 18 +++++ tests/xrdp/gfx/gfx_codec_h264_preferred.toml | 18 +++++ tests/xrdp/gfx/gfx_codec_order_undefined.toml | 18 +++++ tests/xrdp/gfx/gfx_codec_rfx_only.toml | 18 +++++ tests/xrdp/gfx/gfx_codec_rfx_preferred.toml | 18 +++++ .../xrdp/gfx/gfx_codec_rfx_preferred_odd.toml | 18 +++++ tests/xrdp/test_tconfig.c | 45 +++++++++++- xrdp/gfx.toml | 3 + xrdp/xrdp_tconfig.c | 70 +++++++++++++++++++ xrdp/xrdp_tconfig.h | 7 ++ 12 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 tests/xrdp/gfx/gfx.toml create mode 100644 tests/xrdp/gfx/gfx_codec_h264_only.toml create mode 100644 tests/xrdp/gfx/gfx_codec_h264_preferred.toml create mode 100644 tests/xrdp/gfx/gfx_codec_order_undefined.toml create mode 100644 tests/xrdp/gfx/gfx_codec_rfx_only.toml create mode 100644 tests/xrdp/gfx/gfx_codec_rfx_preferred.toml create mode 100644 tests/xrdp/gfx/gfx_codec_rfx_preferred_odd.toml diff --git a/tests/xrdp/Makefile.am b/tests/xrdp/Makefile.am index 02f8b3e2..e9fab447 100644 --- a/tests/xrdp/Makefile.am +++ b/tests/xrdp/Makefile.am @@ -1,6 +1,5 @@ AM_CPPFLAGS = \ -DXRDP_TOP_SRCDIR=\"$(top_srcdir)\" \ - -DXRDP_CFG_PATH=\"${sysconfdir}/xrdp\" \ -I$(top_builddir) \ -I$(top_srcdir)/xrdp \ -I$(top_srcdir)/libxrdp \ @@ -21,7 +20,14 @@ EXTRA_DIST = \ test_not4_8bit.bmp \ test_not4_24bit.bmp \ test1.jpg \ - test_alpha_blend.png + test_alpha_blend.png \ + gfx/gfx.toml\ + gfx/gfx_codec_order_undefined.toml \ + gfx/gfx_codec_h264_preferred.toml \ + gfx/gfx_codec_h264_only.toml \ + gfx/gfx_codec_rfx_preferred.toml \ + gfx/gfx_codec_rfx_preferred_odd.toml \ + gfx/gfx_codec_rfx_only.toml TESTS = test_xrdp check_PROGRAMS = test_xrdp diff --git a/tests/xrdp/gfx/gfx.toml b/tests/xrdp/gfx/gfx.toml new file mode 100644 index 00000000..af3fcf86 --- /dev/null +++ b/tests/xrdp/gfx/gfx.toml @@ -0,0 +1,40 @@ +[codec] +order = [ "H.264", "RFX" ] + +[x264.default] +preset = "ultrafast" +tune = "zerolatency" +profile = "main" # profile is forced to baseline if preset == ultrafast +vbv_max_bitrate = 0 +vbv_buffer_size = 0 +fps_num = 24 +fps_den = 1 + +[x264.lan] +# inherits default + +[x264.wan] +vbv_max_bitrate = 15000 +vbv_buffer_size = 1500 + +[x264.broadband_high] +preset = "superfast" +vbv_max_bitrate = 8000 +vbv_buffer_Size = 800 + +[x264.satellite] +preset = "superfast" +vbv_max_bitrate = 5000 +vbv_buffer_size = 500 + +[x264.broadband_low] +preset = "veryfast" +tune = "zerolatency" +vbv_max_bitrate = 1600 +vbv_buffer_size = 66 + +[x264.modem] +preset = "fast" +tune = "zerolatency" +vbv_max_bitrate = 1200 +vbv_buffer_size = 50 diff --git a/tests/xrdp/gfx/gfx_codec_h264_only.toml b/tests/xrdp/gfx/gfx_codec_h264_only.toml new file mode 100644 index 00000000..86fa7d70 --- /dev/null +++ b/tests/xrdp/gfx/gfx_codec_h264_only.toml @@ -0,0 +1,18 @@ +[codec] +order = [ "H.264" ] + +[x264.default] +preset = "ultrafast" +tune = "zerolatency" +profile = "main" # profile is forced to baseline if preset == ultrafast +vbv_max_bitrate = 0 +vbv_buffer_size = 0 +fps_num = 24 +fps_den = 1 + +[x264.lan] +[x264.wan] +[x264.broadband_high] +[x264.satellite] +[x264.broadband_low] +[x264.modem] diff --git a/tests/xrdp/gfx/gfx_codec_h264_preferred.toml b/tests/xrdp/gfx/gfx_codec_h264_preferred.toml new file mode 100644 index 00000000..7d5b11ad --- /dev/null +++ b/tests/xrdp/gfx/gfx_codec_h264_preferred.toml @@ -0,0 +1,18 @@ +[codec] +order = [ "H.264", "RFX" ] + +[x264.default] +preset = "ultrafast" +tune = "zerolatency" +profile = "main" # profile is forced to baseline if preset == ultrafast +vbv_max_bitrate = 0 +vbv_buffer_size = 0 +fps_num = 24 +fps_den = 1 + +[x264.lan] +[x264.wan] +[x264.broadband_high] +[x264.satellite] +[x264.broadband_low] +[x264.modem] diff --git a/tests/xrdp/gfx/gfx_codec_order_undefined.toml b/tests/xrdp/gfx/gfx_codec_order_undefined.toml new file mode 100644 index 00000000..7432cb97 --- /dev/null +++ b/tests/xrdp/gfx/gfx_codec_order_undefined.toml @@ -0,0 +1,18 @@ +[codec] +order = [ ] + +[x264.default] +preset = "ultrafast" +tune = "zerolatency" +profile = "main" # profile is forced to baseline if preset == ultrafast +vbv_max_bitrate = 0 +vbv_buffer_size = 0 +fps_num = 24 +fps_den = 1 + +[x264.lan] +[x264.wan] +[x264.broadband_high] +[x264.satellite] +[x264.broadband_low] +[x264.modem] diff --git a/tests/xrdp/gfx/gfx_codec_rfx_only.toml b/tests/xrdp/gfx/gfx_codec_rfx_only.toml new file mode 100644 index 00000000..9ab14ea2 --- /dev/null +++ b/tests/xrdp/gfx/gfx_codec_rfx_only.toml @@ -0,0 +1,18 @@ +[codec] +order = [ "RFX" ] + +[x264.default] +preset = "ultrafast" +tune = "zerolatency" +profile = "main" # profile is forced to baseline if preset == ultrafast +vbv_max_bitrate = 0 +vbv_buffer_size = 0 +fps_num = 24 +fps_den = 1 + +[x264.lan] +[x264.wan] +[x264.broadband_high] +[x264.satellite] +[x264.broadband_low] +[x264.modem] diff --git a/tests/xrdp/gfx/gfx_codec_rfx_preferred.toml b/tests/xrdp/gfx/gfx_codec_rfx_preferred.toml new file mode 100644 index 00000000..c09d029b --- /dev/null +++ b/tests/xrdp/gfx/gfx_codec_rfx_preferred.toml @@ -0,0 +1,18 @@ +[codec] +order = [ "RFX", "H.264" ] + +[x264.default] +preset = "ultrafast" +tune = "zerolatency" +profile = "main" # profile is forced to baseline if preset == ultrafast +vbv_max_bitrate = 0 +vbv_buffer_size = 0 +fps_num = 24 +fps_den = 1 + +[x264.lan] +[x264.wan] +[x264.broadband_high] +[x264.satellite] +[x264.broadband_low] +[x264.modem] diff --git a/tests/xrdp/gfx/gfx_codec_rfx_preferred_odd.toml b/tests/xrdp/gfx/gfx_codec_rfx_preferred_odd.toml new file mode 100644 index 00000000..ff5c7015 --- /dev/null +++ b/tests/xrdp/gfx/gfx_codec_rfx_preferred_odd.toml @@ -0,0 +1,18 @@ +[codec] +order = [ "RFX", "H.264", "RFX" ] + +[x264.default] +preset = "ultrafast" +tune = "zerolatency" +profile = "main" # profile is forced to baseline if preset == ultrafast +vbv_max_bitrate = 0 +vbv_buffer_size = 0 +fps_num = 24 +fps_den = 1 + +[x264.lan] +[x264.wan] +[x264.broadband_high] +[x264.satellite] +[x264.broadband_low] +[x264.modem] diff --git a/tests/xrdp/test_tconfig.c b/tests/xrdp/test_tconfig.c index 586278eb..5219b267 100644 --- a/tests/xrdp/test_tconfig.c +++ b/tests/xrdp/test_tconfig.c @@ -6,6 +6,8 @@ #include "test_xrdp.h" #include "xrdp.h" +#define GFXCONF_STUBDIR XRDP_TOP_SRCDIR "/tests/xrdp/gfx/" + START_TEST(test_tconfig_gfx_always_success) { ck_assert_int_eq(1, 1); @@ -15,7 +17,7 @@ END_TEST START_TEST(test_tconfig_gfx_x264_load_basic) { struct xrdp_tconfig_gfx gfxconfig; - int rv = tconfig_load_gfx(XRDP_TOP_SRCDIR "/xrdp/gfx.toml", &gfxconfig); + int rv = tconfig_load_gfx(GFXCONF_STUBDIR "/gfx.toml", &gfxconfig); ck_assert_int_eq(rv, 0); @@ -31,6 +33,46 @@ START_TEST(test_tconfig_gfx_x264_load_basic) } END_TEST +START_TEST(test_tconfig_gfx_codec_order) +{ + struct xrdp_tconfig_gfx gfxconfig; + + /* H264 earlier */ + tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_h264_preferred.toml", &gfxconfig); + ck_assert_int_gt(gfxconfig.codec.h264_idx, -1); + ck_assert_int_gt(gfxconfig.codec.rfx_idx, -1); + ck_assert_int_lt(gfxconfig.codec.h264_idx, gfxconfig.codec.rfx_idx); + + /* H264 only */ + tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_h264_only.toml", &gfxconfig); + ck_assert_int_gt(gfxconfig.codec.h264_idx, -1); + ck_assert_int_eq(gfxconfig.codec.rfx_idx, -1); + + /* RFX earlier */ + tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_rfx_preferred.toml", &gfxconfig); + ck_assert_int_gt(gfxconfig.codec.h264_idx, -1); + ck_assert_int_gt(gfxconfig.codec.rfx_idx, -1); + ck_assert_int_lt(gfxconfig.codec.rfx_idx, gfxconfig.codec.h264_idx); + + /* RFX appears twice like: RFX, H264, RFX */ + tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_rfx_preferred_odd.toml", &gfxconfig); + ck_assert_int_gt(gfxconfig.codec.h264_idx, -1); + ck_assert_int_gt(gfxconfig.codec.rfx_idx, -1); + ck_assert_int_lt(gfxconfig.codec.rfx_idx, gfxconfig.codec.h264_idx); + + /* RFX only */ + tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_rfx_only.toml", &gfxconfig); + ck_assert_int_eq(gfxconfig.codec.h264_idx, -1); + ck_assert_int_gt(gfxconfig.codec.rfx_idx, -1); + + /* H264 is preferred if order undefined */ + tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_order_undefined.toml", &gfxconfig); + ck_assert_int_gt(gfxconfig.codec.h264_idx, -1); + ck_assert_int_gt(gfxconfig.codec.rfx_idx, -1); + ck_assert_int_lt(gfxconfig.codec.h264_idx, gfxconfig.codec.rfx_idx); +} +END_TEST + /******************************************************************************/ Suite * make_suite_tconfig_load_gfx(void) @@ -43,6 +85,7 @@ make_suite_tconfig_load_gfx(void) tc_tconfig_load_gfx = tcase_create("xrdp_tconfig_load_gfx"); tcase_add_test(tc_tconfig_load_gfx, test_tconfig_gfx_always_success); tcase_add_test(tc_tconfig_load_gfx, test_tconfig_gfx_x264_load_basic); + tcase_add_test(tc_tconfig_load_gfx, test_tconfig_gfx_codec_order); suite_add_tcase(s, tc_tconfig_load_gfx); diff --git a/xrdp/gfx.toml b/xrdp/gfx.toml index 5b7f27c4..af3fcf86 100644 --- a/xrdp/gfx.toml +++ b/xrdp/gfx.toml @@ -1,3 +1,6 @@ +[codec] +order = [ "H.264", "RFX" ] + [x264.default] preset = "ultrafast" tune = "zerolatency" diff --git a/xrdp/xrdp_tconfig.c b/xrdp/xrdp_tconfig.c index d16a0f4a..19ad4194 100644 --- a/xrdp/xrdp_tconfig.c +++ b/xrdp/xrdp_tconfig.c @@ -199,6 +199,73 @@ tconfig_load_gfx_x264_ct(toml_table_t *tfile, const int connection_type, return 0; } +static int tconfig_load_gfx_order(toml_table_t *tfile, struct xrdp_tconfig_gfx *config) +{ + /* + * This config loader is not responsible to check if xrdp is built with + * H264/RFX support. Just loads configurations as-is. + */ + + TCLOG(LOG_LEVEL_TRACE, "[codec]"); + + int h264_found = 0; + int rfx_found = 0; + + config->codec.h264_idx = -1; + config->codec.rfx_idx = -1; + + toml_table_t *codec; + toml_array_t *order; + + if ((codec = toml_table_in(tfile, "codec")) != NULL && + (order = toml_array_in(codec, "order")) != NULL) + { + for (int i = 0; ; i++) + { + toml_datum_t datum = toml_string_at(order, i); + + if (datum.ok) + { + if (h264_found == 0 && + (g_strcasecmp(datum.u.s, "h264") == 0 || + g_strcasecmp(datum.u.s, "h.264") == 0)) + { + h264_found = 1; + config->codec.h264_idx = i; + } + if (rfx_found == 0 && + g_strcasecmp(datum.u.s, "rfx") == 0) + { + rfx_found = 1; + config->codec.rfx_idx = i; + } + free(datum.u.s); + } + else + { + break; + } + } + } + + if (h264_found == 0 && rfx_found == 0) + { + /* prefer H264 if no priority found */ + config->codec.h264_idx = 0; + config->codec.rfx_idx = 1; + + TCLOG(LOG_LEVEL_WARNING, "[codec] could not get GFX codec order, using default order" + " h264_idx [%d], rfx_idx [%d]", + config->codec.h264_idx, config->codec.rfx_idx); + + return 1; + } + + TCLOG(LOG_LEVEL_DEBUG, "[codec] h264_idx [%d], rfx_idx [%d]", + config->codec.h264_idx, config->codec.rfx_idx); + return 0; +} + int tconfig_load_gfx(const char *filename, struct xrdp_tconfig_gfx *config) { @@ -225,6 +292,9 @@ tconfig_load_gfx(const char *filename, struct xrdp_tconfig_gfx *config) memset(config, 0, sizeof(struct xrdp_tconfig_gfx)); + /* Load GFX order */ + tconfig_load_gfx_order(tfile, config); + /* First of all, read the default params and override later */ tconfig_load_gfx_x264_ct(tfile, 0, config->x264_param); diff --git a/xrdp/xrdp_tconfig.h b/xrdp/xrdp_tconfig.h index 6a5ba970..8364dcd8 100644 --- a/xrdp/xrdp_tconfig.h +++ b/xrdp/xrdp_tconfig.h @@ -42,8 +42,15 @@ struct xrdp_tconfig_gfx_x264_param int fps_den; }; +struct xrdp_tconfig_gfx_codec_order +{ + int h264_idx; + int rfx_idx; +}; + struct xrdp_tconfig_gfx { + struct xrdp_tconfig_gfx_codec_order codec; /* store x264 parameters for each connection type */ struct xrdp_tconfig_gfx_x264_param x264_param[NUM_CONNECTION_TYPES]; }; From 7238f8f99d1d123d28335dc152b1257b4fdc5113 Mon Sep 17 00:00:00 2001 From: Koichiro Iwao Date: Mon, 26 Aug 2024 18:18:08 +0900 Subject: [PATCH 2/5] Introduce XRDP_H264 macro to indicate any H264 encoder enabled --- xrdp/xrdp.h | 8 ++++++++ xrdp/xrdp_mm.c | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/xrdp/xrdp.h b/xrdp/xrdp.h index 32286f9b..c40557eb 100644 --- a/xrdp/xrdp.h +++ b/xrdp/xrdp.h @@ -38,6 +38,14 @@ #include "xrdp_client_info.h" #include "log.h" +#if defined(XRDP_X264) || defined(XRDP_OPENH264) || defined(XRDP_NVENC) +#if !defined(XRDP_H264) +#define XRDP_H264 1 +#endif +#else +#undef XRDP_H264 +#endif + /* xrdp.c */ long g_xrdp_sync(long (*sync_func)(long param1, long param2), long sync_param1, diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index 16549464..a22f2f19 100644 --- a/xrdp/xrdp_mm.c +++ b/xrdp/xrdp_mm.c @@ -1440,7 +1440,7 @@ xrdp_mm_egfx_caps_advertise(void *user, int caps_count, /* prefer h264, todo use setting in xrdp.ini for this */ if (best_h264_index >= 0) { -#if defined(XRDP_X264) || defined(XRDP_NVENC) +#if defined(XRDP_H264) best_index = best_h264_index; self->egfx_flags = XRDP_EGFX_H264; #endif From 2c2585cc90e525b72620440cb857a151c042d6f5 Mon Sep 17 00:00:00 2001 From: Koichiro Iwao Date: Mon, 26 Aug 2024 18:23:10 +0900 Subject: [PATCH 3/5] GFX: use the preferred codec preferred in the config (H264 or RFX) --- xrdp/xrdp_mm.c | 22 +++++++++++++++++----- xrdp/xrdp_types.h | 3 +++ xrdp/xrdp_wm.c | 10 +++++++++- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index a22f2f19..1853b7b2 100644 --- a/xrdp/xrdp_mm.c +++ b/xrdp/xrdp_mm.c @@ -1432,19 +1432,31 @@ xrdp_mm_egfx_caps_advertise(void *user, int caps_count, break; } } +#if defined(XRDP_H264) + struct xrdp_tconfig_gfx_codec_order co = self->wm->gfx_config->codec; + bool_t use_h264 = (best_h264_index >= 0 && (best_pro_index < 0 || (co.h264_idx >= 0 && co.h264_idx < co.rfx_idx))); + + if (use_h264) + { + best_index = best_h264_index; + self->egfx_flags = XRDP_EGFX_H264; + } + else if (best_pro_index >= 0) + { + best_index = best_pro_index; + self->egfx_flags = XRDP_EGFX_RFX_PRO; + } +#else if (best_pro_index >= 0) { best_index = best_pro_index; self->egfx_flags = XRDP_EGFX_RFX_PRO; } - /* prefer h264, todo use setting in xrdp.ini for this */ if (best_h264_index >= 0) { -#if defined(XRDP_H264) - best_index = best_h264_index; - self->egfx_flags = XRDP_EGFX_H264; -#endif } +#endif + if (best_index >= 0) { LOG(LOG_LEVEL_INFO, " replying version 0x%8.8x flags 0x%8.8x", diff --git a/xrdp/xrdp_types.h b/xrdp/xrdp_types.h index 110b1d00..c5c80975 100644 --- a/xrdp/xrdp_types.h +++ b/xrdp/xrdp_types.h @@ -30,6 +30,7 @@ #include "guid.h" #include "scancode.h" #include "xrdp_client_info.h" +#include "xrdp_tconfig.h" #define MAX_NR_CHANNELS 16 #define MAX_CHANNEL_NAME 16 @@ -581,6 +582,8 @@ struct xrdp_wm /* configuration derived from xrdp.ini */ struct xrdp_config *xrdp_config; + /* configuration derived from gfx.toml */ + struct xrdp_tconfig_gfx *gfx_config; struct xrdp_region *screen_dirty_region; int last_screen_draw_time; diff --git a/xrdp/xrdp_wm.c b/xrdp/xrdp_wm.c index c93d959d..1efffc15 100644 --- a/xrdp/xrdp_wm.c +++ b/xrdp/xrdp_wm.c @@ -115,8 +115,9 @@ xrdp_wm_create(struct xrdp_process *owner, self->target_surface = self->screen; self->current_surface_index = 0xffff; /* screen */ - /* to store configuration from xrdp.ini */ + /* to store configuration from xrdp.ini, gfx.toml */ self->xrdp_config = g_new0(struct xrdp_config, 1); + self->gfx_config = g_new0(struct xrdp_tconfig_gfx, 1); /* Load the channel config so libxrdp can check whether drdynvc is enabled or not */ @@ -163,6 +164,11 @@ xrdp_wm_delete(struct xrdp_wm *self) g_free(self->xrdp_config); } + if (self->gfx_config) + { + g_free(self->gfx_config); + } + /* free self */ g_free(self); } @@ -643,6 +649,8 @@ xrdp_wm_init(struct xrdp_wm *self) load_xrdp_config(self->xrdp_config, self->session->xrdp_ini, self->screen->bpp); + tconfig_load_gfx(XRDP_CFG_PATH "/gfx.toml", self->gfx_config); + /* Remove a font loaded on the previous config */ xrdp_font_delete(self->default_font); self->painter->font = NULL; /* May be set to the default_font */ From 1ac216da1d9c311b04c01d229333617b3e1d3815 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Wed, 28 Aug 2024 12:00:23 +0100 Subject: [PATCH 4/5] Rework codec order in tconfig Replaced codec idx values with a list of codecs from the config file. Added some logging for debugging --- tests/xrdp/test_tconfig.c | 32 +++++----- xrdp/xrdp_mm.c | 47 +++++++-------- xrdp/xrdp_tconfig.c | 119 +++++++++++++++++++++++++++----------- xrdp/xrdp_tconfig.h | 25 +++++++- 4 files changed, 147 insertions(+), 76 deletions(-) diff --git a/tests/xrdp/test_tconfig.c b/tests/xrdp/test_tconfig.c index 5219b267..5a7b6e5c 100644 --- a/tests/xrdp/test_tconfig.c +++ b/tests/xrdp/test_tconfig.c @@ -39,37 +39,37 @@ START_TEST(test_tconfig_gfx_codec_order) /* H264 earlier */ tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_h264_preferred.toml", &gfxconfig); - ck_assert_int_gt(gfxconfig.codec.h264_idx, -1); - ck_assert_int_gt(gfxconfig.codec.rfx_idx, -1); - ck_assert_int_lt(gfxconfig.codec.h264_idx, gfxconfig.codec.rfx_idx); + ck_assert_int_eq(gfxconfig.codec.codec_count, 2); + ck_assert_int_eq(gfxconfig.codec.codecs[0], XTC_H264); + ck_assert_int_eq(gfxconfig.codec.codecs[1], XTC_RFX); /* H264 only */ tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_h264_only.toml", &gfxconfig); - ck_assert_int_gt(gfxconfig.codec.h264_idx, -1); - ck_assert_int_eq(gfxconfig.codec.rfx_idx, -1); + ck_assert_int_eq(gfxconfig.codec.codec_count, 1); + ck_assert_int_eq(gfxconfig.codec.codecs[0], XTC_H264); /* RFX earlier */ tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_rfx_preferred.toml", &gfxconfig); - ck_assert_int_gt(gfxconfig.codec.h264_idx, -1); - ck_assert_int_gt(gfxconfig.codec.rfx_idx, -1); - ck_assert_int_lt(gfxconfig.codec.rfx_idx, gfxconfig.codec.h264_idx); + ck_assert_int_eq(gfxconfig.codec.codec_count, 2); + ck_assert_int_eq(gfxconfig.codec.codecs[0], XTC_RFX); + ck_assert_int_eq(gfxconfig.codec.codecs[1], XTC_H264); /* RFX appears twice like: RFX, H264, RFX */ tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_rfx_preferred_odd.toml", &gfxconfig); - ck_assert_int_gt(gfxconfig.codec.h264_idx, -1); - ck_assert_int_gt(gfxconfig.codec.rfx_idx, -1); - ck_assert_int_lt(gfxconfig.codec.rfx_idx, gfxconfig.codec.h264_idx); + ck_assert_int_eq(gfxconfig.codec.codec_count, 2); + ck_assert_int_eq(gfxconfig.codec.codecs[0], XTC_RFX); + ck_assert_int_eq(gfxconfig.codec.codecs[1], XTC_H264); /* RFX only */ tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_rfx_only.toml", &gfxconfig); - ck_assert_int_eq(gfxconfig.codec.h264_idx, -1); - ck_assert_int_gt(gfxconfig.codec.rfx_idx, -1); + ck_assert_int_eq(gfxconfig.codec.codec_count, 1); + ck_assert_int_eq(gfxconfig.codec.codecs[0], XTC_RFX); /* H264 is preferred if order undefined */ tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_codec_order_undefined.toml", &gfxconfig); - ck_assert_int_gt(gfxconfig.codec.h264_idx, -1); - ck_assert_int_gt(gfxconfig.codec.rfx_idx, -1); - ck_assert_int_lt(gfxconfig.codec.h264_idx, gfxconfig.codec.rfx_idx); + ck_assert_int_eq(gfxconfig.codec.codec_count, 2); + ck_assert_int_eq(gfxconfig.codec.codecs[0], XTC_H264); + ck_assert_int_eq(gfxconfig.codec.codecs[1], XTC_RFX); } END_TEST diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index 1853b7b2..4e11dc8e 100644 --- a/xrdp/xrdp_mm.c +++ b/xrdp/xrdp_mm.c @@ -1357,7 +1357,6 @@ xrdp_mm_egfx_caps_advertise(void *user, int caps_count, struct xrdp_mm *self; struct xrdp_bitmap *screen; int index; - int best_index; int best_h264_index; int best_pro_index; int error; @@ -1385,7 +1384,6 @@ xrdp_mm_egfx_caps_advertise(void *user, int caps_count, } /* sort by version */ g_qsort(ver_flags, caps_count, sizeof(struct ver_flags_t), cmpverfunc); - best_index = -1; best_h264_index = -1; best_pro_index = -1; for (index = 0; index < caps_count; index++) @@ -1432,31 +1430,34 @@ xrdp_mm_egfx_caps_advertise(void *user, int caps_count, break; } } -#if defined(XRDP_H264) - struct xrdp_tconfig_gfx_codec_order co = self->wm->gfx_config->codec; - bool_t use_h264 = (best_h264_index >= 0 && (best_pro_index < 0 || (co.h264_idx >= 0 && co.h264_idx < co.rfx_idx))); - if (use_h264) + int best_index = -1; + struct xrdp_tconfig_gfx_codec_order *co = &self->wm->gfx_config->codec; + char cobuff[64]; + + LOG(LOG_LEVEL_INFO, "Codec search order is %s", + tconfig_codec_order_to_str(co, cobuff, sizeof(cobuff))); + for (index = 0 ; index < (unsigned int)co->codec_count ; ++index) { - best_index = best_h264_index; - self->egfx_flags = XRDP_EGFX_H264; - } - else if (best_pro_index >= 0) - { - best_index = best_pro_index; - self->egfx_flags = XRDP_EGFX_RFX_PRO; - } -#else - if (best_pro_index >= 0) - { - best_index = best_pro_index; - self->egfx_flags = XRDP_EGFX_RFX_PRO; - } - if (best_h264_index >= 0) - { - } +#if defined(XRDP_H264) + if (co->codecs[index] == XTC_H264 && best_h264_index >= 0) + { + LOG(LOG_LEVEL_INFO, "Matched H264 mode"); + best_index = best_h264_index; + self->egfx_flags = XRDP_EGFX_H264; + break; + } #endif + if (co->codecs[index] == XTC_RFX && best_pro_index >= 0) + { + LOG(LOG_LEVEL_INFO, "Matched RFX mode"); + best_index = best_pro_index; + self->egfx_flags = XRDP_EGFX_RFX_PRO; + break; + } + } + if (best_index >= 0) { LOG(LOG_LEVEL_INFO, " replying version 0x%8.8x flags 0x%8.8x", diff --git a/xrdp/xrdp_tconfig.c b/xrdp/xrdp_tconfig.c index 19ad4194..9d345243 100644 --- a/xrdp/xrdp_tconfig.c +++ b/xrdp/xrdp_tconfig.c @@ -48,6 +48,54 @@ #define X264_DEFAULT_FPS_NUM 24 #define X264_DEFAULT_FPS_DEN 1 +const char * +tconfig_codec_order_to_str( + const struct xrdp_tconfig_gfx_codec_order *codec_order, + char *buff, + unsigned int bufflen) +{ + if (bufflen < (8 * codec_order->codec_count)) + { + snprintf(buff, bufflen, "???"); + } + else + { + unsigned int p = 0; + int i; + for (i = 0 ; i < codec_order->codec_count; ++i) + { + if (p > 0) + { + buff[p++] = ','; + } + + switch (codec_order->codecs[i]) + { + case XTC_H264: + buff[p++] = 'H'; + buff[p++] = '2'; + buff[p++] = '6'; + buff[p++] = '4'; + break; + + case XTC_RFX: + buff[p++] = 'R'; + buff[p++] = 'F'; + buff[p++] = 'X'; + break; + + default: + buff[p++] = '?'; + buff[p++] = '?'; + buff[p++] = '?'; + } + } + buff[p++] = '\0'; + } + + return buff; +} + static int tconfig_load_gfx_x264_ct(toml_table_t *tfile, const int connection_type, struct xrdp_tconfig_gfx_x264_param *param) @@ -201,6 +249,8 @@ tconfig_load_gfx_x264_ct(toml_table_t *tfile, const int connection_type, static int tconfig_load_gfx_order(toml_table_t *tfile, struct xrdp_tconfig_gfx *config) { + char buff[64]; + /* * This config loader is not responsible to check if xrdp is built with * H264/RFX support. Just loads configurations as-is. @@ -211,8 +261,7 @@ static int tconfig_load_gfx_order(toml_table_t *tfile, struct xrdp_tconfig_gfx * int h264_found = 0; int rfx_found = 0; - config->codec.h264_idx = -1; - config->codec.rfx_idx = -1; + config->codec.codec_count = 0; toml_table_t *codec; toml_array_t *order; @@ -231,13 +280,15 @@ static int tconfig_load_gfx_order(toml_table_t *tfile, struct xrdp_tconfig_gfx * g_strcasecmp(datum.u.s, "h.264") == 0)) { h264_found = 1; - config->codec.h264_idx = i; + config->codec.codecs[config->codec.codec_count] = XTC_H264; + ++config->codec.codec_count; } if (rfx_found == 0 && g_strcasecmp(datum.u.s, "rfx") == 0) { rfx_found = 1; - config->codec.rfx_idx = i; + config->codec.codecs[config->codec.codec_count] = XTC_RFX; + ++config->codec.codec_count; } free(datum.u.s); } @@ -251,18 +302,19 @@ static int tconfig_load_gfx_order(toml_table_t *tfile, struct xrdp_tconfig_gfx * if (h264_found == 0 && rfx_found == 0) { /* prefer H264 if no priority found */ - config->codec.h264_idx = 0; - config->codec.rfx_idx = 1; + config->codec.codecs[0] = XTC_H264; + config->codec.codecs[1] = XTC_RFX; + config->codec.codec_count = 2; - TCLOG(LOG_LEVEL_WARNING, "[codec] could not get GFX codec order, using default order" - " h264_idx [%d], rfx_idx [%d]", - config->codec.h264_idx, config->codec.rfx_idx); + TCLOG(LOG_LEVEL_WARNING, "[codec] could not get GFX codec order, " + "using default order %s", + tconfig_codec_order_to_str(&config->codec, buff, sizeof(buff))); return 1; } - TCLOG(LOG_LEVEL_DEBUG, "[codec] h264_idx [%d], rfx_idx [%d]", - config->codec.h264_idx, config->codec.rfx_idx); + TCLOG(LOG_LEVEL_DEBUG, "[codec] %s", + tconfig_codec_order_to_str(&config->codec, buff, sizeof(buff))); return 0; } @@ -273,42 +325,39 @@ tconfig_load_gfx(const char *filename, struct xrdp_tconfig_gfx *config) char errbuf[200]; toml_table_t *tfile; + memset(config, 0, sizeof(struct xrdp_tconfig_gfx)); + if ((fp = fopen(filename, "r")) == NULL) { TCLOG(LOG_LEVEL_ERROR, "Error loading GFX config file %s (%s)", filename, g_get_strerror()); return 1; } - else if ((tfile = toml_parse_file(fp, errbuf, sizeof(errbuf))) == NULL) + + if ((tfile = toml_parse_file(fp, errbuf, sizeof(errbuf))) == NULL) { TCLOG(LOG_LEVEL_ERROR, "Error in GFX config file %s - %s", filename, errbuf); fclose(fp); return 1; } - else + + TCLOG(LOG_LEVEL_INFO, "Loading GFX config file %s", filename); + fclose(fp); + + /* Load GFX order */ + tconfig_load_gfx_order(tfile, config); + + /* First of all, read the default params and override later */ + tconfig_load_gfx_x264_ct(tfile, 0, config->x264_param); + + for (int ct = CONNECTION_TYPE_MODEM; ct < NUM_CONNECTION_TYPES; ct++) { - TCLOG(LOG_LEVEL_INFO, "Loading GFX config file %s", filename); - fclose(fp); - - memset(config, 0, sizeof(struct xrdp_tconfig_gfx)); - - /* Load GFX order */ - tconfig_load_gfx_order(tfile, config); - - /* First of all, read the default params and override later */ - tconfig_load_gfx_x264_ct(tfile, 0, config->x264_param); - - for (int ct = CONNECTION_TYPE_MODEM; ct < NUM_CONNECTION_TYPES; ct++) - { - memcpy(&config->x264_param[ct], &config->x264_param[0], - sizeof(struct xrdp_tconfig_gfx_x264_param)); - - tconfig_load_gfx_x264_ct(tfile, ct, config->x264_param); - } - - toml_free(tfile); - - return 0; + config->x264_param[ct] = config->x264_param[0]; + tconfig_load_gfx_x264_ct(tfile, ct, config->x264_param); } + + toml_free(tfile); + + return 0; } diff --git a/xrdp/xrdp_tconfig.h b/xrdp/xrdp_tconfig.h index 8364dcd8..d1fc0f11 100644 --- a/xrdp/xrdp_tconfig.h +++ b/xrdp/xrdp_tconfig.h @@ -42,10 +42,17 @@ struct xrdp_tconfig_gfx_x264_param int fps_den; }; +enum xrdp_tconfig_codecs +{ + XTC_H264, + XTC_RFX +}; + struct xrdp_tconfig_gfx_codec_order { - int h264_idx; - int rfx_idx; + + enum xrdp_tconfig_codecs codecs[2]; + unsigned int codec_count; }; struct xrdp_tconfig_gfx @@ -68,6 +75,20 @@ static const char *const rdpbcgr_connection_type_names[] = 0 }; +/** + * Provide a string representation of a codec order + * + * @param codec_order Codec order struct + * @param buff Buffer for result + * @param bufflen Length of above + * @return Convenience copy of buff + */ +const char * +tconfig_codec_order_to_str( + const struct xrdp_tconfig_gfx_codec_order *codec_order, + char *buff, + unsigned int bufflen); + int tconfig_load_gfx(const char *filename, struct xrdp_tconfig_gfx *config); #endif From 535151127286f7aee80159b137a97a90d9d03e8c Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Thu, 29 Aug 2024 12:17:45 +0100 Subject: [PATCH 5/5] Further changes to selectable H.264 support - Fix CI errors - tconfig_load_gfx() removes H.264 from the supported codec list if significant errors are found loading the H.264 configuration - tconfig_load_gfx() always produces a usable config, even if the specified file can't be loaded --- tests/xrdp/gfx/gfx_missing_h264.toml | 9 ++++ tests/xrdp/test_tconfig.c | 24 +++++++++ xrdp/xrdp_mm.c | 6 ++- xrdp/xrdp_tconfig.c | 81 ++++++++++++++++++++++++---- xrdp/xrdp_tconfig.h | 16 ++++-- 5 files changed, 122 insertions(+), 14 deletions(-) create mode 100644 tests/xrdp/gfx/gfx_missing_h264.toml diff --git a/tests/xrdp/gfx/gfx_missing_h264.toml b/tests/xrdp/gfx/gfx_missing_h264.toml new file mode 100644 index 00000000..90e2eeb4 --- /dev/null +++ b/tests/xrdp/gfx/gfx_missing_h264.toml @@ -0,0 +1,9 @@ +[codec] +order = [ "H.264", "RFX" ] + +[x264.lan] +[x264.wan] +[x264.broadband_high] +[x264.satellite] +[x264.broadband_low] +[x264.modem] diff --git a/tests/xrdp/test_tconfig.c b/tests/xrdp/test_tconfig.c index 5a7b6e5c..81184984 100644 --- a/tests/xrdp/test_tconfig.c +++ b/tests/xrdp/test_tconfig.c @@ -73,6 +73,28 @@ START_TEST(test_tconfig_gfx_codec_order) } END_TEST +START_TEST(test_tconfig_gfx_missing_file) +{ + struct xrdp_tconfig_gfx gfxconfig; + + /* Check RFX config is returned if the file doesn't exist */ + tconfig_load_gfx(GFXCONF_STUBDIR "/no_such_file.toml", &gfxconfig); + ck_assert_int_eq(gfxconfig.codec.codec_count, 1); + ck_assert_int_eq(gfxconfig.codec.codecs[0], XTC_RFX); +} +END_TEST + +START_TEST(test_tconfig_gfx_missing_h264) +{ + struct xrdp_tconfig_gfx gfxconfig; + + /* Check RFX config only is returned if H.264 parameters are missing */ + tconfig_load_gfx(GFXCONF_STUBDIR "/gfx_missing_h264.toml", &gfxconfig); + ck_assert_int_eq(gfxconfig.codec.codec_count, 1); + ck_assert_int_eq(gfxconfig.codec.codecs[0], XTC_RFX); +} +END_TEST + /******************************************************************************/ Suite * make_suite_tconfig_load_gfx(void) @@ -86,6 +108,8 @@ make_suite_tconfig_load_gfx(void) tcase_add_test(tc_tconfig_load_gfx, test_tconfig_gfx_always_success); tcase_add_test(tc_tconfig_load_gfx, test_tconfig_gfx_x264_load_basic); tcase_add_test(tc_tconfig_load_gfx, test_tconfig_gfx_codec_order); + tcase_add_test(tc_tconfig_load_gfx, test_tconfig_gfx_missing_file); + tcase_add_test(tc_tconfig_load_gfx, test_tconfig_gfx_missing_h264); suite_add_tcase(s, tc_tconfig_load_gfx); diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index 4e11dc8e..6b59976d 100644 --- a/xrdp/xrdp_mm.c +++ b/xrdp/xrdp_mm.c @@ -1364,6 +1364,10 @@ xrdp_mm_egfx_caps_advertise(void *user, int caps_count, int flags; struct ver_flags_t *ver_flags; +#if !defined(XRDP_H264) + UNUSED_VAR(best_h264_index); +#endif + LOG(LOG_LEVEL_INFO, "xrdp_mm_egfx_caps_advertise:"); self = (struct xrdp_mm *) user; screen = self->wm->screen; @@ -1437,7 +1441,7 @@ xrdp_mm_egfx_caps_advertise(void *user, int caps_count, LOG(LOG_LEVEL_INFO, "Codec search order is %s", tconfig_codec_order_to_str(co, cobuff, sizeof(cobuff))); - for (index = 0 ; index < (unsigned int)co->codec_count ; ++index) + for (index = 0 ; index < co->codec_count ; ++index) { #if defined(XRDP_H264) if (co->codecs[index] == XTC_H264 && best_h264_index >= 0) diff --git a/xrdp/xrdp_tconfig.c b/xrdp/xrdp_tconfig.c index 9d345243..5b4a1986 100644 --- a/xrdp/xrdp_tconfig.c +++ b/xrdp/xrdp_tconfig.c @@ -318,14 +318,61 @@ static int tconfig_load_gfx_order(toml_table_t *tfile, struct xrdp_tconfig_gfx * return 0; } +/** + * Determines whether a codec is enabled + * @param co Ordered codec list + * @param code Code of codec to look for + * @return boolean + */ +static int +codec_enabled(const struct xrdp_tconfig_gfx_codec_order *co, + enum xrdp_tconfig_codecs code) +{ + for (unsigned short i = 0; i < co->codec_count; ++i) + { + if (co->codecs[i] == code) + { + return 1; + } + } + + return 0; +} + +/** + * Disables a Codec by removing it from the codec list + * @param co Ordered codec list + * @param code Code of codec to remove from list + * + * The order of the passed-in codec list is preserved. + */ +static void +disable_codec(struct xrdp_tconfig_gfx_codec_order *co, + enum xrdp_tconfig_codecs code) +{ + unsigned short j = 0; + for (unsigned short i = 0; i < co->codec_count; ++i) + { + if (co->codecs[i] != code) + { + co->codecs[j++] = co->codecs[i]; + } + } + co->codec_count = j; +} + int tconfig_load_gfx(const char *filename, struct xrdp_tconfig_gfx *config) { FILE *fp; char errbuf[200]; toml_table_t *tfile; + int rv = 0; - memset(config, 0, sizeof(struct xrdp_tconfig_gfx)); + /* Default to just RFX support. in case we can't load anything */ + config->codec.codec_count = 1; + config->codec.codecs[0] = XTC_RFX; + memset(config->x264_param, 0, sizeof(config->x264_param)); if ((fp = fopen(filename, "r")) == NULL) { @@ -344,20 +391,34 @@ tconfig_load_gfx(const char *filename, struct xrdp_tconfig_gfx *config) TCLOG(LOG_LEVEL_INFO, "Loading GFX config file %s", filename); fclose(fp); - /* Load GFX order */ + /* Load GFX codec order */ tconfig_load_gfx_order(tfile, config); - /* First of all, read the default params and override later */ - tconfig_load_gfx_x264_ct(tfile, 0, config->x264_param); - - for (int ct = CONNECTION_TYPE_MODEM; ct < NUM_CONNECTION_TYPES; ct++) + /* H.264 configuration */ + if (codec_enabled(&config->codec, XTC_H264)) { - config->x264_param[ct] = config->x264_param[0]; - tconfig_load_gfx_x264_ct(tfile, ct, config->x264_param); + /* First of all, read the default params */ + if (tconfig_load_gfx_x264_ct(tfile, 0, config->x264_param) != 0) + { + /* We can't read the H.264 defaults. Disable H.264 */ + LOG(LOG_LEVEL_WARNING, "H.264 support will be disabled"); + disable_codec(&config->codec, XTC_H264); + rv = 1; + } + else + { + /* Copy default params to other connection types, and + * then override them */ + for (int ct = CONNECTION_TYPE_MODEM; ct < NUM_CONNECTION_TYPES; + ct++) + { + config->x264_param[ct] = config->x264_param[0]; + tconfig_load_gfx_x264_ct(tfile, ct, config->x264_param); + } + } } - toml_free(tfile); - return 0; + return rv; } diff --git a/xrdp/xrdp_tconfig.h b/xrdp/xrdp_tconfig.h index d1fc0f11..649d6490 100644 --- a/xrdp/xrdp_tconfig.h +++ b/xrdp/xrdp_tconfig.h @@ -50,9 +50,8 @@ enum xrdp_tconfig_codecs struct xrdp_tconfig_gfx_codec_order { - enum xrdp_tconfig_codecs codecs[2]; - unsigned int codec_count; + unsigned short codec_count; }; struct xrdp_tconfig_gfx @@ -89,6 +88,17 @@ tconfig_codec_order_to_str( char *buff, unsigned int bufflen); -int tconfig_load_gfx(const char *filename, struct xrdp_tconfig_gfx *config); +/** + * Loads the GFX config from the specified file + * + * @param filename Name of file to load + * @param config Struct to receive result + * @return 0 for success + * + * In the event of failure, an error is logged. A minimal + * useable configuration is always returned + */ +int +tconfig_load_gfx(const char *filename, struct xrdp_tconfig_gfx *config); #endif