Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GFX: configurable x264 parameters #3214

Merged
merged 4 commits into from
Aug 23, 2024
Merged

Conversation

metalefty
Copy link
Member

This PR makes x264 parameters such as bitrate configurable.

  • Add gfx.toml for configurations for GFX
  • newconfig is a new config parser for toml
  • x264 parameters are stored in an array of structures (idx is connectyionType)
  • Each parameter will be inherited from the default unless redefined for each connection type
    • index 0 is default since connectyionType == 0 is not used in MS-RDPBCGR

Design is not very sophisticated, comments are welcomed.

  • newconfig should be moved to common?

TODO:

  • Make order of preferred codecs (H264 or RFX) configurable

@rowlap
Copy link
Contributor

rowlap commented Aug 19, 2024

Is there scope for fps to match the client, without needing to be hardcoded?

#define X264_DEFAULT_FPS_NUM 24
#define X264_DEFAULT_FPS_DEN 1

We see the artificial capping of refresh rate to <= 30fps to be one of the significant causes of user-perceived latency.

@metalefty
Copy link
Member Author

These macros are not for hardcoding the fps value, actually, the last resort value if fps values are not found anywhere in the config file.

Multiple factors affect the whole refresh rate. There is also a cap on the capture rate on xorgxrdp side. The fps_num and fps_den values can configure the frame rate of x264 encoding but for debugging purposes at the moment. It is not intended for users to simply change fps_num and try to get more than 30fps. It doesn't make significant changes because screen capture rate is around 25fps at the moment.

Network latency is also a big factor in user-perceiving latency. Total latency matters but network latency is not easily controllable. So it makes sense that fps is the only factor we can control. If network latency is 40ms, the total latency will be 80ms (25fps == 40ms/frame). That's huge!

I also would like to tackle the latency issue but not in the scope of this PR right now.

@matt335672
Copy link
Member

Just about to start a review.

One think I must say up front is I'm not keen on the 'newconfig' name. Time has a habit of passing more quickly that any of us like, and all of a sudden, that 'new' can seem quite 'old'.

Just up the road from where we live, there's a national park called the New Forest. I guess it was new in 1086, but here we are over 1000 years later and no-one's quite got round to renaming it yet!

xrdp/xrdp_newconfig.h Outdated Show resolved Hide resolved
xrdp/xrdp_newconfig.h Outdated Show resolved Hide resolved
xrdp/xrdp_newconfig.h Outdated Show resolved Hide resolved
xrdp/xrdp_newconfig.h Outdated Show resolved Hide resolved
xrdp/xrdp_newconfig.c Outdated Show resolved Hide resolved
xrdp/xrdp_newconfig.c Outdated Show resolved Hide resolved
xrdp/xrdp_newconfig.c Outdated Show resolved Hide resolved
xrdp/xrdp_newconfig.c Outdated Show resolved Hide resolved
@metalefty
Copy link
Member Author

One think I must say up front is I'm not keen on the 'newconfig' name. Time has a habit of passing more quickly that any of us like, and all of a sudden, that 'new' can seem quite 'old'.

Just up the road from where we live, there's a national park called the New Forest. I guess it was new in 1086, but here we are over 1000 years later and no-one's quite got round to renaming it yet!

I recalled New York about that. New York is quite younger than New Forest though.

About the "newconfig" name, I don't think it is a good name either and want to change it. Whatever is fine if it recalls TOML or a new config system and doesn't conflict with other names. I have some candidates:

  • stomp (Simple, Stupid, or S-whatever-word TOML parser)
  • tomling (TOML+ing, a male kitten - I didn't know)

I like tomling. It's a cute name.

@matt335672
Copy link
Member

I'm happy with tomling myself, but I'd also be happy with sticking with 'config' if we can make it work with existing files. Naming things is one of the hardest problems in computing I think!

@metalefty
Copy link
Member Author

I also thought about 'config' but there is already xrdp_config structure for storing current ini configurations. So I avoided sticking config or xrdp_config. I would like to collect configurations to new structures for TOML configurations when we completely switch all configurations to TOML.

I've been thinking about the naming, I think simply tconfig would be fine for the new structure.

@metalefty
Copy link
Member Author

Force-pushed. Commits will be squashed into several commits before merging.

@matt335672
Copy link
Member

tconfig looks fine to me!

xrdp/xrdp_tconfig.h Outdated Show resolved Hide resolved
@metalefty
Copy link
Member Author

@matt335672 Anything else?

@matt335672
Copy link
Member

Not from me - LGTM.

@metalefty metalefty merged commit 854d060 into neutrinolabs:devel Aug 23, 2024
14 checks passed
@metalefty metalefty deleted the x264-params branch August 23, 2024 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants