Latest memory leak fix breaks program / memory leaks in audio code

bepaald commented 2 years ago

I believe commit 851e495aeb31cf1bd3e2d530af24811b84eeedc0 breaks the program.

Note that MjpegClass::_mjpeg_buf is a (shallow) copy of the mjpeg_buf pointer allocated in playVideo(). In the new code:

1     if (mjpeg_buf)
2     {
3       mjpeg.cleanup();
4       free(mjpeg_buf);
5       mjpeg_buf = NULL;
6     }

In line 1, you make sure that the buffer is actually allocated before freeing it. Then, line 3 calls MjpegClass::cleanup(), which checks for, and free()'s _mjpeg_buf, which points to the same memory!. After this, mjpeg_buf is NULL, but is unconditionally free()'d anyway in line 4.

When I run this code, I get:

13:41:45.333 -> MP3 audio MJPEG video end
13:41:45.333 -> CORRUPT HEAP: Bad head at 0x3ffd2c5c. Expected 0xabba1234 got 0x3ffcb68c
13:41:45.333 -> 
13:41:45.366 -> assert failed: multi_heap_free multi_heap_poisoning.c:253 (head != NULL)
13:41:45.366 -> 
13:41:45.366 -> 
13:41:45.366 -> Backtrace:0x400836c1:0x3ffb25a00x4008c88d:0x3ffb25c0 0x40091b41:0x3ffb25e0 0x40091787:0x3ffb2710 0x40083b81:0x3ffb2730 0x40091b71:0x3ffb2750 0x400d2b3f:0x3ffb2770 0x400d2e46:0x3ffb27d0 0x400de3bd:0x3ffb2820 
13:41:45.366 -> 
13:41:45.366 -> 
13:41:45.366 -> 
13:41:45.366 -> 
13:41:45.366 -> ELF file SHA256: 0000000000000000
13:41:45.366 -> 
13:41:45.366 -> Rebooting...

Furthermore, the other memory free()'d by MjpegClass::cleanup(), does not look like it can leak. _read_buf is only allocated once, and then reused each time setup() is called again. I have personally not experienced any memory leaks in the code before this commit. I have the following lines at the start of the loop:

  Serial.print(F("Start of loop! Available memory: "));
  Serial.print(esp_get_free_heap_size());
  Serial.println(F(" bytes"));

And playing a couple of (short) files shows the following output:

13:57:37.523 -> Found 7 in root directory!
13:57:37.523 -> Starting playback!
13:57:37.523 -> Start of loop! Available memory: 203392 bytes
13:57:37.523 -> fileNo: 1
13:57:37.523 -> Loading video: /160_30fps_1.mjpeg
13:57:56.607 -> MJPEG video end
13:57:56.607 -> Start of loop! Available memory: 202304 bytes
13:57:56.607 -> fileNo: 2
13:57:56.607 -> Loading video: /athf.mjpeg
13:58:26.596 -> MJPEG video end
13:58:26.596 -> Start of loop! Available memory: 202304 bytes
13:58:26.596 -> fileNo: 3
13:58:26.596 -> Loading video: /simpsons.mjpeg
13:58:56.614 -> MJPEG video end
13:58:56.614 -> Start of loop! Available memory: 202300 bytes
13:58:56.614 -> fileNo: 4
13:58:56.614 -> Loading video: /dexter.mjpeg
13:59:26.598 -> MJPEG video end
13:59:26.631 -> Start of loop! Available memory: 202304 bytes
13:59:26.631 -> fileNo: 5
13:59:26.631 -> Loading video: /160_30fps_2.mjpeg
13:59:54.131 -> MJPEG video end
13:59:54.131 -> Start of loop! Available memory: 202300 bytes
13:59:54.131 -> fileNo: 6
13:59:54.131 -> Loading video: /160_30fps_3.mjpeg
14:00:32.703 -> MJPEG video end
14:00:32.703 -> Start of loop! Available memory: 202304 bytes
14:00:32.737 -> fileNo: 7
14:00:32.737 -> Loading video: /160_30fps_4.mjpeg
14:00:42.773 -> MJPEG video end
14:00:42.773 -> Start of loop! Available memory: 202304 bytes
14:00:42.773 -> fileNo: 1
14:00:42.773 -> Loading video: /160_30fps_1.mjpeg

Note the only (permanent) drop in available memory is before the first video playback, which is before the buffer is allocated.

I should mention that my code is modified from this repo (mostly removing audio playback), but I believe the last commit does not fix a memory leak and instead breaks the program. Maybe if you are seeing a memory leak, it may actually be in the audio code (which I took out)?

bepaald commented 2 years ago

Just from reading the code, I see 3 somewhat obvious memory problems in the audio code. Is it possible you tested the previous memory leak fix using video's without sound?

There are three pointers

static AudioGeneratorMP3 *mp3;
static AudioFileSourceFS *aFile;
static AudioOutputI2S *out;

Which are allocated in playVideo(): https://github.com/SuperMakeSomething/mini-video-player/blob/851e495aeb31cf1bd3e2d530af24811b84eeedc0/Code/miniVideoPlayer/miniVideoPlayer.ino#L219-L221

But they are never deleted anywhere. So each time a new video starts, new memory is allocated to these three variables, and the old memory the pointers pointed to is lost. For each of these three variables you should:

either set them once and never again:

if (!mp3)
mp3 = new AudioGeneratorMP3();

or reset them each time a new one is needed:
```
if (aFile)
delete aFile;
out = new AudioFileSourceFS(etc...
```
Just from looking at the code and guessing what these audio classes probably do, I'm guessing mp3 and out only need to be set once and can be reused for each new audio file (you can use the first option). aFile definitely needs the second option as it needs the new filename every time. So my best guess is you need to (remove MjpegClass::cleanup() and) change lines 219-221 to
```
if (!out)
out = new AudioOutputI2S(0,1,128);
if (!mp3)
mp3 = new AudioGeneratorMP3();
if (aFile)
delete aFile;
aFile = new AudioFileSourceFS(SD, audioFilename.c_str()); //Typecast audioFilename String for AudioFileSourceFS input
```

To be safe, I personally would also explicitly initialize the pointer as NULL (at the top of the file, static AudioGeneratorMP3 *mp3 = NULL;), but that might not be strictly necessary.

Please note I have not tested any of this and I am not running any of this audio code. So it's possible I'm completely wrong on everything :)

SuperMakeSomething commented 2 years ago

Thank you so much for your help with this issue! I just double checked and indeed the commit from yesterday appears to break the code. (After adding the lines, I was testing the ESP32's ability to continuously loop video but only had one file on the SD card. I thought that it was running the loop successfully, but it appears to instead have been resetting every time it finished playing the video.) Based on your comments, I agree that this may instead be an audio issue. I will try this fix and will report back. Cheers!

(Apologies, I accidentally closed this issue, but it is not yet resolved.)

SuperMakeSomething commented 2 years ago

I have incorporated the changes and the code seems to work. When testing with two media files (video + audio) on the SD card, the ESP32 can loop through all of the files 5 times before experiencing the following error and resetting automatically:

Guru Meditation Error: Core 1 panic'ed (LoadProhibited). Exception was unhandled. Core 1 register dump: PC : 0x400d8949 PS : 0x00060130 A0 : 0x800d1c58 A1 : 0x3ffb1ec0
A2 : 0x3ffb8784 A3 : 0x3ffb8794 A4 : 0x00001000 A5 : 0x00000000
A6 : 0x3ffb914c A7 : 0x00000001 A8 : 0x0000270f A9 : 0x3ffb1e90
A10 : 0x00000001 A11 : 0x00000004 A12 : 0xef2d8002 A13 : 0x00000002
A14 : 0x00000000 A15 : 0x00000006 SAR : 0x0000000a EXCCAUSE: 0x0000001c
EXCVADDR: 0x0000100a LBEG : 0x4000c2e0 LEND : 0x4000c2f6 LCOUNT : 0xffffffff

ELF file SHA256: 0000000000000000

Backtrace: 0x400d8949:0x3ffb1ec0 0x400d1c55:0x3ffb1ee0 0x400d20f5:0x3ffb1f50 0x400e3c1d:0x3ffb1fb0 0x40089bc6:0x3ffb1fd0

Rebooting... ets Jul 29 2019 12:21:46

rst:0xc (SW_CPU_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT) configsip: 0, SPIWP:0xee clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00 mode:DIO, clock div:1 load:0x3fff0018,len:4 load:0x3fff001c,len:1216 ho 0 tail 12 room 4 load:0x40078000,len:10944 load:0x40080400,len:6388 entry 0x400806b4

I will need to test with more media files (3, for example) to see if it makes it through less than 5 times, but I believe that this suggests another memory leak somewhere..?

SuperMakeSomething commented 2 years ago

Issue fixed with code modifications by bepaald. Thank you!

SuperMakeSomething / mini-video-player

Latest memory leak fix breaks program / memory leaks in audio code #2