Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

Playing Audio CDs, part 9 - Fixing Glitches

Playing Audio CDs, part 9 - Fixing Glitches

  • Comments 9
When we last left playing back audio, we had playback working, but it was glitching like CRAZY - literally every packet read had a glitch during playback.

So lets see if we can fix the glitching problem.

As I mentioned yesterday, the root cause of the glitches is that we're waiting on the wave playback to complete - we're doing all our I/O through a single buffer.

So what happens if you don't wait for the wave writes to complete?  It's a simple change to make, maybe it will help with playback.

 

HRESULT CDAENoWaitPlayer::PlayTrack(int TrackNumber)
{
    HRESULT hr;
    HANDLE waveWriteEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
    HWAVEOUT waveHandle = OpenWaveForCDAudio(waveWriteEvent);
    if (waveHandle == NULL)
    {
        return E_FAIL;
    }

    TrackNumber -= 1; // Bias the track number by 1 - the track array is )ORIGIN 0.

    CAtlList<CDRomReadData *> readDataList;

    for (DWORD i = 0 ; i < (_TrackList[TrackNumber]._TrackLength / DEF_SECTORS_PER_READ); i += 1)
    {
        CDRomReadData *readData = new CDRomReadData(DEF_SECTORS_PER_READ);
        if (readData == NULL)
        {
            printf("Failed to allocate a block\n");
            return E_FAIL;
        }
        readData->_RawReadInfo.DiskOffset.QuadPart = ((i * DEF_SECTORS_PER_READ) + _TrackList[TrackNumber]._TrackStartAddress)*
                                                      CDROM_COOKED_BYTES_PER_SECTOR;
        readData->_RawReadInfo.TrackMode = CDDA;
        readData->_RawReadInfo.SectorCount = DEF_SECTORS_PER_READ;
        hr = CDRomIoctl(IOCTL_CDROM_RAW_READ, &readData->_RawReadInfo, sizeof(readData->_RawReadInfo), readData->_CDRomData,
                            readData->_CDRomDataLength);
        if (hr != S_OK)
        {
            printf("Failed to read CD Data: %d", hr);
            return hr;
        }
        MMRESULT waveResult;
        readData->_WaveHdr.dwBufferLength = readData->_CDRomAudioLength;
        readData->_WaveHdr.lpData = (LPSTR)readData->_CDRomData;
        readData->_WaveHdr.dwLoops = 0;

        waveResult = waveOutPrepareHeader(waveHandle, &readData->_WaveHdr, sizeof(readData->_WaveHdr));
        if (waveResult != MMSYSERR_NOERROR)
        {
            printf("Failed to prepare wave header: %d", waveResult);
            return HRESULT_FROM_WIN32(waveResult);
        }
        waveResult = waveOutWrite(waveHandle, &readData->_WaveHdr, sizeof(readData->_WaveHdr));
        if (waveResult != MMSYSERR_NOERROR)
        {
            printf("Failed to write wave header: %d", waveResult);
            return HRESULT_FROM_WIN32(waveResult);
        }
        //
        // Add this buffer to the end of the read queue.
        //
        readDataList.AddTail(readData);

        //
        // See if the block at the head of the list is done. If it is, free it and all the other completed
        // blocks at the head of the list.
        //
        // Because we know that the wave APIs complete their data in order, we know that the first buffers in the list
        // will complete before the last - the list will effectively be sorted by completed state
        //
        while (!readDataList.IsEmpty() && readDataList.GetHead()->_WaveHdr.dwFlags & WHDR_DONE)
        {
            CDRomReadData *completedBlock;
            completedBlock = readDataList.RemoveHead();
            waveOutUnprepareHeader(waveHandle, &completedBlock->_WaveHdr, sizeof(completedBlock->_WaveHdr));
            delete completedBlock;
        };
    }
    //
    //    We're done, return
    //
    return S_OK;
}

So what changed?  First off, instead of allocating a single block, we allocate a CDRomReadData object for every single read.  We take advantage of the fact that the wave APIs queue their writes and simply drop the request into the waveOutWrite API when the data's been read.

Since the blocks of data are much smaller than the length of the track, the odds are high that we'll be done with some of the blocks before we finish reading the audio track, so we use the fact that the wave header has a flag that indicates that the wave writer is done with a block to let us know when it's ok to free up the block.

So when I tested this version of the playback, the glitches were gone (good!).  But the playback stopped after a relatively short time - certainly before the end of the track (I was using Ravel's Bolero as my test case - the clarinet solo at the beginning of the crescendo is a great test to listen for glitches).  But Bolero's about 15 minutes long, and the playback was finishing up after about 1 minute or so.

Why?  Because my CDROM drive is faster than the audio playback - the audio data had been read and queued for the entire track, but it hadn't finished playing back.  If you think about it, reading the data was done at 48x (or whatever the speed of the CDROM drive is), but the playback is done at 1x.

So we need to add one more piece to the routine - just before we return S_OK, we need to put the following loop:

    //
    // Now drain the requests in the queue.
    //
    while (!readDataList.IsEmpty())
    {
        if (readDataList.GetHead()->_WaveHdr.dwFlags & WHDR_DONE)
        {
            CDRomReadData *completedBlock;
            completedBlock = readDataList.RemoveHead();
            waveOutUnprepareHeader(waveHandle, &completedBlock->_WaveHdr, sizeof(completedBlock->_WaveHdr));
            delete completedBlock;
        }
        else
        {   
            Sleep(100);
        }
    };

This loop waits until all the queued reads complete (and frees them).

But there's still a problem with the code - on my machine, playing Bolero, there were 4056 CDRomReadData objects on the readDataList queue that had to be freed during the final rundown list.  The PlayCDWMP application took up 156M of memory.  Essentially we'd read all the audio data into memory during the read process.  Not good.  Note that we didn't LEAK the memory (I fixed that problem) - we know where every bit of the memory is.  But we've allocated WAY more than we should have.

Next time, lets see if we can work on fixing the memory management problem.

 

Edit: Raymond gently reminded me :) that Sleep(0) throws the system into a CPU bound loop, so changed the sleep to actually sleep for some time.

Edit2: For those that complained, added the waveOutUnprepareHeader call.

 

  • I presume you intentionally left out the calls to waveOutUnprepareHeader() again for clarity? I have bad memories of forgetting to do this on Windows 95 and discovering unrecoverable leaks of physical memory in the sound card driver as a result.
  • Phaeron,
    Actually I did (I mentioned that in yesterdays article).

    There are two reasons for it: #1: The call's a NOP on Windows XP, and #2 because it only has to be called when releasing the buffer back to the OS.

    Back in the Windows 3.1 days (and potentially for Win9x), waveOutPrepareBuffer called GlobalLock() on the memory, and waveOutUnprepareBuffer called GlobalUnlock() on the buffer. That's unnecessary these days.
  • Regarding the memory management issue (and the sleep) couldn't you just merge todays code with the previous code that waited on the wave event? You could load up the buffer with a number of blocks, then start waiting on the wave event and read a new block once a write completes. You should always have a number of blocks in the buffer and you could keep memory usage down.
  • Wound - I expect that's exactly what Larry has in store for us tomorrow!
  • Isn't it really sloppy to omit the call to waveOutUnprepareBuffer? You know that it does nothing on Windows XP, but I didn't. And maybe someone tries to run your program on Windows 95 or 98. I think that either you should conform to the documentation for the APIs that you use, or the documentation should be updated.
  • Yes, it is really sloppy and it is why you should never trust application software written by anyone who knows anything about how Windows XP works (as opposed to knowing what the documentation says and obeying it).
    If there is an unspoken and unwritten assumption at Microsoft that waveOutUnprepareBuffer is unnecessary then this will cause the spread of application programs that don't conform to the documentation. One day in 10 years' time someone at Microsoft will (in conformance to the documentation) produce an implementation of waveOutUnprepareBuffer that does do something. Some time after this implementation is released and generally in use, it will collide with some of the "we know what we're doing" software that was written and supplied by Microsoft a long time before.
    The time is closer than you think when it becomes impossible to make any change to anything to do with Windows because the interface contracts that people operated with are different from the interface contracts that were written down.
    I mean, even GetObject is still incorrectly documented: silent assumptions are made and if you can't guess what they are then your program won't work.
  • So be it. The waveOutUnprepareHeaders have been added exactly where they belong.
  • Btw, as a sneak peak, the last article in this series (right now I think it'll come out on Friday (assuming that the code I'm writing for todays article goes well)) discusses why, while this code works, it's not suitable for production systems.

    There are a number of issues that I'm simply not covering (because they're beyond my ability to cover in this forum) that render these examples simply as a proof of concept - they show the outlines of how someone would write a program to play audio from a CD, but they do NOT show how you would do it in a production system.
  • " they do NOT show how you would do it in a production system"

    Would you mind speaking a little to the points that cause this to be true? Even if you can't go into gory detail, it'd be interesting to hear what it would take to make this production worthy in outline form.
Page 1 of 1 (9 items)