GFX: add config which to prefer H264 vs RFX

[metalefty]

Not implemented to use loaded config yet.

I'd like to get reviewed whether the structure to save codec priority is appropriate.

[matt335672]

Few minor comments.

[metalefty]

We have 2 codecs for GFX now. It might be more complex when we support a third codec.

[metalefty]

@matt335672 Added to use codec order. Can you have a look at the part?

Commits will be squashed into 2 parts later, adding configuration and using configuration.

[matt335672]

@metalefty - sorry it's taken me a while to get to this, but I wanted to review it properly.

I share your concern about working out the codec order, particularly if we add another codec.

I think there's a better way to do this by replacing the _idx values in xrdp_tconfig.h with an array which just contains the order of the codecs entered by the user, like this:-

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

Then in xrdp_mm.c we can just iterate over the list and find the first match, ignoring any unsupported codecs. This scales for any number of codecs.

I've put together a complete patch (including tests) which illustrates this approach:-

patch.txt

Some of the variable names could maybe do with changing.

What do you think?

[metalefty]

I like that idea. Could you attach a patch git format-patch when you have changed variable names? Then I'll add your changes to this pull request.

[matt335672]

Will do.

I’ve also just realised the code I added to xrdp_mm.c is complete rubbish! I’ll fix that too, and test it. That will be tomorrow now.

[matt335672]

How's this?

0001-Rework-codec-order-in-tconfig.patch

After testing, it occurs to me that for some error conditions (e.g. missing file, badly formatted file) we can default to just "RFX" rather than nothing at all. Then at least we get a match on the codecs. What do you think?

[metalefty]

Agreed.

[matt335672]

Away from my PC ATM. I’ll fix the build errors and add a patch (and test) to default to RFX tomorrow, time permitting

[matt335672]

Try this one:-

0001-Further-changes-to-selectable-H.264-support.patch.txt

Two tests added:-

1. If the user tries to load a missing file, an RFX-only config is returned
2. If the config specified H.264, but there are no default for H.264, an RFX-only config is returned

CI should be fine now.

[matt335672]

PS: Feel free to edit/squash as appropriate.

[metalefty]

Thanks, wonderful! LGTM.