diff --git a/Blackboard/Done/Bug_lecture_open_home.md b/Blackboard/Done/Bug_lecture_open_home.md new file mode 100644 index 00000000..d3e4b97a --- /dev/null +++ b/Blackboard/Done/Bug_lecture_open_home.md @@ -0,0 +1,82 @@ +# Bug : Duplication de piste en position 0 lors de la lecture + +**Statut** : Terminé +**Crate** : pmocontrol + +--- + +## Description initiale du bug + +### Contexte +Lecture d'une playlist liée (bindée) à une queue OpenHome ou interne. + +### Comportement observé +1. Sélection d'une playlist → la queue se charge correctement +2. Lecture démarre à la piste 1 → OK +3. Fin de la piste 1 → passage à la piste 2 +4. **BUG** : Après un certain délai (~60s), la piste 2 est **dupliquée en position 0** +5. La lecture continue depuis cette nouvelle position 0 + +### Symptôme clé +Toute piste en cours de lecture finit par être dupliquée en première position de la queue. + +--- + +## Analyse et cause racine + +### Mécanisme du bug +La fonction `sync_queue` (appelée lors des refreshes périodiques de playlist toutes les 60 secondes) comparait les items **uniquement par leur URI**. + +Si le MediaServer retournait une URI légèrement différente pour le même morceau (tokens de session, encodage différent, etc.), l'item courant n'était pas reconnu dans la nouvelle playlist et était préservé en position 0, créant une duplication. + +### Flux problématique +``` +1. Playlist attachée → lecture piste N +2. Refresh périodique (60s) → sync_queue() +3. Comparaison URI courante vs URIs playlist +4. URI non trouvée → piste préservée en position 0 +5. Résultat : duplication +``` + +--- + +## Solution implémentée + +### Principe +Extension de la logique de comparaison pour utiliser l'URI **OU** le `didl_id` comme critère d'identification. Le `didl_id` est l'identifiant DIDL-Lite stable assigné par le MediaServer, indépendant de l'URI de streaming. + +### Fichiers modifiés + +| Fichier | Modifications | +|---------|---------------| +| `pmocontrol/src/queue/interne.rs` | Comparaison par `didl_id` en fallback + logs diagnostic | +| `pmocontrol/src/queue/openhome.rs` | Fonction `items_match` + modification `sync_queue` et `lcs_flags` | + +### Code clé + +```rust +// pmocontrol/src/queue/openhome.rs +fn items_match(a: &PlaybackItem, b: &PlaybackItem) -> bool { + a.uri == b.uri || a.didl_id == b.didl_id +} +``` + +```rust +// pmocontrol/src/queue/interne.rs +let new_idx = items.iter().position(|item| item.uri == current_uri) + .or_else(|| items.iter().position(|item| item.didl_id == current_didl_id)); +``` + +--- + +## Diagnostic + +Pour activer les logs : + +```bash +RUST_LOG=pmocontrol::queue=debug +``` + +Messages de trace : +- `sync_queue: current item found in new playlist` → Comportement normal +- `sync_queue: current item NOT found in new playlist` → Cas problématique (ne devrait plus apparaître) diff --git a/Blackboard/Done/bug_lecture_queue_interne.md b/Blackboard/Done/bug_lecture_queue_interne.md new file mode 100644 index 00000000..63b50015 --- /dev/null +++ b/Blackboard/Done/bug_lecture_queue_interne.md @@ -0,0 +1,45 @@ +# Bug lecture queue interne - RESOLU + +## Tâche originale + +**Crate concernée** : pmocontrol + +**Problème rapporté** : Lors de la lecture sur un Renderer avec queue interne, si l'utilisateur clique sur un item de la queue pour déclencher sa lecture, tout semble se passer normalement pendant une seconde. Puis, avant que la lecture ne démarre réellement, le lecteur passe à la piste suivante. + +--- + +## Synthèse de la résolution + +### Cause racine + +Race condition dans la logique d'auto-advance du watcher. Quand l'utilisateur sélectionne une piste : +1. Les commandes UPnP `SetAVTransportURI` + `Play` sont envoyées +2. Le renderer passe brièvement par un état `STOPPED` pendant l'initialisation +3. Le watcher détecte ce `STOPPED` et déclenche l'auto-advance vers la piste suivante + +Le système ne distinguait pas un état `STOPPED` transitoire (initialisation) d'un état `STOPPED` réel (fin de piste). + +### Solution + +Ajout d'un flag `has_played_since_track_start` dans `MusicRendererState` : + +- **Remis à `false`** au démarrage d'une nouvelle piste (`play_from_index`, `play_from_queue`, etc.) et lors d'un `stop()` +- **Passé à `true`** quand l'état `PLAYING` est détecté par le watcher +- **L'auto-advance n'est autorisé** que si le flag est `true` + +Ainsi, un état `STOPPED` transitoire (avant que `PLAYING` ne soit observé) n'entraîne plus d'auto-advance. + +### Fichier modifié + +- `pmocontrol/src/music_renderer/musicrenderer.rs` + +### Méthodes ajoutées/modifiées + +- `MusicRendererState.has_played_since_track_start` (nouveau champ) +- `set_has_played_flag()`, `clear_has_played_flag()`, `check_and_clear_has_played_flag()` (nouvelles méthodes) +- `handle_state_change()` (modifié pour utiliser le flag) +- `play_current_from_queue()`, `play_next_from_queue()`, `play_from_index()`, `play_from_queue()`, `stop()` (modifiés pour réinitialiser le flag) + +--- + +**Statut** : Corrigé et testé diff --git a/Blackboard/Report/Bug_lecture_open_home.md b/Blackboard/Report/Bug_lecture_open_home.md new file mode 100644 index 00000000..65ab1e73 --- /dev/null +++ b/Blackboard/Report/Bug_lecture_open_home.md @@ -0,0 +1,76 @@ +# Rapport : Bug de duplication de piste en position 0 + +## Résumé + +Correction d'un bug où la piste en cours de lecture était dupliquée en position 0 de la queue après un certain temps. + +## Problème identifié + +### Symptôme +Lors de la lecture d'une playlist liée à une queue (OpenHome ou interne), après le passage à une nouvelle piste, celle-ci finissait par être dupliquée en première position de la queue. + +### Cause racine +La fonction `sync_queue` (utilisée lors des refreshes périodiques de playlist toutes les 60 secondes) comparait les items uniquement par leur URI. Si le MediaServer retournait une URI légèrement différente pour le même morceau (tokens de session, encodage différent, etc.), l'item courant n'était pas reconnu dans la nouvelle playlist et était préservé en position 0, créant ainsi une duplication. + +### Mécanisme détaillé +1. Une playlist est attachée à un renderer +2. La lecture commence sur la piste N +3. Après 60 secondes, un refresh périodique déclenche `sync_queue` +4. `sync_queue` compare l'URI de la piste courante avec les URIs de la playlist rafraîchie +5. Si les URIs ne correspondent pas exactement, la piste courante est considérée comme "absente" de la playlist +6. La logique de préservation insère alors la piste courante en position 0 +7. Résultat : duplication de la piste + +## Solution appliquée + +### Modification de la logique de comparaison + +La comparaison des items a été étendue pour utiliser l'URI **OU** le `didl_id` comme critère d'identification. Le `didl_id` est l'identifiant DIDL-Lite stable assigné par le MediaServer, indépendant de l'URI de streaming. + +### Fichiers modifiés + +#### 1. `pmocontrol/src/queue/interne.rs` + +- Ajout de la comparaison par `didl_id` en fallback dans `sync_queue` +- Ajout de logs de diagnostic pour tracer les cas de non-correspondance + +```rust +// Avant +let new_idx = items.iter().position(|item| item.uri == current_uri); + +// Après +let new_idx = items.iter().position(|item| item.uri == current_uri) + .or_else(|| items.iter().position(|item| item.didl_id == current_didl_id)); +``` + +#### 2. `pmocontrol/src/queue/openhome.rs` + +- Ajout de la fonction `items_match` pour encapsuler la logique de comparaison +- Modification de `sync_queue` pour utiliser URI ou `didl_id` +- Modification de `lcs_flags` (algorithme LCS) pour utiliser la même logique de comparaison + +```rust +fn items_match(a: &PlaybackItem, b: &PlaybackItem) -> bool { + a.uri == b.uri || a.didl_id == b.didl_id +} +``` + +## Tests recommandés + +1. Attacher une playlist à un renderer OpenHome +2. Lancer la lecture +3. Attendre plusieurs cycles de refresh (> 60 secondes) +4. Vérifier que la queue ne contient pas de duplications +5. Cliquer sur différentes pistes et vérifier le même comportement + +## Diagnostic + +Pour activer les logs de diagnostic : + +```bash +RUST_LOG=pmocontrol::queue=debug +``` + +Les messages suivants permettent de tracer le comportement : +- `sync_queue: current item found in new playlist` - Comportement normal +- `sync_queue: current item NOT found in new playlist, preserving as first item` - Cas problématique (ne devrait plus apparaître avec le fix) diff --git a/Blackboard/Report/bug_lecture_queue_interne.md b/Blackboard/Report/bug_lecture_queue_interne.md new file mode 100644 index 00000000..1d43c3a7 --- /dev/null +++ b/Blackboard/Report/bug_lecture_queue_interne.md @@ -0,0 +1,79 @@ +# Rapport : Correction du bug de lecture sur queue interne + +## Problème + +Lors de la lecture sur un Renderer avec queue interne, si l'utilisateur clique sur un item de la queue pour déclencher sa lecture, tout semble se passer normalement pendant une seconde. Puis, avant que la lecture ne démarre réellement, le lecteur passe à la piste suivante. + +## Analyse + +### Cause identifiée + +Le problème était une **race condition** dans la logique d'auto-advance du watcher. + +Quand l'utilisateur clique sur un item de la queue : +1. `play_queue_index` est appelé dans `ControlPoint` +2. Les commandes UPnP `SetAVTransportURI` + `Play` sont envoyées au renderer +3. Le renderer peut passer brièvement par un état `STOPPED` pendant l'initialisation de la nouvelle piste +4. Le watcher (polling toutes les 500ms) détecte cet état `STOPPED` +5. Comme la lecture était lancée depuis la queue (`PlaybackSource::FromQueue`), l'auto-advance se déclenche et passe à la piste suivante + +### Détail technique + +La logique d'auto-advance dans `handle_state_change` vérifie si `is_playing_from_queue()` retourne `true` pour décider de passer à la piste suivante quand l'état `STOPPED` est détecté. Cependant, il n'y avait aucun mécanisme pour distinguer : +- Un état `STOPPED` transitoire pendant l'initialisation d'une nouvelle piste +- Un état `STOPPED` réel indiquant la fin de lecture d'une piste + +## Solution implémentée + +Ajout d'un flag `has_played_since_track_start` dans `MusicRendererState` qui permet de tracker si l'état `PLAYING` a été observé depuis le dernier démarrage de piste. + +### Logique du flag + +1. **Quand on démarre une nouvelle piste** (`play_from_index`, `play_from_queue`, `play_next_from_queue`, `play_current_from_queue`) : le flag est remis à `false` + +2. **Quand le watcher détecte l'état `PLAYING`** : le flag passe à `true` + +3. **Quand le watcher détecte l'état `STOPPED`** : + - Si `has_played_since_track_start == true` : c'est une vraie fin de piste → auto-advance autorisé + - Si `has_played_since_track_start == false` : c'est un état transitoire pendant l'initialisation → auto-advance bloqué + +4. **Quand `stop()` est appelé** : le flag est remis à `false` + +## Fichiers modifiés + +### `pmocontrol/src/music_renderer/musicrenderer.rs` + +1. **Ajout du champ `has_played_since_track_start`** dans `MusicRendererState` : +```rust +struct MusicRendererState { + // ... + /// Flag indicating that a PLAYING state has been observed since the last track start. + /// This prevents auto-advance on transient STOPPED states during track initialization. + /// Auto-advance is only allowed when this flag is true. + has_played_since_track_start: bool, +} +``` + +2. **Ajout des méthodes de gestion du flag** : + - `set_has_played_flag()` : met le flag à `true` + - `clear_has_played_flag()` : met le flag à `false` (publique) + - `check_and_clear_has_played_flag()` : vérifie et remet à `false` + +3. **Modification de `handle_state_change`** : + - Sur `PLAYING` : appelle `set_has_played_flag()` + - Sur `STOPPED` avec `is_playing_from_queue()` : vérifie `check_and_clear_has_played_flag()` avant d'auto-advance + +4. **Modification des méthodes de démarrage de lecture** : + - `play_current_from_queue()` + - `play_next_from_queue()` + - `play_from_index()` + - `play_from_queue()` + - `stop()` + + Toutes appellent `clear_has_played_flag()` pour réinitialiser le flag. + +## Tests effectués + +- Clic sur différents items de la queue : la piste sélectionnée est bien jouée sans saut +- Lecture normale jusqu'à la fin d'une piste : l'auto-advance vers la piste suivante fonctionne correctement +- Arrêt manuel (stop) : pas d'auto-advance intempestif diff --git a/Blackboard/Todo/bug_url_cover.md b/Blackboard/Todo/bug_url_cover.md new file mode 100644 index 00000000..b4993223 --- /dev/null +++ b/Blackboard/Todo/bug_url_cover.md @@ -0,0 +1,16 @@ +**Il faut suivre les instructions générales placées dans le fichier : Blackboard/Rules.md** + +## Crate concernée +- **pmoplaylist** +- **pmocache** +- **pmoaudiocache** +- **pmocover** +- **pmodidl** + +Le bug doit se situer dans la crâte PMO Playlist. Les autres crates ne sont cités car elles doivent être utilisées par PMO Playlist pour cette fonctionnalité. + +## Description du bug + +Le document didl Généré par les PMO playlists, Possède une URL absolue pour le flux audio, Mais relative pour l'URL de la cover. Les deux entités flux audio et cover sont stockées dans des caches PMO audio cache et PMO Cover respectivement. + +Il faut comprendre comment est construite l'URL absolue du flux audio et appliquer la même recette à l'URL de la cover. diff --git a/pmocontrol/src/music_renderer/musicrenderer.rs b/pmocontrol/src/music_renderer/musicrenderer.rs index ba33df38..7a6de226 100644 --- a/pmocontrol/src/music_renderer/musicrenderer.rs +++ b/pmocontrol/src/music_renderer/musicrenderer.rs @@ -91,6 +91,10 @@ struct MusicRendererState { user_stop_requested: bool, /// Sleep timer for auto-stop functionality. sleep_timer: SleepTimer, + /// Flag indicating that a PLAYING state has been observed since the last track start. + /// This prevents auto-advance on transient STOPPED states during track initialization. + /// Auto-advance is only allowed when this flag is true. + has_played_since_track_start: bool, } #[derive(Clone)] @@ -422,25 +426,39 @@ impl MusicRenderer { "Renderer stopped by user request; not auto-advancing" ); self.set_playback_source(PlaybackSource::None); + self.clear_has_played_flag(); } else if self.is_playing_from_queue() { - debug!( - renderer = self.info.friendly_name(), - "Renderer stopped after queue-driven playback; advancing" - ); - if let Err(err) = self.play_next_from_queue() { - error!( + // Only auto-advance if we have actually seen a PLAYING state + // since the track was started. This prevents auto-advance on + // transient STOPPED states during track initialization. + if self.check_and_clear_has_played_flag() { + debug!( renderer = self.info.friendly_name(), - error = %err, - "Auto-advance failed; clearing queue playback state" + "Renderer stopped after queue-driven playback; advancing" + ); + if let Err(err) = self.play_next_from_queue() { + error!( + renderer = self.info.friendly_name(), + error = %err, + "Auto-advance failed; clearing queue playback state" + ); + self.set_playback_source(PlaybackSource::None); + } + } else { + debug!( + renderer = self.info.friendly_name(), + "Renderer stopped but no PLAYING state seen yet; ignoring (likely track initialization)" ); - self.set_playback_source(PlaybackSource::None); } } else { self.set_playback_source(PlaybackSource::None); + self.clear_has_played_flag(); } } PlaybackState::Playing => { self.mark_external_if_idle(); + // Mark that we have seen a PLAYING state - auto-advance is now allowed + self.set_has_played_flag(); } _ => {} } @@ -566,6 +584,11 @@ impl MusicRenderer { /// Play the current item from the queue. pub fn play_current_from_queue(&self) -> Result<(), ControlPointError> { + // Reset the has_played flag before starting playback to prevent + // auto-advance on transient STOPPED states during track initialization. + // The flag will be set back to true when PLAYING state is detected. + self.clear_has_played_flag(); + self.backend .lock() .expect("Backend mutex poisoned") @@ -574,6 +597,11 @@ impl MusicRenderer { /// Advance to and play the next item from the queue. pub fn play_next_from_queue(&self) -> Result<(), ControlPointError> { + // Reset the has_played flag before starting playback to prevent + // auto-advance on transient STOPPED states during track initialization. + // The flag will be set back to true when PLAYING state is detected. + self.clear_has_played_flag(); + self.backend .lock() .expect("Backend mutex poisoned") @@ -584,6 +612,11 @@ impl MusicRenderer { /// Play from a specific index in the queue. pub fn play_from_index(&self, index: usize) -> Result<(), ControlPointError> { + // Reset the has_played flag before starting playback to prevent + // auto-advance on transient STOPPED states during track initialization. + // The flag will be set back to true when PLAYING state is detected. + self.clear_has_played_flag(); + self.backend .lock() .expect("Backend mutex poisoned") @@ -618,6 +651,12 @@ impl MusicRenderer { /// Transport control: stop pub fn stop(&self) -> Result<(), ControlPointError> { + // Reset the has_played flag when stopping playback. + // This ensures that if we start a new track, the flag will be false + // until PLAYING state is observed, preventing auto-advance on + // transient STOPPED states during track initialization. + self.clear_has_played_flag(); + self.backend.lock().expect("Backend mutex poisoned").stop() } @@ -972,6 +1011,11 @@ impl MusicRenderer { /// /// Uses the backend's play_from_queue which preserves the queue for all backends. pub fn play_from_queue(&self) -> Result<(), ControlPointError> { + // Reset the has_played flag before starting playback to prevent + // auto-advance on transient STOPPED states during track initialization. + // The flag will be set back to true when PLAYING state is detected. + self.clear_has_played_flag(); + let backend = self.backend.lock().expect("Backend mutex poisoned"); backend.play_from_queue() } @@ -1027,6 +1071,31 @@ impl MusicRenderer { was_requested } + // --- Has-Played Flag Management (for auto-advance protection) --- + + /// 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; + } + + /// 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; + } + + /// 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 has_played = state.has_played_since_track_start; + state.has_played_since_track_start = false; + has_played + } + // --- Sleep Timer Management --- /// Starts the sleep timer with the given duration in seconds. diff --git a/pmocontrol/src/queue/interne.rs b/pmocontrol/src/queue/interne.rs index 8b835f67..8e95904d 100644 --- a/pmocontrol/src/queue/interne.rs +++ b/pmocontrol/src/queue/interne.rs @@ -132,24 +132,49 @@ impl QueueBackend for InternalQueue { } fn sync_queue(&mut self, items: Vec) -> Result<(), ControlPointError> { + use tracing::debug; + if items.is_empty() { return self.replace_queue(Vec::new(), None); } // Récupérer l'item actuel - let current = self - .current_index - .and_then(|idx| self.items.get(idx).map(|item| (idx, item.uri.clone()))); + let current = self.current_index.and_then(|idx| { + self.items + .get(idx) + .map(|item| (idx, item.uri.clone(), item.didl_id.clone())) + }); - if let Some((_current_idx, current_uri)) = current { - // Chercher l'item actuel dans la nouvelle liste (par URI) - let new_idx = items.iter().position(|item| item.uri == current_uri); + if let Some((_current_idx, current_uri, current_didl_id)) = current { + // Chercher l'item actuel dans la nouvelle liste (par URI d'abord, puis par didl_id) + let new_idx = items + .iter() + .position(|item| item.uri == current_uri) + .or_else(|| { + items + .iter() + .position(|item| item.didl_id == current_didl_id) + }); if let Some(new_idx) = new_idx { // Item trouvé dans la nouvelle liste + debug!( + renderer = self.renderer_id.0.as_str(), + current_uri = current_uri.as_str(), + new_idx, + "sync_queue: current item found in new playlist" + ); self.replace_queue(items, Some(new_idx)) } else { - // Item pas trouvé, le garder comme premier + // Item pas trouvé - cela ne devrait pas arriver si la playlist n'a pas changé + // Loguer pour diagnostic + debug!( + renderer = self.renderer_id.0.as_str(), + current_uri = current_uri.as_str(), + current_didl_id = current_didl_id.as_str(), + new_items_count = items.len(), + "sync_queue: current item NOT found in new playlist, preserving as first item" + ); let current_item = self.items[self.current_index.unwrap()].clone(); let mut new_items = Vec::with_capacity(items.len() + 1); new_items.push(current_item); diff --git a/pmocontrol/src/queue/openhome.rs b/pmocontrol/src/queue/openhome.rs index 20618bfd..534361c6 100644 --- a/pmocontrol/src/queue/openhome.rs +++ b/pmocontrol/src/queue/openhome.rs @@ -444,6 +444,14 @@ fn build_metadata_xml(item: &PlaybackItem) -> String { xml } +/// Compare two PlaybackItems for equality. +/// Items are considered equal if they have the same URI OR the same didl_id. +/// This allows matching items even when the MediaServer returns different URIs +/// for the same logical track (e.g., with session tokens or different encodings). +fn items_match(a: &PlaybackItem, b: &PlaybackItem) -> bool { + a.uri == b.uri || a.didl_id == b.didl_id +} + fn lcs_flags(current: &[PlaybackItem], desired: &[PlaybackItem]) -> (Vec, Vec) { let m = current.len(); let n = desired.len(); @@ -451,7 +459,7 @@ fn lcs_flags(current: &[PlaybackItem], desired: &[PlaybackItem]) -> (Vec, for i in 0..m { for j in 0..n { - if current[i].uri == desired[j].uri { + if items_match(¤t[i], &desired[j]) { dp[i + 1][j + 1] = dp[i][j] + 1; } else { dp[i + 1][j + 1] = dp[i + 1][j].max(dp[i][j + 1]); @@ -464,7 +472,7 @@ fn lcs_flags(current: &[PlaybackItem], desired: &[PlaybackItem]) -> (Vec, let (mut i, mut j) = (m, n); while i > 0 && j > 0 { - if current[i - 1].uri == desired[j - 1].uri { + if items_match(¤t[i - 1], &desired[j - 1]) { keep_current[i - 1] = true; keep_desired[j - 1] = true; i -= 1; @@ -616,6 +624,7 @@ impl QueueBackend for OpenHomeQueue { idx, snapshot.items[idx].backend_id, snapshot.items[idx].uri.clone(), + snapshot.items[idx].didl_id.clone(), )) }); @@ -626,9 +635,16 @@ impl QueueBackend for OpenHomeQueue { "OpenHome playlist state" ); - if let Some((playing_idx, playing_id, playing_uri)) = playing_info { - // Find if the currently playing item is in the new playlist (by URI) - let new_playing_idx = items.iter().position(|item| item.uri == playing_uri); + if let Some((playing_idx, playing_id, playing_uri, playing_didl_id)) = playing_info { + // Find if the currently playing item is in the new playlist (by URI first, then by didl_id) + let new_playing_idx = items + .iter() + .position(|item| item.uri == playing_uri) + .or_else(|| { + items + .iter() + .position(|item| item.didl_id == playing_didl_id) + }); if let Some(pivot_idx) = new_playing_idx { // CASE 2: Currently playing item IS in the new playlist