espressif / esp-dsp

DSP library for ESP-IDF
Apache License 2.0
465 stars 87 forks source link

Error in FIR benchmark tests (DSP-74) #17

Closed brianbienvenu closed 1 year ago

brianbienvenu commented 3 years ago

Should this line reference fir2 instead of fir1? https://github.com/espressif/esp-dsp/blob/1009c9d548e99fc85e97ea5de56eb2dbd8da964b/test/test_dsp.c#L110

Compiled as in the repo, I get these numbers from the overall benchmark (ie, test '2'):

| **FIR Filters**                                          |          |          |
+----------------------------------------------------------+----------+----------+
| dsps_fir_f32 1024 input samples and 256 coefficients     |  1339099 |  5147987 |
+----------------------------------------------------------+----------+----------+
| dsps_fird_f32 1024 samples, 256 coeffs and decimation 4  |    19495 |    36495 |

In the Espressif docs, I see the decimating FIR numbers are about 2x higher: https://docs.espressif.com/projects/esp-dsp/en/latest/esp-dsp-benchmarks.html

I think that fir1.decim is uninitialised, so the results aren't stable. I've gotten different results when recompiling, without changing anything significant.

Changing that reference to fir2 gives me:

| **FIR Filters**                                          |          |          |
+----------------------------------------------------------+----------+----------+
| dsps_fir_f32 1024 input samples and 256 coefficients     |  1339102 |  5147994 |
+----------------------------------------------------------+----------+----------+
| dsps_fird_f32 1024 samples, 256 coeffs and decimation 4  |   350247 |  1317839 |

These numbers are now 9x and 16x higher than in the Espressif docs, but are about what I would expect - decimating by 4 requires about 4x less CPU time.

dmitry1945 commented 3 years ago

I will look today, but it looks like you are right.

Thanks, Dmitry

dmitry1945 commented 1 year ago

Hi @brianbienvenu the issue is fixed. Thank you very much! I will close the issue.

Thank you and Best Regards, Dmitry