Vorstellung Playlist Optimierung

das sehe ich kritisch, nur weil du die Funktion verwendest/magst gilt das nicht für alle/für jeden

Dann muss bei sowas imho ein Text dabei warum dieses oder jene Funtkion besser ist, am besten mit einem Beispiel wo z.B klar zu sehen ist das der Code lesbarer/kürzer wird…

Und wir brauchen einen Migrationspfad wenn unser gcc mal soweit ist, was dann?

Den Punkt verstehe ich jetzt nicht. Du musst die Funktionen nicht verwenden, wenn du nicht willst. Ich überschreibe nicht eine existierende Funktion aus der stdlib, die Funktionen gibt es nicht, da unser gcc aktuell std++17 kann. Wenn wir std++20 oder std++23 hätten, würde ich mir die Arbeit nicht machen.
Und die Funktionen sind nicht selbst gestrickt, sondern kommen vom gcc.

Vor allem std::to_array ist zB eine quality of life Improvement beim Programmieren. Als Beispiel der Code hier: ESPuino/src/SdCard.cpp at aae3fcca7826c50caabefee2f6dacbd0b3618093 · laszloh/ESPuino · GitHub

Ohne std::to_array würde der grob so aussehen müssen:

constexpr std::array<const char*, 12> audioFileSufix {{
		".mp3",
		".aac",
		".m4a",
		".wav",
		// usw
}};

Und wenn man dann einen neuen Eintrag hinzufügen oder löschen will, muss die hart kodierte 12 richtig gestellt werden.

Dann wird laut der stdlib ein feature-test makro (d.h. ein #define) in dem Header der Funktion gesetzt. Bei std::to_array ist das zB cpp_lib_to_array in dem Header „array“ (siehe hier)
Sobald dieser gesetzt ist wird mein Codeteil durch den Präprozessor ausgeblendet.

Bin da kein Experte aber ist c++20 nicht jetzt schon möglich (ohne zusätzliche Packages)?

build_flags = -std=gnu++2a
build_unflags = -std=gnu++11

Hm, interessting. Hab bis jetzt nur std++20 getestet. Werde mich mal das WE damit beschäftigen. Vielleicht brauchen wir zumindest das std::to_array nicht händisch machen.

Leider nein, -std=gnu++2a aktiviert die in gcc implementierten Sprach-Features von C++20; die stdlib bleibt die gleiche. Es gäbe std::experimental::to_array, welche aber nicht ganz wie die Funktion in gcc Version > 10 ist.

Aber, nach ein wenig Tests kann ich mit std++17 auch halbwegs gut arbeiten, ohne std::to_array zu nutzen. Man lernt nie aus, damit kann ich nur sage: CTAD to the rescue!

Leider macht das den Code ein wenig mehr copy & paste, sobald man etwas Anderes als ein Array von POD machen möchte:
Spielwiese: Compiler Explorer

Ich pushe heute Abend ein Update zu der PR nachdem ich es getestet habe.

2 „Gefällt mir“

Hattest du schon Zeit das zu testen?

Ja, eigentlich schon vor zwei Tagen… Hatte vergessen zu pushen :man_facepalming:

Ich habe auch gleich die Konflikte mit der aktuellen dev branche gelöst.

2 „Gefällt mir“

@laszloh da Teil 1 nun gemergt ist: Könntest du ggf. Playlist improvements by laszloh · Pull Request #269 · biologist79/ESPuino · GitHub einmal auf dev rebasen?

Um flexibler zu sein würde ich gerne anstelle im Playlist vector direkt ein char pointer zu hinterlegten, den pointer in einer PlaylistItem Klasse Kapseln.
Spricht da etwas dagegen?

Ja, kann ich gerne angehen.

Es wird eine größere Überarbeitung der PR sein, ich hatte auch vor das Interface zu der Playlist zu verbessern.

Ich werde auch nur das Interface hier kurz posten, sobald dieser steht um zu schauen, wie ihr es findet und ob es Verbesserungspotenzial gibt.

So, hier der erste Entwurf. Das Playlist Item in einer eigenen Klasse zu Kapseln, finde ich eine gute Idee:

Und hier die zum Bild gehörige Playlist.h

Playlist.hpp
#pragma once

#include <WString.h>
#include <stdlib.h>

	/// @brief Object representing a single entry in the playlist
	struct MediaItem {
		const String uri {String()}; //< playable uri of the entry

		struct Metadata {
			String title {String()}; //< title of the track
			String artist {String()}; //< artist
			String albumTitle {String()}; //< album title
			String albumArtist {String()}; //< album artist, use artist if not set
			String displayTitle {String()}; //< overrides title if set
			size_t trackNumber; //< track number _in the album_
			size_t totalTrackCount; //< total number of track _in the album_
			uint8_t releaseYear; //< year of the song/album release
			uint8_t releaseMonth; //< month of the song/album release
			uint8_t releaseDay; //< day of the song/album release

			// embedded artwork
			String artworkMimeType {String()}; //< mime string for the album artwork
			std::vector<uint8_t> artworkData {std::vector<uint8_t>()}; //< byte array of the artwork

			String artworkUri {String()}; //< uri pointing to the external artwork

			Metadata() = default;

			/// @brief Compare operator overload for MediaMetadata, returns true if lhs==rhs
			friend bool operator==(Metadata const &lhs, Metadata const &rhs) {
				return lhs.title == rhs.title && lhs.artist == rhs.artist && lhs.albumTitle == rhs.albumTitle
					&& lhs.albumArtist == rhs.albumArtist && lhs.displayTitle == rhs.displayTitle
					&& lhs.trackNumber == rhs.trackNumber && lhs.totalTrackCount == rhs.totalTrackCount
					&& lhs.releaseYear == rhs.releaseYear && lhs.releaseMonth == rhs.releaseMonth
					&& lhs.releaseDay == rhs.releaseDay && lhs.artworkMimeType == rhs.artworkMimeType
					&& lhs.artworkData == rhs.artworkData && lhs.artworkUri == rhs.artworkUri;
			}
		};
		std::unique_ptr<Metadata> metadata {nullptr}; //< optional metadata for the entry

		/// @brief Compare operator overload for MediaItem, returns true if lhs==rhs
		friend bool operator==(MediaItem const &lhs, MediaItem const &rhs) {
			const auto comparePointer = [](const std::unique_ptr<Metadata> &lhs, const std::unique_ptr<Metadata> &rhs) -> bool {
				if (lhs == rhs) {
					return true;
				}
				if (lhs && rhs) {
					return *lhs == *rhs;
				}
				return false;
			};

			return lhs.uri == rhs.uri && comparePointer(lhs.metadata, rhs.metadata);
		}

		MediaItem() = default;
		MediaItem(const String &uri)
			: uri(uri) { }
		~MediaItem() = default;
	};

/// @brief Interface class representing a playlist
class Playlist {
public:
	/**
	 * @brief signature for the compare function
	 * @param a First element for the compare
	 * @param a Second element for the compare
	 * @return true if the expression a<b allpies (so if the first element is *less* than the second)
	 */
	using CompareFunc = std::function<bool(const MediaItem &a, const MediaItem &b)>;

	Playlist() = default;
	virtual ~Playlist() = default;



	/**
	 * @brief get the status of the playlist
	 * @return true If the playlist has at least 1 playable entry
	 * @return false If the playlist is invalid
	 */
	virtual bool isValid() const { return false; }

	/**
	 * @brief Allow explicit bool conversions, like when calling `if(playlist)`
	 * @see isValid()
	 * @return A bool conversion representing the status of the playlist
	 */
	explicit operator bool() const { return isValid(); }

	/**
	 * @brief Get the number of entries in the playlist
	 * @return size_t The number of MediaItem elemenets in the underlying container
	 */
	virtual size_t size() const = 0;

	/**
	 * @brief Get the element at index
	 * @param idx The queried index
	 * @return const MediaItem the data at the index
	 */
	virtual const MediaItem &at(size_t idx) const = 0;

	/**
	 * @brief Add an item at the end of the container
	 * @param item The new item ot be added
	 * @return const MediaItem the data at the index
	 */
	virtual void addMediaItem(const MediaItem &&item) = 0;

	/**
	 * @brief Add an item at the end of the container
	 * @param item The new item ot be added
	 * @param idx after which entry it'll be added
	 * @return const MediaItem the data at the index
	 */
	virtual void addMediaItem(const MediaItem &&item, int idx) = 0;

	/**
	 * @brief Sort the underlying container according to the supplied sort functions
	 * @param comp The compare function to use, defaults to strcmp between the two uri objects
	 */
	virtual void sort(CompareFunc comp = [](const MediaItem &a, const MediaItem &b) -> bool { return a.uri < b.uri; }) { }

	/**
	 * @brief Randomize the underlying entries
	 */
	virtual void shuffle() { }

	/**
	 * @brief Array opertor override for item access
	 * @param idx the queried index 
	 * @return const MediaItem& Reference to the MediaItem at the index
	 */
	const MediaItem &operator[](size_t idx) const { return at(idx); };
};

Alle Funktionen in Playlist sind virtual (d.h. können bzw. müssen) von der eigentlichen Implementation bereit gestellt werden. Ich muss mal tief in mich gehen, ob und welche gemeinsamen Funktionen in der Playlist Sinn machen und diese dann in die Klasse schieben.
Über die PlaylistFlags & PlaylistType habe ich mir noch keine genauen Gedanken gemacht (flags könnten zB Repeate Track, Repeate Playlist, etc sein, type zB Files, Webstream, Audiobook, etc.)

Das MediaItem hält aktuell eine URI/Pfad zu dem Objekt und optional ein Pointer auf die Metadaten (mit den Feldern sind die Metadaten leer aktuell 150 byte). Abstraktion habe ich (noch) nicht vorgesehen, können wir überlegen, ob und wo hier eine Virtualisierung Sinn macht (zB. definitiv den Destruktor, damit man überhaupt ableiten kann).
Ob die ID3-Tags schon vor dem Abspielen ausgelesen werden können habe ich noch nicht geprüft, würde uns aber sicherlich das Leben leichter machen (zB könnten wir den Album Cover vorab aus der MP3 laden, wenn wir PSRAM zur Verfügung haben).

Wie gesagt, ein erster schneller Entwurf. Wie immer, Ideen willkommen :slight_smile:

1 „Gefällt mir“

Die Übersetzung des playModes in entsprechende Flags hatte ich auch schon überlegt. Das dürfte den control loop vereinfachen. Gleichzeitig könnte z.B. ein M3U file über entsprechende Direktiven/Attribute selbst steuern ob es wiederholt werden soll, die letzte Position gespeichert werden soll, etc…
Die „light“ Variante davon wäre es, den playMode in die Playlist zu verschieben der dann von dem control loop genutzt wird. So müsste man jetzt nicht die control loop Logik anfassen aber man könnte z.B. aus einer M3U Datei den playMode festlegen.

Bei Metadaten fehlen z.B. noch die Dauer und Kapitel. Artwork könnte man auch wieder in eine eigene Klasse schieben. Die Metadaten würde ich aber auch erstmal komplett weglassen. Wenn wir nicht eine neue Parser Bibliothek einbauen wollen, müsste hier sowieso erstmal die audioI2C lib so umgebaut werden, das man die Parser Logik unabhängig nutzen kann.

Nur damit ihr euch nicht wundert, ich hab kein Ragequit gemacht :wink:

Mein Github Account wurde gestern gesperrt und ist aktuell nicht sichtbar (ich warte noch auf den Github support und hoffe dass nicht alles gelöscht ist). Bis es wieder funktioniert habe ich als Backup meine ESPuino Branch auf Gitlab hochgeladen.

Aktuelle Implementation der Playlist Klasse findet ihr hier: Files · feature/playlist-class · Laszlo Hegedüs / ESPuino · GitLab

Warst du unartig? :grimacing:

Nicht das ich wüsste, ich war immer ein guter Junge, hab auch nie was vom Krampus bekommen :laughing:. Mal schauen, wie lang der Support braucht.

Mein github Account wurde vorgestern gesperrt. Krass, dass das wohl kein Einzelfall war. Github support hat sich noch nicht gemeldet.

Ja, ich befürchte, das wird länger dauern. Hab auch noch kein Mucks von denen gehört. Was echt scheiße ist, ich hab einige Repos von denen ich keine lokalen Kopien habe (jupp, ich weiß „no backup, no mercy“, hatte nicht erwartet Github mal zu verlieren). Werde auf jeden Fall jetzt ein Backup System mit Gitea lokal bei mir aufsetzten (plus ernsthaft Gitlab überlegen).

So viel zu Rants :smiley: Back to topic

Bis dahin habe ich die Playlist Klasse und die beiden Standrad Implementationen fertig gestellt. Es funktioniert auf meiner Testumgebung.

Wo ich mir gerade unsicher bin, sind zwei Punkte:

  1. Benötigen wir wirklich die Klasse SingleFilePlaylist wenn es nur 1 Eintrag gibt? Mein Bauchgefühl sagt, dass wir das auch mit der „normalen“ Playlist abdecken könnten.
  2. Aktuell habe ich das lesen & Interpretiere eines Ordners in die Klasse FolderPlaylist gepackt. Gefühlt hat es da aber nichts verloren…

Wie seht ihr das?

Also sollten die bei mir der Meinung sein, dass sie den Account deaktivieren müssen, dann ist das ESPuino-Repo da weg. Ich kann MS eh leiden wie Bauchweh.

Das dürfte überflüssig sein. Das Sparren des vector Overheads ist es meiner Meinung nach nicht wert.

Unter Software Architektur Gesichtspunkten macht das Trennen sicher Sinn.

Hab nur ganz kurz drüber geguckt. Aktuell wird im Code überall auf das uri Attribut des MediaItems direkt zugegriffen. Das würde ich hinter einem Getter verstecken.

@gesperrte
2FA ist aber aktiv bei euch?

Nicht das da irgendwo was hakt…

Passt, werde ich dann mal angehen die beiden Klassen zu vereinfachen.

Ja, die Integration ist aktuell sehr improvisiert, wollte nur schauen, dass alles funktioniert (wie die Iteratoren, die mir sicherlich 3 graue Haare gekostet haben X’D). Werde da ein Getter einbauen, virtuel nehme ich an, damit man das Überschreiben kann.

Die Metadaten werde ich auch noch überarbeiten, mir schwebt ein Key/Value System vor (keys für die Einträge der Metdaten wie Title, Artist, etc).
Muss mir nur überlegen, wie ich die unterschiedlichen Datentypen repräsentiere (also zB String für Title, uint32 für duration, etc) und abspeichere; std::map wäre sicher ein no-brainer mit relativ wenig overhead (ca zwei pointer / eintrag für red-black-tree).

Jupp, ist aktiv, seit längerem, war zwingend notwendig für Contributors. Das ist ja wieso ich so verblüfft bin.