Merge lp://qastaging/~mixxxdevelopers/mixxx/features_vcman into lp://qastaging/~mixxxdevelopers/mixxx/trunk

Hey Bill,

Looking good -- here are my comments:

* In VinylControlManager, it's a little sketchy to me that SoundManager is passed in and that it's used to scan through its list of enabled inputs -- seems like VCManager shouldn't know those details of SoundManager. Would it be possible to determine which VC decks were enabled by just keeping track of onInputConnect/onInputDisconnect calls? Also naming the parameter 'deck' is a little misleading since one turntable can control multiple decks (or will in the future?) Since you're checking for a matching AudioInput index, doesn't this just test whether the N'th VC input is enabled? Anyway I would just replace this with a QSet<AudioInput> that you add to onInputConnect and remove from onInputDisconnect, that way you can just make this test whether an appropriate AudioInput is in the set.

* I don't think I understand how QSemaphore protects m_proxies in this case. QList is not thread safe so two threads should never access it at once -- this includes using QList::at, so all accesses to the list should be guarded by a monolithic QMutex (yea .. it sucks). I would use a QMutex and then make sure to unlock it the second you get the VCProxy that you need from the list (or in the case of creating the VC proxy, create it, then lock and insert it and unlock).

Otherwise everything seems fine to me -- we should have someone test it out first to verify that it actually works ;)

> * In VinylControlManager, it's a little sketchy to me that SoundManager is
> passed in and that it's used to scan through its list of enabled inputs --
> seems like VCManager shouldn't know those details of SoundManager. Would
> it be possible to determine which VC decks were enabled by just keeping
> track of onInputConnect/onInputDisconnect calls?

Fixed... ended up being way simpler once I wasn't just doing a straight copy+paste from the old code.

> Also naming the parameter
> 'deck' is a little misleading since one turntable can control multiple
> decks (or will in the future?) Since you're checking for a matching
> AudioInput index, doesn't this just test whether the N'th VC input is
> enabled?

The 'deck' parameter actually refers to the deck which the vinyl control input is to control (so deck=1 refers to [Channel1] stuff), so aside from perhaps misguided naming, I think this is correct. But yes, this just makes life easier (saves some code duplication) for some GUI code in mixxx.cpp.

> * I don't think I understand how QSemaphore protects m_proxies in this
> case. QList is not thread safe so two threads should never access it at
> once -- this includes using QList::at, so all accesses to the list should
> be guarded by a monolithic QMutex (yea .. it sucks). I would use a QMutex
> and then make sure to unlock it the second you get the VCProxy that you
> need from the list (or in the case of creating the VC proxy, create it,
> then lock and insert it and unlock).
>

Is QList::at (now QVector::at) really not save to call concurrently? I had a look at the Qt source and it looks like a totally safe operation (immediately, ::at is const so state isn't being modified). The only case I could think of in which this would be a problem is if the list was being modified while ::at was called, which was how I came to the semaphore use: any number of callback threads (which will never be greater than the number of vinyl control objects) can call receiveBuffer and submit samples, but writing requires a total lockout of all list readers so I acquire all of the semaphore's 'resources' (and block until I do) before doing any modification. As I only allocate memory for the vector once and simply replace elements afterwards, I'm wondering if I could just make a mutex for each individual vector slot and use that to keep from replacing a pointer while I'm reading it in another thread.

Some minor fixes below but looks good to me.

> Is QList::at (now QVector::at) really not save to call concurrently?

Basically, no none of them are. The Qt docs don't claim they are thread safe at all. While you're right, the many-reader case will generally not cause issues since it doesn't update state we don't have guarantees that the list/vector isn't being grown/shrunk when we do something to it.

You're assuming that m_proxies never changes. Things like m_proxies.size() aren't ok to call without a lock protecting the entire list if the size of that list changes. Today it's a safe assumption that m_proxies never changes, so they should happen to work fine, but in the future we might change things and forget to update the locking strategy for m_proxies. I would switch to using a QReadWriteLock just to be safe.. it allows many readers concurrently but locks them out when someone is locking it for writing.

- In mixxx.cpp the else block sets m_pVCManager to NULL, but this variable doesn't exist if VINYLCONTROL isn't defined.

Today we don't really support updating the # of decks on the fly and there are only as many spots for proxies as there are physical decks. I'm a little wary of passing in the number of decks a fixed value that doesn't change because this will be a pain later when potentially we let you add decks to Mixxx on the fly, or when the skin can specify how many decks it's made for, etc. etc. What I mean is that it isn't un-imaginable that the number of decks could change on the fly. We can fix this when we n-deck'ify vinyl control though.

> You're assuming that m_proxies never changes. Things like m_proxies.size()
> aren't ok to call without a lock protecting the entire list if the size of
> that list changes. Today it's a safe assumption that m_proxies never changes,
> so they should happen to work fine, but in the future we might change things
> and forget to update the locking strategy for m_proxies. I would switch to
> using a QReadWriteLock just to be safe.. it allows many readers concurrently
> but locks them out when someone is locking it for writing.
>

Indeed, I wrote my locking strategy under the assumptions of how I wrote the code today and not how it might be modified in a year, but I don't guess I see this as making it particularly wrong -- but QReadWriteLock does everything I want, and that's the more obvious approach anyway.

> - In mixxx.cpp the else block sets m_pVCManager to NULL, but this variable
> doesn't exist if VINYLCONTROL isn't defined.
>
Good catch, I have to have it declared as it's passed to the DlgPreferences ctor (which is why it's NULLed) but I think I discovered this after I wrote the VinylControlManager instance into mixxx.h.

> Today we don't really support updating the # of decks on the fly and there are
> only as many spots for proxies as there are physical decks. I'm a little wary
> of passing in the number of decks a fixed value that doesn't change because
> this will be a pain later when potentially we let you add decks to Mixxx on
> the fly, or when the skin can specify how many decks it's made for, etc. etc.
> What I mean is that it isn't un-imaginable that the number of decks could
> change on the fly. We can fix this when we n-deck'ify vinyl control though.

VinylControlManager will now create a VinylControlProxy for whatever index it receives, from 0 to 255 (and why _not_ support 256 decks, VDJ has 99...). I didn't bother to mess with the big config-load/save blocks, those will have to be fixed (i.e. generalized and iterated-over) for n-decks.

Let me know about these changes and I'll merge into trunk if I still get the thumbs-up, thanks for looking at it! fyi, I've tested using a couple of instances of mixxx and crazy jack routing so I'm fairly confident in its correctness, at least in the cases I tested it.

LGTM