Code review comment for lp:~mixxxdevelopers/mixxx/features_cuefile

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Hey Phil,

Really sorry for taking so long to get to this! This all looks nice, I just have some general issues with lack of n-deck support and the duplication between EngineShoutcast and EngineRecord:

- The metadata recording is hard coded to work with only 2 decks. I'd really rather not add more 2-deck code if we don't have to. Can you use the [Master],num_decks control (alternatively, thread the PlayerManager into EngineRecord)? Every deck has an "orientation" control which indicates the side of the crossfader it is attached to. You can use this in conjunction with its volume/pregain faders to figure out if they are "hearable". PlayerInfo is N-Deck ready, so that shouldn't be a problem.

- Assuming you'll change this given the above comment anyway, but ControlObject::getControl shouldn't be used in code that gets called on a schedule. As written it'll be doing 5 locks of the global ControlObject static mutex per call (plus 5 hash lookups) to getActiveTracks() when it could be written just a little more verbosely to get 0 per call.

- There's now a fair amount of duplicated code between EngineShoutcast and EngineRecord. Can you pull the current-metadata-detection code out into a common class that they both use? You could even build it into the PlayerInfo singleton class since that already exists.

- Minor nit: EngineRecord::m_pMetaDataLife is an int so its prefix should be m_iMetaDataFile

- Maybe pull 5000 out into a constant? UPDATE_RATE_MS or something. also add a comment about what it is

thanks,
RJ

review: Needs Fixing

« Back to merge proposal