Vorstellung Playlist Optimierung

Das sollte erst gefixt werden. Das hat auf jeden Fall funktioniert, als ich das damals implementiert habe.

Klar, kann ich machen. @tueddy kannst du die M3U hochladen, dann schaue ich es mir morgen an.

Ich habe das auch, dachte es liegt an meiner Playlist.
Ich hätte noch ein Idee , wenn du schonmal dabei bist.
Durchschalten über die Endpunkte der Liste hinaus. Also von Ende wieder nach 1 und von 1 wieder ans Ende. Ich hatte das vor einiger Zeit nach @biologist ´s Tipp implementiert. Das ist bei Webradio ganz praktisch. Das war echt einfach, ich muss mal danach suchen.

Hier die Playlist (Das könnte sogar die von @compactflash gewesen sein). Hatte mal stack-canary exception weil hier auch SSL verwendet wird. Dazu habe zum Test die Stackgröße des Audiotask erhöht. Aber auch dann beendet sich die Playlist nach Sender 4:

#EXTM3U
# http://web.ard.de/radio/radionet/
# http://www.wdr.de/wdrlive/wdr2player.html
#EXTINF:0,103.7 UnserDing (MP3)
http://streaming01.sr-online.de/unserding_2.m3u
#EXTINF:0,1LIVE (MP3)
http://www.wdr.de/wdrlive/media/einslive.m3u
#EXTINF:0,1LIVE diggi (MP3)
http://www.wdr.de/wdrlive/media/einslivedigi.m3u
#EXTINF:0,1LIVE Kassettendeck (MP3)
http://www.wdr.de/wdrlive/media/1LIVE_kassettendeck.m3u
#EXTINF:0,Radio Bremen vier (http)
http://icecast.radiobremen.de/rb/bremenvier/live/mp3/128/stream.mp3
#EXTINF:0,Radio Bremen vier (https)
https://icecast.radiobremen.de/rb/bremenvier/live/mp3/64/stream.mp3

nee, hier ist meine
Webradioliste1.txt (662 Bytes)

Hm, hab den Absturz. Liegt an wdr.de, die leiten den nicht gefundenen link http://www.wdr.de/wdrlive/media/1LIVE_kassettendeck.m3u auf eine https 404-Tut-uns-Leid Seite um. Mein mp3play stirbt mit einem Stack canary :laughing:

Hab den Fehler gefunden: https://github.com/biologist79/ESPuino/blob/894c6d8392b4f1aa92d3c73bf10635e8b4b59e1b/src/AudioPlayer.cpp#L809

Das Problem ist, dass die Verbindung zum Server funktioniert, aber audio->isRunning() ist false. Damit kommt es zu einem Timeout mit settleCount um die Playlist wird abgebrochen.

Ich bin am Fixen :wink:

edit

Die Änderung ist hier. Ich habe auch die Stack Größe um knapp 2k erhöht um keine Ausfälle bei https streams zu haben. Sollte das nicht gewpnscht sein, kann ich das natürlich zurück ändern.

3 „Gefällt mir“

Danke für die Fehleranalyse & den Fix, ich probiers gleich aus!

Ja HTTPS verbraucht enorm viel Speicher! Empfehlung für Webstreams, sucht Euch immer die http-Adresse ohne SSL, meist bieten die Sender beides an.

Wir haben letztens die Stackgröße schonmal angehoben. Ich meine es steht dadurch auch weniger Heapspeicher zur Verfügung? Frage ist halt wie weit man es hier treibt…
Ich bin mir unsicher bzgl. Stackerhöhung. Vielleicht lassen wir das im Dev-Branch einfach mal laufen…

Edit:

Der Bugfix für einen fehlerhaften Eintrag in der M3U-Playlist klappt hervorragend!
Es wird bis zum Timeout gewartet und dann auf den nächsten Track gewechselt. So kann man in der Weboberfläche auch sehen warum das ungültig ist, im Titel steht dann z.B. „(5/13): HTTP/1.0 404 Not Found“. :+1:

2 „Gefällt mir“

Ja, also ich wäre da auch vorsichtig, das schon wieder anzuheben. Vor allem gleich um so viel.

Ansonsten, wenn das gescheit läuft, dann sollte man vielleicht den aktuellen DEV in den Master mergen. Und erst DANACH die Playlist-Optimierung in den DEV übernehmen.

2 „Gefällt mir“

Ich habe den letzten Commit mit dem M3U Fix extrahiert und eine neue PR gestartet (dabei gleich auch die Stackgröße wieder auf 6k stellen). Denn diese Funktioniert auch ohne den restlichen Commits für die Playlist Optimierung.

5 „Gefällt mir“

Danke für deine Arbeit!

1 „Gefällt mir“

das funktioniert nicht im „Rückwärtsgang“ , da wird dann wieder der nächste Track vorwärts angesprungen, Man kann jetzt jedoch mit nochmaligem Tastendruck rüberspringen , was vorher nicht ging.

1 „Gefällt mir“

Ich bekomme jetzt nach dem Fix sporadisch entweder die Meldung „Keine Playlist“ order einen Crash beim Auflegen einer Karte (Hörbuch von SD):

D [59121] Gebe Speicher der alten Playlist frei (Freier Speicher: 74032 Bytes)
D [59121] Freier Speicher nach Aufräumen: 74032 Bytes
D [59143] Freier Speicher: 73880 Bytes
N [59143] Playlist-Generierung
N [59149] Anzahl gültiger Files/Webstreams: 2
N [59150] Modus: Spiele alle Tracks (alphabetisch sortiert) des Ordners '/2 kleine WoeGuru Meditation Error: Core  1 panic'ed (LoadProhibited). Exception was unhandled.

Core  1 register dump:
PC      : 0x400949ca  PS      : 0x00060230  A0      : 0x800d403e  A1      : 0x3ffda8a0
A2      : 0x00000000  A3      : 0x030b0031  A4      : 0x00000003  A5      : 0x00000000  
A6      : 0x00000000  A7      : 0xff000000  A8      : 0x030b0031  A9      : 0x00000000  
A10     : 0x00000000  A11     : 0x00000068  A12     : 0x3f401330  A13     : 0x00000001
A14     : 0x00060220  A15     : 0x00000001  SAR     : 0x0000000a  EXCCAUSE: 0x0000001c
EXCVADDR: 0x030b0031  LBEG    : 0x40085755  LEND    : 0x4008575d  LCOUNT  : 0x00000027  


Backtrace: 0x400949c7:0x3ffda8a0 0x400d403b:0x3ffda8c0

  #0  0x400949c7:0x3ffda8a0 in strncmp at /builds/idf/crosstool-NG/.build/HOST-x86_64-w64-mingw32/xtensa-esp32-elf/src/newlib/newlib/libc/string/strncmp.c:65
  #1  0x400d403b:0x3ffda8c0 in AudioPlayer_Task(void*) at src/AudioPlayer.cpp:680




ELF file SHA256: 19d587ebfe3061d0

E (5532) esp_core_dump_flash: Core dump flash config is corrupted! CRC=0x7bd5c66f instead of 0x0
Rebooting...

Es scheint als ob die Playlist unterm Hintern weggezogen und NULL wird. Dann crasht es im Audioplayer Zeile 680 beim Stringvergleich auf „http“?
Kommentiere ich den Fix aus klappt es wieder. Könnt Ihr das bestätigen?

1 „Gefällt mir“

Ja, kann ich nachstellen. Es scheint eine Race Condition zwischen dem neuen Timeout im AudioPlayer_TrackQueueDispatcher und AudioPlayer_TrackQueueDispatcher zu sein. Ich vermute, dass gPlayProperties.trackFinished durch den Timeout gesetzt wird und damit beim Empfangen der neuen Playlist der erste Track sofort übersprungen wird

Könnt ihr prüfen, ob der folgende Ptach abhilfe schafft?

diff --git a/src/AudioPlayer.cpp b/src/AudioPlayer.cpp
index 5d9b888..fc9792b 100644
--- a/src/AudioPlayer.cpp
+++ b/src/AudioPlayer.cpp
@@ -348,7 +348,7 @@ void AudioPlayer_Task(void *parameter) {
 #endif
 
 	constexpr uint32_t playbackTimeout = 2000;
-	uint32_t playbackTimeoutStart = 0;
+	uint32_t playbackTimeoutStart = millis();
 
 	AudioPlayer_CurrentVolume = AudioPlayer_GetInitVolume();
 	audio->setPinout(I2S_BCLK, I2S_LRC, I2S_DOUT);
@@ -404,12 +404,19 @@ void AudioPlayer_Task(void *parameter) {
 		trackQStatus = xQueueReceive(gTrackQueue, &gPlayProperties.playlist, 0);
 		if (trackQStatus == pdPASS || gPlayProperties.trackFinished || trackCommand != NO_ACTION) {
 			if (trackQStatus == pdPASS) {
-				if (gPlayProperties.pausePlay) {
-					gPlayProperties.pausePlay = false;
-				}
 				audio->stopSong();
 				Log_Printf(LOGLEVEL_NOTICE, newPlaylistReceived, gPlayProperties.numberOfTracks);
 				Log_Printf(LOGLEVEL_DEBUG, "Free heap: %u", ESP.getFreeHeap());
+				playbackTimeoutStart = millis();
+				gPlayProperties.pausePlay = false;
+				gPlayProperties.repeatCurrentTrack = false;
+				gPlayProperties.repeatPlaylist = false;
+				gPlayProperties.sleepAfterCurrentTrack = false;
+				gPlayProperties.sleepAfterPlaylist = false;
+				gPlayProperties.saveLastPlayPosition = false;
+				gPlayProperties.playUntilTrackNumber = 0;
+				gPlayProperties.trackFinished = false;
+				gPlayProperties.playlistFinished = false;
 
 #ifdef MQTT_ENABLE
 				publishMqtt(topicPlaymodeState, gPlayProperties.playMode, false);
@@ -985,12 +992,6 @@ void AudioPlayer_TrackQueueDispatcher(const char *_itemToPlay, const uint32_t _l
 	gPlayProperties.playMode = _playMode;
 	gPlayProperties.numberOfTracks = strtoul(*(musicFiles - 1), NULL, 10);
 	// Set some default-values
-	gPlayProperties.repeatCurrentTrack = false;
-	gPlayProperties.repeatPlaylist = false;
-	gPlayProperties.sleepAfterCurrentTrack = false;
-	gPlayProperties.sleepAfterPlaylist = false;
-	gPlayProperties.saveLastPlayPosition = false;
-	gPlayProperties.playUntilTrackNumber = 0;
 
 #ifdef PLAY_LAST_RFID_AFTER_REBOOT
 	// Store last RFID-tag to NVS

Bei mir läuft es mit der Änderung ohne Fehler. Ich muss schauen, ob es eine schönere Lösung gibt.

1 „Gefällt mir“

Ja, das behebt bei mir die Exception :+1:. Wäre das durch den anderen Playlist-PR überhaupt noch relevant oder ist das unabhängig davon?

Update:
funktioniert leider doch nicht in allen Fällen. Habe zum testen den Audio-Task mal auf Core 0 zugewiesen, jetzt kommt die exakt selbe Exception wieder auch mit dem Fix. Zugegeben etwas ein Grenzfall :slight_smile:

1 „Gefällt mir“

Grml… Hab das Problem. Wir setzten playMode in Zeile 985, potentiel lange bevor wir die Playlist in die Queue zB in Zeile 1055 schieben. Hier schlägt leider Multithreading (bzw SMP) zu, je mehr Dateien in dem Ordner sind und je mehr operationen ausgeführt werden müssen, desto öfters kann ich den Fehler erzwingen ( also zB wenn alle Dateien Random angespielt werden sollen). Wenn wir lange brauchen, kann der mp3 Thread zum Zug kommen (der auch höhere Prio hat).

Wird der Dispatcher zwischen Zeile 985 und 1025 unterbrochen und der AudioTask läuft, ist mit dem gesetzten playMode & numberOfTracks ist die Bedingung für die Audio Timeout gegeben. Der setzt gPlayProperties.trackFinished und wir versuchen zu dem 2. Track zu springen, die Playlist ist aber zu dem Zeitpunkt noch immer ein nullptr. Damit exlodiert strncmp in Zeile 680.

diff --git a/src/AudioPlayer.cpp b/src/AudioPlayer.cpp
index 5d9b888..8538be4 100644
--- a/src/AudioPlayer.cpp
+++ b/src/AudioPlayer.cpp
@@ -348,7 +348,7 @@ void AudioPlayer_Task(void *parameter) {
 #endif
 
 	constexpr uint32_t playbackTimeout = 2000;
-	uint32_t playbackTimeoutStart = 0;
+	uint32_t playbackTimeoutStart = millis();
 
 	AudioPlayer_CurrentVolume = AudioPlayer_GetInitVolume();
 	audio->setPinout(I2S_BCLK, I2S_LRC, I2S_DOUT);
@@ -404,12 +404,19 @@ void AudioPlayer_Task(void *parameter) {
 		trackQStatus = xQueueReceive(gTrackQueue, &gPlayProperties.playlist, 0);
 		if (trackQStatus == pdPASS || gPlayProperties.trackFinished || trackCommand != NO_ACTION) {
 			if (trackQStatus == pdPASS) {
-				if (gPlayProperties.pausePlay) {
-					gPlayProperties.pausePlay = false;
-				}
 				audio->stopSong();
 				Log_Printf(LOGLEVEL_NOTICE, newPlaylistReceived, gPlayProperties.numberOfTracks);
 				Log_Printf(LOGLEVEL_DEBUG, "Free heap: %u", ESP.getFreeHeap());
+				playbackTimeoutStart = millis();
+				gPlayProperties.pausePlay = false;
+				gPlayProperties.repeatCurrentTrack = false;
+				gPlayProperties.repeatPlaylist = false;
+				gPlayProperties.sleepAfterCurrentTrack = false;
+				gPlayProperties.sleepAfterPlaylist = false;
+				gPlayProperties.saveLastPlayPosition = false;
+				gPlayProperties.playUntilTrackNumber = 0;
+				gPlayProperties.trackFinished = false;
+				gPlayProperties.playlistFinished = false;
 
 #ifdef MQTT_ENABLE
 				publishMqtt(topicPlaymodeState, gPlayProperties.playMode, false);
@@ -424,7 +431,7 @@ void AudioPlayer_Task(void *parameter) {
 			}
 			if (gPlayProperties.trackFinished) {
 				gPlayProperties.trackFinished = false;
-				if (gPlayProperties.playMode == NO_PLAYLIST) {
+				if (gPlayProperties.playMode == NO_PLAYLIST || gPlayProperties.playlist == nullptr) {
 					gPlayProperties.playlistFinished = true;
 					continue;
 				}
@@ -985,12 +992,6 @@ void AudioPlayer_TrackQueueDispatcher(const char *_itemToPlay, const uint32_t _l
 	gPlayProperties.playMode = _playMode;
 	gPlayProperties.numberOfTracks = strtoul(*(musicFiles - 1), NULL, 10);
 	// Set some default-values
-	gPlayProperties.repeatCurrentTrack = false;
-	gPlayProperties.repeatPlaylist = false;
-	gPlayProperties.sleepAfterCurrentTrack = false;
-	gPlayProperties.sleepAfterPlaylist = false;
-	gPlayProperties.saveLastPlayPosition = false;
-	gPlayProperties.playUntilTrackNumber = 0;
 
 #ifdef PLAY_LAST_RFID_AFTER_REBOOT
 	// Store last RFID-tag to NVS

Hier der neue Patch, wichtig ist die Änderung in Zeile 427 if (gPlayProperties.playMode == NO_PLAYLIST || gPlayProperties.playlist == nullptr). Damit brechen wir auch ab, wenn die Playlist noch nicht „da“ ist.

Das ist nur ein schneller Bugfix, die saubere Lösung wäre alle Daten zu der neuen Playlist gleichzeitig zu übertragen, die Race Condition bleibt weiterhin bestehen (wir kleben nur ein Pflaster drüber). Alternativ können wir auch 0db1e86 reverten. Dass sollte das Problem auch wieder unter den Teppich kehren :slight_smile:.

Das Problem wäre aktuell mit dem 3. Patch (Umstellung auf die Playlist Klasse) verschwunden, da die Playlist alle Informationen bündelt. Werde ich aber für Patch 2 vorsehen (Umstellung von Queue auf Mutex + Smart Pointer).

3 „Gefällt mir“

Sehr cool! Danke dir!

Aber dann wäre ich eher für die saubere Lösung in deinem schon bestehenden PR und für ein revert von dem Bugfix-commit im aktuellen dev-branch.
Wird sonst vermutlich auch immer schwerer das wieder zusammen zu mergen.

Dann teste ich lieber gleich auf der finalen Lösung intensiver, das schau ich mir morgen dann Mal genauer an :slight_smile:

2 „Gefällt mir“

@laszloh Das passt & Vielen Dank für Deine Mühe (Obwohl das eigentlich gar nicht Deine Baustelle ist). Hab’s jetzt eingecheckt, hoffe ich hab jetzt nix vergessen.

Das sind genau die Probleme die für ein Refactoring der Playlist sprechen. Aber Plan ist jetzt mal erst die Verbesserungen aus dem DEV-Branch in den Master zu übernehmen & dann die neue Playlist im DEV weiter zu entwickeln.

3 „Gefällt mir“

Wie weit ist den der Pull Request Playlist improvements by laszloh · Pull Request #269 · biologist79/ESPuino · GitHub ?
Ich baue auch gerade an ein paar Features und stolpere auch über die aktuelle Playlist Implementierung.
Der Umbau auf Playlist Klassen würde da einige unschöne Hacks vermeiden.

Wir hatten das Thema zurückgestellt, weil plötzlich noch verschiedene Probleme mit Audiorucklern aufgetreten sind.

Aus meiner Sicht kann das Thema angegangen werden. Was mir aber ehrlich gesagt nicht gefällt, das ist der „Trend“ im ESPuino-Code, dass immer weniger dokumentiert wird (generell; der PR hier ist jetzt für mich nur der Aufhänger, das mal zu äußern). Also wenn ich mir zB die cpp.h anschaue, dann habe ich ehrlich gesagt nicht die geringste Ahnung, was da passiert.

Als ich ESPuino damals alleine geschrieben habe, hatte zB jede (oder nahezu jede) Funktion eine Beschreibung darüber, was sie macht. Da braucht’s (für mich) jetzt auch nicht die große Doxygen-Keule, sondern ich hätte gerne, dass über jeder Methode, die mehr als ein GETer oder SETer ist, eine Kurzbeschreibung steht (vielleicht 1-3 Zeilen), in der steht, was gemacht wird.
Kommentarzeilen in Funktionen/Methoden finde ich auch gut, aber hier sehe ich eigentlich auch keinen Engpass.

Ich weiß, dass ich diesen Zielzustand so nie öffentlich formuliert habe, insofern bin ich vermutlich ein Stück weit selbst Schuld, dass es so ist, wie es ist. Aber ich formuliere hiermit, dass ich gerne mehr Doku hätte :slight_smile:.

4 „Gefällt mir“

Jupp, die PR ist erstmal ruhend. Wir hatten uns geeinigt, dass das in 3 PR’s aufeteilt wird. Vorstellung Playlist Optimierung - #17 von laszloh

Nr 1 ist schon da, aber wir hatten, wie biologist geschrieben hat, Probleme bei der Wiedergabe und wollten keine großen Änderungen einführen, bis das Problem gelöst war. Ich halte gerade die PR up-to-date zu der dev-branch.

Bezüglich Kommentierung werde ich mich bessern, vor allem wenn es Richtung der 3. PR geht. Ich muss auch sagen, mit der Version der cpp.h bin ich auch nicht zufrieden. Ich denke, die neue cpp.h ist bezüglich Kommentierung besser (bzw, ich verlinke einfach auf die Doku der Funktion, die ich damit implementiere). Kommentierung in der ist sparse, weil der Code 1:1 copy paste aus der c++20 stdlib ist (backport von Funktionen, die ich verwende/mag, die es aber in unserer gcc nicht existiert).

4 „Gefällt mir“