Delay pan #1151
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -233,6 +233,7 @@ CServer::CServer ( const int iNewMaxNumChan, | |
| const bool bNUseDoubleSystemFrameSize, | ||
| const bool bNUseMultithreading, | ||
| const bool bDisableRecording, | ||
| const bool bNDelayPan, | ||
| const ELicenceType eNLicenceType ) : | ||
| bUseDoubleSystemFrameSize ( bNUseDoubleSystemFrameSize ), | ||
| bUseMultithreading ( bNUseMultithreading ), | ||
|
|
@@ -252,6 +253,7 @@ CServer::CServer ( const int iNewMaxNumChan, | |
| &ConnLessProtocol ), | ||
| bDisableRecording ( bDisableRecording ), | ||
| bAutoRunMinimized ( false ), | ||
| bDelayPan ( bNDelayPan ), | ||
| eLicenceType ( eNLicenceType ), | ||
| bDisconnectAllClientsOnQuit ( bNDisconnectAllClientsOnQuit ), | ||
| pSignalHandler ( CSignalHandler::getSingletonP() ) | ||
|
|
@@ -330,7 +332,6 @@ CServer::CServer ( const int iNewMaxNumChan, | |
| iServerFrameSizeSamples = SYSTEM_FRAME_SIZE_SAMPLES; | ||
| } | ||
|
|
||
| // To avoid audio clitches, in the entire realtime timer audio processing | ||
| // routine including the ProcessData no memory must be allocated. Since we | ||
| // do not know the required sizes for the vectors, we allocate memory for | ||
|
|
@@ -341,6 +342,7 @@ CServer::CServer ( const int iNewMaxNumChan, | |
| vecvecfGains.Init ( iMaxNumChannels ); | ||
| vecvecfPannings.Init ( iMaxNumChannels ); | ||
| vecvecsData.Init ( iMaxNumChannels ); | ||
| vecvecsData2.Init ( iMaxNumChannels ); | ||
| vecvecsSendData.Init ( iMaxNumChannels ); | ||
| vecvecfIntermediateProcBuf.Init ( iMaxNumChannels ); | ||
| vecvecbyCodedData.Init ( iMaxNumChannels ); | ||
|
|
@@ -357,6 +359,7 @@ CServer::CServer ( const int iNewMaxNumChan, | |
|
|
||
| // we always use stereo audio buffers (which is the worst case) | ||
| vecvecsData[i].Init ( 2 /* stereo */ * DOUBLE_SYSTEM_FRAME_SIZE_SAMPLES /* worst case buffer size */ ); | ||
| vecvecsData2[i].Init ( 2 /* stereo */ * DOUBLE_SYSTEM_FRAME_SIZE_SAMPLES /* worst case buffer size */ ); | ||
|
Member
There was a problem hiding this comment. Is this also needed if delayed panning is off? If not, do we need to initialize it (does it have any performance impact)? (also applies to line 345). There was a problem hiding this comment. Detlef Hennings is working on being able to operate the delay panning in different modes. This is about the distribution of the singers in the virtual space. Therefore it would be good to keep the parameter '-P panmode' free for this and similar developments and to use for delay panning only --DelayPan (or similar) for delay panning. Furthermore, delay panning is so important for singers that it should be possible to activate it via a checkbox.
Member
There was a problem hiding this comment.
I think this is already part of this PR: https://github.com/jamulussoftware/jamulus/blob/0c6b3fbfc261b1b078bbd43ed939107deb07132e/src/serverdlgbase.ui
Contributor
Author
There was a problem hiding this comment.
Right now this is a server setting. I'd also prefer this to be a client setting but it isn't yet. If it is we wouldn't need the command line option.
Member
There was a problem hiding this comment. This would probably need a new protocol message. There was a problem hiding this comment. Thought about it once more: it is a good idea after all to make this a client setting. Thanks for asking!
Collaborator
There was a problem hiding this comment. There would need to be:
I also think maybe get the server support finished and released "as is" before adding the client side. I definitely think it's worth having, though. There was a problem hiding this comment. As far as our 9 month experience with vocal ensembles and choirs goes, once the participants have heard the impression of delay panning, they would never want to go back to volume panning. And with acoustic instrumentalists it seems to be similar. I suppose that the preference will mainly depend on the music style. For instance, choir and classical music servers could preferably use delay panning and rock music servers might use volume panning (well, I assume that). Then, at least for a first implementation, the pan type decision can be made on the server side. And public servers may announce which panning type they provide.
Contributor
Author
There was a problem hiding this comment. I‘d also like to get this done. The sooner the better. People would benefit immediately. The client part can come later and this should go out right away (3.7.0). But it is not on me to decide. I too can’t imagine anyone wanting to go back to volume panning. I‘d been entertaining the idea to provide a client setting in a later PR. The grounds are laid and there isn’t much to add to the server logic except the protocol bit. Mixing is per client already so we only need to make
Member
There was a problem hiding this comment. The problem is, because we want to be certain about the quality and fully understand operation of the code that goes into a release, every additional feature causes further delay in the release of 3.7.0, which already is later than we would have liked. That is not to express any doubt about the code quality or its usefulness, it just takes time. It will definitely be fully reviewed for inclusion in 3.7.1, and hopefully that release will not be too far into the future. |
||
|
|
||
| // (note that we only allocate iMaxNumChannels buffers for the send | ||
| // and coded data because of the OMP implementation) | ||
|
|
@@ -934,6 +937,16 @@ static CTimingMeas JitterMeas ( 1000, "test2.dat" ); JitterMeas.Measure(); // TE | |
| FutureSynchronizer.waitForFinished(); | ||
| FutureSynchronizer.clearFutures(); | ||
| } | ||
| if ( bDelayPan ) | ||
| { | ||
| for ( int i = 0; i < iNumClients; i++ ) | ||
| { | ||
| for ( int j = 0; j < 2 * (iServerFrameSizeSamples); j++ ) | ||
| { | ||
| vecvecsData2[i][j] = vecvecsData[i][j]; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
|
|
@@ -1196,60 +1209,112 @@ void CServer::MixEncodeTransmitData ( const int iChanCnt, | |
| else | ||
| { | ||
| // Stereo target channel ----------------------------------------------- | ||
|
|
||
| const int maxPanDelay = MAX_DELAY_PANNING_SAMPLES; | ||
|
|
||
| int iLpan, iRpan, iPan; | ||
|
|
||
| for ( j = 0; j < iNumClients; j++ ) | ||
| { | ||
| // get a reference to the audio data and gain/pan of the current client | ||
| const CVector<int16_t>& vecsData = vecvecsData[j]; | ||
| const CVector<int16_t>& vecsData2 = vecvecsData2[j]; | ||
|
|
||
| const float fGain = vecvecfGains[iChanCnt][j]; | ||
| const float fPan = bDelayPan ? 0.5f : vecvecfPannings[iChanCnt][j]; | ||
|
pljones marked this conversation as resolved.
|
||
| const int iPanDel = lround( (float)( 2 * maxPanDelay - 2 ) * ( vecvecfPannings[iChanCnt][j] - 0.5f ) ); | ||
| const int iPanDelL = ( iPanDel > 0 ) ? iPanDel : 0; | ||
| const int iPanDelR = ( iPanDel < 0 ) ? -iPanDel : 0; | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. We now have with
Same here,
|
||
| // calculate combined gain/pan for each stereo channel where we define | ||
| // the panning that center equals full gain for both channels | ||
| const float fGainL = MathUtils::GetLeftPan ( fPan, false ) * fGain; | ||
| const float fGainR = MathUtils::GetRightPan ( fPan, false ) * fGain; | ||
|
|
||
|
Comment on lines
-1211
to
-1212
Member
There was a problem hiding this comment. This code seems to get lost and was annotated to be a speed optimization. I can't tell how important it was. Saving some multiplications sounds really minor (if the cost is two additional equality checks), but as this is part of the performance-critical code path we may want to double-check. Edit: Just noticed that this seems to have been done deliberately to simplify the logic? |
||
| if ( vecNumAudioChannels[j] == 1 ) | ||
| { | ||
| // mono: copy same mono data in both out stereo audio channels | ||
| for ( i = 0, k = 0; i < iServerFrameSizeSamples; i++, k += 2 ) | ||
| { | ||
| // left/right channel | ||
| if ( bDelayPan ) | ||
| { | ||
| // pan address shift | ||
|
|
||
| // left channel | ||
| iLpan = i - iPanDelL; | ||
| if ( iLpan < 0 ) | ||
| { | ||
| // get from second | ||
| iLpan = iLpan + iServerFrameSizeSamples; | ||
| vecfIntermProcBuf[k] += vecsData2[iLpan] * fGainL; | ||
| } | ||
| else | ||
| { | ||
| vecfIntermProcBuf[k] += vecsData[iLpan] * fGainL; | ||
| } | ||
|
|
||
| // right channel | ||
| iRpan = i - iPanDelR; | ||
| if ( iRpan < 0 ) | ||
| { | ||
| // get from second | ||
| iRpan = iRpan + iServerFrameSizeSamples; | ||
| vecfIntermProcBuf[k + 1] += vecsData2[iRpan] * fGainR; | ||
| } | ||
| else | ||
| { | ||
| vecfIntermProcBuf[k + 1] += vecsData[iRpan] * fGainR; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| vecfIntermProcBuf[k] += vecsData[i] * fGainL; | ||
| vecfIntermProcBuf[k + 1] += vecsData[i] * fGainR; | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // stereo | ||
| for ( i = 0; i < ( 2 * iServerFrameSizeSamples ); i++ ) | ||
| { | ||
| // left/right channel | ||
| if ( bDelayPan ) | ||
| { | ||
| // pan address shift | ||
| if ( (i & 1) == 0 ) | ||
| { | ||
| iPan = i - 2 * iPanDelL; // if even : left channel | ||
| } | ||
| else | ||
| { | ||
| iPan = i - 2 * iPanDelR; // if odd : right channel | ||
| } | ||
| // interleaved channels | ||
| if ( iPan < 0 ) | ||
| { | ||
| // get from second | ||
| iPan = iPan + 2 * iServerFrameSizeSamples; | ||
| vecfIntermProcBuf[i] += vecsData2[iPan] * fGain; | ||
| } | ||
| else | ||
| { | ||
| vecfIntermProcBuf[i] += vecsData[iPan] * fGain; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| if ( (i & 1) == 0 ) | ||
| { | ||
| // if even : left channel | ||
| vecfIntermProcBuf[i] += vecsData[i] * fGainL; | ||
| } | ||
| else | ||
| { | ||
| // if odd : right channel | ||
| vecfIntermProcBuf[i] += vecsData[i] * fGainR; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need two cli flags? Wouldn't one be enough?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes one would be enough. I realized this too after I made the PR. Two options would be useful should we change the default in a later release. We’d still need to decide on the default. I’d like on as default in which case you need the
—nodelaypanoption. With off as default you want the-P/—delayedpanoption.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If switching it on means more processing power, I would not enable it by default (there is and was a lot going on with optimizing server performance and adding something which needs more power is counter productive). You can always switch it on of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember that Volker mentioned that this delay pan might not be a better experience for everybody, for example bands. So I also wouldn't enable it per default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe someone with a band could try it?