Merge lp:~mixxxdevelopers/mixxx/fixes_soundcardSync into lp:mixxx/1.9

[Sean M. Pappalardo]

Merging fixes_soundcardSync which now causes Mixxx to lock to the clock of the master output card.

This branch re-works when Mixxx calculates new sound buffers and now only does it in sync with the sound card that is selected as the master output. (If no master out is selected, it uses one of the direct deck outputs.) This eliminates pops and stutters on the master/direct deck output at least.

If only one card is enabled, it functions as the master.

There is commented code in SoundManager::requestBuffer() that will print whenever a slave sound card drops or repeats a buffer which is very useful for testing.

[Phillip Whelan (pwhelan)]

Resubmit this for trunk, since this did not make the feature freeze for 1.9.

review: Disapprove

[lp:~mixxxdevelopers/mixxx/fixes_soundcardSync]

2553. By Phillip Whelan on 2011-02-02

Merging from the 1.9 branch in preparation for merging into the 1.9 release.

[Phillip Whelan (pwhelan)]

I have reviewed the code and it looks all good; The code is now more elegant, is more flexible and creates no new problems.

I tested this on my desktop; an Ubuntu 10.10 running a vanilla kernel, and experienced no regressions on my Delta 1010LT multi-out card.

Reconsidering my earlier position, I think this should be merged for Mixxx 1.9.1; I would delay it until after the first release, but then get it out as soon as possible. This is only to not further delay the 1.9.0 release. The code is simple enough, and despite how critical it is, it is worth it since it will benefit a large number of novice users.

review: Approve (code review)

=== modified file 'mixxx/src/sounddeviceportaudio.cpp'
--- mixxx/src/sounddeviceportaudio.cpp	2010-12-01 06:29:29 +0000
+++ mixxx/src/sounddeviceportaudio.cpp	2010-12-05 22:36:10 +0000
@@ -102,12 +102,12 @@
     if (m_dSampleRate <= 0) {
         m_dSampleRate = 44100.0f;
     }
-    qDebug() << "m_dSampleRate" << m_dSampleRate;
+    qDebug() << "Requested sample rate:" << m_dSampleRate;
 
     //Get latency in milleseconds
     qDebug() << "framesPerBuffer:" << m_framesPerBuffer;
     double latencyMSec = m_framesPerBuffer / m_dSampleRate * 1000;
-    qDebug() << "latency in milliseconds:" << latencyMSec;
+    qDebug() << "Mixxx latency in milliseconds:" << latencyMSec;
 
     qDebug() << "output channels:" << m_outputParams.channelCount << "| input channels:"
         << m_inputParams.channelCount;
@@ -210,6 +210,12 @@
     }
     else
         qDebug() << "PortAudio: Started stream successfully";
+    
+    // Get the actual details of the stream & update Mixxx's data
+    const PaStreamInfo* streamDetails = Pa_GetStreamInfo(m_pStream);
+    m_dSampleRate = streamDetails->sampleRate;
+    latencyMSec = streamDetails->outputLatency*1000;
+    qDebug() << "Actual sample rate: " << m_dSampleRate << "Hz, latency:" << latencyMSec << "ms";
 
     //Update the samplerate and latency ControlObjects, which allow the waveform view to properly correct
     //for the latency.
@@ -334,7 +340,7 @@
     {
         assert(iFrameSize > 0);
         QHash<AudioOutput, const CSAMPLE*> outputAudio
-            = m_pSoundManager->requestBuffer(m_audioOutputs, framesPerBuffer);
+            = m_pSoundManager->requestBuffer(m_audioOutputs, framesPerBuffer, this, Pa_GetStreamTime(m_pStream));
 
         // Reset sample for each open channel
         memset(output, 0, framesPerBuffer * iFrameSize * sizeof(*output));

=== modified file 'mixxx/src/soundmanager.cpp'
--- mixxx/src/soundmanager.cpp	2010-12-01 06:29:29 +0000
+++ mixxx/src/soundmanager.cpp	2010-12-05 22:36:10 +0000
@@ -42,9 +42,7 @@
     m_pConfig = pConfig;
     m_pMaster = _master;
 
-    iNumDevicesOpenedForOutput = 0;
-    iNumDevicesOpenedForInput = 0;
-    iNumDevicesHaveRequestedBuffer = 0;
+    clearOperativeVariables();
 
     //These are ControlObjectThreadMains because all the code that
     //uses them is called from the GUI thread (stuff like opening soundcards).
@@ -100,6 +98,17 @@
 }
 
 /**
+ * Clears all variables used in operation.
+ * @note This is in a function because it's done in a few places
+ */
+void SoundManager::clearOperativeVariables()
+{
+    iNumDevicesOpenedForOutput = 0;
+    iNumDevicesOpenedForInput = 0;
+    m_pClkRefDevice = NULL;
+}
+
+/**
  * Returns a pointer to the EngineMaster instance this SoundManager
  * is using.
  * @note pointer is const at this point because this is only being inserted
@@ -196,7 +205,7 @@
 {
     //qDebug() << "SoundManager::closeDevices()";
     QListIterator<SoundDevice*> dev_it(m_devices);
-
+    
     //requestBufferMutex.lock(); //Ensures we don't kill a stream in the middle of a callback call.
                                  //Note: if we're using Pa_StopStream() (like now), we don't need
                                  //      to lock. PortAudio stops the threads nicely.
@@ -208,9 +217,7 @@
     //requestBufferMutex.unlock();
 
     //requestBufferMutex.lock();
-    iNumDevicesOpenedForOutput = 0;
-    iNumDevicesOpenedForInput = 0;
-    iNumDevicesHaveRequestedBuffer = 0;
+    clearOperativeVariables();
     //requestBufferMutex.unlock();
 
     m_outputBuffers.clear(); // anti-cruft (safe because outputs only have
@@ -349,7 +356,7 @@
 {
     qDebug() << "SoundManager::setupDevices()";
     int err = 0;
-    iNumDevicesOpenedForOutput = iNumDevicesOpenedForInput = 0;
+    clearOperativeVariables();
     int devicesAttempted = 0;
     int devicesOpened = 0;
 
@@ -400,12 +407,19 @@
             switch (out.getType()) {
             case AudioPath::MASTER:
                 m_outputBuffers[out] = m_pMaster->getMasterBuffer();
+                // Set the master output device's clock as the reference
+                //  to avoid any clock-related disturbances to the public address
+                m_pClkRefDevice = device;
                 break;
             case AudioPath::HEADPHONES:
                 m_outputBuffers[out] = m_pMaster->getHeadphoneBuffer();
                 break;
             case AudioPath::DECK:
                 m_outputBuffers[out] = m_pMaster->getChannelBuffer(out.getIndex());
+                // If a reference device has not yet been set (such as when the Master output
+                //  isn't being used with an external mixer,) use the first deck output
+                //  to avoid any clock-related disturbances on at least one public output
+                if (!m_pClkRefDevice) m_pClkRefDevice = device;
                 break;
             default:
                 break;
@@ -427,9 +441,17 @@
             }
         }
     }
+    
+    if (!m_pClkRefDevice) {
+        QList<SoundDevice*> outputDevices = getDeviceList(m_config.getAPI(), true, false);
+        SoundDevice* device = outputDevices.first();
+        qWarning() << "Output sound device clock reference not set! Using" << device->getDisplayName();
+        m_pClkRefDevice = device;
+    }
+    else qDebug() << "Using" << m_pClkRefDevice->getDisplayName() << "as output sound device clock reference";
 
-    qDebug() << "iNumDevicesOpenedForOutput:" << iNumDevicesOpenedForOutput;
-    qDebug() << "iNumDevicesOpenedForInput:" << iNumDevicesOpenedForInput;
+    qDebug() << iNumDevicesOpenedForOutput << "output sound devices opened";
+    qDebug() << iNumDevicesOpenedForInput << "input sound devices opened";
 
     // returns OK if we were able to open all the devices the user
     // wanted
@@ -483,33 +505,53 @@
 
 //Requests a buffer in the proper format, if we're prepared to give one.
 QHash<AudioOutput, const CSAMPLE*>
-SoundManager::requestBuffer(QList<AudioOutput> outputs, unsigned long iFramesPerBuffer)
+SoundManager::requestBuffer(QList<AudioOutput> outputs, unsigned long iFramesPerBuffer, SoundDevice* device, double streamTime)
 {
     Q_UNUSED(outputs); // unused, we just give the caller the full hash -bkgood
     //qDebug() << "SoundManager::requestBuffer()";
-
-    //qDebug() << "numOpenedDevices" << iNumOpenedDevices;
-    //qDebug() << "iNumDevicesHaveRequestedBuffer" << iNumDevicesHaveRequestedBuffer;
-
-    //When the first device requests a buffer...
-    if (requestBufferMutex.tryLock())
+    
+    /*
+    // Display when sound cards drop or duplicate buffers (use for testing only)
+    if (iNumDevicesOpenedForOutput>1) {
+        // Running total of requested frames
+        long currentFrameCount = 0;
+        if (m_deviceFrameCount.contains(device)) currentFrameCount=m_deviceFrameCount.value(device);
+        m_deviceFrameCount.insert(device, currentFrameCount+iFramesPerBuffer);  // Overwrites existing value if already present
+        // Get current time in milliseconds
+//         uint t = QDateTime::currentDateTime().toTime_t()*1000+QDateTime::currentDateTime().toString("zzz").toUint();
+        
+        if (device != m_pClkRefDevice) {  // If not the reference device,
+            // Detect dropped frames/buffers
+            long sdifference = m_deviceFrameCount.value(m_pClkRefDevice)-m_deviceFrameCount.value(device);
+            QString message = "dropped";
+            if (sdifference < 0) message = "duplicated";
+            if (sdifference != 0) {
+                m_deviceFrameCount.clear();
+                message = QString("%1 %2 %3 frames (%4 buffers)")
+                            .arg(device->getDisplayName())
+                            .arg(message)
+                            .arg(fabs(sdifference))
+                            .arg(fabs(sdifference)/iFramesPerBuffer);
+                qWarning() << message;
+            }
+        }
+    }
+    //  End dropped/duped buffer display
+    */
+
+    //When the clock reference device requests a buffer...
+    if (device == m_pClkRefDevice && requestBufferMutex.tryLock())
     {
-        if (iNumDevicesHaveRequestedBuffer == 0)
-        {
-            //First, sync control parameters with changes from GUI thread
-            sync();
-
-            //Process a block of samples for output. iFramesPerBuffer is the
-            //number of samples for one channel, but the EngineObject
-            //architecture expects number of samples for two channels
-            //as input (buffer size) so...
-            m_pMaster->process(0, 0, iFramesPerBuffer*2);
-
-        }
-        iNumDevicesHaveRequestedBuffer++;
-
-        if (iNumDevicesHaveRequestedBuffer >= iNumDevicesOpenedForOutput)
-            iNumDevicesHaveRequestedBuffer = 0;
+        // Only generate a new buffer for the clock reference card
+//         qDebug() << "New buffer for" << device->getDisplayName() << "of size" << iFramesPerBuffer;
+        //First, sync control parameters with changes from GUI thread
+        sync();
+
+        //Process a block of samples for output. iFramesPerBuffer is the
+        //number of samples for one channel, but the EngineObject
+        //architecture expects number of samples for two channels
+        //as input (buffer size) so...
+        m_pMaster->process(0, 0, iFramesPerBuffer*2);
 
         requestBufferMutex.unlock();
     }
@@ -617,4 +659,4 @@
     }
     //TODO: Add pass-through option here (and push it into EngineMaster)...
     //      (or maybe save it, and then have requestBuffer() push it into EngineMaster)...
-}
+}
\ No newline at end of file

=== modified file 'mixxx/src/soundmanager.h'
--- mixxx/src/soundmanager.h	2010-11-25 05:44:03 +0000
+++ mixxx/src/soundmanager.h	2010-12-05 22:36:10 +0000
@@ -60,7 +60,7 @@
         int setConfig(SoundManagerConfig config);
         void checkConfig();
         QHash<AudioOutput, const CSAMPLE*>
-            requestBuffer(QList<AudioOutput> outputs, unsigned long iFramesPerBuffer);
+            requestBuffer(QList<AudioOutput> outputs, unsigned long iFramesPerBuffer, SoundDevice*, double streamTime=0);
         void pushBuffer(QList<AudioInput> inputs, short *inputBuffer, 
                         unsigned long iFramesPerBuffer, unsigned int iFrameSize);
     signals:
@@ -68,6 +68,8 @@
     public slots:
         void sync();
     private:
+        void clearOperativeVariables();
+        
         EngineMaster *m_pMaster;
         ConfigObject<ConfigValue> *m_pConfig;
         QList<SoundDevice*> m_devices;
@@ -75,12 +77,13 @@
         QString m_hostAPI;
         QHash<AudioOutput, const CSAMPLE*> m_outputBuffers;
         QHash<AudioInput, short*> m_inputBuffers; /** Audio received from input */
+        QHash<SoundDevice*, long> m_deviceFrameCount;   /** Sound card sync */
+        SoundDevice* m_pClkRefDevice;  /** Sound card sync */
 #ifdef __VINYLCONTROL__
         QList<VinylControlProxy*> m_VinylControl;
 #endif        
         unsigned int iNumDevicesOpenedForOutput;
         unsigned int iNumDevicesOpenedForInput;
-        unsigned int iNumDevicesHaveRequestedBuffer;
         QMutex requestBufferMutex;
         SoundManagerConfig m_config;
         SoundDevice *m_pErrorDevice;

=== modified file 'mixxx/src/soundmanagerconfig.h'
--- mixxx/src/soundmanagerconfig.h	2010-09-27 06:39:39 +0000
+++ mixxx/src/soundmanagerconfig.h	2010-12-05 22:36:10 +0000
@@ -29,7 +29,7 @@
 
 #define DEFAULT_API "None"
 
-const unsigned int MAX_LATENCY = 6; // this represents latency values from 1 ms to about
+const unsigned int MAX_LATENCY = 7; // this represents latency values from 1 ms to about
                                     // 180 ms, should be sufficient -- bkgood
 // 20100726 latencies ~80 and ~170 are utterly broken right now, at least on
 // linux/ALSA (although I think this is a Mixxx bug somewhere around