ValveSoftware / steam-audio

Steam Audio
https://valvesoftware.github.io/steam-audio/
Apache License 2.0
2.2k stars 152 forks source link

iplSimulatorRunReflections will crash, if any IPLReflectionEffect channel count differ from Simulator channel count #324

Closed Oldnice closed 3 months ago

Oldnice commented 3 months ago

System Information

Issue Description

If IPLReflectionEffect is created with differing channel count from simulator channel count, reflection simulation will eventually crash (possibly due to reflection IR buffer being smaller than the simulator internal IR buffer). I use the term "IR buffer", but this is based on the source variable names. I've actually quite a limited notion of what I'm looking at in the core sources at this point, or what the correct terminology is

I'm not sure if this scenario is supposed to be supported, or if the exception should just be caught more gracefully. I'm also not yet well-versed enough with the core sources to suggest an elegant solution for the problem, hence the lack of a pull request. I'm open to developing a solution, if any engineers with in-depth knowledge of the core have any suggestions.

Steps to reproduce

1. Reflection effect setup

Reflection effect channel count is set with the following code:

IPLReflectionEffectSettings effectSettings = IPLReflectionEffectSettings();
effectSettings.numChannels = 1;

which causes following initialization in phonon:

OverlapSaveConvolutionEffect::OverlapSaveConvolutionEffect(...)
{
  ...
  //Channel count = 1
  mPrevFFTIR = ipl::make_unique<OverlapSaveFIR>(mNumChannels, mIRSize, mFrameSize);
  ...
}

2. Simulator setup

Simulator is assigned maxOrder:

IPLSimulationSettings simulationSettings = IPLSimulationSettings();
simulationSettings.maxOrder = 1;

which causes following initialization in phonon:

SimulationData::SimulationData(...)
{
  ...
  //Channel count = 4
  auto numChannels = SphericalHarmonics::numCoeffsForOrder(maxOrder);
  ...
  //Buffer creation
  reflectionOutputs.overlapSaveFIR.initBuffers(numChannels, irSize, frameSize);
  ...
}

3. Run reflections, getting output & reflection effect apply (buffer swapping)

calling iplSourceGetOutputs(...), iplReflectionEffectApply(...) and iplReflectionEffectApply(...) a few times will apparently cause the reflection effect IR buffer to be swapped within the source IR buffer, which I'm assuming happens in the following function (called via iplReflectionEffectApply):

bool OverlapSaveConvolutionEffect::apply(...)
{
  ...
  if (crossfade)
  {
  ...
    //Swap buffers here
    mPrevFFTIR.swap(params.fftIR->readBuffer);
  }
  ...
}

The buffer is (I think) eventually swapped from IPLReflectionEffect -> source IR buffer (read) -> source IR buffer (write)

4. Reflection simulation (crash)

Calling iplSimulatorRunReflections(...) at this point will crash (if IPLReflectionEffect buffer is already swapped to source write buffer):

void SimulationManager::simulateIndirect()
{
  ...
  else if (mIndirectType != IndirectEffectType::Parametric)
  {
    //Source IR writeBuffer is used here:
    //Impulse response buffer is 4 channels, but source IR buffer is only 1!
    mPartitioner->partition(*impulseResponse, numChannels, numSamples, *source->reflectionOutputs.overlapSaveFIR.writeBuffer);
    ...
  }
  ...
}

void OverlapSavePartitioner::partition(...)
{
  for (auto i = 0; i < numChannels; ++i)
  {
    ...
    for (auto j = 0; j < fftIR.numBlocks(); ++j)
    {
      ...
      //Crash (on i==1), due to source IR buffer not containing enough channels
      memcpy(mTempIRBlock.data(), &ir[i][j * mFrameSize], numSamplesToCopy * sizeof(float));
      mFFT.applyForward(mTempIRBlock.data(), fftIR[i][j]);
    }
  }
}

crash