Merge lp://qastaging/~mixxxdevelopers/mixxx/engine-control-refactor into lp://qastaging/~mixxxdevelopers/mixxx/trunk

(First commit to this branch was 10/2011.. it sure took long enough ;) )

I'm not qualified to review this so I just have some concerns & questions:

- The Qt docs attach warnings to specifying signal-slot connection types (e.g. Qt::DirectConnection,) saying that Qt automatically uses an appropriate one. (If the signal and slot are in the same thread, Qt::DirectConnection is used automatically.) They say if you find yourself needing to force this, then the threading model is suspect. http://qt-project.org/doc/qt-4.8/threads-qobject.html#signals-and-slots-across-threads
- Please explain your warning about CallbackControl::valueChanged*() as that seems to contradict the above (unless CC is expected to run in the thread of the slot, which would use DirectConnection anyway.)
- What does "callback-safe" mean?
- EngineClipping constructor: the comment for m_ctrlClipping still speaks of ControlPotMeter but the new code uses ControlObject. Which is correct?
- I know you didn't change this but while I'm looking at it, in EnginePassthrough::receiveBuffer: we should change the buffer overflow and underflow detection to assert on the conditional so if it happens, we know what failed. (Asserting on false isn't very informative.) For extra points, use Q_ASSERT_X and add "buffer over/underflow" text.

Thanks for taking a look Sean!

On Thu, Jul 12, 2012 at 4:07 AM, Sean M. Pappalardo
<email address hidden>wrote:

> - The Qt docs attach warnings to specifying signal-slot connection types
> (e.g. Qt::DirectConnection,) saying that Qt automatically uses an
> appropriate one. (If the signal and slot are in the same thread,
> Qt::DirectConnection is used automatically.) They say if you find yourself
> needing to force this, then the threading model is suspect.
> http://qt-project.org/doc/qt-4.8/threads-qobject.html#signals-and-slots-across-threads

All controls and objects that are part of the mixing engine are created in
the main thread and as a result live in the main thread (according to Qt).
That means that for any AutoConnection in the engine code, if an emitted
signal is not in the main thread (e.g. the callback thread) it will be a
QueuedConnection and the slot will be invoked by the main thread. Qt can
never invoke a QueuedConnection using the callback thread because the
callback thread doesn't have an event loop. In order to make
AutoConnections work, we would have to move every single object in the
engine to the engine thread. This is a lot of code for little extra benefit
and a DirectConnection gives you assurance that the slot is a direct call
(which is always what we want in the engine -- we never want a
QueuedConnection ever so why give Qt the option?)

> - Please explain your warning about CallbackControl::valueChanged*() as
> that seems to contradict the above (unless CC is expected to run in the
> thread of the slot, which would use DirectConnection anyway.)
>

As I mentioned above, if you use an AutoConnection, any signal emitted from
engine code will have its slot invoked by the main thread, so all
CallbackControl signals should be connected to using a DirectConnection.

As for your point about CallbackControl running in the thread of the
signal, the signal is always emitted in the callback thread. This is
because whenever a ControlObject emits a valueChanged() signal, that
value-changed signal is written to a lock-free FIFO by
ControlWatcher/CallbackControlManager. At the beginning of the callback,
CallbackControlManager reads all the updates from this FIFO and updates all
the CallbackControls. This causes all the CallbackControls to emit
valueChanged() if they were changed, but the emit happens in the callback
thread. Since any slots connection to CallbackControls must use
DirectConnection, the slots are also run in the callback thread so
everything is safe. This is in direct contrast to trunk today in which you
connect directly to the ControlObject's valueChanged signal and your slot
is usually invoked by the main thread (and sometimes, the controller
thread).

> - What does "callback-safe" mean?
>

Callback-safe means
1) non-blocking .. calling a section of code, emitting a certain signal,
etc. will not cause the callback thread to block
2) the thread running the code is the callback -- this branch makes the
entire engine non-thread-safe so any code that would cause engine code to
be invoked in a thread other than the callback thread is not callback safe.

> - EngineClipping constructor: the comment for m_ctrlClippi...

Hi RJ,

Thank you for refectoring. It is definitive a step in the right directions it looks quite good.

But reading your code and the legacy code I was first a little lost. Do we have a documentation about the details outside the code? I am afraid any new Mixxx developer will have the same experience.

Here are some comments:
* Why do you need CallbackControl::m_updates? It look like that it is never read.
* Qts emit is not lock free. Even if you use a direct connection, the "signalSlotLock" is used when checking if there is a queued connection. So we have to be sure we can always lock. I think this is the case because we do not connect or unconnect at runtime, right?
* I think you can reduce the Fifo size. If it is required to have a fifo bigger than ~3, I think we have a problem anyway because outdated values are processed.

---

Here some additional thoughts (I hope you will not kill me for that):

For my feeling we need a control object refactoring that is a little more advanced.
Since I have not understand the whole picture it may be rubbish, but I hope it helps nevertheless.

* For me a control object value always represents the current value of something NOW. I don't like the current concept with a sync thread which shovel the current values between the control object proxys. The effect is that we may have different values for one control object at the same time.
* The sync thread runs with the audio buffer size. This may triggers a qt Bug up to 4.7 which confuses the order of the slot updates.
* The current concept is very hard to understand at all for new developers.
* new / malloc are relative slow functions so the current concept with QQueue and sync is not the fastest one.
* I have still not managed to get a jerk free waveform progress. I am not sure if the control objects are part of the problem, but I have the feeling that the qt signal queue from Mixxx is quiet overcrowded.

Idea:
* The problem can be very easy solved on 64 bit architectures, because writing a double is an atomic operation. So setting a new value to a control object can be done from any thread directly to the control object value itself. The updates events can be handled by qt as usual, because its almost lock free if we ban connect and disconnect and all other locking functions.
* For the 32 bit architecture we may consider to introducer a 32 bit control object like int or float. (I like to have an int control object anyway). For the required double control objects, we need a ring buffer slightly different to that uses by RJ. It needs to have one element for each possible user thread to garantee lock free access. The current element should be marked by a QAtomicPointer. (Last writer wins)
http://woboq.com/blog/introduction-to-lockfree-programming.html does not entirely fit to our problem but gives a hint how it may work.

Kind regards,

Daniel

Why aren't you updating from trunk if that's where this will end up?