VirtualBox

Changeset 103134 in vbox


Ignore:
Timestamp:
Jan 31, 2024 9:21:06 AM (10 months ago)
Author:
vboxsync
Message:

Audio: More locking needed to avoid debug assertions when draining a stream on stream disable. This debug assertion will happen because EMT and the mixer's async I/O thread work on the same circular buffer. There can be situations, if no locking is being used, that the async I/O thread just has consumed the remaining data just a tad before EMT looks for remaining data within the same buffer when disabling the stream. Also, don't immediately (hard) reset the circular buffer when a stream gets disabled, as this will wipe the already announced amount of remaining data when draining a stream. bugref:10354

Location:
trunk/src/VBox/Devices/Audio
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/VBox/Devices/Audio/AudioMixer.cpp

    r99415 r103134  
    20122012    RT_NOREF(idStream);
    20132013
     2014    int rc = RTCritSectEnter(&pSink->CritSect);
     2015    AssertRCReturn(rc, rc);
     2016
    20142017    /*
    20152018     * Figure how much that we can push down.
    … …  
    20232026    Log3Func(("idStream=%u: cbSinkWritable=%#RX32 cbCircBufReadable=%#RX32 -> cbToTransfer=%#RX32 @%#RX64\n",
    20242027              idStream, cbSinkWritable, cbCircBufReadable, cbToTransfer, offStream));
    2025     AssertMsg(!(pSink->fStatus & AUDMIXSINK_STS_DRAINING) || cbCircBufReadable == pSink->cbDmaLeftToDrain,
     2028    /* Note: There now can be more data in the DMA buffer than initially announced.  See @bugref{10354}. */
     2029    AssertMsg(!(pSink->fStatus & AUDMIXSINK_STS_DRAINING) || cbCircBufReadable >= pSink->cbDmaLeftToDrain,
    20262030              ("cbCircBufReadable=%#x cbDmaLeftToDrain=%#x\n", cbCircBufReadable, pSink->cbDmaLeftToDrain));
    20272031
    … …  
    20812085        Assert(cbToTransfer2 == 0);
    20822086
     2087    Log3Func(("idStream=%u: cbCircBufUsed=%RX32 left\n", idStream, (uint32_t)RTCircBufUsed(pCircBuf)));
     2088
     2089    RTCritSectLeave(&pSink->CritSect);
     2090
     2091    LogFlowFuncLeave();
    20832092    return offStream;
    20842093}

  • trunk/src/VBox/Devices/Audio/DevHdaStream.cpp

    r98456 r103134  
    798798     */
    799799
    800     /* (Re-)Allocate the stream's internal DMA buffer,
    801      * based on the timing *and* PCM properties we just got above. */
    802     if (pStreamR3->State.pCircBuf)
    803     {
    804         RTCircBufDestroy(pStreamR3->State.pCircBuf);
    805         pStreamR3->State.pCircBuf = NULL;
    806         pStreamR3->State.StatDmaBufSize = 0;
    807         pStreamR3->State.StatDmaBufUsed = 0;
    808     }
    809     pStreamShared->State.offWrite = 0;
    810     pStreamShared->State.offRead  = 0;
    811 
    812800    /*
    813801     * The default internal ring buffer size must be:
    … …  
    858846    if (RT_SUCCESS(rc))
    859847    {
    860         rc = RTCircBufCreate(&pStreamR3->State.pCircBuf, cbCircBuf);
    861         if (RT_SUCCESS(rc))
     848        /**
     849         * Note: Only re-create the DMA buffer if the size actually has changed.
     850         *
     851         *       Otherwise do *not* reset the stream's circular buffer here, as the audio mixer still relies on
     852         *       previously announced DMA data (via AudioMixerSinkDrainAndStop()) and processes it asynchronously.
     853         *       Resetting the buffer here will cause a race condition.  See @bugref{10354}. */
     854        if (pStreamR3->State.StatDmaBufSize != cbCircBuf)
    862855        {
    863             pStreamR3->State.StatDmaBufSize = cbCircBuf;
    864 
    865             /*
    866              * Forward the timer frequency hint to TM as well for better accuracy on
    867              * systems w/o preemption timers (also good for 'info timers').
    868              */
    869             PDMDevHlpTimerSetFrequencyHint(pDevIns, pStreamShared->hTimer, uTransferHz);
     856            /* (Re-)Allocate the stream's internal DMA buffer,
     857             * based on the timing *and* PCM properties we just got above. */
     858            if (pStreamR3->State.pCircBuf)
     859            {
     860                RTCircBufDestroy(pStreamR3->State.pCircBuf);
     861                pStreamR3->State.pCircBuf = NULL;
     862                pStreamR3->State.StatDmaBufSize = 0;
     863                pStreamR3->State.StatDmaBufUsed = 0;
     864            }
     865            pStreamShared->State.offWrite = 0;
     866            pStreamShared->State.offRead  = 0;
     867
     868            rc = RTCircBufCreate(&pStreamR3->State.pCircBuf, cbCircBuf);
     869            if (RT_SUCCESS(rc))
     870            {
     871                pStreamR3->State.StatDmaBufSize = cbCircBuf;
     872
     873                /*
     874                 * Forward the timer frequency hint to TM as well for better accuracy on
     875                 * systems w/o preemption timers (also good for 'info timers').
     876                 */
     877                PDMDevHlpTimerSetFrequencyHint(pDevIns, pStreamShared->hTimer, uTransferHz);
     878            }
    870879        }
    871880    }
    … …  
    10041013    pStreamShared->State.fInputPreBuffered = false;
    10051014
    1006     if (pStreamR3->State.pCircBuf)
    1007         RTCircBufReset(pStreamR3->State.pCircBuf);
    1008     pStreamShared->State.offWrite = 0;
    1009     pStreamShared->State.offRead  = 0;
     1015    /* Note: Do *not* reset the stream's circular buffer here, as the audio mixer still relies on
     1016     *       previously announced DMA data (via AudioMixerSinkDrainAndStop()) and processes it asynchronously.
     1017     *       Resetting the buffer here will cause a race condition.  See @bugref{10354}. */
    10101018
    10111019    /* Report that we're done resetting this stream. */
    … …  
    10421050    if (pSink)
    10431051    {
     1052        AudioMixerSinkLock(pSink);
     1053
    10441054        if (fEnable)
    10451055        {
    … …  
    10611071            rc = AudioMixerSinkDrainAndStop(pSink,
    10621072                                            pStreamR3->State.pCircBuf ? (uint32_t)RTCircBufUsed(pStreamR3->State.pCircBuf) : 0);
     1073
     1074        AudioMixerSinkUnlock(pSink);
    10631075    }
    10641076    if (   RT_SUCCESS(rc)

  • trunk/src/VBox/Devices/Audio/DevIchAc97.cpp

    r99739 r103134  
    17311731    LogFunc(("[SD%RU8]\n", pStream->u8SD));
    17321732
    1733     if (pStreamCC->State.pCircBuf)
    1734         RTCircBufReset(pStreamCC->State.pCircBuf);
     1733    /* Note: Do *not* reset the stream's circular buffer here, as the audio mixer still relies on
     1734     *       previously announced DMA data (via AudioMixerSinkDrainAndStop()) and processes it asynchronously.
     1735     *       Resetting the buffer here will cause a race condition.  See @bugref{10354}. */
    17351736
    17361737    pStream->Regs.bdbar    = 0;
    … …  
    21992200    if (   pStreamCC->State.pCircBuf
    22002201        && RTCircBufSize(pStreamCC->State.pCircBuf) == cbCircBuf)
    2201         RTCircBufReset(pStreamCC->State.pCircBuf);
     2202    {
     2203        /* Note: Do *not* reset the stream's circular buffer here, as the audio mixer still relies on
     2204         *       previously announced DMA data (via AudioMixerSinkDrainAndStop()) and processes it asynchronously.
     2205         *       Resetting the buffer here will cause a race condition.  See @bugref{10354}. */
     2206    }
    22022207    else
    22032208    {

  • trunk/src/VBox/Devices/Audio/DevSB16.cpp

    r98456 r103134  
    111111typedef struct SB16STREAMSTATE
    112112{
     113    /** Critical section for this stream. */
     114    RTCRITSECT              CritSect;
    113115    /** Flag indicating whether this stream is in enabled state or not. */
    114116    bool                    fEnabled;
    … …  
    336338static int  sb16StreamOpen(PPDMDEVINS pDevIns, PSB16STATE pThis, PSB16STREAM pStream);
    337339static void sb16StreamClose(PPDMDEVINS pDevIns, PSB16STATE pThis, PSB16STREAM pStream);
     340DECLINLINE(void) sb16StreamLock(PSB16STREAM pStream);
     341DECLINLINE(void) sb16StreamUnlock(PSB16STREAM pStream);
    338342DECLINLINE(PAUDMIXSINK) sb16StreamIndexToSink(PSB16STATE pThis, uint8_t uIdx);
    339343static void sb16StreamTransferScheduleNext(PSB16STATE pThis, PSB16STREAM pStream, uint32_t cSamples);
    … …  
    19241928*********************************************************************************************************************************/
    19251929
     1930/**
     1931 * Reads DMA output data into the internal DMA buffer.
     1932 *
     1933 * @returns VBox status code.
     1934 * @param   pStream     The SB16 stream to read DMA output data from.
     1935 * @param   uDmaChan    DMA channel to read from.
     1936 * @param   offDma      DMA offset (in bytes) to start reading from.
     1937 * @param   cbDma       DMA area size in bytes.
     1938 * @param   cbToRead    How much bytes to read in total.
     1939 * @param   pcbRead     Where to return the DMA bytes read.
     1940 *
     1941 * @thread  µEMT
     1942 */
    19261943static int sb16StreamDoDmaOutput(PSB16STATE pThis, PSB16STREAM pStream, int uDmaChan, uint32_t offDma, uint32_t cbDma,
    19271944                                 uint32_t cbToRead, uint32_t *pcbRead)
    … …  
    19301947    //Assert(cbToRead <= cbFree); /** @todo Add statistics for overflows. */
    19311948    cbToRead = RT_MIN(cbToRead, cbFree);
     1949
     1950    sb16StreamLock(pStream);
    19321951
    19331952    uint32_t cbReadTotal = 0;
    … …  
    19491968                break;
    19501969            *pcbRead = 0;
     1970
     1971            sb16StreamUnlock(pStream);
    19511972            return rc;
    19521973        }
    … …  
    19571978            AudioHlpFileWrite(pStream->Dbg.Runtime.pFileDMA, pv, cbRead);
    19581979
     1980        Log3Func(("Writing %#RX32 bytes to DMA buffer\n", cbRead));
     1981
    19591982        RTCircBufReleaseWriteBlock(pStream->State.pCircBuf, cbRead);
     1983
     1984        Log3Func(("Now %#RX32 bytes in buffer\n", RTCircBufUsed(pStream->State.pCircBuf)));
    19601985
    19611986        Assert(cbToRead >= cbRead);
    … …  
    19711996    pStream->State.StatDmaBufUsed = (uint32_t)RTCircBufUsed(pStream->State.pCircBuf);
    19721997
     1998    Log3Func(("[SD%RU8] cbCircBufUsed=%#RX64\n", pStream->uIdx, RTCircBufUsed(pStream->State.pCircBuf)));
     1999
     2000    sb16StreamUnlock(pStream);
    19732001    return VINF_SUCCESS;
    19742002}
    … …  
    20122040    else
    20132041    {
    2014         rc = AudioMixerSinkDrainAndStop(pSink, pStream->State.pCircBuf ? (uint32_t)RTCircBufUsed(pStream->State.pCircBuf) : 0);
     2042        /* We need to lock the stream here in order to synchronize the amount of bytes we have to drain
     2043         * here with the mixer's async I/O thread.  See @bugref{10354}. */
     2044        sb16StreamLock(pStream);
     2045
     2046        uint32_t const cbToDrain = pStream->State.pCircBuf ? (uint32_t)RTCircBufUsed(pStream->State.pCircBuf) : 0;
     2047        Log3Func(("cbToDrain=%#RX32\n", cbToDrain));
     2048
     2049        sb16StreamUnlock(pStream);
     2050
     2051        rc = AudioMixerSinkDrainAndStop(pSink, cbToDrain);
    20152052        AssertRCReturn(rc, rc);
     2053
     2054
    20162055    }
    20172056
    … …  
    20682107{
    20692108    LogFlowFuncEnter();
     2109
     2110    int rc = RTCritSectInit(&pStream->State.CritSect);
     2111    AssertRCReturn(rc, rc);
    20702112
    20712113    pStream->Dbg.Runtime.fEnabled = pThis->Dbg.fEnabled;
    … …  
    21122154    }
    21132155
     2156    int rc2 = RTCritSectDelete(&pStream->State.CritSect);
     2157    AssertRC(rc2);
     2158
    21142159    if (pStream->State.pCircBuf)
    21152160    {
    … …  
    21782223
    21792224/**
     2225 * Locks a SB16 stream for serialized access.
     2226 *
     2227 * @param   pStream             The SB16 stream to lock.
     2228 */
     2229DECLINLINE(void) sb16StreamLock(PSB16STREAM pStream)
     2230{
     2231    int rc2 = RTCritSectEnter(&pStream->State.CritSect);
     2232    AssertRC(rc2);
     2233}
     2234
     2235/**
     2236 * Unlocks a formerly locked SB16 stream.
     2237 *
     2238 * @param   pStream             The SB16 stream to unlock.
     2239 */
     2240DECLINLINE(void) sb16StreamUnlock(PSB16STREAM pStream)
     2241{
     2242    int rc2 = RTCritSectLeave(&pStream->State.CritSect);
     2243    AssertRC(rc2);
     2244}
     2245
     2246/**
    21802247 * Opens a SB16 stream with its current mixer settings.
    21812248 *
    … …  
    22102277             PDMAudioPropsSampleBits(&pStream->Cfg.Props)));
    22112278
    2212     /* (Re-)create the stream's internal ring buffer. */
    2213     if (pStream->State.pCircBuf)
    2214     {
    2215         RTCircBufDestroy(pStream->State.pCircBuf);
    2216         pStream->State.pCircBuf = NULL;
    2217     }
    2218 
    22192279    /** @todo r=bird: two DMA periods is probably too little. */
    22202280    const uint32_t cbCircBuf = PDMAudioPropsMilliToBytes(&pStream->Cfg.Props,
    22212281                                                         (RT_MS_1SEC / pStream->uTimerHz) * 2 /* Use double buffering here */);
    2222 
    2223     int rc = RTCircBufCreate(&pStream->State.pCircBuf, cbCircBuf);
    2224     AssertRCReturn(rc, rc);
    2225     pStream->State.StatDmaBufSize = (uint32_t)RTCircBufSize(pStream->State.pCircBuf);
    2226 
    2227     /* Set scheduling hint. */
    2228     pStream->Cfg.Device.cMsSchedulingHint = RT_MS_1SEC / RT_MIN(pStream->uTimerHz, 1);
    2229 
    2230     PAUDMIXSINK pMixerSink = sb16StreamIndexToSink(pThis, pStream->uIdx);
    2231     AssertPtrReturn(pMixerSink, VERR_INVALID_POINTER);
    2232 
    2233     sb16RemoveDrvStreams(pDevIns, pThis,
    2234                          sb16StreamIndexToSink(pThis, pStream->uIdx), pStream->Cfg.enmDir, pStream->Cfg.enmPath);
    2235 
    2236     rc = sb16AddDrvStreams(pDevIns, pThis, pMixerSink, &pStream->Cfg);
    2237     if (RT_SUCCESS(rc))
    2238     {
    2239         if (RT_LIKELY(!pStream->Dbg.Runtime.fEnabled))
    2240         { /* likely */ }
    2241         else
     2282    int rc = VINF_SUCCESS;
     2283
     2284    /**
     2285     * Note: Only re-create the DMA buffer if the size actually has changed.
     2286     *
     2287     *       Otherwise do *not* reset the stream's circular buffer here, as the audio mixer still relies on
     2288     *       previously announced DMA data (via AudioMixerSinkDrainAndStop()) and processes it asynchronously.
     2289     *       Resetting the buffer here will cause a race condition.  See @bugref{10354}. */
     2290    if (pStream->State.StatDmaBufSize != cbCircBuf)
     2291    {
     2292        Log3Func(("Re-creating ring buffer (%#RX32 -> %#RX32)\n", pStream->State.StatDmaBufSize, cbCircBuf));
     2293
     2294        /* (Re-)create the stream's internal ring buffer. */
     2295        if (pStream->State.pCircBuf)
    22422296        {
    2243             /* Make sure to close + delete a former debug file, as the PCM format has changed (e.g. U8 -> S16). */
    2244             if (AudioHlpFileIsOpen(pStream->Dbg.Runtime.pFileDMA))
     2297            RTCircBufDestroy(pStream->State.pCircBuf);
     2298            pStream->State.pCircBuf = NULL;
     2299        }
     2300
     2301        rc = RTCircBufCreate(&pStream->State.pCircBuf, cbCircBuf);
     2302        AssertRCReturn(rc, rc);
     2303        pStream->State.StatDmaBufSize = (uint32_t)RTCircBufSize(pStream->State.pCircBuf);
     2304
     2305        /* Set scheduling hint. */
     2306        pStream->Cfg.Device.cMsSchedulingHint = RT_MS_1SEC / RT_MIN(pStream->uTimerHz, 1);
     2307
     2308        PAUDMIXSINK pMixerSink = sb16StreamIndexToSink(pThis, pStream->uIdx);
     2309        AssertPtrReturn(pMixerSink, VERR_INVALID_POINTER);
     2310
     2311        sb16RemoveDrvStreams(pDevIns, pThis,
     2312                             sb16StreamIndexToSink(pThis, pStream->uIdx), pStream->Cfg.enmDir, pStream->Cfg.enmPath);
     2313
     2314        rc = sb16AddDrvStreams(pDevIns, pThis, pMixerSink, &pStream->Cfg);
     2315        if (RT_SUCCESS(rc))
     2316        {
     2317            if (RT_LIKELY(!pStream->Dbg.Runtime.fEnabled))
     2318            { /* likely */ }
     2319            else
    22452320            {
    2246                 AudioHlpFileClose(pStream->Dbg.Runtime.pFileDMA);
    2247                 AudioHlpFileDelete(pStream->Dbg.Runtime.pFileDMA);
     2321                /* Make sure to close + delete a former debug file, as the PCM format has changed (e.g. U8 -> S16). */
     2322                if (AudioHlpFileIsOpen(pStream->Dbg.Runtime.pFileDMA))
     2323                {
     2324                    AudioHlpFileClose(pStream->Dbg.Runtime.pFileDMA);
     2325                    AudioHlpFileDelete(pStream->Dbg.Runtime.pFileDMA);
     2326                }
     2327
     2328                int rc2 = AudioHlpFileOpen(pStream->Dbg.Runtime.pFileDMA, AUDIOHLPFILE_DEFAULT_OPEN_FLAGS,
     2329                                           &pStream->Cfg.Props);
     2330                AssertRC(rc2);
    22482331            }
    2249 
    2250             int rc2 = AudioHlpFileOpen(pStream->Dbg.Runtime.pFileDMA, AUDIOHLPFILE_DEFAULT_OPEN_FLAGS,
    2251                                        &pStream->Cfg.Props);
    2252             AssertRC(rc2);
    22532332        }
    22542333    }
    … …  
    23032382 * @param   pStream         The SB16 stream.
    23042383 * @param   pSink           The mixer sink to push to.
     2384 *
     2385 * @thread  MixAIO-n
     2386 *
     2387 * @note    Takes the stream's lock.
    23052388 */
    23062389static void sb16StreamPushToMixer(PSB16STREAM pStream, PAUDMIXSINK pSink)
    23072390{
     2391    sb16StreamLock(pStream);
     2392
    23082393#ifdef LOG_ENABLED
    23092394    uint64_t const offReadOld = pStream->State.offRead;
     2395    Log3Func(("[SD%RU8] cbCircBufUsed=%#RX64\n", pStream->uIdx, RTCircBufUsed(pStream->State.pCircBuf)));
    23102396#endif
    23112397    pStream->State.offRead = AudioMixerSinkTransferFromCircBuf(pSink,
    … …  
    23162402                                                               ? pStream->Dbg.Runtime.pFileStream :*/ NULL);
    23172403
    2318     Log3Func(("[SD%RU8] transferred=%#RX64 bytes -> @%#RX64\n", pStream->uIdx,
    2319               pStream->State.offRead - offReadOld, pStream->State.offRead));
     2404    Log3Func(("[SD%RU8] transferred=%#RX64 bytes -> @%#RX64 (cbCircBufUsed=%#RX64)\n", pStream->uIdx,
     2405              pStream->State.offRead - offReadOld, pStream->State.offRead, RTCircBufUsed(pStream->State.pCircBuf)));
    23202406
    23212407    /* Update buffer stats. */
    23222408    pStream->State.StatDmaBufUsed = (uint32_t)RTCircBufUsed(pStream->State.pCircBuf);
     2409
     2410    sb16StreamUnlock(pStream);
    23232411}
    23242412
    … …  
    23282416 *
    23292417 * For output streams this moves data from the internal DMA buffer (in which
    2330  * ichac97R3StreamUpdateDma put it), thru the mixer and to the various backend
     2418 * sb16StreamDoDmaOutput put it), thru the mixer and to the various backend
    23312419 * audio devices.
     2420 *
     2421 * @thread  MixAIO-n
    23322422 */
    23332423static DECLCALLBACK(void) sb16StreamUpdateAsyncIoJob(PPDMDEVINS pDevIns, PAUDMIXSINK pSink, void *pvUser)
Note: See TracChangeset for help on using the changeset viewer.

© 2024 Oracle Support Privacy / Do Not Sell My Info Terms of Use Trademark Policy Automated Access Etiquette