search

OpenLightingProject / ola

The Open Lighting Architecture - The Travel Adaptor for the Lighting Industry
https://www.openlighting.org/ola/
Other
641 stars 204 forks source link

StreamingClient cannot process multiple universes but OlaClient can #1793

Open DaAwesomeP opened 1 year ago

DaAwesomeP commented 1 year ago

Hello!

This is a continuation of the discussion here: https://groups.google.com/g/open-lighting/c/6yBsDBnpANU

There are two related issues:

  1. StreamingClient::SendDMX() contains a delay in the form of m_ss->RunOnce(): https://github.com/OpenLightingProject/ola/blob/9604d481c2787bbb6a36141312d28157f18bc0ea/ola/StreamingClient.cpp#L141-L168
    • Code comments indicate that this is to prevent a race condition, but it has the (probably) unintended consequence of making it impossible to send multiple universes destined for one frame with this method.
    • In my tests of sending 24 universes, this call causes the overhead to balloon to 30ms. Removing this call (I did not happen to experience the race but of course that does not mean it does not exist) dropped the overhead to 0ms.
    • A method does not currently exist (although it could be implemented) to call m_ss->RunOnce() only once and then send multiple universes after that would solve this issue.
  2. The use of m_ss->RunOnce() to prevent a race is made unclear by the fact that the corresponding API for OlaClientCore::SendDMX() (called by OlaClient::SendDMX()) does not make the same call to m_ss->RunOnce() : https://github.com/OpenLightingProject/ola/blob/9604d481c2787bbb6a36141312d28157f18bc0ea/ola/OlaClientCore.cpp#L469-L496
    • When called without any callback settings, it makes the same exact call to m_stub->StreamDmxData() as found in StreamingClient but without calling RunOnce().
    • A key difference is that this function is a returns a void and not a bool, but it is still able to check if the socket is open prior to calling m_stub->StreamDmxData() by checking m_connected.

cc @nomis52 @peternewman

DaAwesomeP commented 1 year ago

An example/test to reproduce the latency issue may be found here: https://github.com/DaAwesomeP/ola/commit/7c7ca32a627b4a6f7a42f507879bee152242607c

I may open a pull to add this example in the future.

DaAwesomeP commented 1 year ago

OK, I have updated the example to allow you to test StreamingClient and OlaClientWrapper side by side with the -a flag. The difference in delay between the two APIs is confirmed: https://github.com/DaAwesomeP/ola/commit/2140930eeea4d08d62cea937852dcb1c9b69c8b6

peternewman commented 1 year ago

@DaAwesomeP do you think you'd have time to open a PR to get the initial speed-up fix into 0.10 branch for our upcoming release?

DaAwesomeP commented 1 year ago

@peternewman What's the deadline? I won't have any time until later this week and early next week.

peternewman commented 1 year ago

I'm not really sure @DaAwesomeP . I've asked in #1792 but not had clarification yet. I suspect that sort of timescale should be okay, I've still got a bit of faffing to do for some final bits too.

peternewman commented 1 year ago

Given the main speedup will be in 0.10.9, I've pushed any possible further improvements onto the next (or a future) release.