talker93 / 2022-MUSI6106

Template project for assignments and exercises for the class MUSI6106
GNU General Public License v3.0
0 stars 0 forks source link

A3: these are the issues with the version that you submitted as assignment solution #38

Open alexanderlerch opened 2 years ago

alexanderlerch commented 2 years ago

55: always check function arguments in user facing functions. 60: better reset your instance to ensure it works for multiple calls 69: if I understand correctly, your ringbuffer has to have a length of IR Length 73: never just copy the pointer, that leads to a huge mess (who frees the memory, can it be overwritten or is it reused somewhere, will it be needed after freed by someone else, etc.) 75,82: avoid implicit type casts 126: this doesn't work with a hann window... 179: length of pfMul1/2 can be 1! 188: what's with all these fft length multiplications? only one for the IR is necessary 271: I don't follow the code, but you ignore the needed latency 335/344: checkData must return value

couldn't make it work for impulse responses longer than one block

talker93 commented 2 years ago

55, 60, 179, 335/344 solved in latest version.

talker93 commented 2 years ago

271: A necessary latency is added at two positions:

int iUsed = 0;
// when the boundry met
while(iLengthOfBuffers+m_pCRingBuffIn -> getNumValuesInBuffer() - iUsed >= m_iBlockLength)
{
    m_bBoundryIsMet = true;

    // 1.Before calculation
    // 1.1. split current frame and fill up the input ring buffer
    int value2fill = m_iBlockLength - m_pCRingBuffIn -> getNumValuesInBuffer();
    m_pCRingBuffIn->putPostInc(&pfInputBuffer[iUsed], value2fill);
    // 1.2. write out the rest output ring buffer
    m_pCRingBuffOut->getPostInc(&pfOutputBuffer[iUsed], value2fill);
    // 1.3 add the index for audio IO
    iUsed += value2fill;

    // 2. FFT and multiplication
    // 2.1 overlap blocks for one layer
    m_pCRingBuffIn -> getPostInc(m_pfInputBuffer, m_iBlockLength);
    CVectorFloat::setZero(m_pfLayerBuffer, (m_iDivNums+1)*m_iBlockLength);
    for(int i = 0; i < m_iDivNums; i++)
    {
        fftMul(m_pfBlockBuffer, m_pfInputBuffer, i, m_iBlockLength);
        CVectorFloat::add_I(&m_pfLayerBuffer[i*m_iBlockLength], m_pfBlockBuffer, m_iFftLength);
    }
    // 2.2 overlap layers
    CVectorFloat::add_I(m_pfLayerBuffer, &m_pfOutputBuffer[m_iBlockLength], m_iDivNums*m_iBlockLength);
    // 2.3 send to buffer
    CVectorFloat::copy(m_pfOutputBuffer, m_pfLayerBuffer, m_iBlockLength*(m_iDivNums+1));
    // 2.4 send to ring buffer
    m_pCRingBuffOut -> putPostInc(m_pfOutputBuffer, m_iBlockLength);

    // 3. After calculation
    // 3.1 copy the rest part of input into ring buffer
    m_pCRingBuffIn->putPostInc(&pfInputBuffer[iUsed], (iLengthOfBuffers-value2fill)%m_iBlockLength);
    // 3.2 write out the initial values from each FFT results
    m_pCRingBuffOut -> getPostInc(&pfOutputBuffer[iUsed], (iLengthOfBuffers-value2fill)%m_iBlockLength);
    // 3.3 add the index for audio IO
    iUsed += (iLengthOfBuffers-value2fill)%m_iBlockLength;
}

// bypass these function when boundries have been met
if(!m_bBoundryIsMet)
{
    // copy value from input into ring buffer
    m_pCRingBuffIn->putPostInc(pfInputBuffer, iLengthOfBuffers);
    // copy value from ring buffer into output
    m_pCRingBuffOut -> getPostInc(pfOutputBuffer, iLengthOfBuffers);
}

m_bBoundryIsMet = false;

The first time latency addition is line 302, where block boundry hasn't been met, the while loop isn't functional, therefore m_pCRingBuffOut keeps output 0. The second time latency addition is line 267, where the boundry has been met, but the the FFT calculation hasn't been executed, and the m_pCRingBuffOut keeps dumping its rest 0.

talker93 commented 2 years ago

188: Maybe I didn't understand the fast convolution method correctly, I implemented FFT multiplication for each x[n] and h[n] instead of using convolution. But the result is correct and its time consumption is 1000 times shorter than direct convolution, which should be an acceptable result.

talker93 commented 2 years ago

126: Compared with Hamming and Hann, the results should be the same.

image
talker93 commented 2 years ago

69: both m_pCRingBuffIn and m_pCRingBuffOut don't have to have IR length, I use them for storing input and output cache, and these cache will be refreshed everytime when the input volume meets the bountry of m_iBlockLength.

alexanderlerch commented 2 years ago

126: Compared with Hamming and Hann, the results should be the same. image

Sorry, partitioned convolution as we introduced it only works with rectangular windows.

talker93 commented 2 years ago

126: Compared with Hamming and Hann, the results should be the same. image

Sorry, partitioned convolution as we introduced it only works with rectangular windows.

Sorry about my frenquent commit recently, you might were checking my old version, I updated the pre window to a kNoWindow since last Thursday. Also, I still don't fully understand the post window and its influence, can this make difference to my result?

alexanderlerch commented 2 years ago

As I said in the title, I did only grade the submitted version. You got the bonus points for the updated one.

talker93 commented 2 years ago

Understand, thanks for the bonus points. I'm posting my update here just for letting you know that I'm still updating my program. And you might want to answer some questions or provide suggestions if you have time.

alexanderlerch commented 2 years ago

I am happy to answer questions, but you have to ask specifics. I cannot answer questions vaguely asking if something is wrong.

talker93 commented 2 years ago

One question, what does the post window do in the FFT class? can it make difference for my result?

alexanderlerch commented 2 years ago

The post window is for the optional application of a window function after the IFFT. You do not want a post window function for convolution.

talker93 commented 2 years ago

Oh, I got it. The 3rd parameter doesn't work when the 4th set to kNoWindow.