Vorstellung Playlist Optimierung

Software
61

Ja. Bei mir hat es angefangen mit einer Email „Password reset notification due to suspicious activity“, wie ich dann das Passwort resetet habe, wurde dann das Account suspended.

62

@laszloh Files · 62da89bba95b8a733be72cd803dd43359cecfbf4 · Laszlo Hegedüs / ESPuino · GitLab ist aktuell kaputt (Kompiliet nicht, da .uri Zugriff noch nicht umgestellt auf getURI()). Ist das der aktuelle Stand oder ggf. ein commit noch nicht gepusht?

63

Ja, das war noch der aktuelle Stand, sollte baer jetzt mit Rewrite AudioPlayer_NvsRfidWriteWrapper to use new playlist class (af7e8996) · Commits · Laszlo Hegedüs / ESPuino · GitLab funktionieren (Compiler läuft durch, ist aber noch ungetestet).

Ich hatte in den letzten Woche leider wenig Zeit mich um ESPuino zu kümmern (Umzug in RL und Umzug von Github auf GitLab und aufsetzten eines lokalen Backup-Systems haben mich sehr unter Wasser getaucht).

64

Ich habe die Branch ein wenig aufgeräumt. Von meiner Seite funktioniert diese jetzt stabil. Da Github sich noch immer nicht rührt, hab ich mir mal ein neues Account gemacht um PR’s einfacher abhandeln zu können.

PR: Add Playlist class by laszloh2 · Pull Request #310 · biologist79/ESPuino · GitHub

Link zu dem Entwicklungszweig: Files · feature/playlist-class · Laszlo Hegedüs / ESPuino · GitLab

4 „Gefällt mir“
65

Danke.
Da ja viel kopiert wird und der copy constructor nicht virtual sein kann, macht mir das gerade bei einer vom MedaItem abgeleiteten Klasse Ärger (zusätzliche Attribute gehen beim Kopieren verlohren, weil nur der Basis Klassen copy constructor aufgeruffen wird). Da müsste man ein virtual copy constructor pattern verwenden um das zu lösen.

Das viele kopieren führt auch zu anderen Problemen, ich kann z.B. recht zuverlässig ein Crash beim Cover image laden triggern:
In der Web.cpp gibt es z.B. folgenden Code:

const char *coverFileName = gPlayProperties.playlist->at(gPlayProperties.currentTrackNumber).getUri().c_str();
String decodedCover = "/.cache";

Problem: getUri() gibt ne Kopie zurück, die ist aber nach der Zeile auch direkt wieder out of scope und der coverFileName pointer damit ungültig. Direkt nach dem Aufruff funktioniert das noch, aber der decodedCover Konstruktor recycled den Speicher vom vorherigen url Buffer, nutzung des coverFileName führt entsprechend zu unerwarteten Ergebnissen.

Wenn man es so umbaut funktioneirt es dann zuverlässig:

String coverFileNameString = gPlayProperties.playlist->at(gPlayProperties.currentTrackNumber).getUri();
const char *coverFileName = coverFileNameString.c_str();
String decodedCover = "/.cache";

Im AudioPlayer wird getUrl analog dazu genutzt und entsprechend aktuell auch nicht ganz zuverlässig.

Ich würde dazu tendieren, weniger zu kopieren und mehr mit Referenzenzu arbeiten (Bei der Playlist Erstellung direkt aufm HEAP allokieren und dann nurnoch Referenzen herumreichen). Das sollte beide Probleme lösen und tendenziell auch ein wenig Speicherfreundlicher sein.

1 „Gefällt mir“
66

Argh, verdammt :person_facepalming: (bzw Kopf + Wand). Das kann so nicht funktionieren. Ich kann die MediaItem nicht als Objekt in den Vektor geben, das „schneidet“ bei den Derived-Klassen immer weg.

Tendiere gerade zu unqiue_ptr, sollte reichen. Oder lieber shared_ptr?

Auf jeden Fall, back to the drawing board.

67

Aktuell greifen z.B. audio task und web server potentiell parallel auf die Playlist zu was tendenziell zu entsprechenden race conditions/crashes führen kann wenn die palylist z.B. weggelöscht wird während der webserver noch drauf zugreift. Würde entsprechend direkt zu einem shared_ptr tendieren.

1 „Gefällt mir“
68

So, redesign ist fertig.

Änderungen:

Paylist.hpp:

  • MediaItem wird als std::shared_ptr gespeichert
    • Template Funktion emplaceMediaItem(Args &&...args) um nicht immer make_shared schreiben zu müssen (die Funktionen funktionieren nur, wenn die Klasse auf MediaItem konvertiert / „reduziert“ werden kann)
    • Template Funktion addMediaItem(D &&item), die eine Referenz nimmt und eine Kopie von der Klasser erstellt (weiß ich noch nicht, ob ich das so drin lassen werde)
  • Die Funktion at() gibt weiterhin eine Referenz auf das MediaItem zurück

MediaItem:

  • getUri gibt eine const Referenz zurück habe ich rückgängig gemacht, da sonst dynamisch generierte Pfade (zb für relative Pfade) nicht zurück gegeben werden können

Ich habe die Änderungen noch nicht in die alten commits eingebettet, somit sollte es einfacher sein, diese nachzuverfolgen.

69

@freddy hattest du schon zeit die Änderungen anzuschauen / zu testen?

70

Noch nicht, bin aktuell anderweitig Eingebunden.

71

Kann man dir/euch da irgendwie helfen. Ein Codereview bekomme ich nicht hin, da ich kaum verstehe, was du da gemacht hast. Aber etwas testen kann ich sicherlich auch.

72

Ja, gerne. Wenn du die Branch testen kannst wäre es hervorragend.
Also alles was sich um die Playlist steht ausprobieren. Bei solchen Tests macht es nichts, wenn du den Code nicht so genau kennst. Teilweise hilft das Fehler zu finden, die sonst nicht getestet werden(weil das ja-eh-Funktioniert) :wink: