Merge pull request #3413 from matt335672/detect_noopenh264

Cope with broken OpenH264 encoder

.github/workflows/coverity.yml

@@ -15,6 +15,7 @@ jobs:
       CONF_FLAGS_amd64_max: "--enable-ipv6 --enable-jpeg --enable-fuse --enable-mp3lame
                   --enable-fdkaac --enable-opus --enable-rfxcodec --enable-painter
                   --enable-pixman --enable-utmp
+                  --enable-x264 --enable-openh264
                   --with-imlib2 --with-freetype2 --enable-tests"
     steps:
       - uses: actions/checkout@v4

xrdp/xrdp_encoder.c

@@ -132,6 +132,56 @@ xrdp_enc_data_done_destructor(void *item, void *closure)
     g_free(enc_done);
 }
 
+/*****************************************************************************/
+/**
+ * Sets the methods used by the software H.264 module
+ */
+static void
+set_h264_encoder_methods(struct xrdp_encoder *self)
+{
+    const char *encoder_name = NULL;
+#if defined(XRDP_X264) && defined(XRDP_OPENH264)
+    struct xrdp_tconfig_gfx gfxconfig;
+    tconfig_load_gfx(GFX_CONF, &gfxconfig);
+
+    switch (gfxconfig.h264_encoder)
+    {
+        case XTC_H264_OPENH264:
+            encoder_name = "OpenH264";
+            self->xrdp_encoder_h264_create = xrdp_encoder_openh264_create;
+            self->xrdp_encoder_h264_delete = xrdp_encoder_openh264_delete;
+            self->xrdp_encoder_h264_encode = xrdp_encoder_openh264_encode;
+            break;
+        case XTC_H264_X264:
+        default:
+            /* x264 is the default H.264 software encoder */
+            encoder_name = "x264";
+            self->xrdp_encoder_h264_create = xrdp_encoder_x264_create;
+            self->xrdp_encoder_h264_delete = xrdp_encoder_x264_delete;
+            self->xrdp_encoder_h264_encode = xrdp_encoder_x264_encode;
+            break;
+    }
+#elif defined(XRDP_OPENH264)
+    encoder_name = "OpenH264";
+    self->xrdp_encoder_h264_create = xrdp_encoder_openh264_create;
+    self->xrdp_encoder_h264_delete = xrdp_encoder_openh264_delete;
+    self->xrdp_encoder_h264_encode = xrdp_encoder_openh264_encode;
+#elif defined(XRDP_X264)
+    encoder_name = "x264";
+    self->xrdp_encoder_h264_create = xrdp_encoder_x264_create;
+    self->xrdp_encoder_h264_delete = xrdp_encoder_x264_delete;
+    self->xrdp_encoder_h264_encode = xrdp_encoder_x264_encode;
+#endif
+
+    // Don't log the library we're going to use if we
+    // couldn't load it.
+    if (encoder_name != NULL && self->mm->libh264_loaded)
+    {
+        LOG(LOG_LEVEL_INFO, "xrdp_encoder_create: using %s for "
+            "software encoder", encoder_name);
+    }
+}
+
 /*****************************************************************************/
 struct xrdp_encoder *
 xrdp_encoder_create(struct xrdp_mm *mm)
@@ -176,7 +226,7 @@ xrdp_encoder_create(struct xrdp_mm *mm)
         self->process_enc = process_enc_jpg;
     }
 #if defined(XRDP_X264) || defined(XRDP_OPENH264)
-    else if (mm->egfx_flags & XRDP_EGFX_H264)
+    else if (mm->libh264_loaded && (mm->egfx_flags & XRDP_EGFX_H264) != 0)
     {
         LOG(LOG_LEVEL_INFO,
             "xrdp_encoder_create: starting h264 codec session gfx");
@@ -185,7 +235,7 @@ xrdp_encoder_create(struct xrdp_mm *mm)
         client_info->capture_format = XRDP_nv12_709fr;
         self->gfx = 1;
     }
-    else if (client_info->h264_codec_id != 0)
+    else if (mm->libh264_loaded && client_info->h264_codec_id != 0)
     {
         LOG(LOG_LEVEL_INFO, "xrdp_encoder_create: starting h264 codec session");
         self->codec_id = client_info->h264_codec_id;
@@ -313,42 +363,7 @@ xrdp_encoder_create(struct xrdp_mm *mm)
     /* make sure frames_in_flight is at least 1 */
     self->frames_in_flight = MAX(self->frames_in_flight, 1);
 
-#if defined(XRDP_X264) && defined(XRDP_OPENH264)
-    struct xrdp_tconfig_gfx gfxconfig;
-    tconfig_load_gfx(GFX_CONF, &gfxconfig);
-
-    switch (gfxconfig.h264_encoder)
-    {
-        case XTC_H264_OPENH264:
-            LOG(LOG_LEVEL_INFO, "xrdp_encoder_create: using OpenH264 for "
-                "software encoder");
-            self->xrdp_encoder_h264_create = xrdp_encoder_openh264_create;
-            self->xrdp_encoder_h264_delete = xrdp_encoder_openh264_delete;
-            self->xrdp_encoder_h264_encode = xrdp_encoder_openh264_encode;
-            break;
-        case XTC_H264_X264:
-        default:
-            /* x264 is the default H.264 software encoder */
-            LOG(LOG_LEVEL_INFO, "xrdp_encoder_create: using x264 for "
-                "software encoder");
-            self->xrdp_encoder_h264_create = xrdp_encoder_x264_create;
-            self->xrdp_encoder_h264_delete = xrdp_encoder_x264_delete;
-            self->xrdp_encoder_h264_encode = xrdp_encoder_x264_encode;
-            break;
-    }
-#elif defined(XRDP_OPENH264)
-    LOG(LOG_LEVEL_INFO, "xrdp_encoder_create: using OpenH264 for "
-        "software encoder");
-    self->xrdp_encoder_h264_create = xrdp_encoder_openh264_create;
-    self->xrdp_encoder_h264_delete = xrdp_encoder_openh264_delete;
-    self->xrdp_encoder_h264_encode = xrdp_encoder_openh264_encode;
-#elif defined(XRDP_X264)
-    LOG(LOG_LEVEL_INFO, "xrdp_encoder_create: using x264 for "
-        "software encoder");
-    self->xrdp_encoder_h264_create = xrdp_encoder_x264_create;
-    self->xrdp_encoder_h264_delete = xrdp_encoder_x264_delete;
-    self->xrdp_encoder_h264_encode = xrdp_encoder_x264_encode;
-#endif
+    set_h264_encoder_methods(self);
 
     /* create thread to process messages */
     tc_thread_create(proc_enc_msg, self);
@@ -732,7 +747,6 @@ process_enc_h264(struct xrdp_encoder *self, XRDP_ENC_DATA *enc)
     LOG_DEVEL(LOG_LEVEL_INFO, "process_enc_h264: dummy func");
     return 0;
 }
-
 #endif
 
 /*****************************************************************************/

xrdp/xrdp_encoder_openh264.c

@@ -323,3 +323,34 @@ xrdp_encoder_openh264_encode(void *handle, int session, int left, int top,
     }
     return 0;
 }
+
+/*****************************************************************************/
+int
+xrdp_encoder_openh264_install_ok(void)
+{
+    int rv;
+
+    // Declare something with maximal alignment we can take the address
+    // of to pass to WelsCreateSVCEncoder. This object is not directly
+    // accessed.
+    //
+    // Note we can't use the ISVCEncoder type directly, as in C++ this
+    // is an abstract class.
+    long double dummy;
+
+    ISVCEncoder *p = (ISVCEncoder *)&dummy;
+
+    // The real OpenH264 library will ALWAYS change the value of the
+    // passed-in pointer
+    // The noopenh264 library will NEVER change the value of the passed-in
+    // pointer
+    // For both libraries, the relevant source is in
+    // codec/encoder/plus/src/welsEncoderExt.cpp
+    WelsCreateSVCEncoder(&p);
+    rv = (p != (ISVCEncoder *)&dummy); // Did the passed-in value change
+    // If p is &dummy or NULL, this call does nothing, otherwise resources
+    // are deallocated.
+    WelsDestroySVCEncoder(p);
+
+    return rv;
+}

xrdp/xrdp_encoder_openh264.h

@@ -36,4 +36,15 @@ xrdp_encoder_openh264_encode(void *handle, int session, int left, int top,
                              char *cdata, int *cdata_bytes,
                              int connection_type, int *flags_ptr);
 
+/**
+ * Test OpenH264 library is installed correctly
+ *
+ * It's possible on some distros to install the noopenh264 package
+ * (https://gitlab.com/freedesktop-sdk/noopenh264) for building. If this
+ * is installed at runtime, openh264 cannot be used.
+ * @return Boolean (!= 0 -> working)
+ */
+int
+xrdp_encoder_openh264_install_ok(void);
+
 #endif

xrdp/xrdp_mm.c

@@ -37,6 +37,10 @@
 #include "xrdp_channel.h"
 #include <limits.h>
 
+#if defined(XRDP_OPENH264)
+#include "xrdp_encoder_openh264.h"
+#endif
+
 /* Forward declarations */
 static int
 xrdp_mm_chansrv_connect(struct xrdp_mm *self, const char *port);
@@ -46,6 +50,26 @@ xrdp_mm_connect_sm(struct xrdp_mm *self);
 static int
 xrdp_mm_send_unicode_shutdown(struct xrdp_mm *self, struct trans *trans);
 
+/*****************************************************************************/
+static void
+init_libh264_loaded(struct xrdp_mm *self)
+{
+#if defined(XRDP_OPENH264)
+    // Note that if this fails, and x264 is also configured, x264
+    // will not be considered as a fallback.
+    self->libh264_loaded = xrdp_encoder_openh264_install_ok();
+    if (!self->libh264_loaded)
+    {
+        LOG(LOG_LEVEL_ERROR, "OpenH264 Codec is not installed correctly. "
+            "H.264 will not be used");
+    }
+#elif defined (XRDP_H264)
+    self->libh264_loaded = 1;
+#else
+    self->libh264_loaded = 0;
+#endif
+}
+
 /*****************************************************************************/
 struct xrdp_mm *
 xrdp_mm_create(struct xrdp_wm *owner)
@@ -61,6 +85,8 @@ xrdp_mm_create(struct xrdp_wm *owner)
 
     self->uid = -1; /* Never good to default UIDs to 0 */
 
+    init_libh264_loaded(self);
+
     LOG_DEVEL(LOG_LEVEL_INFO, "xrdp_mm_create: bpp %d mcs_connection_type %d "
               "jpeg_codec_id %d v3_codec_id %d rfx_codec_id %d "
               "h264_codec_id %d",
@@ -72,7 +98,7 @@ xrdp_mm_create(struct xrdp_wm *owner)
               self->wm->client_info->h264_codec_id);
 
     if ((self->wm->client_info->gfx == 0) &&
-            ((self->wm->client_info->h264_codec_id != 0) ||
+            ((self->wm->client_info->h264_codec_id != 0 && self->libh264_loaded) ||
              (self->wm->client_info->jpeg_codec_id != 0) ||
              (self->wm->client_info->rfx_codec_id != 0)))
     {

xrdp/xrdp_types.h

@@ -444,6 +444,10 @@ struct xrdp_mm
     enum xrdp_egfx_flags egfx_flags;
     int gfx_delay_autologin;
     int mod_uses_wm_screen_for_gfx;
+    /* Whether a working h.264 library is loaded.
+     * We check this at run-time, so that we can fall-back to GFX if
+     * the H.264 library is installed incorrectly */
+    int libh264_loaded;  /* != 0 => H.264 can be used */
     /* Resize on-the-fly control */
     struct display_control_monitor_layout_data *resize_data;
     struct list *resize_queue;