pschatzmann / arduino-libhelix

A simple MP3 and AAC Decoder (not only) for Arduino based on libhelix
GNU General Public License v3.0
64 stars 21 forks source link

I might have found a bug in CommonHelix.h #16

Closed DanielLCopeland closed 2 weeks ago

DanielLCopeland commented 2 weeks ago

Hello!

My setup: I have an ESP32-S3 with two tasks, with one task getting audio data from various sources, and the other task writing to an i2s DAC. The two tasks communicate audio data through a ring buffer. This works fine with 99% of URL streams and MP3 files, but very occasionally, I will get a glitch and my second decoder writer task will fail to reset the watchdog and cause a reset.

Please excuse me I have only been learning C++ for the past 2 years. but I think I have traced the issue down to the following function:

 bool removeInvalidData(int pos) {
    if (pos > 0) {
      LOG_HELIX(LogLevelHelix::Info, "removing: %d bytes", pos);
      frame_buffer.clearArray(pos);
      return true;
    } else if (pos < 0) {
      frame_buffer.reset();
      return false;
    }
    return true;
  }

I noticed that "pos" never gets tested for when it's actually zero, and so I modified the function like so:

bool removeInvalidData(int pos) {
    if (pos > 0) {
      LOG_HELIX(LogLevelHelix::Info, "removing: %d bytes", pos);
      frame_buffer.clearArray(pos);
      return true;
    } else if (pos <= 0) {  /* Changed here */
      frame_buffer.reset();
      return false;
    }
    return true;
  }

Changing the "pos < 0" to "pos <= 0" fixed the issue and I had no more lockups. Maybe this is something you're aware of already, I don't know. I'm not familiar enough with your library, maybe there's a reason it's like that? I also don't know if this "fix" breaks anything else, I just know it's working for me.

Thank you for your time and thank you for the Arduino Audio Tools library, I think it's a very good library.

pschatzmann commented 2 weeks ago

Thanks for your feedback: I committed your correction