chmorgan / esp-audio-player

Apache License 2.0
14 stars 5 forks source link

Infinite Loop Issue on Decoding Error in audio_mp3.cpp #5

Closed alibukharai closed 2 months ago

alibukharai commented 1 year ago

Description: I have been working with the audio component in the project and have encountered an issue when an error occurs during the decoding of an MP3 file. Specifically, when any error happens during decoding, the program gets stuck in a continuous loop indefinitely. This behavior occurs in the decode_mp3 function, as referenced in this line: audio_mp3.cpp#L139.

The crossponding error logs are attached below. 1

Proposed solution: To address this issue, I propose adding an event to the audio_player module. This event will allow proper handling of decoding errors and prevent the program from getting stuck in the loop. I have identified the relevant location to add the event in the audio_player.h file, as referenced in this line: audio_player.h#L75.

typedef enum { // ... AUDIO_PLAYER_REQUEST_ERROR, // New event type for handling decoding errors // ... } audio_player_event_type_t;

chmorgan commented 1 year ago

@alibukharai how are you able to reproduce this failure condition? Do you have a file that exhibits this behavior? I believe I had some files with some bad data in them and continuing to read got to a point where they worked, and should burn through the data in the file really quickly too, but I can see how several megabytes of bad data could take a long time to read and error through...

alibukharai commented 1 year ago

@alibukharai how are you able to reproduce this failure condition? Do you have a file that exhibits this behavior? I believe I had some files with some bad data in them and continuing to read got to a point where they worked, and should burn through the data in the file really quickly too, but I can see how several megabytes of bad data could take a long time to read and error through...

@chmorgan Thank you for your response. The MP3 data is received through an HTTPS Text-to-Speech API. I store this data in a buffer and then pass both the buffer and its length to the audio_player.cpp API to play the MP3. Below is the code snippet that handles the MP3 playback after the HTTPS finish event:

#define MAX_FILE_SIZE       (1*1024*1024)
    case HTTP_EVENT_ON_HEADER:
        ESP_LOGI(TAG, "HTTP_EVENT_ON_HEADER");
        file_total_len = 0;
        break;
    case HTTP_EVENT_ON_DATA:
        ESP_LOGI(TAG, "HTTP_EVENT_ON_DATA, len=%d", evt->data_len);
        if ((file_total_len + evt->data_len) < MAX_FILE_SIZE) {
            memcpy(audio_rx_buffer + file_total_len, (char *)evt->data, evt->data_len);
            file_total_len += evt->data_len;
        }
        break;
    case HTTP_EVENT_ON_FINISH:
        ESP_LOGI(TAG, "HTTP_EVENT_ON_FINISH:%d, %d K", file_total_len, file_total_len / 1024);
        audio_player_play(audio_rx_buffer, file_total_len); // Link: [https://github.com/chmorgan/esp-audio-player/blob/2221e2c848c9f073f9d6ab4fc6a5cd9b46de20c7/audio_player.cpp#L471]
        break;

The challenge is that reproducing this error is not straightforward. However, if such an error occurs, is there any way to provide guidance on how to handle it? The logs indicate a status error of -8 in mp3dec.h.

chmorgan commented 1 year ago

@alibukharai well -8 looks like an internal data field/format error. Today we don't do a good job of indicating we are skipping due to errors, that could be something you could use to decide to abort playback entirely, vs. continuing for hundreds of ms to try to play the data file.

chmorgan commented 1 year ago

I'd be open to suggestions for api/error reporting changes but keep in mind they could break existing users.

chmorgan commented 8 months ago

@alibukharai can you retest with the latest changes on main? It looks like this issue may have been addressed.

chmorgan commented 2 months ago

Closing as I think the issue was resolved with some fixes that were contributed.