jath03 / openrgb-python

A python client for the OpenRGB SDK
GNU General Public License v3.0
112 stars 22 forks source link

TCP_NODELAY #66

Closed baldingwizard closed 1 month ago

baldingwizard commented 1 year ago

Hi there. Thanks so much for providing openrgb/openrgb-python. It's fantastic to have such freedom to interact with our hardware without having to rely on the "official" means. In tinkering on a new Linux environment I noticed that there's a significant limit to the client/server connection when using openrgb-python. I've tracked it down to the TCP_NODELAY not being set on the client socket. I don't have the time (or setup - I'm currently re-building my linux environments) to submit as an actual patch but I'll let you know what I changed locally. Basically, in the 'start_connection' function in network.py you can set the TCP_NODELAY using :

self.sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)

This can be done immediately after the 'settimeout' call. After this change you can get significantly faster updates of the LEDs. Note that this might cause some people problems if they have written code to rely on the delay for their visual effect - in which case you should add this as an option to the OpenRGBClient() creation or as a separate method to the 'client' object. Hope this is clear and helps. Kind regards, Rich

jath03 commented 1 year ago

Hi Rich, thanks for the appreciation.
The change sounds good, but I want to run a few benchmarks of my own before pushing this. What were you doing that was too slow for your?

baldingwizard commented 1 year ago

I was just messing around with a custom flame effect based on the CPU usage - so it swirls and flickers and changes to red based on CPU load. It wasn't too slow but I wondered how fast it could go to see what was possible. I was getting 100 'zone.show()' calls in about 9.3 seconds (so about 10 frames a second) before the change. Afterwards it was completing 100 calls in around 4.8 seconds (around 20 frames a second). Obviously this was just on my system so you absolutely should run your own tests. Also, such changes to timing could break anyone's code that's relying on it for animation speed so any change should ideally be configurable and probably default to the old behaviour.

jath03 commented 1 year ago

Try with zone.show(fast=True), just for comparison. For things like this, I think the default should probably be the better behavior, but I'll definitely add an option to revert to the old behavior.

baldingwizard commented 1 year ago

Haha - now that makes a big difference. With 'fast=True' on the old openrgb version my code completes in about 0.4s for 1000 frames - but the lights continue updating for about 3 or 4 seconds afterwards as, I presume, the back-end couldn't keep up with the client. After the patch the same code completes in 0.25s for 1000 frame and, again, the lights continue updating for 3 or 4 seconds. So, still some improvement but with the 'fast' option it's way faster than the back-end can keep up with and so it gets out of sync. Is there any call that tells the client to wait for the server to catch up?

jath03 commented 1 year ago

Yeah, that call is called time.sleep ;)

Jokes aside, there's probably something to be done on the server side. I'm pretty sure openrgb isn't doing any buffering itself so I'd guess it's the tcp packets themselves that are getting queued and openrgb is just consuming them slower than they're being sent. There's probably some socket option that would fix it, but I don't really have time to look into it right now.

jath03 commented 1 year ago

Well... when I say fix it ^, I mean just dropping packets that it can't accept so you don't get that weird delay. Actually fixing the problem might be a bit trickier.

baldingwizard commented 1 year ago

Cool - sounds good. Yeah, client/server over sockets can be quite tricksy! :-D From a performance point of view I'm happy for now, since I can just make sure I limit my frame rate to avoid overflowing the communication and it's good to know that the 'zone.show(fast=True)' does the trick and is safe to use (I didn't understand what was going on so assumed that couldn't be "fast") and that gives me what I need. I'll just be careful not to flood it!