neutrinolabs / xrdp

xrdp: an open source RDP server
http://www.xrdp.org/
Apache License 2.0
5.77k stars 1.73k forks source link

GFX: configurable x264 parameters #3214

Closed metalefty closed 2 months ago

metalefty commented 3 months ago

This PR makes x264 parameters such as bitrate configurable.

Design is not very sophisticated, comments are welcomed.

TODO:

rowlap commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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!

metalefty commented 3 months ago

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:

I like tomling. It's a cute name.

matt335672 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

matt335672 commented 2 months ago

tconfig looks fine to me!

metalefty commented 2 months ago

@matt335672 Anything else?

matt335672 commented 2 months ago

Not from me - LGTM.