StreamingClient cannot process multiple universes but OlaClient can

[DaAwesomeP]

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():
   

   ola/ola/StreamingClient.cpp

   Lines 141 to 168 in 9604d48

   	bool StreamingClient::Send(unsigned int universe, uint8_t priority,
   	const DmxBuffer &data) {
   	if (!m_stub || !m_socket->ValidReadDescriptor())
   	return false;
   	// We select() on the fd here to see if the remove end has closed the
   	// connection. We could skip this and rely on the EPIPE delivered by the
   	// write() below, but that introduces a race condition in the unittests.
   	m_socket_closed = false;
   	m_ss->RunOnce();
   	if (m_socket_closed) {
   	Stop();
   	return false;
   	}
   	ola::proto::DmxData request;
   	request.set_universe(universe);
   	request.set_data(data.Get());
   	request.set_priority(priority);
   	m_stub->StreamDmxData(NULL, &request, NULL, NULL);
   	if (m_socket_closed) {
   	Stop();
   	return false;
   	}
   	return true;
   	}

   • 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() :
   

   ola/ola/OlaClientCore.cpp

   Lines 469 to 496 in 9604d48

   	void OlaClientCore::SendDMX(unsigned int universe,
   	const DmxBuffer &data,
   	const SendDMXArgs &args) {
   	ola::proto::DmxData request;
   	request.set_universe(universe);
   	request.set_data(data.Get());
   	request.set_priority(args.priority);
   	if (args.callback) {
   	// Full request
   	RpcController *controller = new RpcController();
   	ola::proto::Ack *reply = new ola::proto::Ack();
   	if (m_connected) {
   	CompletionCallback *cb = ola::NewSingleCallback(
   	this,
   	&OlaClientCore::HandleGeneralAck,
   	controller, reply, args.callback);
   	m_stub->UpdateDmxData(controller, &request, reply, cb);
   	} else {
   	controller->SetFailed(NOT_CONNECTED_ERROR);
   	HandleGeneralAck(controller, reply, args.callback);
   	}
   	} else if (m_connected) {
   	// stream data
   	m_stub->StreamDmxData(NULL, &request, NULL, NULL);
   	}
   	}

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

An example/test to reproduce the latency issue may be found here: DaAwesomeP@7c7ca32

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

[DaAwesomeP]

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: DaAwesomeP@2140930

[peternewman]

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

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

[peternewman]

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]

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