Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correlation function arm_correlate_f32 has wrong output array length expectation for NEON #120

Open
yuvpg opened this issue Sep 21, 2023 · 5 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request neon

Comments

@yuvpg
Copy link

yuvpg commented Sep 21, 2023

arm_correlate_f32() expecting the output array size to be 2*srcALen - 1 instead of srcALen + srcBLen - 1 for ARM_MATH_DSP case.

Maybe just changing line 361 will be enough to fix the issue.

Now the output is unexpected, and call leads to buffer overflow if you don't guess output array size correctly:

    const int kerN = 7;
    float ker[kerN] = {+1,+1,+1,-1,-1,+1,-1};
    const int dataN = 21;
    float data[dataN] = {0, 0,+1,+1,+1,-1,-1,+1,-1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};

    const int resN = 2*dataN-1; // why so long array needed?
    float res[resN];

    for (int i = 0; i < resN; i++) {
        res[i] = NAN;
    }

    arm_correlate_f32(data, dataN, ker, kerN, res);

results in

res={nan,nan,nan,nan,nan,nan,nan,nan,nan,nan,nan,nan,nan,nan,0,0,-1,0,-1,0,-1,0,7,0,-1,0,-1,0,-1,0,0,0,0,0,0,0,0,0,0,0,0};
@christophe0606 christophe0606 added the bug Something isn't working label Sep 22, 2023
@christophe0606
Copy link
Contributor

In the documentation I can read:

The pDst should be initialized to all zeros before being used.

and also in documentation:

size of buffer should 2 * max(srcALen, srcBLen) - 1

So I don't see any bug. It behaves as expected but I agree that it may be improved to use less memory.

I think the reason it is done like that is to:

  • Avoid tests in the code
  • Avoid having to pad the entries so that buffers have same lengths

@christophe0606 christophe0606 added enhancement New feature or request and removed bug Something isn't working labels Nov 27, 2023
@rohardy
Copy link

rohardy commented Mar 14, 2024

From the documentation :

The result c[n] is of length 2 * max(srcALen, srcBLen) - 1 and is defined over the interval n=0, 1, 2, ..., (2 * max(srcALen, srcBLen) - 2). The output result is written to pDst and the calling function must allocate 2 * max(srcALen, srcBLen) - 1 words for the result.

but should not the result c[n] be defined over the interval n=0, 1, 2, ..., srcALen + srcBLen - 2 ? Since a full correlation returns an array of size srcALen + srcBLen - 1 .

Edit

Depending of whether scrALen is greater than srcBLen (or not), c[n] is not defined on the same interval.

If srcALen < srcBLen then n=0, 1, 2, ..., srcALen + srcBLen - 2.
If srcALen > srcBLen then n=srcALen-srcBLen,..., 2*srcALen - 2

I found the documentation a bit misleading about the definition domain of c.

@christophe0606
Copy link
Contributor

@rohardy The output is padded with zeros. It is longer than what is strictly needed.

@rohardy
Copy link

rohardy commented Mar 14, 2024

@christophe0606 I understand, but I think the documentation could be more precise about where the actual result lays.

@christophe0606 christophe0606 added the documentation Improvements or additions to documentation label Mar 14, 2024
@christophe0606
Copy link
Contributor

@rohardy You're right. Documentation will be improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request neon
Projects
None yet
Development

No branches or pull requests

3 participants