diff --git a/pmocontrol/src/control_point.rs b/pmocontrol/src/control_point.rs index 073ed397..b4210ca8 100644 --- a/pmocontrol/src/control_point.rs +++ b/pmocontrol/src/control_point.rs @@ -1512,6 +1512,20 @@ impl ControlPoint { container_id: &str, auto_play: bool, ) -> anyhow::Result<()> { + // CRITICAL: When attaching a new playlist to a renderer, we must UNCONDITIONALLY + // clear the queue first. This is different from refreshing an already-attached + // playlist (which uses gentle sync to avoid interrupting playback). + // + // Attach workflow: Stop (if playing) → Clear → Fill → Play + // Update workflow: Gentle sync (preserve current item, use LCS) + info!( + renderer = renderer_id.0.as_str(), + server = server_id.0.as_str(), + container = container_id, + "Attaching new playlist: clearing queue first" + ); + self.clear_queue(renderer_id)?; + let binding = PlaylistBinding { server_id: server_id.clone(), container_id: container_id.to_string(), diff --git a/pmocontrol/src/control_point/openhome_queue.rs b/pmocontrol/src/control_point/openhome_queue.rs index d02368e2..3e861280 100644 --- a/pmocontrol/src/control_point/openhome_queue.rs +++ b/pmocontrol/src/control_point/openhome_queue.rs @@ -58,20 +58,51 @@ impl OpenHomeQueue { track_ids.push(entry.id); } - let current_id = self - .info_client - .as_ref() - .and_then(|client| client.id().ok()); - let previous_index = self - .current_index - .and_then(|idx| if idx < track_ids.len() { Some(idx) } else { None }); - let mut current_index = current_id - .and_then(|id| track_ids.iter().position(|entry_id| *entry_id == id)) - .or(previous_index); + // Try multiple methods to determine the currently playing track, from most to least reliable: + // 1. Info.Id() - Direct ID query (fastest, but fails if track no longer in playlist) + // 2. Info.Track() - Returns URI, which we can search for (works even if track removed) + // 3. None - No current track can be determined + let current_id = self.info_client.as_ref().and_then(|client| { + // Try Info.Id() first + if let Ok(id) = client.id() { + debug!( + renderer = self.renderer_id.0.as_str(), + track_id = id, + "Detected current track via Info.Id()" + ); + return Some(id); + } - if current_index.is_none() && !track_ids.is_empty() { - current_index = Some(0); - } + // If Id() fails, try Track() to get the URI and search for it + if let Ok(track_info) = client.track() { + debug!( + renderer = self.renderer_id.0.as_str(), + track_uri = track_info.uri.as_str(), + "Info.Id() failed, searching for current track by URI from Info.Track()" + ); + return entries + .iter() + .find(|entry| entry.uri == track_info.uri) + .map(|entry| { + debug!( + renderer = self.renderer_id.0.as_str(), + found_id = entry.id, + found_uri = entry.uri.as_str(), + "Found current track ID by matching URI" + ); + entry.id + }); + } + + debug!( + renderer = self.renderer_id.0.as_str(), + "Both Info.Id() and Info.Track() failed, cannot determine current track" + ); + None + }); + + let current_index = current_id + .and_then(|id| track_ids.iter().position(|entry_id| *entry_id == id)); self.items = items; self.track_ids = track_ids; @@ -283,6 +314,389 @@ impl OpenHomeQueue { .copied() .ok_or_else(|| anyhow!("Failed to resolve OpenHome track id at index {}", index)) } + + /// CASE 1: Replace queue while preserving the currently playing item as first. + /// The currently playing item is NOT in the new playlist, so we keep it as the first + /// item and append the entire new playlist after it. + fn replace_queue_preserve_current( + &mut self, + new_items: Vec, + playing_idx: usize, + playing_id: u32, + ) -> Result<()> { + // Re-read the current playlist state to get fresh IDs + // This minimizes race conditions where IDs become invalid between our last refresh + // and now (due to UPnP events from the server) + let current_entries = self.playlist.read_all_tracks()?; + let current_ids: Vec = current_entries.iter().map(|e| e.id).collect(); + + debug!( + renderer = self.renderer_id.0.as_str(), + fresh_id_count = current_ids.len(), + cached_id_count = self.track_ids.len(), + "Re-read playlist before deletions to avoid stale ID errors" + ); + + // Find the playing track in the fresh list + let fresh_playing_idx = current_ids.iter().position(|&id| id == playing_id); + + if fresh_playing_idx.is_none() { + debug!( + renderer = self.renderer_id.0.as_str(), + playing_id, + "Playing track not found in fresh playlist - renderer state may have changed, aborting modification" + ); + // The playing track is gone - don't try to manipulate the playlist + return Ok(()); + } + + // Delete everything except the currently playing item (using fresh IDs) + for &track_id in current_ids.iter().rev() { + if track_id != playing_id { + self.playlist.delete_id_if_exists(track_id)?; + } + } + + // Rebuild: [currently_playing, new_items...] + let mut rebuilt_items = Vec::with_capacity(1 + new_items.len()); + let mut rebuilt_ids = Vec::with_capacity(1 + new_items.len()); + + rebuilt_items.push(self.items[playing_idx].clone()); + rebuilt_ids.push(playing_id); + + let mut previous_id = playing_id; + for item in new_items { + let metadata = build_metadata_xml(&item); + let new_id = self.playlist.insert(previous_id, &item.uri, &metadata)?; + previous_id = new_id; + rebuilt_ids.push(new_id); + rebuilt_items.push(self.item_with_openhome_id(item, new_id)); + } + + self.items = rebuilt_items; + self.track_ids = rebuilt_ids; + self.current_index = Some(0); // Currently playing is now at index 0 + + debug!( + renderer = self.renderer_id.0.as_str(), + "Gentle sync completed: preserved playing track as first item (not in new playlist)" + ); + + Ok(()) + } + + /// CASE 2: Replace queue with double-LCS (before and after the pivot). + /// The currently playing item IS in the new playlist, so we use it as a pivot + /// and apply LCS separately to the portions before and after it. + fn replace_queue_with_pivot( + &mut self, + new_items: Vec, + pivot_idx_new: usize, + pivot_id: u32, + ) -> Result<()> { + debug!( + renderer = self.renderer_id.0.as_str(), + pivot_id, + pivot_idx_new, + new_playlist_len = new_items.len(), + "Starting replace_queue_with_pivot - will re-read from OpenHome" + ); + + // Re-read the current playlist state from OpenHome (the ONLY source of truth) + // This is CRITICAL to avoid deleting IDs that no longer exist, which can + // put the renderer (upmpdcli) into a degraded state where Info.TransportState() + // starts returning HTTP 500 errors. + let current_entries = self.playlist.read_all_tracks()?; + + // Convert entries to PlaybackItems - this is the REAL current state + let mut fresh_items = Vec::with_capacity(current_entries.len()); + let mut fresh_ids = Vec::with_capacity(current_entries.len()); + for entry in ¤t_entries { + fresh_items.push(self.playback_item_from_entry(entry)); + fresh_ids.push(entry.id); + } + + debug!( + renderer = self.renderer_id.0.as_str(), + fresh_count = fresh_items.len(), + "Re-read playlist from OpenHome (source of truth)" + ); + + // Find the pivot in the fresh list + let fresh_pivot_idx = fresh_ids.iter().position(|&id| id == pivot_id); + + if fresh_pivot_idx.is_none() { + debug!( + renderer = self.renderer_id.0.as_str(), + pivot_id, + "Pivot track not found in fresh playlist - renderer state changed, aborting" + ); + return Ok(()); + } + + let fresh_pivot_idx = fresh_pivot_idx.unwrap(); + + // Split fresh data at the pivot - use ONLY fresh data, ignore cache + let old_before: Vec = fresh_items[..fresh_pivot_idx].to_vec(); + let old_after: Vec = fresh_items[fresh_pivot_idx + 1..].to_vec(); + let old_ids_before: Vec = fresh_ids[..fresh_pivot_idx].to_vec(); + let old_ids_after: Vec = fresh_ids[fresh_pivot_idx + 1..].to_vec(); + + let new_before = &new_items[..pivot_idx_new]; + let new_after = &new_items[pivot_idx_new + 1..]; + + // LCS on the AFTER part (using fresh data from OpenHome) + let (keep_old_after, keep_new_after) = lcs_flags(&old_after, new_after); + + // LCS on the BEFORE part (using fresh data from OpenHome) + let (keep_old_before, keep_new_before) = lcs_flags(&old_before, new_before); + + // Delete items marked for deletion in AFTER part (reverse order) + for (idx, &track_id) in old_ids_after.iter().enumerate().rev() { + if !keep_old_after[idx] { + debug!( + renderer = self.renderer_id.0.as_str(), + track_id, + position = "AFTER pivot", + "RENDERER OP: DeleteId({})", + track_id + ); + self.playlist.delete_id_if_exists(track_id)?; + } + } + + // Delete items marked for deletion in BEFORE part (reverse order) + for (idx, &track_id) in old_ids_before.iter().enumerate().rev() { + if !keep_old_before[idx] { + debug!( + renderer = self.renderer_id.0.as_str(), + track_id, + position = "BEFORE pivot", + "RENDERER OP: DeleteId({})", + track_id + ); + self.playlist.delete_id_if_exists(track_id)?; + } + } + + // Rebuild the playlist: [BEFORE, PIVOT, AFTER] + let mut rebuilt_items = Vec::with_capacity(new_items.len()); + let mut rebuilt_ids = Vec::with_capacity(new_items.len()); + + // Collect IDs of kept items in BEFORE part (in order) + let remaining_before: Vec = old_ids_before + .iter() + .enumerate() + .filter_map(|(idx, &id)| if keep_old_before[idx] { Some(id) } else { None }) + .collect(); + + let mut remaining_before_idx = 0; + let mut previous_id = OPENHOME_PLAYLIST_HEAD_ID; + + // Rebuild BEFORE part + for (idx, item) in new_before.iter().enumerate() { + if keep_new_before[idx] { + let existing_id = remaining_before[remaining_before_idx]; + remaining_before_idx += 1; + previous_id = existing_id; + rebuilt_ids.push(existing_id); + rebuilt_items.push(self.item_with_openhome_id(item.clone(), existing_id)); + debug!( + renderer = self.renderer_id.0.as_str(), + track_id = existing_id, + position = "BEFORE pivot", + "KEPT existing track ID {}", + existing_id + ); + } else { + let metadata = build_metadata_xml(item); + let new_id = self.playlist.insert(previous_id, &item.uri, &metadata)?; + debug!( + renderer = self.renderer_id.0.as_str(), + after_id = previous_id, + new_id, + position = "BEFORE pivot", + "RENDERER OP: Insert(after={}) -> new_id={}", + previous_id, + new_id + ); + previous_id = new_id; + rebuilt_ids.push(new_id); + rebuilt_items.push(self.item_with_openhome_id(item.clone(), new_id)); + } + } + + // Add PIVOT (keeps its ID!) + rebuilt_ids.push(pivot_id); + rebuilt_items.push(self.item_with_openhome_id(new_items[pivot_idx_new].clone(), pivot_id)); + previous_id = pivot_id; + debug!( + renderer = self.renderer_id.0.as_str(), + pivot_id, + pivot_idx_new, + "PIVOT preserved with ID {} at index {}", + pivot_id, + pivot_idx_new + ); + + // Collect IDs of kept items in AFTER part (in order) + let remaining_after: Vec = old_ids_after + .iter() + .enumerate() + .filter_map(|(idx, &id)| if keep_old_after[idx] { Some(id) } else { None }) + .collect(); + + let mut remaining_after_idx = 0; + + // Rebuild AFTER part + for (idx, item) in new_after.iter().enumerate() { + if keep_new_after[idx] { + let existing_id = remaining_after[remaining_after_idx]; + remaining_after_idx += 1; + previous_id = existing_id; + rebuilt_ids.push(existing_id); + rebuilt_items.push(self.item_with_openhome_id(item.clone(), existing_id)); + debug!( + renderer = self.renderer_id.0.as_str(), + track_id = existing_id, + position = "AFTER pivot", + "KEPT existing track ID {}", + existing_id + ); + } else { + let metadata = build_metadata_xml(item); + let new_id = self.playlist.insert(previous_id, &item.uri, &metadata)?; + debug!( + renderer = self.renderer_id.0.as_str(), + after_id = previous_id, + new_id, + position = "AFTER pivot", + "RENDERER OP: Insert(after={}) -> new_id={}", + previous_id, + new_id + ); + previous_id = new_id; + rebuilt_ids.push(new_id); + rebuilt_items.push(self.item_with_openhome_id(item.clone(), new_id)); + } + } + + self.items = rebuilt_items; + self.track_ids = rebuilt_ids; + self.current_index = Some(pivot_idx_new); // Pivot is at its new position + + // VERIFICATION: Check that pivot ID is preserved + let final_pivot_id = self.track_ids.get(pivot_idx_new).copied(); + if final_pivot_id != Some(pivot_id) { + return Err(anyhow!( + "CRITICAL BUG: Pivot ID changed from {} to {:?} during replace_queue_with_pivot!", + pivot_id, + final_pivot_id + )); + } + + debug!( + renderer = self.renderer_id.0.as_str(), + pivot_idx = pivot_idx_new, + pivot_id, + final_playlist_len = self.track_ids.len(), + pivot_verified = true, + "Gentle sync completed: double-LCS with pivot (playing track preserved)" + ); + + Ok(()) + } + + /// Standard LCS-based replacement (used when no currently playing item). + fn replace_queue_standard_lcs( + &mut self, + items: Vec, + current_index: Option, + ) -> Result<()> { + let (keep_current, keep_desired) = lcs_flags(&self.items, &items); + + let items_to_keep = keep_current.iter().filter(|&&k| k).count(); + let items_to_delete = keep_current.iter().filter(|&&k| !k).count(); + let items_to_add = keep_desired.iter().filter(|&&k| !k).count(); + + debug!( + renderer = self.renderer_id.0.as_str(), + keep = items_to_keep, + delete = items_to_delete, + add = items_to_add, + "LCS computed: minimizing OpenHome playlist operations" + ); + + // If we're replacing everything (keep=0), use delete_all() instead of + // individual delete_id() calls. This is much more robust for live playlists + // where track IDs can become invalid between refresh and deletion. + if items_to_keep == 0 && items_to_delete > 0 { + debug!( + renderer = self.renderer_id.0.as_str(), + "Using delete_all() for complete replacement (more robust for live playlists)" + ); + self.playlist.delete_all()?; + self.track_ids.clear(); + self.items.clear(); + } else { + // Selective deletion when keeping some items + for idx in (0..self.track_ids.len()).rev() { + if !keep_current[idx] { + let track_id = self.track_ids[idx]; + // Use delete_id_if_exists() to handle cases where another control point + // may have already modified the playlist + self.playlist.delete_id_if_exists(track_id)?; + self.track_ids.remove(idx); + self.items.remove(idx); + } + } + } + + let remaining_ids = self.track_ids.clone(); + let mut remaining_idx = 0usize; + let mut previous_id = OPENHOME_PLAYLIST_HEAD_ID; + let mut rebuilt_items = Vec::with_capacity(items.len()); + let mut rebuilt_ids = Vec::with_capacity(items.len()); + + for (idx, item) in items.into_iter().enumerate() { + if keep_desired[idx] { + if remaining_idx >= remaining_ids.len() { + return Err(anyhow!( + "OpenHome playlist refresh bookkeeping mismatch (kept entries underflow)" + )); + } + let existing_id = remaining_ids[remaining_idx]; + remaining_idx += 1; + previous_id = existing_id; + rebuilt_ids.push(existing_id); + rebuilt_items.push(self.item_with_openhome_id(item, existing_id)); + } else { + let metadata = build_metadata_xml(&item); + let new_id = self.playlist.insert(previous_id, &item.uri, &metadata)?; + previous_id = new_id; + rebuilt_ids.push(new_id); + rebuilt_items.push(self.item_with_openhome_id(item, new_id)); + } + } + + if remaining_idx != remaining_ids.len() { + return Err(anyhow!( + "OpenHome playlist refresh bookkeeping mismatch (kept entries overflow)" + )); + } + + let previous_index = self + .current_index + .and_then(|idx| if idx < rebuilt_ids.len() { Some(idx) } else { None }); + let normalized = current_index + .filter(|&i| i < rebuilt_ids.len()) + .or(previous_index) + .or_else(|| if rebuilt_ids.is_empty() { None } else { Some(0) }); + self.items = rebuilt_items; + self.track_ids = rebuilt_ids; + self.current_index = normalized; + Ok(()) + } } pub fn didl_id_from_metadata(xml: &str) -> Option { @@ -425,94 +839,86 @@ impl QueueBackend for OpenHomeQueue { // differences. Without this, any drift between our cache and the renderer // (e.g., manual edits from another control point) would keep the stale items. self.refresh_from_openhome()?; + + // Try to get the currently playing track ID from the renderer. + // Note: Some OpenHome renderers (like upmpdcli) don't reliably support Info.Id(), + // so we fall back to using our internal current_index pointer. + let currently_playing_id_from_renderer = self + .info_client + .as_ref() + .and_then(|client| client.id().ok()); + + // Find the currently playing item in our local state. + // Priority: 1) Renderer-reported ID, 2) Our internal current_index + let playing_info = if let Some(id) = currently_playing_id_from_renderer { + // CASE: Renderer explicitly reported the playing track ID + self.track_ids + .iter() + .position(|&tid| tid == id) + .map(|idx| (idx, id, self.items[idx].uri.clone())) + } else if let Some(idx) = self.current_index { + // CASE: Use our internal pointer (fallback for renderers without Info.Id() support) + if idx < self.track_ids.len() && idx < self.items.len() { + let id = self.track_ids[idx]; + let uri = self.items[idx].uri.clone(); + debug!( + renderer = self.renderer_id.0.as_str(), + current_index = idx, + track_id = id, + "Using internal current_index as fallback (renderer didn't report playing ID)" + ); + Some((idx, id, uri)) + } else { + None + } + } else { + None + }; + debug!( renderer = self.renderer_id.0.as_str(), actual_items = self.items.len(), + currently_playing_id_from_renderer = ?currently_playing_id_from_renderer, + playing_info_detected = playing_info.is_some(), "OpenHome playlist state refreshed before replace_queue" ); - let (keep_current, keep_desired) = lcs_flags(&self.items, &items); + 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); - let items_to_keep = keep_current.iter().filter(|&&k| k).count(); - let items_to_delete = keep_current.iter().filter(|&&k| !k).count(); - let items_to_add = keep_desired.iter().filter(|&&k| !k).count(); + if let Some(pivot_idx) = new_playing_idx { + // CASE 2: Currently playing item IS in the new playlist + // Use gentle double-LCS strategy: preserve the pivot and sync before/after separately + debug!( + renderer = self.renderer_id.0.as_str(), + playing_idx, + pivot_idx, + "Gentle sync: currently playing item found in new playlist at index {}", + pivot_idx + ); - debug!( - renderer = self.renderer_id.0.as_str(), - keep = items_to_keep, - delete = items_to_delete, - add = items_to_add, - "LCS computed: minimizing OpenHome playlist operations" - ); + self.replace_queue_with_pivot(items, pivot_idx, playing_id)?; + } else { + // CASE 1: Currently playing item NOT in the new playlist + // Keep it as first item and append the new playlist after it + debug!( + renderer = self.renderer_id.0.as_str(), + playing_idx, + "Gentle sync: currently playing item not in new playlist, preserving as first item" + ); - // If we're replacing everything (keep=0), use delete_all() instead of - // individual delete_id() calls. This is much more robust for live playlists - // where track IDs can become invalid between refresh and deletion. - if items_to_keep == 0 && items_to_delete > 0 { + self.replace_queue_preserve_current(items, playing_idx, playing_id)?; + } + } else { + // No currently playing item or can't determine it - use standard LCS debug!( renderer = self.renderer_id.0.as_str(), - "Using delete_all() for complete replacement (more robust for live playlists)" + "No currently playing item, using standard LCS sync" ); - self.playlist.delete_all()?; - self.track_ids.clear(); - self.items.clear(); - } else { - // Selective deletion when keeping some items - for idx in (0..self.track_ids.len()).rev() { - if !keep_current[idx] { - let track_id = self.track_ids[idx]; - // Use delete_id_if_exists() to handle cases where another control point - // may have already modified the playlist - self.playlist.delete_id_if_exists(track_id)?; - self.track_ids.remove(idx); - self.items.remove(idx); - } - } + self.replace_queue_standard_lcs(items, current_index)?; } - let remaining_ids = self.track_ids.clone(); - let mut remaining_idx = 0usize; - let mut previous_id = OPENHOME_PLAYLIST_HEAD_ID; - let mut rebuilt_items = Vec::with_capacity(items.len()); - let mut rebuilt_ids = Vec::with_capacity(items.len()); - - for (idx, item) in items.into_iter().enumerate() { - if keep_desired[idx] { - if remaining_idx >= remaining_ids.len() { - return Err(anyhow!( - "OpenHome playlist refresh bookkeeping mismatch (kept entries underflow)" - )); - } - let existing_id = remaining_ids[remaining_idx]; - remaining_idx += 1; - previous_id = existing_id; - rebuilt_ids.push(existing_id); - rebuilt_items.push(self.item_with_openhome_id(item, existing_id)); - } else { - let metadata = build_metadata_xml(&item); - let new_id = self.playlist.insert(previous_id, &item.uri, &metadata)?; - previous_id = new_id; - rebuilt_ids.push(new_id); - rebuilt_items.push(self.item_with_openhome_id(item, new_id)); - } - } - - if remaining_idx != remaining_ids.len() { - return Err(anyhow!( - "OpenHome playlist refresh bookkeeping mismatch (kept entries overflow)" - )); - } - - let previous_index = self - .current_index - .and_then(|idx| if idx < rebuilt_ids.len() { Some(idx) } else { None }); - let normalized = current_index - .filter(|&i| i < rebuilt_ids.len()) - .or(previous_index) - .or_else(|| if rebuilt_ids.is_empty() { None } else { Some(0) }); - self.items = rebuilt_items; - self.track_ids = rebuilt_ids; - self.current_index = normalized; Ok(()) } diff --git a/pmocontrol/src/openhome_client.rs b/pmocontrol/src/openhome_client.rs index 70aed400..bbcbb832 100644 --- a/pmocontrol/src/openhome_client.rs +++ b/pmocontrol/src/openhome_client.rs @@ -198,11 +198,13 @@ impl OhPlaylistClient { || err_msg.contains("not found") || err_msg.contains("does not exist") || err_msg.contains("unknown") + || err_msg.contains("500") // HTTP 500 = renderer in inconsistent state + || err_msg.contains("Action Failed") // UPnP error 501 { warn!( control_url = self.control_url.as_str(), id, - "DeleteId silently ignored - ID does not exist (likely modified by another control point)" + "DeleteId silently ignored - ID does not exist or renderer in inconsistent state (likely modified by events)" ); Ok(false) } else { @@ -735,16 +737,26 @@ fn parse_track_list(payload: &str) -> Result> { fn parse_track_entry(elem: &Element) -> Result { let id_text = extract_child_text_local(elem, "Id")?; + + // Some renderers (like upmpdcli) return a comma-separated list of IDs in the Id element + // when using ReadList. This is a non-standard compact format that we cannot parse properly + // because we need to fetch each track individually to get its Uri and Metadata. + // Return an error to force the fallback to individual Read() calls. if id_text.contains(',') { debug!( raw_entry = %elem.name, raw_id = id_text.as_str(), - "Unexpected multi-value Id element in OpenHome TrackList entry" + "Multi-value Id element detected - forcing fallback to individual reads" ); + return Err(anyhow!( + "Renderer returned comma-separated IDs in single Entry - need individual reads" + )); } + let id = id_text .parse::() .map_err(|_| anyhow!("Invalid OpenHome Entry Id: {}", id_text))?; + let uri = extract_child_text_local(elem, "Uri")?; let metadata_xml = extract_child_text_optional_local(elem, "Metadata")?.unwrap_or_default(); @@ -962,7 +974,7 @@ pub(crate) fn decode_base64(input: &str) -> Result> { fn is_invalid_entry_id_error(err: &anyhow::Error) -> bool { let msg = format!("{err}"); - msg.contains("Invalid OpenHome Entry Id") + msg.contains("Invalid OpenHome Entry Id") || msg.contains("comma-separated IDs") } #[cfg(test)]