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] 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