Thread: Sound lags in multi-thread version

  1. #16
    Registered User VirtualAce's Avatar
    Join Date
    Aug 2001
    Posts
    9,607
    2ms isn't high performance?

    This:
    Code:
    ////////////////////////////////////////////
    //Do NOT use this function on older cards //
    //Will cause glitching sound effects      //  
    ////////////////////////////////////////////
    void DXAudio::SetLatency(DWORD dwLatencyMs)
    {
      IDirectMusicAudioPath *pPath;
      if (SUCCEEDED(Performance->GetDefaultAudioPath(&pPath)))
      {
        IKsControl *pControl;
        pPath->GetObjectInPath(0,
                               DMUS_PATH_PORT,
                               0,
                               GUID_All_Objects,
                               0,
                               IID_IKsControl,
                               (void **)&pControl);
    
        if (pControl)
        {
          KSPROPERTY ksp;
          DWORD dwData;
          ULONG cb;
          dwData=dwLatencyMs;
          ksp.Set=GUID_DMUS_PROP_WritePeriod;
          ksp.Id=0;
          ksp.Flags=KSPROPERTY_TYPE_SET;
          pControl->KsProperty(&ksp,sizeof(ksp),&dwData,sizeof(dwData),&cb);
    
          dwData=dwLatencyMs;
          ksp.Set=GUID_DMUS_PROP_WriteLatency;
          ksp.Id=0;
          ksp.Flags=KSPROPERTY_TYPE_SET;
          pControl->KsProperty(&ksp,sizeof(ksp),&dwData,sizeof(dwData),&cb);
          pControl->Release();
        }
        pPath->Release();
      }
    }
    dials down the latency to the specified value. DirectMusic had troubles with high latency in early revisions but was later fixed. This code is taken for the most part from DirectX9 Audio Exposed which was written by the people who designed and implemented DirectMusic for DirectX 9.0. Most are probably not using DirectMusic but RAD game tools is a bit high for my tastes and I think they are a bit hyped since nearly every game that uses the Miles portion of the system has major issues with Creative Labs cards on full audio acceleration. The 'high performance' interfaces you speak of are not any better than dialing down the latency in the driver.

    I will probably move to OpenAL but I still will try to stay away from the low level mixing. I would really gain nothing by doing this.

    Good discussion points but the problem is probably one that any game will experience under a heavy load. Modern games seem to have sound cut-outs that would probably not be noticed by most. This is probably due to some type of scheme where they ensure the most recent sound is played and dial down or remove the older ones. I will let all of you know how this system works in an actual 3D app that is doing far more than this simple 2D one. I expect it will do fine but if not then I will definitely have to take a different approach and I will look into some of the one's that have been suggested.

  2. #17
    Registered User Codeplug's Avatar
    Join Date
    Mar 2003
    Posts
    4,981
    >> To me [setting an event] seems less efficient than my current design.
    Less efficient? Where current design == running 99% of the time ... constantly looking at the queue.

    What you have now is the least efficient - 1) either wasting CPU time while no data is available, 2) or not doing anything at all (busy snoozing) while data needs to be processed, or 3) spinning on, and processing data that needs to be processed. A simple Win32 Event will eliminate 1) and 2) while keeping 3).

    Bottom line - if you see any Sleep() calls in multi-threaded code, huge red flags and loud sirens should be going off in your head.

    >> It sleeps to give cycles back to the CPU so the OS doesn't slow down. Without the sleep the tight loop slows down the rest of the system.
    So the call to Sleep is only a workaround to the real issue, which is the code is written as a tight loop - basically a polling loop. Why poll, when you can simply spin only when there's data to spin on? Any issue of "who should get more CPU time" should be controlled by priority in the OS, or by custom synchronization. Calling Sleep is the coarsest of all methods to achieve this - especially for something approaching real time requirements.

    gg

  3. #18
    Registered User VirtualAce's Avatar
    Join Date
    Aug 2001
    Posts
    9,607
    All of you finally convinced me to use the event system in Windows. Here is the new loop and other relevant changes and it does not lag at all:

    Code:
    void DXSoundEmitter::Loop()
    {
        WaitForSingleObject(m_SoundEvent,INFINITE);
    
        switch (m_SoundState)
        {
            case SOUND_SYS_ACTIVE:
            {
                if (m_PlayList.size() > 0)
                {
                    EnterCriticalSection(&m_CS);
                    
                    m_PlayingID = m_PlayList.front();
                    Play(m_PlayingID);
                    m_PlayList.pop();
                    
                    LeaveCriticalSection(&m_CS);
                }
            }
            break;
            case SOUND_SYS_TERMINATING:
            {
                Stop();
            }
            break;
        }
    }
    
    void DXSoundEmitter::playSound(unsigned int ID)
    {
        EnterCriticalSection(&m_CS);
    
        m_PlayList.push(ID);
        SetEvent(m_SoundEvent);
    
        LeaveCriticalSection(&m_CS);
    }
    
    void DXSoundEmitter::flushThread()
    {
        EnterCriticalSection (&m_CS);
    
        m_SoundState = SOUND_SYS_TERMINATING;
        SetEvent(m_SoundEvent);
    
        LeaveCriticalSection(&m_CS);
    }
    Last edited by VirtualAce; 08-27-2008 at 01:13 AM.

  4. #19
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    The code looks subject to a race condition.

    Since there's no code to reset the event, I'm assuming you're using an auto-reset event. The moment you wait successfully on it, it is reset.

    If you start two sounds at about the same time, it could be that both actions are performed before the sound thread wakes up. Thus, the event is set twice, and there are two elements in the queue, but of course the event is a two-state variable and the second setting is lost.

    The sound thread only consumes one sound when it wakes up, no matter how many there are in the queue.

    You'd start lagging behind, with the queue slowly filling up and old sounds being played when you schedule a new one.

    You need to fully consume the queue elements whenever you wake up.


    Also, having the m_PlayList.size() call outside the critical section is not safe. The data structure may be in a transitional state (within the push), and size() may report wrong results or even crash (esp. if it's a linked list that doesn't store the size).
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  5. #20
    Registered User Codeplug's Avatar
    Join Date
    Mar 2003
    Posts
    4,981
    Here's a simple shared Q that I often use:
    Code:
    #include <deque>
    #include <stdexcept>
    
    template <typename T>
    class deque_MT
    {
        mutable CRITICAL_SECTION m_cs;
        mutable HANDLE m_consumerEvent;
    
        // RAII for CS
        struct Lock
        {
            CRITICAL_SECTION &m_cs;
            Lock(CRITICAL_SECTION &cs) : m_cs(cs) {::EnterCriticalSection(&m_cs);}
            ~Lock() {::LeaveCriticalSection(&m_cs);}
        };//Lock
    
        std::deque<T> m_queue;
    
    public:
        deque_MT() throw (runtime_error)
        {
            m_consumerEvent = ::CreateEventW(0, TRUE, FALSE, 0);
            if (!m_consumerEvent)
                throw std::runtime_error("deque_MT::deque_MT() CreateEvent failed");
            
            if (!::InitializeCriticalSectionAndSpinCount(&m_cs, 4000))
            {
                ::CloseHandle(m_consumerEvent);
                throw std::runtime_error("deque_MT::deque_MT() Init-CS failed");
            }//if
        }//constructor
    
        ~deque_MT()
        {
            ::CloseHandle(m_consumerEvent);
            ::DeleteCriticalSection(&m_cs);
        }//destructor
    
        HANDLE consumer_event() const {return m_consumerEvent;}
    
        size_t size() const {Lock l(m_cs); return m_queue.size();}
        bool empty() const {return size() == 0;}
    
        void clear() 
        {
            Lock l(m_cs); 
            m_queue.clear();
            ::ResetEvent(m_consumerEvent);
        }//clear
    
        // Add an object to the queue and signal the consumer event
        void enqueue(const T& val)
        {
            Lock l(m_cs); 
            m_queue.push_front(val);
            ::SetEvent(m_consumerEvent);
        }//enqueue
    
        // Remove next object from the queue, returns false if Q is empty
        // NOTE: With multiple consumers, event can be signaled with an empty Q
        bool dequeue(T &val) throw(runtime_error)
        {
            Lock l(m_cs);
            size_t sz = m_queue.size();
            if (sz == 0)
                return false;
    
            if (sz == 1) // this will drain the Q
                ::ResetEvent(m_consumerEvent);
    
            val = m_queue.back();
            m_queue.pop_back();
            return true;
        }//dequeue
    };//class deque_MT
    Things to note:

    STL container completely protected by CS.

    Manual reset event - stays signaled until Q is drained.

    Scope of CS limited to only what needs protection. I noticed that Play(m_PlayingID) is called within the scope of m_CS - which would unnecessarily block threads calling DXSoundEmitter::playSound(), diminishing throughput. If multiple "things" need synchronization, consider using a synchronization object for each "thing".

    So here is how I would re-factor the posted code:
    Code:
    void DXSoundEmitter::Loop()
    {
        const size_t NumWaitObjs = 2;
        const HANDLE WaitObjs[NumWaitObjs] =
        {
            m_ExitEvent, // <-- new member variable, auto reset event
            m_PlayList.consumer_event(),
        };//WaitObjs
    
        enum
        {
            // must keep these in sync with WaitObjs array
            EXIT_SIGNALED     = WAIT_OBJECT_0 + 0,
            PLAYLIST_SIGNALED = WAIT_OBJECT_0 + 1,
        };
    
        DWORD status = WaitForMultipleObjects(NumWaitObjs, WaitObjs, 
                                              FALSE, INFINITE);
        if (status == PLAYLIST_SIGNALED)
        {
            // if only a single consumer (ie this is the only thread that can cause
            // the consumer event to reset), then a signaled event guarantees there's
            // something in the Q - but safety first
            unsigned int nextID;
            if (m_PlayList.dequeue(nextID))
            {
                m_PlayingID = nextID;
                Play(m_PlayingID);
            }//if
        }//if
        else if (status == EXIT_SIGNALED)
        {
            Stop();
            return;
        }//else if
        else
        {
            // TODO - log unexpected return from WFMO
        }//else
    }//Loop
    
    void DXSoundEmitter::playSound(unsigned int ID)
    {
        m_PlayList.enqueue(ID);
    }//playSound
    
    void DXSoundEmitter::flushThread()
    {
        ::SetEvent(m_ExitEvent);
    }//flushThread
    I used another event for exit notification. Not much different from what you had - but since deque_MT<> took ownership of the the protecting CS, another synchronization object was needed. If m_SoundState ever has more states than just run/stop, what you can do is rename m_ExitEvent to m_SoundStateEvent - then signal the event whenever a state change is made externally (you'll need to assume atomicity of reads/writes or use interlocked reads/writes).

    When using WFMO (and not waiting on all objects to be signaled), keep in mind that the first event in the array has "priority" over the second event. So in the above example, if the exit event were second, the thread may not exit as quickly as you would like (or at all) under a high m_PlayList load.

    As an exercise, an alternative method for signaling thread exit could be to use a sentinel ID of 0:
    Code:
    void DXSoundEmitter::playSound(unsigned int ID)
    {
        if (ID != 0)
            m_PlayList.enqueue(ID);
    }//playSound
    
    void DXSoundEmitter::flushThread()
    {
        m_PlayList.clear();
        m_PlayList.enqueue(0);
    }//flushThread
    Couple things to note here: The call to clear() will reset the consumer event. This means that the "status == PLAYLIST_SIGNALED" code could be executing even though the Q is actually empty. We check for a false return from dequeue() so we're ok there.
    You may also be thinking: "Hey, we can now spin on WFSO instead of WFMO. Surely, checking a single event is faster than checking an array of events!" Premature optimization at its finest! The metric to monitor in this case is throughput. Any small speedup in the looping construct would be completely washed out by the overhead incurred by the work-horse: Play().

    So that "exercise" was really a lesson in premature optimization - not that anyone *needed* a lesson of course :) I simply found myself needing the lesson as I thought "how can I make this exit notification as fast and efficient as possible?". As always, follow K.I.S.S., profile, then optimize.

    gg
    Last edited by Codeplug; 08-27-2008 at 08:30 AM.

  6. #21
    Registered User Codeplug's Avatar
    Join Date
    Mar 2003
    Posts
    4,981
    Also ensure that your threads call CoInitializeEx(0, COINIT_MULTITHREADED) before doing COM stuff in that thread.

    gg

  7. #22
    Registered User VirtualAce's Avatar
    Join Date
    Aug 2001
    Posts
    9,607
    Thanks for the good information. You are right that the thread only consumes one sound when it wakes up and I did realize this but it did not seem to affect anything while testing.

    I like the queue and I may use it for the other threads as well. Thanks.

  8. #23
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    Although, I wouldn't expose the queue's consumer event. I'd just make dequeue block.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  9. #24
    Registered User Codeplug's Avatar
    Join Date
    Mar 2003
    Posts
    4,981
    Adding a blocking interface isn't hard. But exposing the handle is needed for use with WFMO - as is usually the case in my own usage.
    Code:
        void dequeue_b(T &val) throw(std::runtime_error)
        {
            for (;;)
            {
                DWORD status = WaitForSingleObject(m_consumerEvent, INFINITE);
                if (status != WAIT_OBJECT_0)
                    throw std::runtime_error("deque_MT::dequeue_b() WFSO failed");
    
                if (dequeue(val))
                    return;
            }//for
        }//dequeue_b
    gg

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Problem building Quake source
    By Silvercord in forum Game Programming
    Replies: 16
    Last Post: 07-11-2010, 09:13 AM
  2. C++ Threading?
    By draggy in forum C++ Programming
    Replies: 5
    Last Post: 08-16-2005, 12:16 PM
  3. [code] Win32 Thread Object
    By Codeplug in forum Windows Programming
    Replies: 0
    Last Post: 06-03-2005, 03:55 PM
  4. Low latency sound effects
    By VirtualAce in forum Game Programming
    Replies: 0
    Last Post: 12-21-2004, 01:58 AM
  5. Win32 Thread Object Model Revisted
    By Codeplug in forum Windows Programming
    Replies: 5
    Last Post: 12-15-2004, 08:50 AM