Merge lp://qastaging/~rryan/mixxx/atomic-co into lp://qastaging/~mixxxdevelopers/mixxx/atomic-co

I'm getting really excited about these control changes!

* Create a value wrapper around COBase values -- ControlNumericPrivate (CNP).
* Add a static creation method / hash index of existing CNPs similar to CO.
* Add a ControlBehavior class hierarchy that encodes all the behaviors that CO/COPushButton/COTTRotary/COPotmeter/COLogpotmere/COLinpotmeter/etc. all used to have built in.
* Remove all value-space to parameter-space conversion methods from CO hierarchy (e.g. valueToWidget/valueFromWidget).
* Get rid of CO / COT proxying. CO and COT both refer to CNP now and are equals in the sense that they both make requests to CNP. The only difference is that CO also creates a CNP when it is created for the first time and sets its behavior while a COT only binds to an existing CNP.

* Add valueChangedFromEngine and valueChangedByThis signals to CO and COT respectively. You'll see that CNP solves the issue that was previously solved by the sync thread -- all mutations (add/sub/set/reset) to the CNP include a pointer to the setter. This is re-broadcast by the CNP and so CO/COT can check the pointer and choose to emit either valueChanged() or valueChangedFromEngine/valueChangedByThis().

I'm only keeping the name valueChangedFromEngine name on the CO to keep backwards compatibility with other engine branches that are still unmerged (e.g. engine-control-refactor).

I'm really liking where this is going. There is really no difference between CO and COT except the creation of a CNP and setting of a Behavior in the CO. We could easily swap this out with a static factory method like Control::createPushButton(options...) that returned a COT-style object (that we could finally rename as Control* to keep things shorter). We could also flatten the CO hierarchy if the behavior-setting was done by a factory/static method. ControlPotmeter is the only standout there since other parts of Mixxx introspect on the control using dynamic_cast. Those hacks should be replaced with something more generic at the CNP layer that lets you ask things about the behavior.

Hi RJ,

Here only some edges in advanced, not a complete review.

* Since ControlObjectBase is not a base class any more, we should rename it to ControlObjectAtomic
* the name ControlNumericPrivate is fine for now, but if we want to finaly allow other numeric types, this should be more specific. What about ControlDoublePrivate?
http://mixxx.org/wiki/doku.php/revamped_control_system#control_object_types
* I am not sure if I like your m_pBehavior logic. It looks like you intended to make it also non blocking thread save
But I think it is not required, because only the CO owner should be allowed to set the behaviour of the CO.
It is required to change from elsewhere?
I think your solution might be not entirely save, because this check may fail:
return m_pBehavior ? m_pBehavior->defaultValue(default_value) : default_value;
m_pBehavior may change to NULL or from NULL between the check and the use.
ControlObjectBase<ControlNumericBehaviour> would be a solution but again I don't think it is required.
* We schould check the same for

.. m_defaultValue.
* I would like to get rid of sub and add. It may implies that it is an atomic operation.
* We may introduce a One reader version, instead where sub and add is save.
* The same issue may occur for all other read modify write actions inside the user code.
* We only have to decide if we verify the use by the CPU (overhead) or if we rely on the correctness of the user code?

I like the idea of having a COT, which controls the value change signal for the own changes.

I am not sure if I have fully understand the new behaviour bit here are my comments:

I do not like the idea that ControlObject receives signals. For me it is not clear in which thread it lives.
And I would like to allow two QObjects in the same thread communicate with signals.

What about this:
* An Object has a COT member with set(double value);
* The COT calls set(value, this) of the CO
* The CO emits valueChanged(double value, QObject* sender) in any case
* COT is connected to this signal and remits valueChanged(double value) in case this != sender

This allows that all other QObjects will receive the valueChange signals regardless in which thread it lives.
The resend of COT is "cheap" a direct connection in any case.

Code that is resend resistant, can work with CO without COT the wrapper.

COT should be renamed to something like ControlObjectValve

> * Since ControlObjectBase is not a base class any more, we should rename it to ControlObjectAtomic

Sounds good to me.

> * the name ControlNumericPrivate is fine for now, but if we want to finaly allow other numeric types, this should be more specific. What about ControlDoublePrivate?
http://mixxx.org/wiki/doku.php/revamped_control_system#control_object_types

Good point, I hadn't considered we would want uint types. ControlDoublePrivate seems good. Maybe behaviors should be templated to save common logic across the value types?

> * I am not sure if I like your m_pBehavior logic. It looks like you intended to make it also non blocking thread save But I think it is not required, because only the CO owner should be allowed to set the behaviour of the CO. It is required to change from elsewhere?

I didn't want to limit who could setBehavior(). We may want to do it from elsewhere in the future?

> I think your solution might be not entirely save, because this check may fail:
return m_pBehavior ? m_pBehavior->defaultValue(default_value) : default_value;
m_pBehavior may change to NULL or from NULL between the check and the use.
ControlObjectBase<ControlNumericBehaviour> would be a solution but again I don't think it is required.

Oops! Good point. I'll fix that. The only reason I used QAtomicPointer was to do a fetchAndSetRelaxed() to atomically swap the pointer for a new one. All other accesses should only need normal pointer access.

> * We schould check the same for m_defaultValue.

In this case I think we should keep COBase for defaultValue (And in the future other Control properties like a 'disabled'). In the future (e.g. in effects) the user may be able to tweak the default value of a control that represents an effect parameter so it's possible that it would change rapidly in response to user input and be accessed at the same time by another thread.

> * I would like to get rid of sub and add. It may implies that it is an atomic operation.

We could remove them. The only use of sub/add is in RateControl at the moment. I kind of like them as part of the API but I agree it gives the impression of atomicity.

> * We may introduce a One reader version, instead where sub and add is save.

Hm, given we only found one place to use sub/add I don't know if it's worth the effort.

> * The same issue may occur for all other read modify write actions inside the user code.

Yea, ControlPotmeter for example doesn't use add()/sub() for increment/decrement. It has the same issue where it isn't atomic. Could we extend the COBase API to include add/sub? That's a little odd because COBase isn't just for numeric types. It could allow you to implement operator+/operator- on a custom type if you want to atomically do something to it. Or in the future we could use c++11 lambdas and pass a lambda into COBase that is be used to mutate the value.

> * We only have to decide if we verify the use by the CPU (overhead) or if we rely on the correctness of the user code?

I think a balance is good. Relying on user code to be correct too much will get us into trouble since not everyone writing user code is fully aware of all the details of the control system (a good ex...

> I do not like the idea that ControlObject receives signals. For me it is not clear in which thread it lives.

The only signal it gets is a DirectConnection so the thread shouldn't matter.

> And I would like to allow two QObjects in the same thread communicate with signals.

Could you explain more? I don't think I get it.

> What about this:
> * An Object has a COT member with set(double value);
> * The COT calls set(value, this) of the CO
> * The CO emits valueChanged(double value, QObject* sender) in any case
> * COT is connected to this signal and remits valueChanged(double value) in case this != sender

> This allows that all other QObjects will receive the valueChange signals regardless in which thread it lives.
> The resend of COT is "cheap" a direct connection in any case.

Yea, I also consider DirectConnection to be cheap performance-wise.

Basically, what I've implemented is almost exactly what you're describing except it has some additional benefits. Think of it like this:

* I have renamed ControlObject to ControlNumericPrivate.
* I pulled out all the logic that was previously specialized by sub-classing ControlObject into ControlBehavior.
* All of the previous ControlObject classes (CO, ControlPotmeter, ControlPushButton, etc.) have now been converted into ControlObjectThreads except they also create a CNP and set its behavior if it does not exist.

> Code that is resend resistant, can work with CO without COT the wrapper.

No code in Mixxx has been consciously written to be resend resistant except the slots that intentionally listen to both valueChanged and valueChangedFromEngine. If any other code is resend resistant then that is purely an accident :). Making CO emit valueChanged() for every set() is super dangerous.

The benefits are:

* Separating the behavior of a control from its type. In the future when we have multiple control types that you can lookup it will be handy to be able to represent the behavior separately from the plumbing of the values. It could also allow injection of logic into a control from outside. For example, if you wanted to make a custom control that only allowed set() under a certain circumstance, you could sub-class a behavior to describe the logic you desire and then set your custom behavior on your control.

* No more master/slave relationship between CO and COT. The reason it was this way was because the CO system was designed to only be used in the engine. Today controls are defined and used everywhere in Mixxx and it's really more of a shared key-value store than it is a way to communicate with the engine. Plus, we have to deal with ugly things like NULL-ifying our pointer to the CO when it is deleted in the COT and every time you want to create a COT, you have to call ControlObject::getControl() first. The way I have it, there is still a master/slave but the master is not available to anyone for direct use. CNP is the master, both CO and COT are slaves.

* This will benefit the engine-control-refactor branch. In that branch, CO is not used to communicate with the engine anymore. There is a new type, a CallbackControl which is connected to the CO system via a FIFO of control changes. The...

How much rewriting is this going to require, such as for my master_sync branch which has a lot of controlobject fiddliness?

> How much rewriting is this going to require, such as for my master_sync branch
> which has a lot of controlobject fiddliness?

This will be definitely a problem if we go to much a head with this branch.
I think in the current state there is some assembly line work required but since a controlObject is still a control Object there should be no logic changes required.

So I think we should try to finish and merge a branch soon, which has proved that ControlObjectBase is working.

> How much rewriting is this going to require, such as for my master_sync branch which has a lot of controlobject fiddliness?

For this branch, if we keep the separation between valueChanged and valueChangedFromEngine then it shouldn't need many/any changes.

For engine-control-refactor, the changes will be significant (hopefully mostly cosmetic though, the logic will need a review). I suggest getting master_sync merged before engine-control-refactor :).

> Maybe behaviors should be templated to save common logic across
> the value types?

Yes, we have only to deal with the not temple-atable Qt signals

> > * I would like to get rid of sub and add. It may implies that it is an
> atomic operation.
>
> We could remove them. The only use of sub/add is in RateControl at the moment.
> I kind of like them as part of the API but I agree it gives the impression of
> atomicity.

OK then, lets remove them

> > * The same issue may occur for all other read modify write actions inside
> the user code.
>
> Yea, ControlPotmeter for example doesn't use add()/sub() for
> increment/decrement. It has the same issue where it isn't atomic. Could we
> extend the COBase API to include add/sub? That's a little odd because COBase
> isn't just for numeric types. It could allow you to implement
> operator+/operator- on a custom type if you want to atomically do something to
> it. Or in the future we could use c++11 lambdas and pass a lambda into COBase
> that is be used to mutate the value.

Yes, we can implement an lockfee "read modify write" in the way ARM did it:
* Watch the current value
* Add or Sub a value
* Check the if the current value was changed in the meanwhile
* If Yes, retry.
I will try it out.

Played around with add and sub and I finaly came to the conclusion to remove it, in vafour of clean code.
It is only used once and it is fully save, if there is only one writer.

> COT should be renamed to something like ControlObjectValve

I'm up for making broad changes like this in the future but to be kind to every currently open branch author we should not rename either CO/COT/COTM/COTW until there are fewer major open branches.

> Played around with add and sub and I finaly came to the conclusion to remove it, in vafour of clean code.
> It is only used once and it is fully save, if there is only one writer.

Ok -- I'll do that in this branch.

> * the name ControlNumericPrivate is fine for now, but if we want to finaly allow other numeric types, this should be more specific. What about ControlDoublePrivate?

Fixed

> I think your solution might be not entirely save, because this check may fail:
return m_pBehavior ? m_pBehavior->defaultValue(default_value) : default_value;

Fixed

> Played around with add and sub and I finaly came to the conclusion to remove it, in vafour of clean code.

Fixed

I've re-reviewed the whole branch and merged with lp:~mixxxdevelopers/mixxx/atomic-co and think it's ready to merge into lp:mixxx. I can't wait to get this tested by bleeding-edgers. :)

"the name ControlNumericPrivate is fine for now, but if we want to finaly allow other numeric types, this should be more specific. What about ControlDoublePrivate?"

What happened to using QVariant?

I think QVariant is not suitable for our approach. Because I don't want to deal with type conversion inside of Mixxx.

I have planed to introduce the following types:

    double (legacy) for higest accuracy
    uint32_t, for fast access, and bool values
    uint8_t[4], for routing Midi Messages
    QString

These types should be modifiable by a common Script API using QScriptValue.

So we have the benefit of the typeless java script code but without performance overhead of QVariant.

http://mixxx.org/wiki/doku.php/revamped_control_system#control_object_types

Thanks, Daniel. That makes sense and it doesn't look to be too hard to add more types in the future if we need them.

Other comments/questions I have as I read through the changes:

* Before you get too far along, shouldn't the "Control" class be called "MixxxControl" or "InternalControl" to differentiate it from other types of controls, like external hardware ones? This will be especially important to avoid confusion if we plan to directly expose these to controller scripts.
* controlbehavior.h: don't the definitions of many of the classes have a bit too much code to have it all in the .h file?
* controlbehavior.h: hard-coding max and minPosition will be problematic if in the future we want to support controllers with 14-bit controls (spread across two messages) without scripting.
* controlvalue.h: what exactly does "sacrifice perfect consistency" mean? That occasionally a read value may be out of date? How likely is that to happen and can we somehow keep track of when it does incase we start seeing strange problems elsewhere as a result?
* midicontroller.cpp, line 270 (QString message): Come on, now. You actually made it harder to read. :)
* various places: isn't "getMidiValue" clearer than "getValueToMidi" since the output is a MIDI-scaled (0..127) value?
* controllinpotmeter.cpp and other places: That creation/deletion stuff looks weird to me, but it makes sense given the comment in control.h. As long as code outside the [Mixxx]Control classes won't have to do that jockeying, I'm good with it.

Oh, isn't a straight 1-bit bool better than a uint32_t for boolean values?

> Oh, isn't a straight 1-bit bool better than a uint32_t for boolean values?

Just noticed: We should introduce an int32_t control Object, not unsigned.

I do not like to have bool ControlObjects, because they have no value for invalid.
There is no performance overhead to store boolean into an int32_t.
It is only a question of min and max value of an control o0bject.

From the script, using boolean is fine.

Hey Sean,

> * Before you get too far along, shouldn't the "Control" class be called "MixxxControl" or "InternalControl" to differentiate it from other types of controls, like external hardware ones? This will be especially important to avoid confusion if we plan to directly expose these to controller scripts.

MixxxControl is just as redundant as ControlObject. Control is much shorter to type. Controller scripts already call these control (getControl/setControl).

> * controlbehavior.h: don't the definitions of many of the classes have a bit too much code to have it all in the .h file?

Probably a good point. I was hoping to promote inline-ability but since it's all virtual that's a lost cause anyway.

> * controlbehavior.h: hard-coding max and minPosition will be problematic if in the future we want to support controllers with 14-bit controls (spread across two messages) without scripting.

This is all copy-pasta from the various Control* implementations. The big bug with Mixxx's control system has always been hard-coding the limits of parameter space to be 0-127 to match with MIDI. I aim to change that in the future, not in this branch. Parameter space should be 0-1 always.

> * controlvalue.h: what exactly does "sacrifice perfect consistency" mean? That occasionally a read value may be out of date? How likely is that to happen and can we somehow keep track of when it does incase we start seeing strange problems elsewhere as a result?

Yep, that's right. See the discussion on the ~mixxxdevelopers/mixxx/atomic-co merge. The number of concurrent readers must exceed INT_MAX for inconsistent reads to occur.

> * midicontroller.cpp, line 270 (QString message): Come on, now. You actually made it harder to read. :)

*shrug* all of this MIDI message string formatting probably belongs in a utility library anyway like src/controllers/midi/util.h.

> * various places: isn't "getMidiValue" clearer than "getValueToMidi" since the output is a MIDI-scaled (0..127) value?

This full function name could be written as getValueToMidiParameterSpace so it's much clearer if you're thinking about it in terms of value space vs. parameter space (it's parallel with widget parameter space). This is another thing I want to totally get rid of from the control system anyway. There should just be one single parameter space for controls (instead of two parameter spaces, MIDI parameters and widget parameters) and the MIDI subsystem should do the translation to/from raw MIDI values within the controller MIDI subsystem.

> * controllinpotmeter.cpp and other places: That creation/deletion stuff looks weird to me, but it makes sense given the comment in control.h. As long as code outside the [Mixxx]Control classes won't have to do that jockeying, I'm good with it.

Yea, unless you want to write a custom behavior this is always going to be an implementation detail. The reason that exists is that the control itself might be located in a place where it is not ok to malloc/free (e.g. the engine callback) so it is the caller of setBehavior's job to manage the destruction of the behavior (e.g. by passing the pointer into a FIFO that is read from by a garbage cleanup thread)...

"There should just be one single parameter space for controls (instead of two parameter spaces, MIDI parameters and widget parameters) and the MIDI subsystem should do the translation to/from raw MIDI values within the controller MIDI subsystem."

For the record, I completely agree. (This was one thing that annoyed me during the controller subsystem rewrite, and it remains responsible for inconsistencies between straight XML-mapped controls and those set from script as well as code duplication to minimize them.)

> "the name ControlNumericPrivate is fine for now, but if we want to finaly allow other numeric types, this should be more specific. What about ControlDoublePrivate?"

> What happened to using QVariant?

I think we should still make a QVariant control so that things like scripts can pass around custom values without recompiling the Mixxx binary to add a new control type. We can also make a QVariant-based wrapper around controls for insertion into the controller engine. I agree w/ Daniel that it doesn't make sense to sacrifice the performance in areas where we know the type we will always use for the control.

> > Oh, isn't a straight 1-bit bool better than a uint32_t for boolean values?

> Just noticed: We should introduce an int32_t control Object, not unsigned.

> I do not like to have bool ControlObjects, because they have no value for invalid.
> There is no performance overhead to store boolean into an int32_t.
> It is only a question of min and max value of an control o0bject.

> From the script, using boolean is fine.

Daniel -- this is part of why your ControlValueAtomic really excites me. One thing that has always been a huge hack in Mixxx is co-opting the value space of a control to represent a disabled state or invalid value. We can just add these as properties of the control outside of its value. This is Control 2.0 stuff we have been talking about for a long time -- finally being able to make the GUI respond to a control being disconnected/inactive by graying it out . This is possible if we represent invalid/disabled as a separate value from the Control value.

And if we did that I think it would be fine to make a boolean control -- the API would be much nicer than what we do today to encode a bool in a double by always checking for v != 0.

> For the record, I completely agree. (This was one thing that annoyed me during the controller subsystem rewrite, and it remains responsible for inconsistencies between straight XML-mapped controls and those set from script as well as code duplication to minimize them.)

Yea, it's been this way since Mixxx 1.0 and it's a huge hack.

If we converted everything to have one parameter space then I think every control should have a setParameter()/getParameter() method that allows you to set the value in parameter space. I don't want to work on this (killing MIDI/widget parameter space) until we've merged this though. This branch diff is already quite large.

I also fully agree on both points, RJ.

> co-opting the value space of a control to represent a disabled state or invalid value

See the "scratch" CO where 0=disabled, but 1=play normal speed. Ugh. That's why I had to make a "scratch2" with a separate "scratch2_enabled" CO.

Barring strenuous objection, I'll go ahead and merge this. Daniel, do you agree this in a merge-worthy state?

Hi RJ,

Yes, I will merge it into lp:~mixxxdevelopers/mixxx/atomic-co now.

But I have not entirely reviewed and test it for merge into trunk.

I think I have not fully understand your COT changes yet so I have some code read todo.

---

the "scratch2" and "scratch2_enabled" think is definitely a subject for change in near future.
But there might be race conditions if we handle the scratch2_enabled like a property. For me it is a part of the atomic info scratch2 itself. If we introduce a int type control object we can easily reserve on value for invalid
eg -100 .. 100 + 0x7fffffff = invalid. A bool becomes 0 .. 1 + 0x7fffffff = invalid;
Or we can introduce a bitfield struct like this
struct {
   int value : 31;
   int invalid : 1;
}
Or if we don't mind using the ring implementation of ControlValueAtomic:
struct {
   double value;
   bool invalid;
}

In this way we never have an invalid flag on a valid value or vice versa.

In that case, let's merge to lp:mixxx. Owen is waiting on this to get
master-sync merged. There will be logic issues we run into where parts of
Mixxx have not expected the CO and COT value to be flattened but we will
have to fix those as we come to them. The sooner we can get trunk testers
trying it out the better.

On Thu, May 16, 2013 at 4:48 PM, Daniel Schürmann <email address hidden>wrote:

> Review: Approve
>
> Hi RJ,
>
> Yes, I will merge it into lp:~mixxxdevelopers/mixxx/atomic-co now.
>
> But I have not entirely reviewed and test it for merge into trunk.
>
> I think I have not fully understand your COT changes yet so I have some
> code read todo.
>
> ---
>
> the "scratch2" and "scratch2_enabled" think is definitely a subject for
> change in near future.
> But there might be race conditions if we handle the scratch2_enabled like
> a property. For me it is a part of the atomic info scratch2 itself. If we
> introduce a int type control object we can easily reserve on value for
> invalid
> eg -100 .. 100 + 0x7fffffff = invalid. A bool becomes 0 .. 1 + 0x7fffffff
> = invalid;
> Or we can introduce a bitfield struct like this
> struct {
> int value : 31;
> int invalid : 1;
> }
> Or if we don't mind using the ring implementation of ControlValueAtomic:
> struct {
> double value;
> bool invalid;
> }
>
> In this way we never have an invalid flag on a valid value or vice versa.
>
>
>
> --
> https://code.launchpad.net/~rryan/mixxx/atomic-co/+merge/163831
> You are the owner of lp:~rryan/mixxx/atomic-co.
>

OK, I will be finished today :-)