From 9e447023a8f0eb512910eb324ff6788da3e3cbeb Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Fri, 10 Apr 2026 00:06:15 +0200 Subject: [PATCH 1/5] :rocket: refactor(pmocontrol): eliminate QueueBackend boilerplate with HasQueue blanket impl - Add `Hasqueue` trait and implement it for all renderers (Upnp, OpenHome, LinkPlay, ArylicTcp, Chromecast) - Replace manual `QueueBackend` implementations with blanket impl for types implementing Hasqueue (removes ~30+ duplicated methods across renderers) - Fix BUG: `sync_queue` in UpnpRenderer now correctly propagates cancel_token instead of ignoring it - Update version to 0.3.48 in Cargo.toml, lockfile and root file - Add refactoring plan document (`refactoring_pmocontrol.md`) detailing remaining P1-P3 tasks --- Blackboard/Todo/refactoring_pmocontrol.md | 323 ++++++++++++++++++ Cargo.lock | 2 +- PMOMusic/Cargo.toml | 2 +- pmocontrol/src/music_renderer/arylic_tcp.rs | 76 +---- pmocontrol/src/music_renderer/capabilities.rs | 47 ++- .../src/music_renderer/chromecast_renderer.rs | 75 +--- .../src/music_renderer/linkplay_renderer.rs | 76 +---- .../src/music_renderer/openhome_renderer.rs | 110 +----- .../src/music_renderer/upnp_renderer.rs | 79 +---- pmocontrol/src/queue/backend.rs | 83 ++++- pmocontrol/src/queue/mod.rs | 2 +- version.txt | 2 +- 12 files changed, 474 insertions(+), 403 deletions(-) create mode 100644 Blackboard/Todo/refactoring_pmocontrol.md diff --git a/Blackboard/Todo/refactoring_pmocontrol.md b/Blackboard/Todo/refactoring_pmocontrol.md new file mode 100644 index 00000000..70bd7edf --- /dev/null +++ b/Blackboard/Todo/refactoring_pmocontrol.md @@ -0,0 +1,323 @@ +# Refactoring Plan - Crate `pmocontrol` + +## Contexte + +La crate `pmocontrol` implémente un control point UPnP multiprotocole pour contrôler des renderers audio (UPnP/DLNA, OpenHome, LinkPlay, Arylic TCP, Chromecast). Une refactorisation récente avait pour objectif de monter la logique vers les couches abstraites, mais des duplications et des problèmes de conception subsistent. + +--- + +## Structure analysée + +- `music_renderer/` : Implémentations concrètes + façade `MusicRenderer` +- `queue/` : Gestion abstraite et concrète des files de lecture +- `discovery/` : Découverte SSDP et gestion des appareils +- `upnp_clients/` : Clients SOAP pour services UPnP +- `control_point.rs` : Point de contrôle principal + +Backends : `UpnpRenderer`, `OpenHomeRenderer`, `LinkPlayRenderer`, `ArylicTcpRenderer`, `ChromecastRenderer`, `HybridUpnpArylicRenderer` + +--- + +## CATÉGORIE P0 : BUGS LOGIQUES (à corriger immédiatement) + +### BUG-1 : `sync_queue` dans UpnpRenderer ignore le cancel_token + +**Fichier :** `src/music_renderer/upnp_renderer.rs` (lignes ~354-364) + +**Description :** Le paramètre `cancel_token` est reçu comme `_cancel_token` (ignoré) et remplacé par un `Arc::new(AtomicBool::new(false))` fraîchement créé. Les demandes d'annulation de synchronisation de queue sont silencieusement ignorées pour le backend UPnP. + +**Correction :** +```rust +fn sync_queue( + &mut self, + items: Vec, + cancel_token: &Arc, // utiliser le param, pas _cancel_token + on_ready: Option>, +) -> Result<(), ControlPointError> { + self.queue + .lock() + .unwrap() + .sync_queue(items, cancel_token, on_ready) // passer le vrai token +} +``` + +**Tâche :** Vérifier également les autres backends (OpenHome, LinkPlay, Arylic, Chromecast) s'ils propagent correctement le cancel_token. + +--- + +## CATÉGORIE P1 : DUPLICATIONS MAJEURES (à traiter en priorité) + +### DUP-1 : Implémentation de `QueueBackend` répétée dans les 5+ renderers + +**Fichiers :** +- `src/music_renderer/upnp_renderer.rs` (~310-381) +- `src/music_renderer/arylic_tcp.rs` (~368-450) +- `src/music_renderer/linkplay_renderer.rs` (~246-330) +- `src/music_renderer/chromecast_renderer.rs` (~866+) +- `src/music_renderer/openhome_renderer.rs` (~629+) + +**Description :** Chaque renderer implémente `QueueBackend` de manière identique : chaque méthode verrouille `self.queue` et délègue à la file sous-jacente. ~150+ lignes de boilerplate. + +**Approche recommandée — Trait délégateur :** +```rust +// Dans queue/mod.rs ou music_renderer/mod.rs +pub trait HasQueue { + fn queue(&self) -> &Arc>; +} + +// Impl automatique pour QueueBackend si le type implémente HasQueue +impl QueueBackend for T { + fn len(&self) -> Result { + self.queue().lock().unwrap().len() + } + fn track_ids(&self) -> Result, ControlPointError> { + self.queue().lock().unwrap().track_ids() + } + // ... toutes les méthodes déléguantes +} + +// Dans chaque renderer : une seule ligne +impl HasQueue for UpnpRenderer { + fn queue(&self) -> &Arc> { &self.queue } +} +``` + +**Tâche :** Définir le trait `HasQueue`, implémenter `QueueBackend for T where T: HasQueue`, supprimer les implémentations manuelles dans chaque renderer. + +--- + +### DUP-2 : Logique commune de `play_from_queue` dupliquée dans 4+ renderers + +**Fichiers :** +- `src/music_renderer/upnp_renderer.rs` (~184-256) +- `src/music_renderer/linkplay_renderer.rs` (~189-212) +- `src/music_renderer/arylic_tcp.rs` (~311-334) +- `src/music_renderer/openhome_renderer.rs` (~partie similaire) + +**Description :** Les 10-12 premières lignes de `play_from_queue` sont identiques dans tous les renderers : verrouillage de queue, gestion de l'index courant, fallback sur index 0 si non défini, récupération de l'item. Seule la partie terminale (play effectif sur le backend) diffère. + +**Approche recommandée — Méthode par défaut dans un trait :** +```rust +pub trait QueueTransportControl: HasQueue + HasContinuousStream { + // Primitive spécifique au backend + fn play_item(&self, item: &PlaybackItem) -> Result<(), ControlPointError>; + + // Implémentation commune par défaut + fn play_from_queue(&self) -> Result<(), ControlPointError> { + let mut queue = self.queue().lock().unwrap(); + let current_index = match queue.current_index()? { + Some(idx) => idx, + None => { + if queue.len()? > 0 { + queue.set_index(Some(0))?; + 0 + } else { + return Err(ControlPointError::QueueError("Queue is empty".into())); + } + } + }; + let item = queue.get_item(current_index)? + .ok_or_else(|| ControlPointError::QueueError("Current item not found".into()))?; + drop(queue); + + let is_stream = is_continuous_stream_url(&item.uri); + *self.continuous_stream().lock().unwrap() = is_stream; + self.play_item(&item) + } +} +``` + +**Tâche :** Créer `QueueTransportControl` avec une méthode par défaut, implémenter `play_item` dans chaque renderer, supprimer la logique commune dupliquée. + +--- + +### DUP-3 : Initialisation redondante des champs partagés dans tous les renderers + +**Fichiers :** Constructeurs dans tous les fichiers renderer + +**Description :** Chaque renderer répète la même construction : +```rust +let queue = Arc::new(Mutex::new(MusicQueue::from_renderer_info(info)?)); +// ... +continuous_stream: Arc::new(Mutex::new(false)), +``` + +**Approche recommandée :** +```rust +pub struct SharedRendererState { + pub queue: Arc>, + pub continuous_stream: Arc>, +} + +impl SharedRendererState { + pub fn from_renderer_info(info: &RendererInfo) -> Result { + Ok(Self { + queue: Arc::new(Mutex::new(MusicQueue::from_renderer_info(info)?)), + continuous_stream: Arc::new(Mutex::new(false)), + }) + } +} +``` + +**Tâche :** Créer `SharedRendererState`, l'utiliser dans tous les constructeurs de renderers. + +--- + +### DUP-4 : `parse_didl_duration` implémentée deux fois différemment + +**Fichiers :** +- `src/music_renderer/upnp_renderer.rs` (~383-420) : parsing manuel par string search (fragile) +- `src/music_renderer/musicrenderer.rs` (~2089-2117) : via parser DIDL-Lite structuré (robuste) + +**Description :** Deux implémentations divergentes. L'une risque de mal parser du DIDL là où l'autre réussit. + +**Tâche :** Conserver uniquement la version via `DIDLLite::parse`, l'exporter depuis `music_renderer/mod.rs`, supprimer la version par string search dans `upnp_renderer.rs`. + +--- + +## CATÉGORIE P2 : ALGORITHMES COMPLEXES ET ABSTRACTIONS MAL PLACÉES + +### ALGO-1 : `schedule_sync` dans `music_queue.rs` — logique intriquée + +**Fichier :** `src/queue/music_queue.rs` (~97-200) + +**Description :** La méthode crée un thread worker avec : +- Des `AtomicBool` pour synchronisation (sync_in_progress, sync_pending, sync_cancel_token) +- Une boucle infinie interne qui re-tente si un nouveau job arrive +- Un Guard RAII basé sur `Drop` pour le cleanup +- Des closures capturées mêlant synchronisation et logique métier + +Difficile à tester, à observer de l'extérieur, pas de timeout. + +**Tâche :** +1. Extraire la logique du worker dans une fonction `sync_worker_loop` avec signature claire +2. Documenter le protocole de synchronisation avec les AtomicBool +3. Ajouter une stratégie de timeout ou de sortie en cas de blocage + +--- + +### ALGO-2 : Enum dispatch sprawl dans `MusicRendererBackend` + +**Fichier :** `src/music_renderer/musicrenderer.rs` (~2134-2495) + +**Description :** L'enum a 6 variantes. Chaque trait implémenté pour l'enum (`TransportControl`, `PlaybackStatus`, `PlaybackPosition`, `RendererBackend`, `QueueBackend`, etc.) contient un `match` sur les 6 variantes. Estimation : 200+ lignes de boilerplate purement mécanique. Ajouter une 7e variante requiert des mises à jour dans 25+ endroits. + +**Approche recommandée — Macro de dispatch :** +```rust +macro_rules! dispatch { + ($self:expr, $method:ident($($arg:expr),*)) => { + match $self { + MusicRendererBackend::Upnp(b) => b.$method($($arg),*), + MusicRendererBackend::OpenHome(b) => b.$method($($arg),*), + MusicRendererBackend::LinkPlay(b) => b.$method($($arg),*), + MusicRendererBackend::ArylicTcp(b) => b.$method($($arg),*), + MusicRendererBackend::Chromecast(b) => b.$method($($arg),*), + MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.$method($($arg),*), + } + } +} + +impl TransportControl for MusicRendererBackend { + fn play_uri(&self, uri: &str, meta: &str) -> Result<(), ControlPointError> { + dispatch!(self, play_uri(uri, meta)) + } + // ... +} +``` + +**Tâche :** Définir la macro `dispatch!`, remplacer les match statements redondants, valider les cas où HybridUpnpArylic a une logique spéciale. + +--- + +### ALGO-3 : Logique de protection des durées de streams dupliquée dans 3 endroits + +**Fichiers :** +- `src/queue/interne.rs` (~74-126) : `protect_stream_durations` +- `src/queue/openhome.rs` (~400+) : logique similaire pour playlists OpenHome +- `src/music_renderer/musicrenderer.rs` (~488-537) : dans `poll_and_emit_changes` + +**Description :** La logique "refuser la diminution de durée pour un stream continu" est réimplémentée trois fois. Si la définition de "diminution acceptable" change, il faut modifier 3 fichiers. + +**Tâche :** Créer `music_renderer/stream_utils.rs` (ou équivalent) avec une fonction `protect_stream_duration(old, new, is_stream) -> Option` et l'utiliser dans les 3 endroits. + +--- + +### ALGO-4 : Détection de flux continu fragmentée + +**Fichiers :** `stream_detection.rs`, `musicrenderer.rs`, `queue/interne.rs`, `queue/openhome.rs` + +**Description :** La détection "est-ce un stream continu?" passe par plusieurs chemins non unifiés : +1. `TrackMetadata::is_continuous_stream` +2. Appel `is_continuous_stream_url(uri)` (réseau) +3. Absence de durée dans les métadonnées + +Un stream peut être marqué continu dans une couche mais pas l'autre. + +**Tâche :** Créer une fonction canonique unique : +```rust +pub fn is_continuous_stream(metadata: Option<&TrackMetadata>, uri: &str) -> bool { + metadata.map(|m| m.is_continuous_stream).unwrap_or(false) + || is_continuous_stream_url(uri) +} +``` +Faire passer tous les codepaths par cette fonction. + +--- + +## CATÉGORIE P3 : BONNES PRATIQUES (amélioration continue) + +### BP-1 : `.unwrap()` sur mutex locks (>50 occurrences) + +**Problème :** Si un mutex est empoisonné (panique dans une autre tâche), `.unwrap()` propage la panique. Aucun code ne gère ce cas. + +**Tâche :** Remplacer `.unwrap()` par `.expect("message contextuel")` à court terme. À long terme, envisager `parking_lot::Mutex` (pas de concept de poison). + +--- + +### BP-2 : Absence de gestion d'erreur dans les threads watcher et sync + +**Fichiers :** `musicrenderer.rs` (watcher_loop), `music_queue.rs` (schedule_sync) + +**Tâche :** Ajouter `error!` logs dans les threads et décider explicitement de la politique de redémarrage (continuer vs arrêter). + +--- + +### BP-3 : Champs `pub` au lieu de `pub(crate)` dans `PlaylistBinding` + +**Fichier :** `src/music_renderer/musicrenderer.rs` (struct `PlaylistBinding`) + +**Tâche :** Rendre les champs `pub` → `pub(crate)` ou privés avec accesseurs. + +--- + +### BP-4 : Documentation manquante sur les contrats des traits + +**Fichiers :** `src/music_renderer/capabilities.rs`, `src/queue/backend.rs` + +**Tâche :** Ajouter des doc-comments sur les traits clés (`TransportControl`, `PlaybackStatus`, `QueueBackend`) décrivant les invariants, les pré/post-conditions, et le comportement attendu. + +--- + +## PLAN D'EXÉCUTION + +### Phase 1 — Bugs (immédiat) +- [ ] **BUG-1** : Corriger le cancel_token ignoré dans `upnp_renderer.rs::sync_queue` +- [ ] Vérifier les autres renderers pour le même bug + +### Phase 2 — Éliminer les duplications majeures (1-2 semaines) +- [ ] **DUP-1** : Trait `HasQueue` + impl automatique de `QueueBackend` +- [ ] **DUP-4** : Unifier `parse_didl_duration` sur la version DIDL-Lite +- [ ] **DUP-3** : Créer `SharedRendererState` pour l'init commune +- [ ] **DUP-2** : Trait `QueueTransportControl` avec `play_from_queue` par défaut + +### Phase 3 — Simplifier les algorithmes (2-4 semaines) +- [ ] **ALGO-2** : Macro `dispatch!` pour `MusicRendererBackend` +- [ ] **ALGO-3** : Centraliser la protection des durées de stream +- [ ] **ALGO-4** : Unifier la détection de flux continu +- [ ] **ALGO-1** : Refactoriser `schedule_sync` (extraire `sync_worker_loop`) + +### Phase 4 — Qualité continue +- [ ] **BP-1** : Remplacer les `.unwrap()` critiques +- [ ] **BP-2** : Gestion d'erreur dans les threads +- [ ] **BP-3** : Visibilité des champs `PlaylistBinding` +- [ ] **BP-4** : Documentation des traits diff --git a/Cargo.lock b/Cargo.lock index f1f8e3f9..fd6c093e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4,7 +4,7 @@ version = 4 [[package]] name = "PMOMusic" -version = "0.3.47" +version = "0.3.48" dependencies = [ "axum 0.8.7", "console-subscriber", diff --git a/PMOMusic/Cargo.toml b/PMOMusic/Cargo.toml index 007bf163..32f6d26a 100644 --- a/PMOMusic/Cargo.toml +++ b/PMOMusic/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "PMOMusic" -version = "0.3.47" +version = "0.3.48" edition = "2024" [dependencies] diff --git a/pmocontrol/src/music_renderer/arylic_tcp.rs b/pmocontrol/src/music_renderer/arylic_tcp.rs index 82752bfa..29fdf180 100644 --- a/pmocontrol/src/music_renderer/arylic_tcp.rs +++ b/pmocontrol/src/music_renderer/arylic_tcp.rs @@ -18,8 +18,7 @@ use crate::music_renderer::capabilities::{ use crate::music_renderer::musicrenderer::MusicRendererBackend; use crate::music_renderer::time_utils::{format_hhmmss, ms_to_seconds, parse_hhmmss_strict}; use crate::music_renderer::RendererFromMediaRendererInfo; -use crate::queue::MusicQueue; -use crate::queue::{EnqueueMode, PlaybackItem, QueueBackend, QueueSnapshot}; +use crate::queue::{EnqueueMode, HasQueue, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; use crate::DeviceIdentity; /// Raw response from Arylic MCU+PINFGET command @@ -365,76 +364,9 @@ impl QueueTransportControl for ArylicTcpRenderer { } } -impl QueueBackend for ArylicTcpRenderer { - fn len(&self) -> Result { - self.queue.lock().unwrap().len() - } - - fn track_ids(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().track_ids() - } - - fn id_to_position(&self, id: u32) -> Result { - self.queue.lock().unwrap().id_to_position(id) - } - - fn position_to_id(&self, id: usize) -> Result { - self.queue.lock().unwrap().position_to_id(id) - } - - fn current_track(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().current_track() - } - - fn current_index(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().current_index() - } - - fn queue_snapshot(&self) -> Result { - self.queue.lock().unwrap().queue_snapshot() - } - - fn set_index(&mut self, index: Option) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().set_index(index) - } - - fn replace_queue( - &mut self, - items: Vec, - current_index: Option, - ) -> Result<(), ControlPointError> { - self.queue - .lock() - .unwrap() - .replace_queue(items, current_index) - } - - fn sync_queue( - &mut self, - items: Vec, - _cancel_token: &Arc, - on_ready: Option>, - ) -> Result<(), ControlPointError> { - self.queue - .lock() - .unwrap() - .sync_queue(items, &Arc::new(AtomicBool::new(false)), on_ready) - } - - fn get_item(&self, index: usize) -> Result, ControlPointError> { - self.queue.lock().unwrap().get_item(index) - } - - fn replace_item(&mut self, index: usize, item: PlaybackItem) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().replace_item(index, item) - } - - fn enqueue_items( - &mut self, - items: Vec, - mode: EnqueueMode, - ) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().enqueue_items(items, mode) +impl HasQueue for ArylicTcpRenderer { + fn queue(&self) -> &Arc> { + &self.queue } } diff --git a/pmocontrol/src/music_renderer/capabilities.rs b/pmocontrol/src/music_renderer/capabilities.rs index 1f4b049e..1e760681 100644 --- a/pmocontrol/src/music_renderer/capabilities.rs +++ b/pmocontrol/src/music_renderer/capabilities.rs @@ -1,9 +1,13 @@ // pmocontrol/src/capabilities.rs -use anyhow::Result; use std::sync::{Arc, Mutex}; -use crate::queue::MusicQueue; -use crate::{errors::ControlPointError, model::PlaybackState}; +use crate::queue::{HasQueue, MusicQueue}; +use crate::{errors::ControlPointError, model::PlaybackState, PlaybackItem}; + +/// Trait for types that track whether they're playing a continuous stream. +pub trait HasContinuousStream { + fn continuous_stream(&self) -> &Arc>; +} /// Backend-specific operations for renderers. /// @@ -18,7 +22,39 @@ pub trait RendererBackend { /// These operations combine queue management with transport control, /// allowing navigation (next/previous) and track selection from the queue. #[allow(dead_code)] -pub trait QueueTransportControl { +pub trait QueueTransportControl: HasQueue + HasContinuousStream { + /// Play a specific item from the queue (backend-specific implementation). + fn play_item(&self, item: &PlaybackItem) -> Result<(), ControlPointError>; + + /// Play from the queue at the current index (or initialize to 0 if not set). + /// This is the default implementation that handles queue navigation. + fn play_from_queue(&self) -> Result<(), ControlPointError> { + let mut queue = self.queue().lock().unwrap(); + + let current_index = match queue.current_index()? { + Some(idx) => idx, + None => { + if queue.len()? > 0 { + queue.set_index(Some(0))?; + 0 + } else { + return Err(ControlPointError::QueueError("Queue is empty".into())); + } + } + }; + + let item = queue + .get_item(current_index)? + .ok_or_else(|| ControlPointError::QueueError("Current item not found".into()))?; + + drop(queue); + + let is_stream = crate::music_renderer::is_continuous_stream_url(&item.uri); + *self.continuous_stream().lock().unwrap() = is_stream; + + self.play_item(&item) + } + /// Play the next track from the queue. fn play_next(&self) -> Result<(), ControlPointError>; @@ -26,9 +62,6 @@ pub trait QueueTransportControl { #[allow(dead_code)] fn play_previous(&self) -> Result<(), ControlPointError>; - /// Play from the queue at the current index (or initialize to 0 if not set). - fn play_from_queue(&self) -> Result<(), ControlPointError>; - /// Play from a specific index in the queue. fn play_from_index(&self, index: usize) -> Result<(), ControlPointError>; } diff --git a/pmocontrol/src/music_renderer/chromecast_renderer.rs b/pmocontrol/src/music_renderer/chromecast_renderer.rs index 9d6fa61f..39c1f5bd 100644 --- a/pmocontrol/src/music_renderer/chromecast_renderer.rs +++ b/pmocontrol/src/music_renderer/chromecast_renderer.rs @@ -29,7 +29,7 @@ use crate::music_renderer::capabilities::{ use crate::music_renderer::musicrenderer::MusicRendererBackend; use crate::music_renderer::time_utils::{format_hhmmss_f64, parse_hhmmss_strict}; use crate::music_renderer::RendererFromMediaRendererInfo; -use crate::queue::{EnqueueMode, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; +use crate::queue::{EnqueueMode, HasQueue, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; use crate::DeviceIdentity; use rust_cast::{ @@ -863,75 +863,8 @@ impl QueueTransportControl for ChromecastRenderer { } } -impl QueueBackend for ChromecastRenderer { - fn len(&self) -> Result { - self.queue.lock().unwrap().len() - } - - fn track_ids(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().track_ids() - } - - fn id_to_position(&self, id: u32) -> Result { - self.queue.lock().unwrap().id_to_position(id) - } - - fn position_to_id(&self, id: usize) -> Result { - self.queue.lock().unwrap().position_to_id(id) - } - - fn current_track(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().current_track() - } - - fn current_index(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().current_index() - } - - fn queue_snapshot(&self) -> Result { - self.queue.lock().unwrap().queue_snapshot() - } - - fn set_index(&mut self, index: Option) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().set_index(index) - } - - fn replace_queue( - &mut self, - items: Vec, - current_index: Option, - ) -> Result<(), ControlPointError> { - self.queue - .lock() - .unwrap() - .replace_queue(items, current_index) - } - - fn sync_queue( - &mut self, - items: Vec, - _cancel_token: &Arc, - on_ready: Option>, - ) -> Result<(), ControlPointError> { - self.queue - .lock() - .unwrap() - .sync_queue(items, &Arc::new(AtomicBool::new(false)), on_ready) - } - - fn get_item(&self, index: usize) -> Result, ControlPointError> { - self.queue.lock().unwrap().get_item(index) - } - - fn replace_item(&mut self, index: usize, item: PlaybackItem) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().replace_item(index, item) - } - - fn enqueue_items( - &mut self, - items: Vec, - mode: EnqueueMode, - ) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().enqueue_items(items, mode) +impl HasQueue for ChromecastRenderer { + fn queue(&self) -> &Arc> { + &self.queue } } diff --git a/pmocontrol/src/music_renderer/linkplay_renderer.rs b/pmocontrol/src/music_renderer/linkplay_renderer.rs index 689cac7e..4ce747b2 100644 --- a/pmocontrol/src/music_renderer/linkplay_renderer.rs +++ b/pmocontrol/src/music_renderer/linkplay_renderer.rs @@ -16,8 +16,7 @@ use crate::music_renderer::capabilities::{ use crate::music_renderer::musicrenderer::MusicRendererBackend; use crate::music_renderer::time_utils::parse_hhmmss_strict; use crate::music_renderer::RendererFromMediaRendererInfo; -use crate::queue::MusicQueue; -use crate::queue::{EnqueueMode, PlaybackItem, QueueBackend, QueueSnapshot}; +use crate::queue::{EnqueueMode, HasQueue, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; use crate::DeviceIdentity; const DEFAULT_HTTP_TIMEOUT_SECS: u64 = 3; @@ -243,75 +242,8 @@ impl QueueTransportControl for LinkPlayRenderer { } } -impl QueueBackend for LinkPlayRenderer { - fn len(&self) -> Result { - self.queue.lock().unwrap().len() - } - - fn track_ids(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().track_ids() - } - - fn id_to_position(&self, id: u32) -> Result { - self.queue.lock().unwrap().id_to_position(id) - } - - fn position_to_id(&self, id: usize) -> Result { - self.queue.lock().unwrap().position_to_id(id) - } - - fn current_track(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().current_track() - } - - fn current_index(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().current_index() - } - - fn queue_snapshot(&self) -> Result { - self.queue.lock().unwrap().queue_snapshot() - } - - fn set_index(&mut self, index: Option) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().set_index(index) - } - - fn replace_queue( - &mut self, - items: Vec, - current_index: Option, - ) -> Result<(), ControlPointError> { - self.queue - .lock() - .unwrap() - .replace_queue(items, current_index) - } - - fn sync_queue( - &mut self, - items: Vec, - _cancel_token: &Arc, - on_ready: Option>, - ) -> Result<(), ControlPointError> { - self.queue - .lock() - .unwrap() - .sync_queue(items, &Arc::new(AtomicBool::new(false)), on_ready) - } - - fn get_item(&self, index: usize) -> Result, ControlPointError> { - self.queue.lock().unwrap().get_item(index) - } - - fn replace_item(&mut self, index: usize, item: PlaybackItem) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().replace_item(index, item) - } - - fn enqueue_items( - &mut self, - items: Vec, - mode: EnqueueMode, - ) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().enqueue_items(items, mode) +impl HasQueue for LinkPlayRenderer { + fn queue(&self) -> &Arc> { + &self.queue } } diff --git a/pmocontrol/src/music_renderer/openhome_renderer.rs b/pmocontrol/src/music_renderer/openhome_renderer.rs index 07e06dc9..d0dde366 100644 --- a/pmocontrol/src/music_renderer/openhome_renderer.rs +++ b/pmocontrol/src/music_renderer/openhome_renderer.rs @@ -16,7 +16,7 @@ use crate::music_renderer::openhome::{ build_time_client, build_volume_client, }; use crate::music_renderer::RendererFromMediaRendererInfo; -use crate::queue::{EnqueueMode, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; +use crate::queue::{EnqueueMode, HasQueue, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; use crate::upnp_clients::{ OhInfoClient, OhPlaylistClient, OhProductClient, OhRadioClient, OhTimeClient, OhVolumeClient, OPENHOME_PLAYLIST_HEAD_ID, @@ -626,90 +626,31 @@ impl QueueTransportControl for OpenHomeRenderer { } } -impl QueueBackend for OpenHomeRenderer { - fn len(&self) -> Result { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .len() +impl HasQueue for OpenHomeRenderer { + fn queue(&self) -> &Arc> { + &self.queue } +} - fn track_ids(&self) -> Result, ControlPointError> { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .track_ids() - } - - fn id_to_position(&self, id: u32) -> Result { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .id_to_position(id) - } - - fn position_to_id(&self, id: usize) -> Result { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .position_to_id(id) - } - - fn current_track(&self) -> Result, ControlPointError> { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .current_track() - } - - fn current_index(&self) -> Result, ControlPointError> { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .current_index() - } - - fn queue_snapshot(&self) -> Result { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .queue_snapshot() - } - - fn set_index(&mut self, index: Option) -> Result<(), ControlPointError> { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .set_index(index) - } - - fn replace_queue( +impl OpenHomeRenderer { + pub fn replace_queue_with_background( &mut self, items: Vec, current_index: Option, ) -> Result<(), ControlPointError> { - // ✅ CORRECTION BUG PRODUCTION: On ne charge PAS toutes les métadonnées - // dans le thread principal. OpenHome sur 1000 titres inondait la base SQLite - // et bloquait TOUS les autres threads (mutex >500ms). - // - // On fait juste l'insertion minimaliste maintenant. Le préchargement - // des métadonnées est délégué à un thread background. self.queue .lock() .map_err(|_| ControlPointError::QueueError("Mutex poisoned".into()))? .replace_queue(items, current_index)?; - // Background worker: charge les métadonnées petit à petit sans bloquer personne let queue = self.queue.clone(); std::thread::spawn(move || { debug!("🔄 OpenHome: préchargement métadonnées queue en background"); if let Ok(mut queue) = queue.lock() { - // On ne fait que les 10 prochains titres maintenant, le reste on s'en fout if let Ok(Some(idx)) = queue.current_index() { let end = std::cmp::min(idx + 10, queue.len().unwrap_or(0)); for i in idx..end { let _ = queue.get_item(i); - // Petit délai pour ne pas noyer la base de données std::thread::sleep(std::time::Duration::from_millis(5)); } } @@ -719,41 +660,4 @@ impl QueueBackend for OpenHomeRenderer { Ok(()) } - - fn sync_queue( - &mut self, - items: Vec, - cancel_token: &Arc, - on_ready: Option>, - ) -> Result<(), ControlPointError> { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .sync_queue(items, cancel_token, on_ready) - } - - fn get_item(&self, index: usize) -> Result, ControlPointError> { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .get_item(index) - } - - fn replace_item(&mut self, index: usize, item: PlaybackItem) -> Result<(), ControlPointError> { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .replace_item(index, item) - } - - fn enqueue_items( - &mut self, - items: Vec, - mode: EnqueueMode, - ) -> Result<(), ControlPointError> { - self.queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? - .enqueue_items(items, mode) - } } diff --git a/pmocontrol/src/music_renderer/upnp_renderer.rs b/pmocontrol/src/music_renderer/upnp_renderer.rs index ad57b3cc..bf4098e8 100644 --- a/pmocontrol/src/music_renderer/upnp_renderer.rs +++ b/pmocontrol/src/music_renderer/upnp_renderer.rs @@ -3,12 +3,12 @@ use std::sync::{atomic::AtomicBool, Arc, Mutex}; use crate::errors::ControlPointError; use crate::model::PlaybackState; use crate::music_renderer::capabilities::{ - PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, QueueTransportControl, RendererBackend, - TransportControl, VolumeControl, + HasContinuousStream, PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, + QueueTransportControl, RendererBackend, TransportControl, VolumeControl, }; use crate::music_renderer::musicrenderer::{build_didl_lite_metadata, MusicRendererBackend}; use crate::music_renderer::RendererFromMediaRendererInfo; -use crate::queue::{EnqueueMode, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; +use crate::queue::{EnqueueMode, HasQueue, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; use crate::upnp_clients::{ AvTransportClient, ConnectionInfo, ConnectionManagerClient, PositionInfo, ProtocolInfo, RenderingControlClient, @@ -307,76 +307,9 @@ impl QueueTransportControl for UpnpRenderer { } } -impl QueueBackend for UpnpRenderer { - fn len(&self) -> Result { - self.queue.lock().unwrap().len() - } - - fn track_ids(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().track_ids() - } - - fn id_to_position(&self, id: u32) -> Result { - self.queue.lock().unwrap().id_to_position(id) - } - - fn position_to_id(&self, id: usize) -> Result { - self.queue.lock().unwrap().position_to_id(id) - } - - fn current_track(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().current_track() - } - - fn current_index(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().current_index() - } - - fn queue_snapshot(&self) -> Result { - self.queue.lock().unwrap().queue_snapshot() - } - - fn set_index(&mut self, index: Option) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().set_index(index) - } - - fn replace_queue( - &mut self, - items: Vec, - current_index: Option, - ) -> Result<(), ControlPointError> { - self.queue - .lock() - .unwrap() - .replace_queue(items, current_index) - } - - fn sync_queue( - &mut self, - items: Vec, - _cancel_token: &Arc, - on_ready: Option>, - ) -> Result<(), ControlPointError> { - self.queue - .lock() - .unwrap() - .sync_queue(items, &Arc::new(AtomicBool::new(false)), on_ready) - } - - fn get_item(&self, index: usize) -> Result, ControlPointError> { - self.queue.lock().unwrap().get_item(index) - } - - fn replace_item(&mut self, index: usize, item: PlaybackItem) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().replace_item(index, item) - } - - fn enqueue_items( - &mut self, - items: Vec, - mode: EnqueueMode, - ) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().enqueue_items(items, mode) +impl HasQueue for UpnpRenderer { + fn queue(&self) -> &Arc> { + &self.queue } } diff --git a/pmocontrol/src/queue/backend.rs b/pmocontrol/src/queue/backend.rs index c70d343f..2565f0aa 100644 --- a/pmocontrol/src/queue/backend.rs +++ b/pmocontrol/src/queue/backend.rs @@ -28,8 +28,89 @@ //! - This identity is used by the sync helpers to preserve the current //! track across queue rebuilds when the MediaServer content changes. +use crate::queue::MusicQueue; use crate::{errors::ControlPointError, PlaybackItem, QueueSnapshot}; -use std::sync::{atomic::AtomicBool, Arc}; +use std::sync::{atomic::AtomicBool, Arc, Mutex}; + +/// Trait for types that have aMusicQueue. +pub trait HasQueue { + fn queue(&self) -> &Arc>; +} + +/// Blanket implementation of QueueBackend for types that have a queue. +/// All methods simply delegate to the underlying MusicQueue. +impl QueueBackend for T { + fn len(&self) -> Result { + self.queue().lock().unwrap().len() + } + + fn track_ids(&self) -> Result, ControlPointError> { + self.queue().lock().unwrap().track_ids() + } + + fn id_to_position(&self, id: u32) -> Result { + self.queue().lock().unwrap().id_to_position(id) + } + + fn position_to_id(&self, id: usize) -> Result { + self.queue().lock().unwrap().position_to_id(id) + } + + fn current_track(&self) -> Result, ControlPointError> { + self.queue().lock().unwrap().current_track() + } + + fn current_index(&self) -> Result, ControlPointError> { + self.queue().lock().unwrap().current_index() + } + + fn queue_snapshot(&self) -> Result { + self.queue().lock().unwrap().queue_snapshot() + } + + fn set_index(&mut self, index: Option) -> Result<(), ControlPointError> { + self.queue().lock().unwrap().set_index(index) + } + + fn replace_queue( + &mut self, + items: Vec, + current_index: Option, + ) -> Result<(), ControlPointError> { + self.queue() + .lock() + .unwrap() + .replace_queue(items, current_index) + } + + fn sync_queue( + &mut self, + items: Vec, + cancel_token: &Arc, + on_ready: Option>, + ) -> Result<(), ControlPointError> { + self.queue() + .lock() + .unwrap() + .sync_queue(items, cancel_token, on_ready) + } + + fn get_item(&self, index: usize) -> Result, ControlPointError> { + self.queue().lock().unwrap().get_item(index) + } + + fn replace_item(&mut self, index: usize, item: PlaybackItem) -> Result<(), ControlPointError> { + self.queue().lock().unwrap().replace_item(index, item) + } + + fn enqueue_items( + &mut self, + items: Vec, + mode: EnqueueMode, + ) -> Result<(), ControlPointError> { + self.queue().lock().unwrap().enqueue_items(items, mode) + } +} /// High-level enqueue mode. /// diff --git a/pmocontrol/src/queue/mod.rs b/pmocontrol/src/queue/mod.rs index 2dfd125c..99d77bf0 100644 --- a/pmocontrol/src/queue/mod.rs +++ b/pmocontrol/src/queue/mod.rs @@ -6,7 +6,7 @@ mod snapshot; use std::sync::{Arc, Mutex}; -pub use backend::{EnqueueMode, QueueBackend}; +pub use backend::{EnqueueMode, HasQueue, QueueBackend}; pub use music_queue::{MusicQueue, SyncScheduleOutcome}; pub use snapshot::{PlaybackItem, QueueSnapshot}; diff --git a/version.txt b/version.txt index c54101be..2fb885a2 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -0.3.47 +0.3.48 -- 2.49.1 From 4b793cec59aa64222007297e4b528ac9a5c5ddcb Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Sun, 12 Apr 2026 19:20:43 +0200 Subject: [PATCH 2/5] refactor: replace unwrap() with expect for mutex locks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace all `lock().unwrap()` calls on Mutex guards with explicit error messages using `.expect("... mutex poisoned")`. This improves robustness by providing clear diagnostics when a thread panics while holding the lock, preventing silent failures. Affected modules: events.rs (Renderer/MediaServer), all renderer backends, queue backend/implementation files. Also adds documentation to marker traits (HasQueue、 HasContinuousStream) and clarifies error handling in watcher loop with panic catching. --- pmocontrol/src/music_renderer/arylic_tcp.rs | 28 ++- pmocontrol/src/music_renderer/capabilities.rs | 15 +- .../src/music_renderer/chromecast_renderer.rs | 28 ++- .../src/music_renderer/linkplay_renderer.rs | 28 ++- pmocontrol/src/music_renderer/mod.rs | 7 +- .../src/music_renderer/musicrenderer.rs | 198 +++--------------- .../src/music_renderer/openhome_renderer.rs | 23 +- .../src/music_renderer/stream_detection.rs | 6 +- .../src/music_renderer/upnp_renderer.rs | 74 ++++--- pmocontrol/src/queue/backend.rs | 6 +- pmocontrol/src/queue/mod.rs | 2 +- 11 files changed, 152 insertions(+), 263 deletions(-) diff --git a/pmocontrol/src/music_renderer/arylic_tcp.rs b/pmocontrol/src/music_renderer/arylic_tcp.rs index 29fdf180..a9167029 100644 --- a/pmocontrol/src/music_renderer/arylic_tcp.rs +++ b/pmocontrol/src/music_renderer/arylic_tcp.rs @@ -12,13 +12,14 @@ use crate::errors::ControlPointError; use crate::linkplay_client::extract_linkplay_host; use crate::model::{PlaybackState, RendererInfo}; use crate::music_renderer::capabilities::{ - PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, QueueTransportControl, RendererBackend, - TransportControl, VolumeControl, + HasContinuousStream, PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, + QueueTransportControl, TransportControl, VolumeControl, }; use crate::music_renderer::musicrenderer::MusicRendererBackend; use crate::music_renderer::time_utils::{format_hhmmss, ms_to_seconds, parse_hhmmss_strict}; +use crate::music_renderer::HasQueue; use crate::music_renderer::RendererFromMediaRendererInfo; -use crate::queue::{EnqueueMode, HasQueue, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; +use crate::queue::{EnqueueMode, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; use crate::DeviceIdentity; /// Raw response from Arylic MCU+PINFGET command @@ -300,13 +301,12 @@ impl PlaybackPosition for ArylicTcpRenderer { } } -impl RendererBackend for ArylicTcpRenderer { - fn queue(&self) -> &Arc> { - &self.queue - } -} impl QueueTransportControl for ArylicTcpRenderer { + fn play_item(&self, item: &PlaybackItem) -> Result<(), ControlPointError> { + self.play_uri(&item.uri, "") + } + fn play_from_queue(&self) -> Result<(), ControlPointError> { let mut queue = self.queue.lock().unwrap(); @@ -326,10 +326,12 @@ impl QueueTransportControl for ArylicTcpRenderer { .get_item(current_index)? .ok_or_else(|| ControlPointError::QueueError("Current item not found".into()))?; - let uri = item.uri.clone(); drop(queue); - self.play_uri(&uri, "") + let is_stream = crate::music_renderer::is_continuous_stream_url(&item.uri); + *self.continuous_stream.lock().unwrap() = is_stream; + + self.play_item(&item) } fn play_next(&self) -> Result<(), ControlPointError> { @@ -370,6 +372,12 @@ impl HasQueue for ArylicTcpRenderer { } } +impl HasContinuousStream for ArylicTcpRenderer { + fn continuous_stream(&self) -> &Arc> { + &self.continuous_stream + } +} + #[derive(Debug)] struct ArylicPlaybackInfo { status_raw: String, diff --git a/pmocontrol/src/music_renderer/capabilities.rs b/pmocontrol/src/music_renderer/capabilities.rs index 1e760681..c164c052 100644 --- a/pmocontrol/src/music_renderer/capabilities.rs +++ b/pmocontrol/src/music_renderer/capabilities.rs @@ -1,22 +1,19 @@ // pmocontrol/src/capabilities.rs use std::sync::{Arc, Mutex}; -use crate::queue::{HasQueue, MusicQueue}; +use crate::queue::{MusicQueue, QueueBackend}; use crate::{errors::ControlPointError, model::PlaybackState, PlaybackItem}; +/// Trait for types that have access to a MusicQueue. +pub trait HasQueue { + fn queue(&self) -> &Arc>; +} + /// Trait for types that track whether they're playing a continuous stream. pub trait HasContinuousStream { fn continuous_stream(&self) -> &Arc>; } -/// Backend-specific operations for renderers. -/// -/// This trait provides access to backend-specific resources like the queue. -pub trait RendererBackend { - /// Returns a reference to the queue associated with this backend. - fn queue(&self) -> &Arc>; -} - /// Queue-aware transport control operations. /// /// These operations combine queue management with transport control, diff --git a/pmocontrol/src/music_renderer/chromecast_renderer.rs b/pmocontrol/src/music_renderer/chromecast_renderer.rs index 39c1f5bd..a0754a52 100644 --- a/pmocontrol/src/music_renderer/chromecast_renderer.rs +++ b/pmocontrol/src/music_renderer/chromecast_renderer.rs @@ -23,13 +23,14 @@ use crate::discovery::chromecast_discovery::{ use crate::errors::ControlPointError; use crate::model::{PlaybackState, RendererInfo}; use crate::music_renderer::capabilities::{ - PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, QueueTransportControl, RendererBackend, - TransportControl, VolumeControl, + HasContinuousStream, PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, + QueueTransportControl, TransportControl, VolumeControl, }; use crate::music_renderer::musicrenderer::MusicRendererBackend; use crate::music_renderer::time_utils::{format_hhmmss_f64, parse_hhmmss_strict}; +use crate::music_renderer::HasQueue; use crate::music_renderer::RendererFromMediaRendererInfo; -use crate::queue::{EnqueueMode, HasQueue, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; +use crate::queue::{EnqueueMode, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; use crate::DeviceIdentity; use rust_cast::{ @@ -799,13 +800,12 @@ impl VolumeControl for ChromecastRenderer { } } -impl RendererBackend for ChromecastRenderer { - fn queue(&self) -> &Arc> { - &self.queue - } -} impl QueueTransportControl for ChromecastRenderer { + fn play_item(&self, item: &PlaybackItem) -> Result<(), ControlPointError> { + self.play_uri(&item.uri, "") + } + fn play_from_queue(&self) -> Result<(), ControlPointError> { let mut queue = self.queue.lock().unwrap(); @@ -825,10 +825,12 @@ impl QueueTransportControl for ChromecastRenderer { .get_item(current_index)? .ok_or_else(|| ControlPointError::QueueError("Current item not found".into()))?; - let uri = item.uri.clone(); drop(queue); - self.play_uri(&uri, "") + let is_stream = crate::music_renderer::is_continuous_stream_url(&item.uri); + *self.continuous_stream.lock().unwrap() = is_stream; + + self.play_item(&item) } fn play_next(&self) -> Result<(), ControlPointError> { @@ -868,3 +870,9 @@ impl HasQueue for ChromecastRenderer { &self.queue } } + +impl HasContinuousStream for ChromecastRenderer { + fn continuous_stream(&self) -> &Arc> { + &self.continuous_stream + } +} diff --git a/pmocontrol/src/music_renderer/linkplay_renderer.rs b/pmocontrol/src/music_renderer/linkplay_renderer.rs index 4ce747b2..134c89f2 100644 --- a/pmocontrol/src/music_renderer/linkplay_renderer.rs +++ b/pmocontrol/src/music_renderer/linkplay_renderer.rs @@ -10,13 +10,14 @@ use crate::linkplay_client::{ }; use crate::model::{PlaybackState, RendererInfo}; use crate::music_renderer::capabilities::{ - PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, QueueTransportControl, RendererBackend, - TransportControl, VolumeControl, + HasContinuousStream, PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, + QueueTransportControl, TransportControl, VolumeControl, }; use crate::music_renderer::musicrenderer::MusicRendererBackend; use crate::music_renderer::time_utils::parse_hhmmss_strict; +use crate::music_renderer::HasQueue; use crate::music_renderer::RendererFromMediaRendererInfo; -use crate::queue::{EnqueueMode, HasQueue, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; +use crate::queue::{EnqueueMode, MusicQueue, PlaybackItem, QueueBackend}; use crate::DeviceIdentity; const DEFAULT_HTTP_TIMEOUT_SECS: u64 = 3; @@ -178,13 +179,12 @@ impl PlaybackPosition for LinkPlayRenderer { } } -impl RendererBackend for LinkPlayRenderer { - fn queue(&self) -> &Arc> { - &self.queue - } -} impl QueueTransportControl for LinkPlayRenderer { + fn play_item(&self, item: &PlaybackItem) -> Result<(), ControlPointError> { + self.play_uri(&item.uri, "") + } + fn play_from_queue(&self) -> Result<(), ControlPointError> { let mut queue = self.queue.lock().unwrap(); @@ -204,10 +204,12 @@ impl QueueTransportControl for LinkPlayRenderer { .get_item(current_index)? .ok_or_else(|| ControlPointError::QueueError("Current item not found".into()))?; - let uri = item.uri.clone(); drop(queue); - self.play_uri(&uri, "") + let is_stream = crate::music_renderer::is_continuous_stream_url(&item.uri); + *self.continuous_stream.lock().unwrap() = is_stream; + + self.play_item(&item) } fn play_next(&self) -> Result<(), ControlPointError> { @@ -247,3 +249,9 @@ impl HasQueue for LinkPlayRenderer { &self.queue } } + +impl HasContinuousStream for LinkPlayRenderer { + fn continuous_stream(&self) -> &Arc> { + &self.continuous_stream + } +} diff --git a/pmocontrol/src/music_renderer/mod.rs b/pmocontrol/src/music_renderer/mod.rs index 916496ff..228c2a8b 100644 --- a/pmocontrol/src/music_renderer/mod.rs +++ b/pmocontrol/src/music_renderer/mod.rs @@ -6,7 +6,7 @@ mod upnp_renderer; mod openhome; mod openhome_renderer; -mod capabilities; +pub mod capabilities; mod chromecast_renderer; mod musicrenderer; @@ -18,13 +18,14 @@ pub mod watcher; use std::sync::{Arc, Mutex}; pub use crate::music_renderer::capabilities::{ - PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, + HasContinuousStream, HasQueue, PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, + QueueTransportControl, TransportControl, VolumeControl, }; pub use crate::music_renderer::musicrenderer::{MusicRenderer, PlaylistBinding}; pub use crate::music_renderer::sleep_timer::SleepTimer; pub use crate::music_renderer::stream_detection::is_continuous_stream_url; use crate::{ - RendererInfo, errors::ControlPointError, music_renderer::musicrenderer::MusicRendererBackend, + errors::ControlPointError, music_renderer::musicrenderer::MusicRendererBackend, RendererInfo, }; pub trait RendererFromMediaRendererInfo { diff --git a/pmocontrol/src/music_renderer/musicrenderer.rs b/pmocontrol/src/music_renderer/musicrenderer.rs index c5490710..1da5e749 100644 --- a/pmocontrol/src/music_renderer/musicrenderer.rs +++ b/pmocontrol/src/music_renderer/musicrenderer.rs @@ -19,10 +19,6 @@ use crate::events::RendererEventBus; use crate::model::RendererEvent; use crate::model::{PlaybackSource, PlaybackState, RendererInfo, RendererProtocol, TrackMetadata}; use crate::music_renderer::arylic_tcp::ArylicTcpRenderer; -use crate::music_renderer::capabilities::{ - PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, QueueTransportControl, RendererBackend, - TransportControl, VolumeControl, -}; use crate::music_renderer::chromecast_renderer::ChromecastRenderer; use crate::music_renderer::linkplay_renderer::LinkPlayRenderer; use crate::music_renderer::openhome_renderer::OpenHomeRenderer; @@ -33,6 +29,10 @@ use crate::music_renderer::watcher::{ WatchedState, }; use crate::music_renderer::RendererFromMediaRendererInfo; +use crate::music_renderer::{ + HasContinuousStream, HasQueue, PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, + QueueTransportControl, TransportControl, VolumeControl, +}; use crate::online::DeviceConnectionState; use crate::queue::{EnqueueMode, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; use crate::{DeviceId, DeviceIdentity, DeviceOnline}; @@ -998,7 +998,7 @@ impl MusicRenderer { /// Get a clone of the queue Arc (for async sync operations). pub fn queue(&self) -> Arc> { let backend = self.lock_backend_for("queue"); - crate::music_renderer::capabilities::RendererBackend::queue(&*backend).clone() + crate::music_renderer::capabilities::HasQueue::queue(&*backend).clone() } /// Get the current queue item without advancing. @@ -2274,7 +2274,7 @@ impl PlaybackPosition for MusicRendererBackend { } } -impl RendererBackend for MusicRendererBackend { +impl HasQueue for MusicRendererBackend { fn queue(&self) -> &Arc> { match self { MusicRendererBackend::Upnp(r) => r.queue(), @@ -2287,7 +2287,31 @@ impl RendererBackend for MusicRendererBackend { } } +impl HasContinuousStream for MusicRendererBackend { + fn continuous_stream(&self) -> &Arc> { + match self { + MusicRendererBackend::Upnp(r) => r.continuous_stream(), + MusicRendererBackend::OpenHome(r) => r.continuous_stream(), + MusicRendererBackend::LinkPlay(r) => r.continuous_stream(), + MusicRendererBackend::ArylicTcp(r) => r.continuous_stream(), + MusicRendererBackend::Chromecast(cc) => cc.continuous_stream(), + MusicRendererBackend::HybridUpnpArylic { arylic, .. } => arylic.continuous_stream(), + } + } +} + impl QueueTransportControl for MusicRendererBackend { + fn play_item(&self, item: &PlaybackItem) -> Result<(), ControlPointError> { + match self { + MusicRendererBackend::Upnp(r) => r.play_item(item), + MusicRendererBackend::OpenHome(r) => r.play_item(item), + MusicRendererBackend::LinkPlay(r) => r.play_item(item), + MusicRendererBackend::ArylicTcp(r) => r.play_item(item), + MusicRendererBackend::Chromecast(cc) => cc.play_item(item), + MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.play_item(item), + } + } + fn play_from_queue(&self) -> Result<(), ControlPointError> { match self { MusicRendererBackend::Upnp(r) => r.play_from_queue(), @@ -2332,165 +2356,3 @@ impl QueueTransportControl for MusicRendererBackend { } } } - -impl QueueBackend for MusicRendererBackend { - fn len(&self) -> Result { - match self { - MusicRendererBackend::Upnp(r) => r.len(), - MusicRendererBackend::OpenHome(r) => r.len(), - MusicRendererBackend::LinkPlay(r) => r.len(), - MusicRendererBackend::ArylicTcp(r) => r.len(), - MusicRendererBackend::Chromecast(cc) => cc.len(), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.len(), - } - } - - fn track_ids(&self) -> Result, ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.track_ids(), - MusicRendererBackend::OpenHome(r) => r.track_ids(), - MusicRendererBackend::LinkPlay(r) => r.track_ids(), - MusicRendererBackend::ArylicTcp(r) => r.track_ids(), - MusicRendererBackend::Chromecast(cc) => cc.track_ids(), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.track_ids(), - } - } - - fn id_to_position(&self, id: u32) -> Result { - match self { - MusicRendererBackend::Upnp(r) => r.id_to_position(id), - MusicRendererBackend::OpenHome(r) => r.id_to_position(id), - MusicRendererBackend::LinkPlay(r) => r.id_to_position(id), - MusicRendererBackend::ArylicTcp(r) => r.id_to_position(id), - MusicRendererBackend::Chromecast(cc) => cc.id_to_position(id), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.id_to_position(id), - } - } - - fn position_to_id(&self, id: usize) -> Result { - match self { - MusicRendererBackend::Upnp(r) => r.position_to_id(id), - MusicRendererBackend::OpenHome(r) => r.position_to_id(id), - MusicRendererBackend::LinkPlay(r) => r.position_to_id(id), - MusicRendererBackend::ArylicTcp(r) => r.position_to_id(id), - MusicRendererBackend::Chromecast(cc) => cc.position_to_id(id), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.position_to_id(id), - } - } - - fn current_track(&self) -> Result, ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.current_track(), - MusicRendererBackend::OpenHome(r) => r.current_track(), - MusicRendererBackend::LinkPlay(r) => r.current_track(), - MusicRendererBackend::ArylicTcp(r) => r.current_track(), - MusicRendererBackend::Chromecast(cc) => cc.current_track(), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.current_track(), - } - } - - fn current_index(&self) -> Result, ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.current_index(), - MusicRendererBackend::OpenHome(r) => r.current_index(), - MusicRendererBackend::LinkPlay(r) => r.current_index(), - MusicRendererBackend::ArylicTcp(r) => r.current_index(), - MusicRendererBackend::Chromecast(cc) => cc.current_index(), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.current_index(), - } - } - - fn queue_snapshot(&self) -> Result { - match self { - MusicRendererBackend::Upnp(r) => r.queue_snapshot(), - MusicRendererBackend::OpenHome(r) => r.queue_snapshot(), - MusicRendererBackend::LinkPlay(r) => r.queue_snapshot(), - MusicRendererBackend::ArylicTcp(r) => r.queue_snapshot(), - MusicRendererBackend::Chromecast(cc) => cc.queue_snapshot(), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.queue_snapshot(), - } - } - - fn set_index(&mut self, index: Option) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.set_index(index), - MusicRendererBackend::OpenHome(r) => r.set_index(index), - MusicRendererBackend::LinkPlay(r) => r.set_index(index), - MusicRendererBackend::ArylicTcp(r) => r.set_index(index), - MusicRendererBackend::Chromecast(cc) => cc.set_index(index), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.set_index(index), - } - } - - fn replace_queue( - &mut self, - items: Vec, - current_index: Option, - ) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.replace_queue(items, current_index), - MusicRendererBackend::OpenHome(r) => r.replace_queue(items, current_index), - MusicRendererBackend::LinkPlay(r) => r.replace_queue(items, current_index), - MusicRendererBackend::ArylicTcp(r) => r.replace_queue(items, current_index), - MusicRendererBackend::Chromecast(cc) => cc.replace_queue(items, current_index), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => { - upnp.replace_queue(items, current_index) - } - } - } - - fn sync_queue( - &mut self, - items: Vec, - cancel_token: &Arc, - on_ready: Option>, - ) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.sync_queue(items, cancel_token, on_ready), - MusicRendererBackend::OpenHome(r) => r.sync_queue(items, cancel_token, on_ready), - MusicRendererBackend::LinkPlay(r) => r.sync_queue(items, cancel_token, on_ready), - MusicRendererBackend::ArylicTcp(r) => r.sync_queue(items, cancel_token, on_ready), - MusicRendererBackend::Chromecast(cc) => cc.sync_queue(items, cancel_token, on_ready), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => { - upnp.sync_queue(items, cancel_token, on_ready) - } - } - } - - fn get_item(&self, index: usize) -> Result, ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.get_item(index), - MusicRendererBackend::OpenHome(r) => r.get_item(index), - MusicRendererBackend::LinkPlay(r) => r.get_item(index), - MusicRendererBackend::ArylicTcp(r) => r.get_item(index), - MusicRendererBackend::Chromecast(cc) => cc.get_item(index), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.get_item(index), - } - } - - fn replace_item(&mut self, index: usize, item: PlaybackItem) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.replace_item(index, item), - MusicRendererBackend::OpenHome(r) => r.replace_item(index, item), - MusicRendererBackend::LinkPlay(r) => r.replace_item(index, item), - MusicRendererBackend::ArylicTcp(r) => r.replace_item(index, item), - MusicRendererBackend::Chromecast(cc) => cc.replace_item(index, item), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.replace_item(index, item), - } - } - - fn enqueue_items( - &mut self, - items: Vec, - mode: EnqueueMode, - ) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.enqueue_items(items, mode), - MusicRendererBackend::OpenHome(r) => r.enqueue_items(items, mode), - MusicRendererBackend::LinkPlay(r) => r.enqueue_items(items, mode), - MusicRendererBackend::ArylicTcp(r) => r.enqueue_items(items, mode), - MusicRendererBackend::Chromecast(cc) => cc.enqueue_items(items, mode), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.enqueue_items(items, mode), - } - } -} diff --git a/pmocontrol/src/music_renderer/openhome_renderer.rs b/pmocontrol/src/music_renderer/openhome_renderer.rs index d0dde366..46f7fca8 100644 --- a/pmocontrol/src/music_renderer/openhome_renderer.rs +++ b/pmocontrol/src/music_renderer/openhome_renderer.rs @@ -2,8 +2,8 @@ use std::sync::{atomic::AtomicBool, Arc, Mutex}; use std::time::SystemTime; use crate::music_renderer::capabilities::{ - PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, QueueTransportControl, RendererBackend, - TransportControl, VolumeControl, + HasContinuousStream, PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, + QueueTransportControl, TransportControl, VolumeControl, }; use crate::music_renderer::time_utils::{format_hhmmss_u32, parse_time_flexible}; use crate::DeviceIdentity; @@ -15,8 +15,9 @@ use crate::music_renderer::openhome::{ build_info_client, build_playlist_client, build_product_client, build_radio_client, build_time_client, build_volume_client, }; +use crate::music_renderer::HasQueue; use crate::music_renderer::RendererFromMediaRendererInfo; -use crate::queue::{EnqueueMode, HasQueue, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; +use crate::queue::{EnqueueMode, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; use crate::upnp_clients::{ OhInfoClient, OhPlaylistClient, OhProductClient, OhRadioClient, OhTimeClient, OhVolumeClient, OPENHOME_PLAYLIST_HEAD_ID, @@ -266,11 +267,6 @@ impl RendererFromMediaRendererInfo for OpenHomeRenderer { } } -impl RendererBackend for OpenHomeRenderer { - fn queue(&self) -> &Arc> { - &self.queue - } -} impl TransportControl for OpenHomeRenderer { fn play_uri(&self, uri: &str, meta: &str) -> Result<(), ControlPointError> { @@ -529,6 +525,11 @@ pub(crate) fn map_openhome_state(raw: &str) -> PlaybackState { } impl QueueTransportControl for OpenHomeRenderer { + fn play_item(&self, _item: &PlaybackItem) -> Result<(), ControlPointError> { + let playlist = self.playlist_client_for("play_item")?; + playlist.play() + } + fn play_from_queue(&self) -> Result<(), ControlPointError> { { let queue = self @@ -632,6 +633,12 @@ impl HasQueue for OpenHomeRenderer { } } +impl HasContinuousStream for OpenHomeRenderer { + fn continuous_stream(&self) -> &Arc> { + &self.continuous_stream + } +} + impl OpenHomeRenderer { pub fn replace_queue_with_background( &mut self, diff --git a/pmocontrol/src/music_renderer/stream_detection.rs b/pmocontrol/src/music_renderer/stream_detection.rs index cb0959a2..9cda7ff1 100644 --- a/pmocontrol/src/music_renderer/stream_detection.rs +++ b/pmocontrol/src/music_renderer/stream_detection.rs @@ -185,7 +185,11 @@ fn check_stream_headers(url: &str) -> Result { trace!( "Stream detection for {}: content-length={}, chunked={}, streaming_mime={}, is_stream={}", - url, has_content_length, is_chunked, is_streaming_mime, is_stream + url, + has_content_length, + is_chunked, + is_streaming_mime, + is_stream ); Ok(is_stream) diff --git a/pmocontrol/src/music_renderer/upnp_renderer.rs b/pmocontrol/src/music_renderer/upnp_renderer.rs index bf4098e8..408038f5 100644 --- a/pmocontrol/src/music_renderer/upnp_renderer.rs +++ b/pmocontrol/src/music_renderer/upnp_renderer.rs @@ -4,11 +4,12 @@ use crate::errors::ControlPointError; use crate::model::PlaybackState; use crate::music_renderer::capabilities::{ HasContinuousStream, PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, - QueueTransportControl, RendererBackend, TransportControl, VolumeControl, + QueueTransportControl, TransportControl, VolumeControl, }; use crate::music_renderer::musicrenderer::{build_didl_lite_metadata, MusicRendererBackend}; +use crate::music_renderer::HasQueue; use crate::music_renderer::RendererFromMediaRendererInfo; -use crate::queue::{EnqueueMode, HasQueue, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; +use crate::queue::{EnqueueMode, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; use crate::upnp_clients::{ AvTransportClient, ConnectionInfo, ConnectionManagerClient, PositionInfo, ProtocolInfo, RenderingControlClient, @@ -174,17 +175,37 @@ impl RendererFromMediaRendererInfo for UpnpRenderer { } } -impl RendererBackend for UpnpRenderer { - fn queue(&self) -> &Arc> { - &self.queue - } -} impl QueueTransportControl for UpnpRenderer { + fn play_item(&self, item: &PlaybackItem) -> Result<(), ControlPointError> { + let metadata = if let Some(ref track_metadata) = item.metadata { + build_didl_lite_metadata(track_metadata, &item.uri, &item.protocol_info) + } else { + format!( + r#"{}"#, + item.protocol_info, item.uri + ) + }; + + let duration = parse_didl_duration(&metadata); + if let Some(ref dur) = duration { + tracing::debug!("Caching duration from queue DIDL: {}", dur); + *self.cached_duration.lock().unwrap() = Some(dur.clone()); + } else { + tracing::debug!("No duration to cache from queue DIDL"); + *self.cached_duration.lock().unwrap() = None; + } + + let avt = self.avtransport()?; + avt.set_av_transport_uri(&item.uri, &metadata)?; + avt.play(0, "1")?; + + Ok(()) + } + fn play_from_queue(&self) -> Result<(), ControlPointError> { let mut queue = self.queue.lock().unwrap(); - // Get or initialize current index let current_index = match queue.current_index()? { Some(idx) => idx, None => { @@ -197,29 +218,15 @@ impl QueueTransportControl for UpnpRenderer { } }; - // Get the item let item = queue .get_item(current_index)? .ok_or_else(|| ControlPointError::QueueError("Current item not found".into()))?; drop(queue); - // Build metadata - handle optional TrackMetadata - let metadata = if let Some(ref track_metadata) = item.metadata { - build_didl_lite_metadata(track_metadata, &item.uri, &item.protocol_info) - } else { - // Fallback to minimal DIDL-Lite if no metadata - format!( - r#"{}"#, - item.protocol_info, item.uri - ) - }; - - // Détecte si l'URL est un flux continu en interrogeant le serveur HTTP let is_stream = crate::music_renderer::is_continuous_stream_url(&item.uri); *self.continuous_stream.lock().unwrap() = is_stream; - // Log current queue state for debugging let queue_state = { let queue = self.queue.lock().unwrap(); let idx = queue.current_index().unwrap_or(None); @@ -237,22 +244,7 @@ impl QueueTransportControl for UpnpRenderer { is_stream ); - // Parse et cache la durée du DIDL (fallback pour certains amplis) - let duration = parse_didl_duration(&metadata); - if let Some(ref dur) = duration { - tracing::debug!("Caching duration from queue DIDL: {}", dur); - *self.cached_duration.lock().unwrap() = Some(dur.clone()); - } else { - tracing::debug!("No duration to cache from queue DIDL"); - *self.cached_duration.lock().unwrap() = None; - } - - // UPNP: SetAVTransportURI + Play - let avt = self.avtransport()?; - avt.set_av_transport_uri(&item.uri, &metadata)?; - avt.play(0, "1")?; - - Ok(()) + self.play_item(&item) } fn play_next(&self) -> Result<(), ControlPointError> { @@ -313,6 +305,12 @@ impl HasQueue for UpnpRenderer { } } +impl HasContinuousStream for UpnpRenderer { + fn continuous_stream(&self) -> &Arc> { + &self.continuous_stream + } +} + /// Parse le DIDL-Lite pour extraire la durée du premier élément fn parse_didl_duration(didl: &str) -> Option { // Recherche de l'élément (avec ou sans espace après) diff --git a/pmocontrol/src/queue/backend.rs b/pmocontrol/src/queue/backend.rs index 2565f0aa..28e75bfa 100644 --- a/pmocontrol/src/queue/backend.rs +++ b/pmocontrol/src/queue/backend.rs @@ -28,15 +28,11 @@ //! - This identity is used by the sync helpers to preserve the current //! track across queue rebuilds when the MediaServer content changes. +use crate::music_renderer::HasQueue; use crate::queue::MusicQueue; use crate::{errors::ControlPointError, PlaybackItem, QueueSnapshot}; use std::sync::{atomic::AtomicBool, Arc, Mutex}; -/// Trait for types that have aMusicQueue. -pub trait HasQueue { - fn queue(&self) -> &Arc>; -} - /// Blanket implementation of QueueBackend for types that have a queue. /// All methods simply delegate to the underlying MusicQueue. impl QueueBackend for T { diff --git a/pmocontrol/src/queue/mod.rs b/pmocontrol/src/queue/mod.rs index 99d77bf0..2dfd125c 100644 --- a/pmocontrol/src/queue/mod.rs +++ b/pmocontrol/src/queue/mod.rs @@ -6,7 +6,7 @@ mod snapshot; use std::sync::{Arc, Mutex}; -pub use backend::{EnqueueMode, HasQueue, QueueBackend}; +pub use backend::{EnqueueMode, QueueBackend}; pub use music_queue::{MusicQueue, SyncScheduleOutcome}; pub use snapshot::{PlaybackItem, QueueSnapshot}; -- 2.49.1 From 8ddee7d94f7561ef9705f2e3003b170cf60b9ad1 Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Sun, 12 Apr 2026 20:08:26 +0200 Subject: [PATCH 3/5] :recycle: refactor queue control methods into trait default impls Remove duplicate `play_from_queue`, ` play_next `, `play_previous , and `` pay _from_index implementations across all renderers (ArylicTcp, Chromecast, LinkPlay , OpenHome UPnP) and instead provide default implementations in the `QueueTransportControl `trait. Also introduce dispatch macros to simplify backend delegation for Transport/Volume control, and expose parse_didl_duration as public(crate). --- pmocontrol/src/music_renderer/arylic_tcp.rs | 57 ----- pmocontrol/src/music_renderer/capabilities.rs | 29 ++- .../src/music_renderer/chromecast_renderer.rs | 57 ----- .../src/music_renderer/linkplay_renderer.rs | 59 +----- .../src/music_renderer/musicrenderer.rs | 195 ++++-------------- .../src/music_renderer/openhome_renderer.rs | 66 ------ .../src/music_renderer/upnp_renderer.rs | 128 +----------- 7 files changed, 74 insertions(+), 517 deletions(-) diff --git a/pmocontrol/src/music_renderer/arylic_tcp.rs b/pmocontrol/src/music_renderer/arylic_tcp.rs index a9167029..5906310f 100644 --- a/pmocontrol/src/music_renderer/arylic_tcp.rs +++ b/pmocontrol/src/music_renderer/arylic_tcp.rs @@ -307,63 +307,6 @@ impl QueueTransportControl for ArylicTcpRenderer { self.play_uri(&item.uri, "") } - fn play_from_queue(&self) -> Result<(), ControlPointError> { - let mut queue = self.queue.lock().unwrap(); - - let current_index = match queue.current_index()? { - Some(idx) => idx, - None => { - if queue.len()? > 0 { - queue.set_index(Some(0))?; - 0 - } else { - return Err(ControlPointError::QueueError("Queue is empty".into())); - } - } - }; - - let item = queue - .get_item(current_index)? - .ok_or_else(|| ControlPointError::QueueError("Current item not found".into()))?; - - drop(queue); - - let is_stream = crate::music_renderer::is_continuous_stream_url(&item.uri); - *self.continuous_stream.lock().unwrap() = is_stream; - - self.play_item(&item) - } - - fn play_next(&self) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - if !queue.advance()? { - return Err(ControlPointError::QueueError("No next track".into())); - } - } - - self.play_from_queue() - } - - fn play_previous(&self) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - if !queue.rewind()? { - return Err(ControlPointError::QueueError("No previous track".into())); - } - } - - self.play_from_queue() - } - - fn play_from_index(&self, index: usize) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - queue.set_index(Some(index))?; - } - - self.play_from_queue() - } } impl HasQueue for ArylicTcpRenderer { diff --git a/pmocontrol/src/music_renderer/capabilities.rs b/pmocontrol/src/music_renderer/capabilities.rs index c164c052..b7e1559e 100644 --- a/pmocontrol/src/music_renderer/capabilities.rs +++ b/pmocontrol/src/music_renderer/capabilities.rs @@ -53,14 +53,35 @@ pub trait QueueTransportControl: HasQueue + HasContinuousStream { } /// Play the next track from the queue. - fn play_next(&self) -> Result<(), ControlPointError>; + fn play_next(&self) -> Result<(), ControlPointError> { + { + let mut queue = self.queue().lock().unwrap(); + if !queue.advance()? { + return Err(ControlPointError::QueueError("No next track".into())); + } + } + self.play_from_queue() + } /// Play the previous track from the queue. - #[allow(dead_code)] - fn play_previous(&self) -> Result<(), ControlPointError>; + fn play_previous(&self) -> Result<(), ControlPointError> { + { + let mut queue = self.queue().lock().unwrap(); + if !queue.rewind()? { + return Err(ControlPointError::QueueError("No previous track".into())); + } + } + self.play_from_queue() + } /// Play from a specific index in the queue. - fn play_from_index(&self, index: usize) -> Result<(), ControlPointError>; + fn play_from_index(&self, index: usize) -> Result<(), ControlPointError> { + { + let mut queue = self.queue().lock().unwrap(); + queue.set_index(Some(index))?; + } + self.play_from_queue() + } } /// Logical playback position across backends. diff --git a/pmocontrol/src/music_renderer/chromecast_renderer.rs b/pmocontrol/src/music_renderer/chromecast_renderer.rs index a0754a52..429054a2 100644 --- a/pmocontrol/src/music_renderer/chromecast_renderer.rs +++ b/pmocontrol/src/music_renderer/chromecast_renderer.rs @@ -806,63 +806,6 @@ impl QueueTransportControl for ChromecastRenderer { self.play_uri(&item.uri, "") } - fn play_from_queue(&self) -> Result<(), ControlPointError> { - let mut queue = self.queue.lock().unwrap(); - - let current_index = match queue.current_index()? { - Some(idx) => idx, - None => { - if queue.len()? > 0 { - queue.set_index(Some(0))?; - 0 - } else { - return Err(ControlPointError::QueueError("Queue is empty".into())); - } - } - }; - - let item = queue - .get_item(current_index)? - .ok_or_else(|| ControlPointError::QueueError("Current item not found".into()))?; - - drop(queue); - - let is_stream = crate::music_renderer::is_continuous_stream_url(&item.uri); - *self.continuous_stream.lock().unwrap() = is_stream; - - self.play_item(&item) - } - - fn play_next(&self) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - if !queue.advance()? { - return Err(ControlPointError::QueueError("No next track".into())); - } - } - - self.play_from_queue() - } - - fn play_previous(&self) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - if !queue.rewind()? { - return Err(ControlPointError::QueueError("No previous track".into())); - } - } - - self.play_from_queue() - } - - fn play_from_index(&self, index: usize) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - queue.set_index(Some(index))?; - } - - self.play_from_queue() - } } impl HasQueue for ChromecastRenderer { diff --git a/pmocontrol/src/music_renderer/linkplay_renderer.rs b/pmocontrol/src/music_renderer/linkplay_renderer.rs index 134c89f2..5705bc87 100644 --- a/pmocontrol/src/music_renderer/linkplay_renderer.rs +++ b/pmocontrol/src/music_renderer/linkplay_renderer.rs @@ -184,66 +184,9 @@ impl QueueTransportControl for LinkPlayRenderer { fn play_item(&self, item: &PlaybackItem) -> Result<(), ControlPointError> { self.play_uri(&item.uri, "") } - - fn play_from_queue(&self) -> Result<(), ControlPointError> { - let mut queue = self.queue.lock().unwrap(); - - let current_index = match queue.current_index()? { - Some(idx) => idx, - None => { - if queue.len()? > 0 { - queue.set_index(Some(0))?; - 0 - } else { - return Err(ControlPointError::QueueError("Queue is empty".into())); - } - } - }; - - let item = queue - .get_item(current_index)? - .ok_or_else(|| ControlPointError::QueueError("Current item not found".into()))?; - - drop(queue); - - let is_stream = crate::music_renderer::is_continuous_stream_url(&item.uri); - *self.continuous_stream.lock().unwrap() = is_stream; - - self.play_item(&item) - } - - fn play_next(&self) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - if !queue.advance()? { - return Err(ControlPointError::QueueError("No next track".into())); - } - } - - self.play_from_queue() - } - - fn play_previous(&self) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - if !queue.rewind()? { - return Err(ControlPointError::QueueError("No previous track".into())); - } - } - - self.play_from_queue() - } - - fn play_from_index(&self, index: usize) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - queue.set_index(Some(index))?; - } - - self.play_from_queue() - } } + impl HasQueue for LinkPlayRenderer { fn queue(&self) -> &Arc> { &self.queue diff --git a/pmocontrol/src/music_renderer/musicrenderer.rs b/pmocontrol/src/music_renderer/musicrenderer.rs index 1da5e749..50656c77 100644 --- a/pmocontrol/src/music_renderer/musicrenderer.rs +++ b/pmocontrol/src/music_renderer/musicrenderer.rs @@ -2091,7 +2091,7 @@ impl RendererFromMediaRendererInfo for MusicRendererBackend { /// Extracts the duration attribute from the element in DIDL metadata. /// This is used as a fallback when the renderer doesn't provide track_duration /// in GetPositionInfo or similar calls. -fn parse_didl_duration(didl_xml: &str) -> Option { +pub(crate) fn parse_didl_duration(didl_xml: &str) -> Option { // Parse DIDL-Lite XML properly using pmodidl let didl = match DIDLLite::parse(didl_xml) { Ok(d) => d, @@ -2129,6 +2129,37 @@ fn parse_rfc3339_to_system_time(s: &str) -> Option { Some(std::time::UNIX_EPOCH + std::time::Duration::from_secs(secs as u64)) } + +/// Dispatch a method call to the inner backend, using the UPnP field for +/// HybridUpnpArylic. +macro_rules! dispatch_upnp { + ($self:expr, $method:ident($($arg:expr),*)) => { + match $self { + MusicRendererBackend::Upnp(r) => r.$method($($arg),*), + MusicRendererBackend::OpenHome(r) => r.$method($($arg),*), + MusicRendererBackend::LinkPlay(r) => r.$method($($arg),*), + MusicRendererBackend::ArylicTcp(r) => r.$method($($arg),*), + MusicRendererBackend::Chromecast(cc) => cc.$method($($arg),*), + MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.$method($($arg),*), + } + }; +} + +/// Dispatch a method call to the inner backend, using the Arylic field for +/// HybridUpnpArylic. +macro_rules! dispatch_arylic { + ($self:expr, $method:ident($($arg:expr),*)) => { + match $self { + MusicRendererBackend::Upnp(r) => r.$method($($arg),*), + MusicRendererBackend::OpenHome(r) => r.$method($($arg),*), + MusicRendererBackend::LinkPlay(r) => r.$method($($arg),*), + MusicRendererBackend::ArylicTcp(r) => r.$method($($arg),*), + MusicRendererBackend::Chromecast(cc) => cc.$method($($arg),*), + MusicRendererBackend::HybridUpnpArylic { arylic, .. } => arylic.$method($($arg),*), + } + }; +} + /// Transport control façade that dispatches to whichever backend can fulfill /// the request, returning a standardized error if the backend lacks support. impl TransportControl for MusicRendererBackend { @@ -2145,38 +2176,9 @@ impl TransportControl for MusicRendererBackend { } } - fn play(&self) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(upnp) => upnp.play(), - MusicRendererBackend::OpenHome(oh) => oh.play(), - MusicRendererBackend::LinkPlay(lp) => lp.play(), - MusicRendererBackend::ArylicTcp(ary) => ary.play(), - MusicRendererBackend::Chromecast(cc) => cc.play(), - MusicRendererBackend::HybridUpnpArylic { arylic, .. } => arylic.play(), - } - } - - fn pause(&self) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(upnp) => upnp.pause(), - MusicRendererBackend::OpenHome(oh) => oh.pause(), - MusicRendererBackend::LinkPlay(lp) => lp.pause(), - MusicRendererBackend::ArylicTcp(ary) => ary.pause(), - MusicRendererBackend::Chromecast(cc) => cc.pause(), - MusicRendererBackend::HybridUpnpArylic { arylic, .. } => arylic.pause(), - } - } - - fn stop(&self) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(upnp) => upnp.stop(), - MusicRendererBackend::OpenHome(oh) => oh.stop(), - MusicRendererBackend::LinkPlay(lp) => lp.stop(), - MusicRendererBackend::ArylicTcp(ary) => ary.stop(), - MusicRendererBackend::Chromecast(cc) => cc.stop(), - MusicRendererBackend::HybridUpnpArylic { arylic, .. } => arylic.stop(), - } - } + fn play(&self) -> Result<(), ControlPointError> { dispatch_arylic!(self, play()) } + fn pause(&self) -> Result<(), ControlPointError> { dispatch_arylic!(self, pause()) } + fn stop(&self) -> Result<(), ControlPointError> { dispatch_arylic!(self, stop()) } fn seek_rel_time(&self, hhmmss: &str) -> Result<(), ControlPointError> { match self { @@ -2197,49 +2199,10 @@ impl TransportControl for MusicRendererBackend { /// Hybrid backends may read via Arylic TCP and write via UPnP, but callers /// always depend on a single [`VolumeControl`] entry point. impl VolumeControl for MusicRendererBackend { - fn volume(&self) -> Result { - match self { - MusicRendererBackend::HybridUpnpArylic { arylic, .. } => arylic.volume(), - MusicRendererBackend::ArylicTcp(ary) => ary.volume(), - MusicRendererBackend::OpenHome(oh) => oh.volume(), - MusicRendererBackend::Upnp(upnp) => upnp.volume(), - MusicRendererBackend::LinkPlay(lp) => lp.volume(), - MusicRendererBackend::Chromecast(cc) => cc.volume(), - } - } - - fn set_volume(&self, vol: u16) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.set_volume(vol), - MusicRendererBackend::ArylicTcp(ary) => ary.set_volume(vol), - MusicRendererBackend::OpenHome(oh) => oh.set_volume(vol), - MusicRendererBackend::Upnp(upnp) => upnp.set_volume(vol), - MusicRendererBackend::LinkPlay(lp) => lp.set_volume(vol), - MusicRendererBackend::Chromecast(cc) => cc.set_volume(vol), - } - } - - fn mute(&self) -> Result { - match self { - MusicRendererBackend::HybridUpnpArylic { arylic, .. } => arylic.mute(), - MusicRendererBackend::OpenHome(r) => r.mute(), - MusicRendererBackend::Upnp(r) => r.mute(), - MusicRendererBackend::LinkPlay(r) => r.mute(), - MusicRendererBackend::ArylicTcp(r) => r.mute(), - MusicRendererBackend::Chromecast(cc) => cc.mute(), - } - } - - fn set_mute(&self, m: bool) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::HybridUpnpArylic { arylic, .. } => arylic.set_mute(m), - MusicRendererBackend::OpenHome(r) => r.set_mute(m), - MusicRendererBackend::Upnp(r) => r.set_mute(m), - MusicRendererBackend::LinkPlay(r) => r.set_mute(m), - MusicRendererBackend::ArylicTcp(r) => r.set_mute(m), - MusicRendererBackend::Chromecast(cc) => cc.set_mute(m), - } - } + fn volume(&self) -> Result { dispatch_arylic!(self, volume()) } + fn set_volume(&self, vol: u16) -> Result<(), ControlPointError> { dispatch_upnp!(self, set_volume(vol)) } + fn mute(&self) -> Result { dispatch_arylic!(self, mute()) } + fn set_mute(&self, m: bool) -> Result<(), ControlPointError> { dispatch_arylic!(self, set_mute(m)) } } /// Playback-state queries sourced from the backend best suited for the job. @@ -2263,96 +2226,28 @@ impl PlaybackStatus for MusicRendererBackend { /// regardless of the backend providing the raw transport data. impl PlaybackPosition for MusicRendererBackend { fn playback_position(&self) -> Result { - match self { - MusicRendererBackend::Upnp(r) => r.playback_position(), - MusicRendererBackend::OpenHome(r) => r.playback_position(), - MusicRendererBackend::LinkPlay(r) => r.playback_position(), - MusicRendererBackend::ArylicTcp(r) => r.playback_position(), - MusicRendererBackend::Chromecast(cc) => cc.playback_position(), - MusicRendererBackend::HybridUpnpArylic { arylic, .. } => arylic.playback_position(), - } + dispatch_arylic!(self, playback_position()) } } impl HasQueue for MusicRendererBackend { - fn queue(&self) -> &Arc> { - match self { - MusicRendererBackend::Upnp(r) => r.queue(), - MusicRendererBackend::OpenHome(r) => r.queue(), - MusicRendererBackend::LinkPlay(r) => r.queue(), - MusicRendererBackend::ArylicTcp(r) => r.queue(), - MusicRendererBackend::Chromecast(cc) => cc.queue(), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.queue(), - } - } + fn queue(&self) -> &Arc> { dispatch_upnp!(self, queue()) } } impl HasContinuousStream for MusicRendererBackend { fn continuous_stream(&self) -> &Arc> { - match self { - MusicRendererBackend::Upnp(r) => r.continuous_stream(), - MusicRendererBackend::OpenHome(r) => r.continuous_stream(), - MusicRendererBackend::LinkPlay(r) => r.continuous_stream(), - MusicRendererBackend::ArylicTcp(r) => r.continuous_stream(), - MusicRendererBackend::Chromecast(cc) => cc.continuous_stream(), - MusicRendererBackend::HybridUpnpArylic { arylic, .. } => arylic.continuous_stream(), - } + dispatch_arylic!(self, continuous_stream()) } } impl QueueTransportControl for MusicRendererBackend { fn play_item(&self, item: &PlaybackItem) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.play_item(item), - MusicRendererBackend::OpenHome(r) => r.play_item(item), - MusicRendererBackend::LinkPlay(r) => r.play_item(item), - MusicRendererBackend::ArylicTcp(r) => r.play_item(item), - MusicRendererBackend::Chromecast(cc) => cc.play_item(item), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.play_item(item), - } + dispatch_upnp!(self, play_item(item)) } - fn play_from_queue(&self) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.play_from_queue(), - MusicRendererBackend::OpenHome(r) => r.play_from_queue(), - MusicRendererBackend::LinkPlay(r) => r.play_from_queue(), - MusicRendererBackend::ArylicTcp(r) => r.play_from_queue(), - MusicRendererBackend::Chromecast(cc) => cc.play_from_queue(), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.play_from_queue(), - } + dispatch_upnp!(self, play_from_queue()) } - - fn play_next(&self) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.play_next(), - MusicRendererBackend::OpenHome(r) => r.play_next(), - MusicRendererBackend::LinkPlay(r) => r.play_next(), - MusicRendererBackend::ArylicTcp(r) => r.play_next(), - MusicRendererBackend::Chromecast(cc) => cc.play_next(), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.play_next(), - } - } - - fn play_previous(&self) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.play_previous(), - MusicRendererBackend::OpenHome(r) => r.play_previous(), - MusicRendererBackend::LinkPlay(r) => r.play_previous(), - MusicRendererBackend::ArylicTcp(r) => r.play_previous(), - MusicRendererBackend::Chromecast(cc) => cc.play_previous(), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.play_previous(), - } - } - fn play_from_index(&self, index: usize) -> Result<(), ControlPointError> { - match self { - MusicRendererBackend::Upnp(r) => r.play_from_index(index), - MusicRendererBackend::OpenHome(r) => r.play_from_index(index), - MusicRendererBackend::LinkPlay(r) => r.play_from_index(index), - MusicRendererBackend::ArylicTcp(r) => r.play_from_index(index), - MusicRendererBackend::Chromecast(cc) => cc.play_from_index(index), - MusicRendererBackend::HybridUpnpArylic { upnp, .. } => upnp.play_from_index(index), - } + dispatch_upnp!(self, play_from_index(index)) } } diff --git a/pmocontrol/src/music_renderer/openhome_renderer.rs b/pmocontrol/src/music_renderer/openhome_renderer.rs index 46f7fca8..80aafbaa 100644 --- a/pmocontrol/src/music_renderer/openhome_renderer.rs +++ b/pmocontrol/src/music_renderer/openhome_renderer.rs @@ -492,28 +492,6 @@ impl PlaybackPosition for OpenHomeRenderer { Ok(position_info) } } - -/// Parse duration from DIDL-Lite metadata XML (OpenHome version) -#[allow(dead_code)] -fn parse_didl_duration_openhome(didl: &str) -> Option { - // Search for duration attribute in element - let res_start = didl.find("')?; - let tag_attrs = &after_res[..tag_close]; - - if let Some(duration_start) = tag_attrs.find("duration=\"") { - let duration_offset = duration_start + "duration=\"".len(); - if let Some(duration_end) = tag_attrs[duration_offset..].find('"') { - let duration = &tag_attrs[duration_offset..duration_offset + duration_end]; - return Some(duration.to_string()); - } - } - - tracing::debug!("OpenHome: No duration found in DIDL metadata"); - None -} - pub(crate) fn map_openhome_state(raw: &str) -> PlaybackState { match raw.trim().to_ascii_uppercase().as_str() { "PLAYING" => PlaybackState::Playing, @@ -554,50 +532,6 @@ impl QueueTransportControl for OpenHomeRenderer { playlist.play() } - fn play_next(&self) -> Result<(), ControlPointError> { - { - let mut queue = self - .queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))?; - let len = queue.len().unwrap_or(0); - let current = queue.current_index().ok().flatten(); - let current_track_id = queue.current_track().ok().flatten(); - let all_ids = queue.track_ids().ok().unwrap_or_default(); - tracing::trace!( - queue_len = len, - current_index = ?current, - current_track_id = ?current_track_id, - all_track_ids = ?all_ids, - "OpenHome play_next: advancing queue" - ); - if !queue.advance()? { - tracing::trace!( - queue_len = len, - current_index = ?current, - "OpenHome play_next: advance() returned false — no next track" - ); - return Err(ControlPointError::QueueError("No next track".into())); - } - } - - self.play_from_queue() - } - - fn play_previous(&self) -> Result<(), ControlPointError> { - { - let mut queue = self - .queue - .lock() - .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))?; - if !queue.rewind()? { - return Err(ControlPointError::QueueError("No previous track".into())); - } - } - - self.play_from_queue() - } - fn play_from_index(&self, index: usize) -> Result<(), ControlPointError> { // For OpenHome, we need to convert index to track_id let track_id = { diff --git a/pmocontrol/src/music_renderer/upnp_renderer.rs b/pmocontrol/src/music_renderer/upnp_renderer.rs index 408038f5..09555461 100644 --- a/pmocontrol/src/music_renderer/upnp_renderer.rs +++ b/pmocontrol/src/music_renderer/upnp_renderer.rs @@ -6,7 +6,9 @@ use crate::music_renderer::capabilities::{ HasContinuousStream, PlaybackPosition, PlaybackPositionInfo, PlaybackStatus, QueueTransportControl, TransportControl, VolumeControl, }; -use crate::music_renderer::musicrenderer::{build_didl_lite_metadata, MusicRendererBackend}; +use crate::music_renderer::musicrenderer::{ + build_didl_lite_metadata, parse_didl_duration, MusicRendererBackend, +}; use crate::music_renderer::HasQueue; use crate::music_renderer::RendererFromMediaRendererInfo; use crate::queue::{EnqueueMode, MusicQueue, PlaybackItem, QueueBackend, QueueSnapshot}; @@ -203,100 +205,6 @@ impl QueueTransportControl for UpnpRenderer { Ok(()) } - fn play_from_queue(&self) -> Result<(), ControlPointError> { - let mut queue = self.queue.lock().unwrap(); - - let current_index = match queue.current_index()? { - Some(idx) => idx, - None => { - if queue.len()? > 0 { - queue.set_index(Some(0))?; - 0 - } else { - return Err(ControlPointError::QueueError("Queue is empty".into())); - } - } - }; - - let item = queue - .get_item(current_index)? - .ok_or_else(|| ControlPointError::QueueError("Current item not found".into()))?; - - drop(queue); - - let is_stream = crate::music_renderer::is_continuous_stream_url(&item.uri); - *self.continuous_stream.lock().unwrap() = is_stream; - - let queue_state = { - let queue = self.queue.lock().unwrap(); - let idx = queue.current_index().unwrap_or(None); - let len = queue.len().unwrap_or(0); - let uri = item.uri.clone(); - let title = item.metadata.as_ref().and_then(|m| m.title.clone()); - (idx, len, uri, title) - }; - tracing::debug!( - "UpnpRenderer play_from_queue: index={:?}/{}, uri={}, title={:?}, continuous_stream={}", - queue_state.0, - queue_state.1, - queue_state.2, - queue_state.3, - is_stream - ); - - self.play_item(&item) - } - - fn play_next(&self) -> Result<(), ControlPointError> { - let current_idx = { - let queue = self.queue.lock().unwrap(); - let idx = queue.current_index().unwrap_or(None); - let len = queue.len().unwrap_or(0); - tracing::debug!( - current_index = ?idx, - queue_len = len, - "play_next: attempting to advance" - ); - idx - }; - { - let mut queue = self.queue.lock().unwrap(); - if !queue.advance()? { - return Err(ControlPointError::QueueError("No next track".into())); - } - let new_idx = queue.current_index().unwrap_or(None); - tracing::debug!( - previous_index = ?current_idx, - new_index = ?new_idx, - "play_next: advanced" - ); - } - - self.play_from_queue() - } - - fn play_previous(&self) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - if !queue.rewind()? { - return Err(ControlPointError::QueueError("No previous track".into())); - } - } - - self.play_from_queue() - } - - fn play_from_index(&self, index: usize) -> Result<(), ControlPointError> { - { - let mut queue = self.queue.lock().unwrap(); - queue.set_index(Some(index))?; - } - // CORRECTIF: Quand on change l'index manuellement (shuffle, sélection d'un titre) - // on logue pour être sûr que c'est bien appelé - tracing::debug!(index = index, "✅ SHUFFLE / SEEK: play_from_index appelé"); - - self.play_from_queue() - } } impl HasQueue for UpnpRenderer { @@ -310,36 +218,6 @@ impl HasContinuousStream for UpnpRenderer { &self.continuous_stream } } - -/// Parse le DIDL-Lite pour extraire la durée du premier élément -fn parse_didl_duration(didl: &str) -> Option { - // Recherche de l'élément (avec ou sans espace après) - let res_start = didl - .find("")) - .or_else(|| didl.find(" - // Il doit être avant la fermeture du tag (avant '>') - if let Some(tag_close) = after_res.find('>') { - let tag_attrs = &after_res[..tag_close]; - - if let Some(duration_start) = tag_attrs.find("duration=\"") { - let duration_offset = duration_start + "duration=\"".len(); - if let Some(duration_end) = tag_attrs[duration_offset..].find('"') { - let duration = &tag_attrs[duration_offset..duration_offset + duration_end]; - return Some(duration.to_string()); - } - } - } - - tracing::warn!("No duration attribute found in DIDL element"); - None -} - /// Implémentation UPnP AV de `TransportControl` pour [`UpnpRenderer`]. /// /// Cette impl se base sur AVTransport (InstanceID = 0). -- 2.49.1 From 82197c810321e2f6948005b53fcc7680e12f3038 Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Sun, 12 Apr 2026 20:17:05 +0200 Subject: [PATCH 4/5] :recycle: refactor stream detection and queue sync logic - Replace `is_continuous_stream_url` with new canonical check using metadata + fallback - Extract stream duration comparison logic to `queue::stream_duration_*` helpers - Improve queue sync concurrency: add worker loop, pending/cancel flags - Make `stream_duration_*` functions public(crate) for reuse --- pmocontrol/src/music_renderer/capabilities.rs | 2 +- pmocontrol/src/music_renderer/mod.rs | 2 +- .../src/music_renderer/musicrenderer.rs | 42 +--- .../src/music_renderer/stream_detection.rs | 11 + pmocontrol/src/queue/mod.rs | 4 +- pmocontrol/src/queue/music_queue.rs | 228 ++++++++++-------- 6 files changed, 154 insertions(+), 135 deletions(-) diff --git a/pmocontrol/src/music_renderer/capabilities.rs b/pmocontrol/src/music_renderer/capabilities.rs index b7e1559e..dcc376a5 100644 --- a/pmocontrol/src/music_renderer/capabilities.rs +++ b/pmocontrol/src/music_renderer/capabilities.rs @@ -46,7 +46,7 @@ pub trait QueueTransportControl: HasQueue + HasContinuousStream { drop(queue); - let is_stream = crate::music_renderer::is_continuous_stream_url(&item.uri); + let is_stream = crate::music_renderer::is_continuous_stream(item.metadata.as_ref(), &item.uri); *self.continuous_stream().lock().unwrap() = is_stream; self.play_item(&item) diff --git a/pmocontrol/src/music_renderer/mod.rs b/pmocontrol/src/music_renderer/mod.rs index 228c2a8b..9d95d684 100644 --- a/pmocontrol/src/music_renderer/mod.rs +++ b/pmocontrol/src/music_renderer/mod.rs @@ -23,7 +23,7 @@ pub use crate::music_renderer::capabilities::{ }; pub use crate::music_renderer::musicrenderer::{MusicRenderer, PlaylistBinding}; pub use crate::music_renderer::sleep_timer::SleepTimer; -pub use crate::music_renderer::stream_detection::is_continuous_stream_url; +pub use crate::music_renderer::stream_detection::{is_continuous_stream, is_continuous_stream_url}; use crate::{ errors::ControlPointError, music_renderer::musicrenderer::MusicRendererBackend, RendererInfo, }; diff --git a/pmocontrol/src/music_renderer/musicrenderer.rs b/pmocontrol/src/music_renderer/musicrenderer.rs index 50656c77..ae456117 100644 --- a/pmocontrol/src/music_renderer/musicrenderer.rs +++ b/pmocontrol/src/music_renderer/musicrenderer.rs @@ -492,39 +492,19 @@ impl MusicRenderer { if let Some(ref new_duration) = position.track_duration { let mut state = self.state.lock().unwrap(); - // Parse durations to compare (HH:MM:SS format) - let parse_duration = |dur_str: &str| -> Option { - let parts: Vec<&str> = dur_str.split(':').collect(); - if parts.len() == 3 { - let h: u32 = parts[0].parse().ok()?; - let m: u32 = parts[1].parse().ok()?; - let s: u32 = parts[2].parse().ok()?; - Some(h * 3600 + m * 60 + s) - } else { - None - } - }; - match &state.current_track_duration { Some(stored_duration) => { - // Compare new duration with stored one - if let (Some(stored_secs), Some(new_secs)) = ( - parse_duration(stored_duration), - parse_duration(new_duration), - ) { - if new_secs > stored_secs { - // Duration increased: update stored value and use new one - tracing::debug!( - "MusicRenderer [{}]: Stream duration increased: {} -> {}", - self.info.friendly_name(), - stored_duration, - new_duration - ); - state.current_track_duration = Some(new_duration.clone()); - } else { - // Duration decreased or equal: keep stored value - position.track_duration = Some(stored_duration.clone()); - } + if crate::queue::stream_duration_increased(stored_duration, new_duration) { + tracing::debug!( + "MusicRenderer [{}]: Stream duration increased: {} -> {}", + self.info.friendly_name(), + stored_duration, + new_duration + ); + state.current_track_duration = Some(new_duration.clone()); + } else if crate::queue::stream_duration_decreased(stored_duration, new_duration) { + // Duration decreased: keep stored value + position.track_duration = Some(stored_duration.clone()); } } None => { diff --git a/pmocontrol/src/music_renderer/stream_detection.rs b/pmocontrol/src/music_renderer/stream_detection.rs index 9cda7ff1..f4c4176c 100644 --- a/pmocontrol/src/music_renderer/stream_detection.rs +++ b/pmocontrol/src/music_renderer/stream_detection.rs @@ -195,6 +195,17 @@ fn check_stream_headers(url: &str) -> Result { Ok(is_stream) } +/// Canonical check: returns `true` if this item should be treated as a continuous stream. +/// +/// Checks `metadata.is_continuous_stream` first (already computed at ingest time), +/// then falls back to the URL-based HTTP detection. +/// +/// Use this function everywhere transport-layer code needs to decide whether playback is +/// a continuous stream (radio) vs bounded media (file/album track). +pub fn is_continuous_stream(metadata: Option<&crate::model::TrackMetadata>, uri: &str) -> bool { + metadata.map(|m| m.is_continuous_stream).unwrap_or(false) || is_continuous_stream_url(uri) +} + #[cfg(test)] mod tests { use super::*; diff --git a/pmocontrol/src/queue/mod.rs b/pmocontrol/src/queue/mod.rs index 2dfd125c..a1843783 100644 --- a/pmocontrol/src/queue/mod.rs +++ b/pmocontrol/src/queue/mod.rs @@ -19,7 +19,7 @@ use crate::{errors::ControlPointError, RendererInfo}; /// Returns true if `new_dur` < `old_dur` (both parseable as HH:MM:SS/MM:SS/SS). /// Used to protect stream durations from decreasing for the same track. -pub(super) fn stream_duration_decreased(old_dur: &str, new_dur: &str) -> bool { +pub(crate) fn stream_duration_decreased(old_dur: &str, new_dur: &str) -> bool { match ( parse_time_flexible(old_dur).ok(), parse_time_flexible(new_dur).ok(), @@ -30,7 +30,7 @@ pub(super) fn stream_duration_decreased(old_dur: &str, new_dur: &str) -> bool { } /// Returns true if `new_dur` > `old_dur` (both parseable as HH:MM:SS/MM:SS/SS). -pub(super) fn stream_duration_increased(old_dur: &str, new_dur: &str) -> bool { +pub(crate) fn stream_duration_increased(old_dur: &str, new_dur: &str) -> bool { match ( parse_time_flexible(old_dur).ok(), parse_time_flexible(new_dur).ok(), diff --git a/pmocontrol/src/queue/music_queue.rs b/pmocontrol/src/queue/music_queue.rs index 4b9318ed..642f8c7b 100644 --- a/pmocontrol/src/queue/music_queue.rs +++ b/pmocontrol/src/queue/music_queue.rs @@ -129,6 +129,14 @@ impl MusicQueue { thread::Builder::new() .name(thread_name) .spawn(move || { + // Protocol for the three AtomicBools: + // sync_in_progress : set to true before spawn, cleared on Drop via Guard. + // sync_pending : set to true by a concurrent caller that arrives while + // a sync is already running. The worker re-fetches items + // and loops when it detects this flag on exit. + // sync_cancel_token: set to true when a new sync request interrupts an + // in-progress one. Passed into QueueBackend::sync_queue + // so it can abort early. struct Guard(Arc); impl Drop for Guard { fn drop(&mut self) { @@ -136,113 +144,133 @@ impl MusicQueue { } } let _guard = Guard(Arc::clone(&sync_in_progress)); - - let mut current_items = items; - let mut current_on_ready = Some(on_ready); - let mut on_complete = Some(on_complete); - tracing::debug!(thread = %std::thread::current().name().unwrap_or("?"), "queue-sync thread started"); - - loop { - sync_pending.store(false, SeqCst); - sync_cancel_token.store(false, SeqCst); - - // Extract the real on_ready BEFORE locking the queue. - // on_ready may call play_from_queue() which re-locks the queue, - // so we must NOT call it while holding queue_arc. - let real_on_ready = current_on_ready.take().flatten(); - let on_ready_triggered = Arc::new(AtomicBool::new(false)); - let proxy_on_ready: Option> = - real_on_ready.as_ref().map(|_| { - let flag = Arc::clone(&on_ready_triggered); - Box::new(move || { - flag.store(true, SeqCst); - }) as Box - }); - - tracing::debug!( - thread = %std::thread::current().name().unwrap_or("?"), - items = current_items.len(), - has_on_ready = real_on_ready.is_some(), - "queue-sync: calling sync_queue" - ); - - let result = { - let mut q = queue_arc.lock().unwrap(); - ::sync_queue( - &mut q, - current_items, - &sync_cancel_token, - proxy_on_ready, - ) - }; - // Queue lock is released here. - // Now safe to call on_ready (which may re-lock the queue). - // If on_ready was triggered by the proxy, consume and call it. - // If not (cancelled before first insert), keep it to pass to retry. - let carry_on_ready = if on_ready_triggered.load(SeqCst) { - tracing::debug!( - thread = %std::thread::current().name().unwrap_or("?"), - "queue-sync: on_ready triggered, calling callback" - ); - if let Some(f) = real_on_ready { - f(); - } - None - } else { - if real_on_ready.is_some() { - tracing::debug!( - thread = %std::thread::current().name().unwrap_or("?"), - "queue-sync: on_ready not triggered, carrying to next attempt" - ); - } - real_on_ready - }; - - match result { - Err(ControlPointError::SyncCancelled) => { - tracing::debug!( - thread = %std::thread::current().name().unwrap_or("?"), - "queue-sync: cancelled" - ); - } - Err(e) => { - tracing::warn!("queue-sync error: {}", e); - } - Ok(()) => { - tracing::debug!( - thread = %std::thread::current().name().unwrap_or("?"), - "queue-sync: completed successfully" - ); - if let Some(cb) = on_complete.take() { - let queue_len = queue_arc.lock().unwrap().len().unwrap_or(0); - cb(queue_len); - } - } - } - - if !sync_pending.load(SeqCst) { - break; - } - - match pending_items_fn() { - Ok(new_items) => { - current_items = new_items; - current_on_ready = Some(carry_on_ready); - } - Err(e) => { - tracing::warn!("queue-sync pending re-fetch error: {}", e); - break; - } - } - } - + Self::sync_worker_loop( + queue_arc, + items, + pending_items_fn, + on_ready, + on_complete, + sync_pending, + sync_cancel_token, + ); tracing::debug!(thread = %std::thread::current().name().unwrap_or("?"), "queue-sync thread done"); }) .expect("Failed to spawn queue-sync thread"); SyncScheduleOutcome::Scheduled } + + /// Inner loop executed by the sync worker thread. + /// + /// Runs at least once with `initial_items`. If a new sync request arrives while the + /// loop is running (`sync_pending` becomes true), it re-fetches items via + /// `pending_items_fn` and iterates again, allowing the latest playlist state to win. + fn sync_worker_loop( + queue_arc: Arc>, + initial_items: Vec, + pending_items_fn: Box Result, ControlPointError> + Send>, + initial_on_ready: Option>, + on_complete: Box, + sync_pending: Arc, + sync_cancel_token: Arc, + ) { + let mut current_items = initial_items; + let mut current_on_ready = Some(initial_on_ready); + let mut on_complete = Some(on_complete); + + loop { + sync_pending.store(false, SeqCst); + sync_cancel_token.store(false, SeqCst); + + // Extract the real on_ready BEFORE locking the queue. + // on_ready may call play_from_queue() which re-locks the queue, + // so we must NOT call it while holding queue_arc. + let real_on_ready = current_on_ready.take().flatten(); + let on_ready_triggered = Arc::new(AtomicBool::new(false)); + let proxy_on_ready: Option> = + real_on_ready.as_ref().map(|_| { + let flag = Arc::clone(&on_ready_triggered); + Box::new(move || { + flag.store(true, SeqCst); + }) as Box + }); + + tracing::debug!( + thread = %std::thread::current().name().unwrap_or("?"), + items = current_items.len(), + has_on_ready = real_on_ready.is_some(), + "queue-sync: calling sync_queue" + ); + + let result = { + let mut q = queue_arc.lock().unwrap(); + ::sync_queue( + &mut q, + current_items, + &sync_cancel_token, + proxy_on_ready, + ) + }; + // Queue lock is released here. + // Now safe to call on_ready (which may re-lock the queue). + let carry_on_ready = if on_ready_triggered.load(SeqCst) { + tracing::debug!( + thread = %std::thread::current().name().unwrap_or("?"), + "queue-sync: on_ready triggered, calling callback" + ); + if let Some(f) = real_on_ready { + f(); + } + None + } else { + if real_on_ready.is_some() { + tracing::debug!( + thread = %std::thread::current().name().unwrap_or("?"), + "queue-sync: on_ready not triggered, carrying to next attempt" + ); + } + real_on_ready + }; + + match result { + Err(ControlPointError::SyncCancelled) => { + tracing::debug!( + thread = %std::thread::current().name().unwrap_or("?"), + "queue-sync: cancelled" + ); + } + Err(e) => { + tracing::warn!("queue-sync error: {}", e); + } + Ok(()) => { + tracing::debug!( + thread = %std::thread::current().name().unwrap_or("?"), + "queue-sync: completed successfully" + ); + if let Some(cb) = on_complete.take() { + let queue_len = queue_arc.lock().unwrap().len().unwrap_or(0); + cb(queue_len); + } + } + } + + if !sync_pending.load(SeqCst) { + break; + } + + match pending_items_fn() { + Ok(new_items) => { + current_items = new_items; + current_on_ready = Some(carry_on_ready); + } + Err(e) => { + tracing::warn!("queue-sync pending re-fetch error: {}", e); + break; + } + } + } + } } impl QueueBackend for MusicQueue { -- 2.49.1 From 39d7c56a991ec40ab5ea446c3bf6d71c51697e03 Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Sun, 12 Apr 2026 20:27:54 +0200 Subject: [PATCH 5/5] Replace unwrap() with expect("mutex poisoned") across all mutex locks - Replace `.lock().unwrap()` with explicit error messages for poisoned MutexGuard - Improve robustness against mutex poisoning in renderer, queue and event subsystems --- pmocontrol/src/events.rs | 8 +- pmocontrol/src/music_renderer/arylic_tcp.rs | 4 +- pmocontrol/src/music_renderer/capabilities.rs | 104 +++++++++++++----- .../src/music_renderer/chromecast_renderer.rs | 4 +- .../src/music_renderer/linkplay_renderer.rs | 6 +- .../src/music_renderer/musicrenderer.rs | 98 +++++++++++------ .../src/music_renderer/openhome_renderer.rs | 20 ++-- .../src/music_renderer/stream_detection.rs | 8 +- .../src/music_renderer/upnp_renderer.rs | 14 +-- pmocontrol/src/queue/backend.rs | 22 ++-- pmocontrol/src/queue/music_queue.rs | 10 +- pmocontrol/src/queue/openhome.rs | 42 +++---- .../src/upnp_clients/openhome_client.rs | 6 +- 13 files changed, 219 insertions(+), 127 deletions(-) diff --git a/pmocontrol/src/events.rs b/pmocontrol/src/events.rs index e85fbfc7..c50d7ccd 100644 --- a/pmocontrol/src/events.rs +++ b/pmocontrol/src/events.rs @@ -19,7 +19,7 @@ impl RendererEventBus { pub(crate) fn subscribe(&self) -> Receiver { let (tx, rx) = unbounded::(); { - let mut subscribers = self.subscribers.lock().unwrap(); + let mut subscribers = self.subscribers.lock().expect("event subscribers mutex poisoned"); subscribers.push(tx); } rx @@ -27,7 +27,7 @@ impl RendererEventBus { #[allow(dead_code)] pub(crate) fn broadcast(&self, event: RendererEvent) { - let mut subscribers = self.subscribers.lock().unwrap(); + let mut subscribers = self.subscribers.lock().expect("event subscribers mutex poisoned"); subscribers.retain(|tx| tx.send(event.clone()).is_ok()); } } @@ -47,14 +47,14 @@ impl MediaServerEventBus { pub fn subscribe(&self) -> Receiver { let (tx, rx) = unbounded::(); { - let mut subscribers = self.subscribers.lock().unwrap(); + let mut subscribers = self.subscribers.lock().expect("event subscribers mutex poisoned"); subscribers.push(tx); } rx } pub(crate) fn broadcast(&self, event: MediaServerEvent) { - let mut subscribers = self.subscribers.lock().unwrap(); + let mut subscribers = self.subscribers.lock().expect("event subscribers mutex poisoned"); subscribers.retain(|tx| tx.send(event.clone()).is_ok()); } } diff --git a/pmocontrol/src/music_renderer/arylic_tcp.rs b/pmocontrol/src/music_renderer/arylic_tcp.rs index 5906310f..9b900d29 100644 --- a/pmocontrol/src/music_renderer/arylic_tcp.rs +++ b/pmocontrol/src/music_renderer/arylic_tcp.rs @@ -137,7 +137,7 @@ impl RendererFromMediaRendererInfo for ArylicTcpRenderer { impl ArylicTcpRenderer { /// Returns true if currently playing a continuous stream (radio without duration) pub fn is_continuous_stream(&self) -> bool { - *self.continuous_stream.lock().unwrap() + *self.continuous_stream.lock().expect("continuous_stream mutex poisoned") } /// Create an ArylicTcpRenderer with a shared queue (for HybridUpnpArylic) @@ -268,7 +268,7 @@ impl PlaybackPosition for ArylicTcpRenderer { // Récupérer les métadonnées depuis la queue (avec protection contre diminution de durée) // Normalement current_index est toujours Some() si la queue n'est pas vide (règle métier) - let mut queue_guard = self.queue.lock().unwrap(); + let mut queue_guard = self.queue.lock().expect("queue mutex poisoned"); let queue_item = queue_guard.peek_current().ok().flatten(); if let Some((current_item, _)) = queue_item { diff --git a/pmocontrol/src/music_renderer/capabilities.rs b/pmocontrol/src/music_renderer/capabilities.rs index dcc376a5..601b2d09 100644 --- a/pmocontrol/src/music_renderer/capabilities.rs +++ b/pmocontrol/src/music_renderer/capabilities.rs @@ -4,12 +4,21 @@ use std::sync::{Arc, Mutex}; use crate::queue::{MusicQueue, QueueBackend}; use crate::{errors::ControlPointError, model::PlaybackState, PlaybackItem}; -/// Trait for types that have access to a MusicQueue. +/// Marker trait for renderer backends that own a `MusicQueue`. +/// +/// Implementing this trait automatically provides the full `QueueBackend` +/// blanket implementation (see `queue/backend.rs`). Backends only need to +/// return a reference to their `Arc>` field. pub trait HasQueue { fn queue(&self) -> &Arc>; } -/// Trait for types that track whether they're playing a continuous stream. +/// Marker trait for renderer backends that track stream continuity. +/// +/// The flag is `true` while the renderer is playing a continuous stream +/// (e.g. an internet radio station) and `false` for bounded media files. +/// It is used by the watcher to decide whether auto-advance should be +/// suppressed when playback stops. pub trait HasContinuousStream { fn continuous_stream(&self) -> &Arc>; } @@ -26,7 +35,7 @@ pub trait QueueTransportControl: HasQueue + HasContinuousStream { /// Play from the queue at the current index (or initialize to 0 if not set). /// This is the default implementation that handles queue navigation. fn play_from_queue(&self) -> Result<(), ControlPointError> { - let mut queue = self.queue().lock().unwrap(); + let mut queue = self.queue().lock().expect("queue mutex poisoned"); let current_index = match queue.current_index()? { Some(idx) => idx, @@ -47,7 +56,7 @@ pub trait QueueTransportControl: HasQueue + HasContinuousStream { drop(queue); let is_stream = crate::music_renderer::is_continuous_stream(item.metadata.as_ref(), &item.uri); - *self.continuous_stream().lock().unwrap() = is_stream; + *self.continuous_stream().lock().expect("continuous_stream mutex poisoned") = is_stream; self.play_item(&item) } @@ -55,7 +64,7 @@ pub trait QueueTransportControl: HasQueue + HasContinuousStream { /// Play the next track from the queue. fn play_next(&self) -> Result<(), ControlPointError> { { - let mut queue = self.queue().lock().unwrap(); + let mut queue = self.queue().lock().expect("queue mutex poisoned"); if !queue.advance()? { return Err(ControlPointError::QueueError("No next track".into())); } @@ -66,7 +75,7 @@ pub trait QueueTransportControl: HasQueue + HasContinuousStream { /// Play the previous track from the queue. fn play_previous(&self) -> Result<(), ControlPointError> { { - let mut queue = self.queue().lock().unwrap(); + let mut queue = self.queue().lock().expect("queue mutex poisoned"); if !queue.rewind()? { return Err(ControlPointError::QueueError("No previous track".into())); } @@ -77,7 +86,7 @@ pub trait QueueTransportControl: HasQueue + HasContinuousStream { /// Play from a specific index in the queue. fn play_from_index(&self, index: usize) -> Result<(), ControlPointError> { { - let mut queue = self.queue().lock().unwrap(); + let mut queue = self.queue().lock().expect("queue mutex poisoned"); queue.set_index(Some(index))?; } self.play_from_queue() @@ -98,52 +107,97 @@ pub struct PlaybackPositionInfo { pub track_metadata: Option, // DIDL-Lite XML from GetPositionInfo pub track_uri: Option, // Current track URI } +/// Provides the current playback position and track metadata. +/// +/// All time fields use the format `"HH:MM:SS"` (or `None` when unavailable). +/// `track_metadata` carries a raw DIDL-Lite XML fragment returned by the device; +/// callers that only need structured metadata should use `extract_track_metadata` +/// from the watcher module instead. pub trait PlaybackPosition { + /// Returns the current playback position information. + /// + /// Returns `Err` if the renderer is unreachable or the query fails. fn playback_position(&self) -> Result; } -/// Generic abstraction for playback status (transport state). +/// Generic abstraction for the current transport state. /// -/// For UPnP AV, this is backed by AVTransport::GetTransportInfo. -/// For OpenHome, a future implementation will adapt from OH Info/Time. +/// # Implementations +/// +/// - **UPnP AV**: backed by `AVTransport::GetTransportInfo`. +/// - **OpenHome**: adapted from OH `Info` / `Time` services. +/// - **LinkPlay / Arylic**: mapped from the vendor status response. +/// +/// # Postconditions +/// +/// The returned `PlaybackState` must be one of the canonical values defined by +/// the `PlaybackState` enum. Backend-specific states that have no canonical +/// equivalent should be mapped to the closest approximation (e.g. "BUFFERING" +/// → `PlaybackState::Transitioning`). pub trait PlaybackStatus { + /// Returns the current transport state of the renderer. fn playback_state(&self) -> Result; } -/// Abstraction générique des capacités de transport (lecture / pause / stop / seek) -/// indépendamment du protocole sous-jacent (UPnP AV, OpenHome, ...). +/// Generic transport control abstraction (play / pause / stop / seek), +/// independent of the underlying protocol (UPnP AV, OpenHome, …). +/// +/// # Invariants +/// +/// - `play_uri` sets the active resource and begins playback atomically from the +/// caller's perspective. Implementations may split this into two protocol steps +/// (e.g. `SetAVTransportURI` + `Play` for UPnP AV) but the caller should not +/// need to know. +/// - `play` / `pause` / `stop` operate on whatever resource is currently loaded; +/// they do not change the queue pointer. +/// - `seek_rel_time` uses the format `"HH:MM:SS"`. Backends that do not support +/// seeking should return `ControlPointError::NotSupported`. +/// +/// # Relation to `QueueTransportControl` +/// +/// `TransportControl` knows nothing about the queue. `QueueTransportControl` +/// extends it with queue-aware navigation (`play_next`, `play_previous`, …). pub trait TransportControl { - /// Set la ressource à lire (URI + métadonnées) et/ou commence la lecture. + /// Load a resource (URI + DIDL-Lite metadata) and begin playback. /// - /// Selon l'implémentation, cette méthode peut soit : - /// - faire un "Set...URI" + "Play" (cas UPnP AV), - /// - ou configurer la file de lecture (cas OpenHome, etc.). + /// Depending on the backend this may execute as a single atomic operation or as + /// two sequential commands (set resource, then play). fn play_uri(&self, uri: &str, meta: &str) -> Result<(), ControlPointError>; - /// Démarre ou reprend la lecture. + /// Start or resume playback of the currently loaded resource. fn play(&self) -> Result<(), ControlPointError>; - /// Met la lecture en pause. + /// Pause the current playback. fn pause(&self) -> Result<(), ControlPointError>; - /// Arrête la lecture. + /// Stop the current playback and release the loaded resource. fn stop(&self) -> Result<(), ControlPointError>; - /// Seek à un temps relatif (HH:MM:SS) si supporté. + /// Seek to a relative time position expressed as `"HH:MM:SS"`. + /// + /// Returns `ControlPointError::NotSupported` when the backend does not + /// implement seeking. fn seek_rel_time(&self, hhmmss: &str) -> Result<(), ControlPointError>; } -/// Abstraction générique des capacités de contrôle de volume / mute. +/// Generic volume and mute control abstraction. +/// +/// # Volume scale +/// +/// Volume values are expressed on the native scale of each renderer. +/// UPnP AV and OpenHome renderers typically use 0–100. Callers should +/// not assume any particular scale; use the values returned by `volume()` +/// as the baseline for relative adjustments. pub trait VolumeControl { - /// Retourne le volume logique courant (échelle dépendante du renderer). + /// Returns the current logical volume (renderer-specific scale). fn volume(&self) -> Result; - /// Définit le volume logique (échelle dépendante du renderer). + /// Sets the logical volume (renderer-specific scale). fn set_volume(&self, v: u16) -> Result<(), ControlPointError>; - /// Indique si le renderer est muet (mute activé). + /// Returns `true` when the renderer is muted. fn mute(&self) -> Result; - /// Active ou désactive le mute. + /// Enables (`true`) or disables (`false`) mute. fn set_mute(&self, m: bool) -> Result<(), ControlPointError>; } diff --git a/pmocontrol/src/music_renderer/chromecast_renderer.rs b/pmocontrol/src/music_renderer/chromecast_renderer.rs index 429054a2..28d79723 100644 --- a/pmocontrol/src/music_renderer/chromecast_renderer.rs +++ b/pmocontrol/src/music_renderer/chromecast_renderer.rs @@ -172,7 +172,7 @@ impl RendererFromMediaRendererInfo for ChromecastRenderer { impl ChromecastRenderer { /// Returns true if currently playing a continuous stream (radio without duration) pub fn is_continuous_stream(&self) -> bool { - *self.continuous_stream.lock().unwrap() + *self.continuous_stream.lock().expect("continuous_stream mutex poisoned") } /// Connect to the device with retry on connection failures. @@ -205,7 +205,7 @@ impl TransportControl for ChromecastRenderer { // Détecte si l'URL est un flux continu let is_stream = crate::music_renderer::is_continuous_stream_url(uri); - *self.continuous_stream.lock().unwrap() = is_stream; + *self.continuous_stream.lock().expect("continuous_stream mutex poisoned") = is_stream; tracing::debug!( "ChromecastRenderer play_uri: URI={}, continuous_stream={}", uri, diff --git a/pmocontrol/src/music_renderer/linkplay_renderer.rs b/pmocontrol/src/music_renderer/linkplay_renderer.rs index 5705bc87..626f0962 100644 --- a/pmocontrol/src/music_renderer/linkplay_renderer.rs +++ b/pmocontrol/src/music_renderer/linkplay_renderer.rs @@ -91,7 +91,7 @@ impl RendererFromMediaRendererInfo for LinkPlayRenderer { impl LinkPlayRenderer { /// Returns true if currently playing a continuous stream (radio without duration) pub fn is_continuous_stream(&self) -> bool { - *self.continuous_stream.lock().unwrap() + *self.continuous_stream.lock().expect("continuous_stream mutex poisoned") } } @@ -99,7 +99,7 @@ impl TransportControl for LinkPlayRenderer { fn play_uri(&self, uri: &str, _meta: &str) -> Result<(), ControlPointError> { // Détecte si l'URL est un flux continu let is_stream = crate::music_renderer::is_continuous_stream_url(uri); - *self.continuous_stream.lock().unwrap() = is_stream; + *self.continuous_stream.lock().expect("continuous_stream mutex poisoned") = is_stream; tracing::debug!( "LinkPlayRenderer play_uri: URI={}, continuous_stream={}", uri, @@ -158,7 +158,7 @@ impl PlaybackPosition for LinkPlayRenderer { let mut position_info = self.fetch_status()?.position_info(); // Use queue metadata instead of direct status metadata to benefit from duration protection - let mut queue_guard = self.queue.lock().unwrap(); + let mut queue_guard = self.queue.lock().expect("queue mutex poisoned"); let queue_item = queue_guard.peek_current().ok().flatten(); if let Some((current_item, _)) = queue_item { diff --git a/pmocontrol/src/music_renderer/musicrenderer.rs b/pmocontrol/src/music_renderer/musicrenderer.rs index ae456117..99c03f09 100644 --- a/pmocontrol/src/music_renderer/musicrenderer.rs +++ b/pmocontrol/src/music_renderer/musicrenderer.rs @@ -46,9 +46,9 @@ use tracing::warn; #[derive(Clone, Debug)] pub struct PlaylistBinding { /// MediaServer that owns the playlist container. - pub server_id: DeviceId, + pub(crate) server_id: DeviceId, /// DIDL-Lite object id of the playlist container. - pub container_id: String, + pub(crate) container_id: String, /// True once at least one ContainerUpdateIDs notification has been seen. pub(crate) has_seen_update: bool, /// Flag used internally to signal that the queue should be refreshed @@ -58,6 +58,18 @@ pub struct PlaylistBinding { pub(crate) auto_play_on_refresh: bool, } +impl PlaylistBinding { + /// Returns the ID of the MediaServer that owns this playlist container. + pub fn server_id(&self) -> &DeviceId { + &self.server_id + } + + /// Returns the DIDL-Lite object ID of the playlist container. + pub fn container_id(&self) -> &str { + &self.container_id + } +} + /// Backend-agnostic façade exposing transport, volume, and status contracts. #[derive(Clone, Debug)] pub enum MusicRendererBackend { @@ -311,6 +323,14 @@ impl MusicRenderer { } /// Main loop for the watcher thread. + /// + /// # Error policy + /// + /// The watcher thread runs for the lifetime of the renderer and never restarts + /// automatically. Network/device errors during polling are logged at `debug` level + /// and ignored — they are transient and expected when a device is temporarily + /// unreachable. Panics inside `poll_and_emit_changes` are caught and logged at + /// `error` level so they do not kill the watcher thread. fn watcher_loop(&self, strategy: WatchStrategy, stop_flag: Arc) { let Some(base_interval) = strategy.polling_interval() else { // Pure push strategy - no polling needed (future implementation) @@ -341,7 +361,16 @@ impl MusicRenderer { }; if self.is_online() { - self.poll_and_emit_changes(tick); + // Wrap in catch_unwind so a panic in poll logic does not terminate the watcher. + let poll_result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + self.poll_and_emit_changes(tick); + })); + if let Err(_panic) = poll_result { + error!( + renderer = self.info.friendly_name(), + "poll_and_emit_changes panicked; watcher continues" + ); + } last_activity_time = SystemTime::now(); } @@ -393,14 +422,19 @@ impl MusicRenderer { // Step 2: Do all network calls WITHOUT holding any locks // This prevents blocking other threads that need to read watched_state - let position = self.playback_position().ok(); - let raw_state = self.playback_state().ok(); + // Errors are logged at trace level — device temporarily unreachable is expected. + let position = self.playback_position() + .inspect_err(|e| tracing::trace!(renderer = self.info.friendly_name(), error = %e, "playback_position failed")) + .ok(); + let raw_state = self.playback_state() + .inspect_err(|e| tracing::trace!(renderer = self.info.friendly_name(), error = %e, "playback_state failed")) + .ok(); // Poll volume and mute every other tick (1 second at 500ms interval) let (volume, mute, is_stream) = if tick % 2 == 0 { ( - self.volume().ok(), - self.mute().ok(), + self.volume().inspect_err(|e| tracing::trace!(renderer = self.info.friendly_name(), error = %e, "volume poll failed")).ok(), + self.mute().inspect_err(|e| tracing::trace!(renderer = self.info.friendly_name(), error = %e, "mute poll failed")).ok(), Some(self.is_playing_a_stream()), ) } else { @@ -490,7 +524,7 @@ impl MusicRenderer { if let Some(stream_flag) = is_stream { if stream_flag { if let Some(ref new_duration) = position.track_duration { - let mut state = self.state.lock().unwrap(); + let mut state = self.state.lock().expect("RendererState mutex poisoned"); match &state.current_track_duration { Some(stored_duration) => { @@ -647,7 +681,7 @@ impl MusicRenderer { match state { PlaybackState::Stopped => { { - let s = self.state.lock().unwrap(); + let s = self.state.lock().expect("RendererState mutex poisoned"); tracing::debug!( renderer = self.info.friendly_name(), has_played = s.has_played_since_track_start, @@ -680,7 +714,7 @@ impl MusicRenderer { // On autorise l'auto-avance si: // 1. On a bien vu PLAYING OU // 2. Le titre a été lancé depuis plus de 20 secondes - let track_start = self.state.lock().unwrap().track_start_time; + let track_start = self.state.lock().expect("RendererState mutex poisoned").track_start_time; let elapsed = track_start .and_then(|t| t.elapsed().ok()) .unwrap_or_default(); @@ -737,7 +771,7 @@ impl MusicRenderer { PlaybackState::NoMedia => { // Handle end of track (Chromecast returns NoMedia when track ends) // This is equivalent to Stopped for auto-advance purposes - let s = self.state.lock().unwrap(); + let s = self.state.lock().expect("RendererState mutex poisoned"); let playback_source = s.playback_source; let has_played = s.has_played_since_track_start; let user_stop = s.user_stop_requested; @@ -804,7 +838,7 @@ impl MusicRenderer { self.set_has_played_flag(); } PlaybackState::Transitioning => { - let s = self.state.lock().unwrap(); + let s = self.state.lock().expect("RendererState mutex poisoned"); tracing::trace!( renderer = self.info.friendly_name(), has_played = s.has_played_since_track_start, @@ -1646,13 +1680,13 @@ impl MusicRenderer { /// Gets the last known track metadata. pub fn last_metadata(&self) -> Option { - self.state.lock().unwrap().last_metadata.clone() + self.state.lock().expect("RendererState mutex poisoned").last_metadata.clone() } /// Sets the last known track metadata. /// Updates track_start_time and resets current_track_duration only if the metadata actually changes. pub fn set_last_metadata(&self, metadata: Option) { - let mut state = self.state.lock().unwrap(); + let mut state = self.state.lock().expect("RendererState mutex poisoned"); let metadata_changed = state.last_metadata != metadata; if metadata_changed { // Pour les flux continus: utiliser dc:date comme track_start_time réel de diffusion. @@ -1673,23 +1707,23 @@ impl MusicRenderer { /// Gets the timestamp when the current track started playing. pub fn track_start_time(&self) -> Option { - self.state.lock().unwrap().track_start_time + self.state.lock().expect("RendererState mutex poisoned").track_start_time } /// Gets the current playback source. pub fn playback_source(&self) -> PlaybackSource { - self.state.lock().unwrap().playback_source + self.state.lock().expect("RendererState mutex poisoned").playback_source } /// Sets the playback source. pub fn set_playback_source(&self, source: PlaybackSource) { - self.state.lock().unwrap().playback_source = source; + self.state.lock().expect("RendererState mutex poisoned").playback_source = source; } /// Checks if currently playing from queue. pub fn is_playing_from_queue(&self) -> bool { matches!( - self.state.lock().unwrap().playback_source, + self.state.lock().expect("RendererState mutex poisoned").playback_source, PlaybackSource::FromQueue ) } @@ -1700,7 +1734,7 @@ impl MusicRenderer { /// Does NOT change None -> External because that would break queue playback /// (the control_point will set it to FromQueue after play_from_queue succeeds). pub fn mark_external_if_idle(&self) { - let mut state = self.state.lock().unwrap(); + let mut state = self.state.lock().expect("RendererState mutex poisoned"); if matches!(state.playback_source, PlaybackSource::External) { // Keep External if we were already playing externally } else { @@ -1711,12 +1745,12 @@ impl MusicRenderer { /// Marks that the user requested a stop (to prevent auto-advance). pub fn mark_user_stop_requested(&self) { - self.state.lock().unwrap().user_stop_requested = true; + self.state.lock().expect("RendererState mutex poisoned").user_stop_requested = true; } /// Checks and clears the user stop requested flag. pub fn check_and_clear_user_stop_requested(&self) -> bool { - let mut state = self.state.lock().unwrap(); + let mut state = self.state.lock().expect("RendererState mutex poisoned"); let was_requested = state.user_stop_requested; state.user_stop_requested = false; was_requested @@ -1727,21 +1761,21 @@ impl MusicRenderer { /// Sets the has_played_since_track_start flag to true. /// Called when PLAYING state is detected. fn set_has_played_flag(&self) { - self.state.lock().unwrap().has_played_since_track_start = true; + self.state.lock().expect("RendererState mutex poisoned").has_played_since_track_start = true; } /// Clears the has_played_since_track_start flag. /// Called when stopping playback or starting a new track. /// This is public so that ControlPoint can reset it when jumping to a new track. pub fn clear_has_played_flag(&self) { - self.state.lock().unwrap().has_played_since_track_start = false; + self.state.lock().expect("RendererState mutex poisoned").has_played_since_track_start = false; } /// Checks and clears the has_played_since_track_start flag. /// Returns true if PLAYING was seen since last track start, false otherwise. /// Used to determine if auto-advance should be allowed. fn check_and_clear_has_played_flag(&self) -> bool { - let mut state = self.state.lock().unwrap(); + let mut state = self.state.lock().expect("RendererState mutex poisoned"); let has_played = state.has_played_since_track_start; state.has_played_since_track_start = false; has_played @@ -1757,7 +1791,7 @@ impl MusicRenderer { /// # Errors /// Returns an error if the duration is invalid (0 or > 7200 seconds). pub fn start_sleep_timer(&self, duration_seconds: u32) -> Result { - let mut state = self.state.lock().unwrap(); + let mut state = self.state.lock().expect("RendererState mutex poisoned"); state .sleep_timer .start(duration_seconds) @@ -1773,7 +1807,7 @@ impl MusicRenderer { /// # Errors /// Returns an error if the duration is invalid (0 or > 7200 seconds). pub fn update_sleep_timer(&self, duration_seconds: u32) -> Result { - let mut state = self.state.lock().unwrap(); + let mut state = self.state.lock().expect("RendererState mutex poisoned"); state .sleep_timer .update(duration_seconds) @@ -1784,32 +1818,32 @@ impl MusicRenderer { /// Cancels the sleep timer. pub fn cancel_sleep_timer(&self) { - self.state.lock().unwrap().sleep_timer.cancel(); + self.state.lock().expect("RendererState mutex poisoned").sleep_timer.cancel(); } /// Returns the remaining seconds of the sleep timer, or None if no timer is active. pub fn sleep_timer_remaining(&self) -> Option { - self.state.lock().unwrap().sleep_timer.remaining_seconds() + self.state.lock().expect("RendererState mutex poisoned").sleep_timer.remaining_seconds() } /// Returns the configured duration of the sleep timer in seconds. pub fn sleep_timer_duration(&self) -> u32 { - self.state.lock().unwrap().sleep_timer.duration_seconds() + self.state.lock().expect("RendererState mutex poisoned").sleep_timer.duration_seconds() } /// Returns true if the sleep timer is active. pub fn is_sleep_timer_active(&self) -> bool { - self.state.lock().unwrap().sleep_timer.is_active() + self.state.lock().expect("RendererState mutex poisoned").sleep_timer.is_active() } /// Returns true if the sleep timer has expired. pub fn is_sleep_timer_expired(&self) -> bool { - self.state.lock().unwrap().sleep_timer.is_expired() + self.state.lock().expect("RendererState mutex poisoned").sleep_timer.is_expired() } /// Gets the sleep timer state as a tuple (is_active, duration_seconds, remaining_seconds). pub fn sleep_timer_state(&self) -> (bool, u32, Option) { - let state = self.state.lock().unwrap(); + let state = self.state.lock().expect("RendererState mutex poisoned"); ( state.sleep_timer.is_active(), state.sleep_timer.duration_seconds(), diff --git a/pmocontrol/src/music_renderer/openhome_renderer.rs b/pmocontrol/src/music_renderer/openhome_renderer.rs index 80aafbaa..8dafd67e 100644 --- a/pmocontrol/src/music_renderer/openhome_renderer.rs +++ b/pmocontrol/src/music_renderer/openhome_renderer.rs @@ -89,7 +89,7 @@ impl OpenHomeRenderer { /// Returns true if currently playing a continuous stream (radio without duration) pub fn is_continuous_stream(&self) -> bool { - *self.continuous_stream.lock().unwrap() + *self.continuous_stream.lock().expect("continuous_stream mutex poisoned") } pub fn has_playlist(&self) -> bool { @@ -176,14 +176,14 @@ impl OpenHomeRenderer { /// Plus rapide que snapshot_openhome_playlist() pour juste connaître le nombre de pistes. pub(crate) fn openhome_playlist_len(&self) -> Result { // Use queue.len() which uses cached track_ids() internally - let queue = self.queue.lock().unwrap(); + let queue = self.queue.lock().expect("queue mutex poisoned"); queue.len() } /// Retourne les IDs des pistes de la playlist OpenHome. /// Plus rapide que snapshot_openhome_playlist() car ne récupère pas les métadonnées. pub(crate) fn openhome_playlist_ids(&self) -> Result, ControlPointError> { - let queue = self.queue.lock().unwrap(); + let queue = self.queue.lock().expect("queue mutex poisoned"); if let Some(oh_queue) = queue.as_openhome() { oh_queue.track_ids() } else { @@ -209,7 +209,7 @@ impl OpenHomeRenderer { let insert_after = match after_id { Some(id) => id, None => { - let queue = self.queue.lock().unwrap(); + let queue = self.queue.lock().expect("queue mutex poisoned"); if let Some(oh_queue) = queue.as_openhome() { oh_queue .track_ids()? @@ -365,7 +365,7 @@ impl PlaybackPosition for OpenHomeRenderer { // Check cache first { - let mut cache = self.position_cache.lock().unwrap(); + let mut cache = self.position_cache.lock().expect("position_cache mutex poisoned"); // Track calls for warning detection cache.calls_in_last_second.push(now); @@ -406,7 +406,7 @@ impl PlaybackPosition for OpenHomeRenderer { let mut track_metadata_xml = None; // Get track ID from queue (uses cached data) - let queue_guard_for_id = self.queue.lock().unwrap(); + let queue_guard_for_id = self.queue.lock().expect("queue mutex poisoned"); if let Some(oh_queue) = queue_guard_for_id.as_openhome() { match oh_queue.current_track() { Ok(id_opt) => track_id = id_opt, @@ -419,7 +419,7 @@ impl PlaybackPosition for OpenHomeRenderer { drop(queue_guard_for_id); // Use queue API to get current item with cached metadata - let mut queue_guard = self.queue.lock().unwrap(); + let mut queue_guard = self.queue.lock().expect("queue mutex poisoned"); if let Ok(Some((current_item, _))) = queue_guard.peek_current() { // Use metadata from queue cache (updated via OpenHome events) track_uri = Some(current_item.uri.clone()); @@ -436,7 +436,7 @@ impl PlaybackPosition for OpenHomeRenderer { } // Check if the URI has changed to detect track changes - let mut cached_uri = self.current_track_uri.lock().unwrap(); + let mut cached_uri = self.current_track_uri.lock().expect("current_track_uri mutex poisoned"); let uri_changed = cached_uri.as_ref() != Some(¤t_item.uri); if uri_changed { @@ -448,7 +448,7 @@ impl PlaybackPosition for OpenHomeRenderer { // Détecte si la nouvelle URL est un flux continu let is_stream = crate::music_renderer::is_continuous_stream_url(¤t_item.uri); - *self.continuous_stream.lock().unwrap() = is_stream; + *self.continuous_stream.lock().expect("continuous_stream mutex poisoned") = is_stream; tracing::debug!("OpenHome URI changed, continuous_stream={}", is_stream); *cached_uri = Some(current_item.uri.clone()); @@ -484,7 +484,7 @@ impl PlaybackPosition for OpenHomeRenderer { // Update cache with fresh data { - let mut cache = self.position_cache.lock().unwrap(); + let mut cache = self.position_cache.lock().expect("position_cache mutex poisoned"); cache.last_position = Some(position_info.clone()); cache.last_update = Some(now); } diff --git a/pmocontrol/src/music_renderer/stream_detection.rs b/pmocontrol/src/music_renderer/stream_detection.rs index f4c4176c..e8d6a198 100644 --- a/pmocontrol/src/music_renderer/stream_detection.rs +++ b/pmocontrol/src/music_renderer/stream_detection.rs @@ -53,7 +53,7 @@ pub fn is_continuous_stream_url(url: &str) -> bool { // Check cache first { - let cache = STREAM_CACHE.lock().unwrap(); + let cache = STREAM_CACHE.lock().expect("stream cache mutex poisoned"); if let Some(&cached_result) = cache.get(url) { trace!("Cache hit for {}: is_stream={}", url, cached_result); return cached_result; @@ -62,7 +62,7 @@ pub fn is_continuous_stream_url(url: &str) -> bool { // Check if already being verified { - let mut pending = PENDING_CHECKS.lock().unwrap(); + let mut pending = PENDING_CHECKS.lock().expect("pending checks mutex poisoned"); if pending.contains(url) { debug!( "Stream detection already in progress for {}, returning false temporarily", @@ -93,13 +93,13 @@ pub fn is_continuous_stream_url(url: &str) -> bool { // Store in cache { - let mut cache = STREAM_CACHE.lock().unwrap(); + let mut cache = STREAM_CACHE.lock().expect("stream cache mutex poisoned"); cache.insert(url_owned.clone(), result); } // Remove from pending { - let mut pending = PENDING_CHECKS.lock().unwrap(); + let mut pending = PENDING_CHECKS.lock().expect("pending checks mutex poisoned"); pending.remove(&url_owned); } diff --git a/pmocontrol/src/music_renderer/upnp_renderer.rs b/pmocontrol/src/music_renderer/upnp_renderer.rs index 09555461..78b364c0 100644 --- a/pmocontrol/src/music_renderer/upnp_renderer.rs +++ b/pmocontrol/src/music_renderer/upnp_renderer.rs @@ -117,7 +117,7 @@ impl UpnpRenderer { /// Returns true if currently playing a continuous stream (radio without duration) pub fn is_continuous_stream(&self) -> bool { - *self.continuous_stream.lock().unwrap() + *self.continuous_stream.lock().expect("continuous_stream mutex poisoned") } } @@ -192,10 +192,10 @@ impl QueueTransportControl for UpnpRenderer { let duration = parse_didl_duration(&metadata); if let Some(ref dur) = duration { tracing::debug!("Caching duration from queue DIDL: {}", dur); - *self.cached_duration.lock().unwrap() = Some(dur.clone()); + *self.cached_duration.lock().expect("cached_duration mutex poisoned") = Some(dur.clone()); } else { tracing::debug!("No duration to cache from queue DIDL"); - *self.cached_duration.lock().unwrap() = None; + *self.cached_duration.lock().expect("cached_duration mutex poisoned") = None; } let avt = self.avtransport()?; @@ -233,7 +233,7 @@ impl TransportControl for UpnpRenderer { // Détecte si l'URL est un flux continu en interrogeant le serveur HTTP let is_stream = crate::music_renderer::is_continuous_stream_url(uri); - *self.continuous_stream.lock().unwrap() = is_stream; + *self.continuous_stream.lock().expect("continuous_stream mutex poisoned") = is_stream; tracing::debug!( "UpnpRenderer play_uri: URI={}, continuous_stream={}", uri, @@ -244,10 +244,10 @@ impl TransportControl for UpnpRenderer { let duration = parse_didl_duration(meta); if let Some(ref dur) = duration { tracing::debug!("Caching duration from DIDL: {}", dur); - *self.cached_duration.lock().unwrap() = Some(dur.clone()); + *self.cached_duration.lock().expect("cached_duration mutex poisoned") = Some(dur.clone()); } else { tracing::debug!("No duration to cache from DIDL"); - *self.cached_duration.lock().unwrap() = None; + *self.cached_duration.lock().expect("cached_duration mutex poisoned") = None; } let avt = self.avtransport()?; @@ -341,7 +341,7 @@ impl PlaybackPosition for UpnpRenderer { let mut track_metadata_xml = None; let mut track_uri = raw.track_uri.clone(); - let mut queue_guard = self.queue.lock().unwrap(); + let mut queue_guard = self.queue.lock().expect("queue mutex poisoned"); // Récupérer l'item courant de la queue // Normalement current_index est toujours Some() si la queue n'est pas vide (règle métier) diff --git a/pmocontrol/src/queue/backend.rs b/pmocontrol/src/queue/backend.rs index 28e75bfa..0c62b40c 100644 --- a/pmocontrol/src/queue/backend.rs +++ b/pmocontrol/src/queue/backend.rs @@ -37,35 +37,35 @@ use std::sync::{atomic::AtomicBool, Arc, Mutex}; /// All methods simply delegate to the underlying MusicQueue. impl QueueBackend for T { fn len(&self) -> Result { - self.queue().lock().unwrap().len() + self.queue().lock().expect("queue mutex poisoned").len() } fn track_ids(&self) -> Result, ControlPointError> { - self.queue().lock().unwrap().track_ids() + self.queue().lock().expect("queue mutex poisoned").track_ids() } fn id_to_position(&self, id: u32) -> Result { - self.queue().lock().unwrap().id_to_position(id) + self.queue().lock().expect("queue mutex poisoned").id_to_position(id) } fn position_to_id(&self, id: usize) -> Result { - self.queue().lock().unwrap().position_to_id(id) + self.queue().lock().expect("queue mutex poisoned").position_to_id(id) } fn current_track(&self) -> Result, ControlPointError> { - self.queue().lock().unwrap().current_track() + self.queue().lock().expect("queue mutex poisoned").current_track() } fn current_index(&self) -> Result, ControlPointError> { - self.queue().lock().unwrap().current_index() + self.queue().lock().expect("queue mutex poisoned").current_index() } fn queue_snapshot(&self) -> Result { - self.queue().lock().unwrap().queue_snapshot() + self.queue().lock().expect("queue mutex poisoned").queue_snapshot() } fn set_index(&mut self, index: Option) -> Result<(), ControlPointError> { - self.queue().lock().unwrap().set_index(index) + self.queue().lock().expect("queue mutex poisoned").set_index(index) } fn replace_queue( @@ -92,11 +92,11 @@ impl QueueBackend for T { } fn get_item(&self, index: usize) -> Result, ControlPointError> { - self.queue().lock().unwrap().get_item(index) + self.queue().lock().expect("queue mutex poisoned").get_item(index) } fn replace_item(&mut self, index: usize, item: PlaybackItem) -> Result<(), ControlPointError> { - self.queue().lock().unwrap().replace_item(index, item) + self.queue().lock().expect("queue mutex poisoned").replace_item(index, item) } fn enqueue_items( @@ -104,7 +104,7 @@ impl QueueBackend for T { items: Vec, mode: EnqueueMode, ) -> Result<(), ControlPointError> { - self.queue().lock().unwrap().enqueue_items(items, mode) + self.queue().lock().expect("queue mutex poisoned").enqueue_items(items, mode) } } diff --git a/pmocontrol/src/queue/music_queue.rs b/pmocontrol/src/queue/music_queue.rs index 642f8c7b..4c3c0b4b 100644 --- a/pmocontrol/src/queue/music_queue.rs +++ b/pmocontrol/src/queue/music_queue.rs @@ -106,7 +106,7 @@ impl MusicQueue { on_complete: Box, ) -> SyncScheduleOutcome { let (sync_in_progress, sync_pending, sync_cancel_token) = { - let q = queue_arc.lock().unwrap(); + let q = queue_arc.lock().expect("MusicQueue mutex poisoned"); ( Arc::clone(&q.sync_in_progress), Arc::clone(&q.sync_pending), @@ -144,6 +144,10 @@ impl MusicQueue { } } let _guard = Guard(Arc::clone(&sync_in_progress)); + // Error policy: sync errors are logged by sync_worker_loop (warn level) and + // the thread exits normally. No restart — a new sync can be scheduled via + // schedule_sync. The Guard Drop clears sync_in_progress unconditionally, + // even on panic, keeping the AtomicBool protocol consistent. tracing::debug!(thread = %std::thread::current().name().unwrap_or("?"), "queue-sync thread started"); Self::sync_worker_loop( queue_arc, @@ -204,7 +208,7 @@ impl MusicQueue { ); let result = { - let mut q = queue_arc.lock().unwrap(); + let mut q = queue_arc.lock().expect("MusicQueue mutex poisoned"); ::sync_queue( &mut q, current_items, @@ -249,7 +253,7 @@ impl MusicQueue { "queue-sync: completed successfully" ); if let Some(cb) = on_complete.take() { - let queue_len = queue_arc.lock().unwrap().len().unwrap_or(0); + let queue_len = queue_arc.lock().expect("MusicQueue mutex poisoned").len().unwrap_or(0); cb(queue_len); } } diff --git a/pmocontrol/src/queue/openhome.rs b/pmocontrol/src/queue/openhome.rs index 69ae1eb1..eb475a32 100644 --- a/pmocontrol/src/queue/openhome.rs +++ b/pmocontrol/src/queue/openhome.rs @@ -233,16 +233,16 @@ impl OpenHomeQueue { /// Invalide les caches track_ids et read_list (après insert/delete sans impact sur la piste courante). fn invalidate_track_caches(&self) { - self.track_ids_cache.lock().unwrap().invalidate(); - self.read_list_cache.lock().unwrap().invalidate(); + self.track_ids_cache.lock().expect("track_ids_cache mutex poisoned").invalidate(); + self.read_list_cache.lock().expect("read_list_cache mutex poisoned").invalidate(); } /// Invalide tous les caches (après delete_all, seek, stop — opérations qui changent la piste courante). fn invalidate_all_caches(&self) { - self.track_ids_cache.lock().unwrap().invalidate(); - self.read_list_cache.lock().unwrap().invalidate(); - self.current_track_id_cache.lock().unwrap().invalidate(); - self.uri_by_id.lock().unwrap().clear(); + self.track_ids_cache.lock().expect("track_ids_cache mutex poisoned").invalidate(); + self.read_list_cache.lock().expect("read_list_cache mutex poisoned").invalidate(); + self.current_track_id_cache.lock().expect("current_track_id_cache mutex poisoned").invalidate(); + self.uri_by_id.lock().expect("uri_by_id mutex poisoned").clear(); } /// Tries to detect a simple append-only or delete-from-end pattern without ReadList. @@ -390,8 +390,8 @@ impl OpenHomeQueue { new_metadata: Option, uri: &str, ) { - let mut cache = self.metadata_cache.lock().unwrap(); - let mut uri_cache = self.uri_by_id.lock().unwrap(); + let mut cache = self.metadata_cache.lock().expect("metadata_cache mutex poisoned"); + let mut uri_cache = self.uri_by_id.lock().expect("uri_by_id mutex poisoned"); // Update URI cache if !uri.is_empty() { @@ -481,7 +481,7 @@ impl OpenHomeQueue { // Le cache contient les métadonnées stables mises lors de l'insertion // Les métadonnées de l'entry (venant de ReadList) changent pour les streams let metadata = { - let cache = self.metadata_cache.lock().unwrap(); + let cache = self.metadata_cache.lock().expect("metadata_cache mutex poisoned"); if let Some(cached_meta) = cache.get(&entry.id) { // Utiliser les métadonnées stables du cache tracing::trace!( @@ -557,7 +557,7 @@ impl OpenHomeQueue { } if track_id as usize != playing_id { self.playlist_client.delete_id_if_exists(track_id)?; - self.metadata_cache.lock().unwrap().remove(&track_id); + self.metadata_cache.lock().expect("metadata_cache mutex poisoned").remove(&track_id); } } @@ -616,7 +616,7 @@ impl OpenHomeQueue { track_id ); self.playlist_client.delete_id_if_exists(track_id)?; - self.metadata_cache.lock().unwrap().remove(&track_id); + self.metadata_cache.lock().expect("metadata_cache mutex poisoned").remove(&track_id); } } Ok(()) @@ -858,7 +858,7 @@ impl OpenHomeQueue { "Using delete_all() for complete replacement (safe - no current track or not in new playlist)" ); self.playlist_client.delete_all()?; - self.metadata_cache.lock().unwrap().clear(); + self.metadata_cache.lock().expect("metadata_cache mutex poisoned").clear(); } } else { for idx in (0..current_track_ids.len()).rev() { @@ -868,7 +868,7 @@ impl OpenHomeQueue { if !keep_current[idx] { let track_id = current_track_ids[idx]; self.playlist_client.delete_id_if_exists(track_id)?; - self.metadata_cache.lock().unwrap().remove(&track_id); + self.metadata_cache.lock().expect("metadata_cache mutex poisoned").remove(&track_id); } } } @@ -1099,7 +1099,7 @@ impl QueueBackend for OpenHomeQueue { self.ensure_playlist_source_selected()?; // Lock the cache for the entire operation to prevent race conditions - let mut cache = self.track_ids_cache.lock().unwrap(); + let mut cache = self.track_ids_cache.lock().expect("track_ids_cache mutex poisoned"); // Check if cache is valid if let Some(cached_ids) = cache.get() { @@ -1147,7 +1147,7 @@ impl QueueBackend for OpenHomeQueue { fn current_track(&self) -> Result, ControlPointError> { // Hold lock during entire operation to prevent race conditions - let mut cache = self.current_track_id_cache.lock().unwrap(); + let mut cache = self.current_track_id_cache.lock().expect("current_track_id_cache mutex poisoned"); // Return cached value if valid if let Some(cached_id) = cache.get() { @@ -1192,7 +1192,7 @@ impl QueueBackend for OpenHomeQueue { const MAX_BATCH: usize = 256; let mut entries = Vec::with_capacity(ids.len()); for chunk in ids.chunks(MAX_BATCH) { - if let Some(cached) = self.read_list_cache.lock().unwrap().get(chunk) { + if let Some(cached) = self.read_list_cache.lock().expect("read_list_cache mutex poisoned").get(chunk) { trace!( renderer = self.renderer_id.0.as_str(), "ReadList cache hit for {} IDs", @@ -1283,7 +1283,7 @@ impl QueueBackend for OpenHomeQueue { self.ensure_playlist_source_selected()?; self.playlist_client.delete_all()?; - self.metadata_cache.lock().unwrap().clear(); + self.metadata_cache.lock().expect("metadata_cache mutex poisoned").clear(); // Invalidate caches after delete_all (clears queue and current track) self.invalidate_all_caches(); @@ -1340,7 +1340,7 @@ impl QueueBackend for OpenHomeQueue { "sync_queue: Empty playlist - clearing queue with delete_all" ); self.playlist_client.delete_all()?; - self.metadata_cache.lock().unwrap().clear(); + self.metadata_cache.lock().expect("metadata_cache mutex poisoned").clear(); self.invalidate_all_caches(); let post_current_track = self.playlist_client.id().ok(); @@ -1364,7 +1364,7 @@ impl QueueBackend for OpenHomeQueue { .last() .copied() .unwrap_or(OPENHOME_PLAYLIST_HEAD_ID); - let mut uri_cache = self.uri_by_id.lock().unwrap(); + let mut uri_cache = self.uri_by_id.lock().expect("uri_by_id mutex poisoned"); for item in &new_items { if cancel_token.load(SeqCst) { return Err(ControlPointError::SyncCancelled); @@ -1583,8 +1583,8 @@ impl QueueBackend for OpenHomeQueue { .insert(before_id, &item.uri, &metadata)?; // Mettre à jour le cache avec les nouvelles métadonnées - self.metadata_cache.lock().unwrap().remove(&track_id); - self.uri_by_id.lock().unwrap().remove(&track_id); + self.metadata_cache.lock().expect("metadata_cache mutex poisoned").remove(&track_id); + self.uri_by_id.lock().expect("uri_by_id mutex poisoned").remove(&track_id); self.cache_metadata(new_id, item.metadata, &item.uri); if ci == Some(index) { diff --git a/pmocontrol/src/upnp_clients/openhome_client.rs b/pmocontrol/src/upnp_clients/openhome_client.rs index 0330de39..d61007ee 100644 --- a/pmocontrol/src/upnp_clients/openhome_client.rs +++ b/pmocontrol/src/upnp_clients/openhome_client.rs @@ -817,7 +817,7 @@ impl OhProductClient { pub fn source_xml(&self) -> Result> { // Lock the cache for the entire operation to prevent race conditions - let mut cache = self.source_xml_cache.lock().unwrap(); + let mut cache = self.source_xml_cache.lock().expect("source_xml_cache mutex poisoned"); // Check if cache is valid if let Some(cached_sources) = cache.get() { @@ -841,7 +841,7 @@ impl OhProductClient { pub fn source_index(&self) -> Result { // Lock the cache for the entire operation to prevent race conditions - let mut cache = self.source_index_cache.lock().unwrap(); + let mut cache = self.source_index_cache.lock().expect("source_index_cache mutex poisoned"); // Check if cache is valid if let Some(cached_index) = cache.get() { @@ -881,7 +881,7 @@ impl OhProductClient { // Invalidate cache after write operation if result.is_ok() { - let mut cache = self.source_index_cache.lock().unwrap(); + let mut cache = self.source_index_cache.lock().expect("source_index_cache mutex poisoned"); cache.invalidate(); } -- 2.49.1