From ac3bb20cbdf97dad627ff3c1c5b68eade64fa59f Mon Sep 17 00:00:00 2001 From: Etienne Dechamps Date: Sat, 25 Feb 2023 11:26:00 +0000 Subject: [PATCH 1/2] dsound: use paUtilBoundedHostBufferSize Currently the DirectSound host API uses the buffer processor in the "paUtilVariableHostBufferSizePartialUsageAllowed" buffer size mode. What's special about this mode is that it allows the buffer processor to leave input frames in the host buffer if there are not enough frames available to fire the stream callback. In practice the buffer processor will only do this in full duplex mode. However, just because we *can* leave these frames in the host buffer, doesn't mean we *should*. It is not clear to me why the DS code was set up to leave data in the host input buffer. The reason stated in a code comment, "because DS can split the host buffer when it wraps around" doesn't make sense to me - the buffer processor seems perfectly capable of handling buffer split and wraparound (through functions such as PaUtil_Set2ndInputFrameCount()) regardless of the buffer size mode. While it is not clear what could be the upside of leaving data in the host buffer, the downsides seem evident: the longer data stays in the input buffer, the more likely it is to get trampled on by the capture cursor (buffer overflow). Copying the data out of the host input buffer and into the buffer processor temp buffer at the earliest opportunity seems like a safer option with no downside. Another downside of this mode is that currently, the behavior with respect to the host *output* buffer seems... confused, with the buffer processor writing a number of frames to the output host buffer that is inconsistent with the number of "frames processed" it returns to the DS code, leading to #770. Arguably we could try to fix the buffer processor, but why fix a mode whose reason to exist is unclear in the first place? Another oddity of this mode is that it leads to the DS code locking the DS input and output buffers for read/write, but the buffer processor might then decide to do nothing with them. These DS Lock/Unlock calls are therefore wasted, reducing efficiency. This commit changes the mode to paUtilBoundedHostBufferSize. Not only does this fix #770, it also has the nice side effect of having the buffer processor provision a temp buffer to match the host buffer size (if paFramesPerBufferUnspecified is used), avoiding unnecessary user buffer splitting. This change was tested using paloopback to test for glitches with various framesPerBuffer and suggested latency parameters, both in half duplex and full duplex mode. No regressions were observed, and paloopback results show that #770 is fixed by this change. Fixes #770 --- src/hostapi/dsound/pa_win_ds.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/hostapi/dsound/pa_win_ds.c b/src/hostapi/dsound/pa_win_ds.c index b0ab18725..038ada4e6 100644 --- a/src/hostapi/dsound/pa_win_ds.c +++ b/src/hostapi/dsound/pa_win_ds.c @@ -2052,20 +2052,6 @@ static PaError OpenStream( struct PaUtilHostApiRepresentation *hostApi, hostOutputSampleFormat = 0; } - result = PaUtil_InitializeBufferProcessor( &stream->bufferProcessor, - inputChannelCount, inputSampleFormat, hostInputSampleFormat, - outputChannelCount, outputSampleFormat, hostOutputSampleFormat, - sampleRate, streamFlags, framesPerBuffer, - 0, /* ignored in paUtilVariableHostBufferSizePartialUsageAllowed mode. */ - /* This next mode is required because DS can split the host buffer when it wraps around. */ - paUtilVariableHostBufferSizePartialUsageAllowed, - streamCallback, userData ); - if( result != paNoError ) - goto error; - - bufferProcessorIsInitialized = 1; - - /* DirectSound specific initialization */ { HRESULT hr; @@ -2131,6 +2117,17 @@ static PaError OpenStream( struct PaUtilHostApiRepresentation *hostApi, DBUG(("DirectSound host buffer size frames: %d, polling period seconds: %f, @ sr: %f\n", stream->hostBufferSizeFrames, stream->pollingPeriodSeconds, sampleRate )); + result = PaUtil_InitializeBufferProcessor(&stream->bufferProcessor, + inputChannelCount, inputSampleFormat, hostInputSampleFormat, + outputChannelCount, outputSampleFormat, hostOutputSampleFormat, + sampleRate, streamFlags, framesPerBuffer, + stream->hostBufferSizeFrames, paUtilBoundedHostBufferSize, + streamCallback, userData); + if (result != paNoError) + goto error; + + bufferProcessorIsInitialized = 1; + /* ------------------ OUTPUT */ if( outputParameters ) From d7bc9c93d76aed3a11038b509a424fd730f1695d Mon Sep 17 00:00:00 2001 From: Etienne Dechamps Date: Sat, 25 Feb 2023 12:15:16 +0000 Subject: [PATCH 2/2] Remove paUtilVariableHostBufferSizePartialUsageAllowed DirectSound was the only host API using this mode, and its usage was removed in the previous commit, making this mode dead code. As discussed in that commit the value proposition of this mode is unclear, and it does not behave correctly around stream start (priming) as shown in #770. --- src/common/pa_process.c | 27 +++++---------------------- src/common/pa_process.h | 13 +------------ src/hostapi/asihpi/pa_linux_asihpi.c | 3 --- 3 files changed, 6 insertions(+), 37 deletions(-) diff --git a/src/common/pa_process.c b/src/common/pa_process.c index 29fca64fa..468693afb 100644 --- a/src/common/pa_process.c +++ b/src/common/pa_process.c @@ -1287,18 +1287,15 @@ static void CopyTempOutputBuffersToHostOutputBuffers( PaUtilBufferProcessor *bp) data from the temporary output buffer into the host output buffers, then from the host input buffers into the temporary input buffers. Calling the streamCallback when necessary. - When processPartialUserBuffers is 0, all available input data will be - consumed and all available output space will be filled. When - processPartialUserBuffers is non-zero, as many full user buffers - as possible will be processed, but partial buffers will not be consumed. + All available input data will be consumed and all available output space + will be filled. */ static unsigned long AdaptingProcess( PaUtilBufferProcessor *bp, - int *streamCallbackResult, int processPartialUserBuffers ) + int *streamCallbackResult ) { void *userInput, *userOutput; unsigned long framesProcessed = 0; unsigned long framesAvailable; - unsigned long endProcessingMinFrameCount; unsigned long maxFramesToCopy; PaUtilChannelDescriptor *hostInputChannels, *hostOutputChannels; unsigned int frameCount; @@ -1310,15 +1307,10 @@ static unsigned long AdaptingProcess( PaUtilBufferProcessor *bp, framesAvailable = bp->hostInputFrameCount[0] + bp->hostInputFrameCount[1];/* this is assumed to be the same as the output buffer's frame count */ - if( processPartialUserBuffers ) - endProcessingMinFrameCount = 0; - else - endProcessingMinFrameCount = (bp->framesPerUserBuffer - 1); - /* Fill host output with remaining frames in user output (tempOutputBuffer) */ CopyTempOutputBuffersToHostOutputBuffers( bp ); - while( framesAvailable > endProcessingMinFrameCount ) + while( framesAvailable >= bp->framesPerUserBuffer ) { if( bp->framesInTempOutputBuffer == 0 && *streamCallbackResult != paContinue ) @@ -1612,16 +1604,7 @@ unsigned long PaUtil_EndBufferProcessing( PaUtilBufferProcessor* bp, int *stream { /* full duplex */ - if( bp->hostBufferSizeMode == paUtilVariableHostBufferSizePartialUsageAllowed ) - { - framesProcessed = AdaptingProcess( bp, streamCallbackResult, - 0 /* dont process partial user buffers */ ); - } - else - { - framesProcessed = AdaptingProcess( bp, streamCallbackResult, - 1 /* process partial user buffers */ ); - } + framesProcessed = AdaptingProcess( bp, streamCallbackResult ); } else if( bp->inputChannelCount != 0 ) { diff --git a/src/common/pa_process.h b/src/common/pa_process.h index 444bdf545..d87c45ab8 100644 --- a/src/common/pa_process.h +++ b/src/common/pa_process.h @@ -225,15 +225,6 @@ typedef enum { /** Nothing is known about the host buffer size. */ paUtilUnknownHostBufferSize, - -/** The host buffer size varies, and the client does not require the buffer - processor to consume all of the input and fill all of the output buffer. This - is useful when the implementation has access to the host API's circular buffer - and only needs to consume/fill some of it, not necessarily all of it, with each - call to the buffer processor. This is the only mode where - PaUtil_EndBufferProcessing() may not consume the whole buffer. -*/ - paUtilVariableHostBufferSizePartialUsageAllowed }PaUtilHostBufferSizeMode; @@ -652,9 +643,7 @@ void PaUtil_BeginBufferProcessing( PaUtilBufferProcessor* bufferProcessor, wasn't generated by the terminating callback. @return The number of frames processed. This usually corresponds to the - number of frames specified by the PaUtil_Set*FrameCount functions, except in - the paUtilVariableHostBufferSizePartialUsageAllowed buffer size mode when a - smaller value may be returned. + number of frames specified by the PaUtil_Set*FrameCount functions. */ unsigned long PaUtil_EndBufferProcessing( PaUtilBufferProcessor* bufferProcessor, int *callbackResult ); diff --git a/src/hostapi/asihpi/pa_linux_asihpi.c b/src/hostapi/asihpi/pa_linux_asihpi.c index db8efcc21..439f75a43 100644 --- a/src/hostapi/asihpi/pa_linux_asihpi.c +++ b/src/hostapi/asihpi/pa_linux_asihpi.c @@ -1322,9 +1322,6 @@ static void PaAsiHpi_StreamComponentDump( PaAsiHpiStreamComponent *streamComp, case paUtilUnknownHostBufferSize: PA_DEBUG(( "[unknown] " )); break; - case paUtilVariableHostBufferSizePartialUsageAllowed: - PA_DEBUG(( "[variable] " )); - break; } PA_DEBUG(( "(%d max)\n", streamComp->tempBufferSize / streamComp->bytesPerFrame )); /* HPI hardware settings */