diff --git a/pmocontrol/src/music_renderer/openhome_renderer.rs b/pmocontrol/src/music_renderer/openhome_renderer.rs index 617b67e2..47002404 100644 --- a/pmocontrol/src/music_renderer/openhome_renderer.rs +++ b/pmocontrol/src/music_renderer/openhome_renderer.rs @@ -174,16 +174,23 @@ impl OpenHomeRenderer { /// Retourne la longueur de la playlist OpenHome sans récupérer toutes les métadonnées. /// Plus rapide que snapshot_openhome_playlist() pour juste connaître le nombre de pistes. pub(crate) fn openhome_playlist_len(&self) -> Result { - let playlist = self.playlist_client_for("openhome_playlist_len")?; - let ids = playlist.id_array()?; - Ok(ids.len()) + // Use queue.len() which uses cached track_ids() internally + let queue = self.queue.lock().unwrap(); + 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 playlist = self.playlist_client_for("openhome_playlist_ids")?; - playlist.id_array() + // Use the queue's cached track_ids() instead of direct id_array() call + let queue = self.queue.lock().unwrap(); + if let MusicQueue::OpenHome(oh_queue) = &*queue { + oh_queue.track_ids() + } else { + Err(ControlPointError::QueueError( + "Not an OpenHome queue".to_string(), + )) + } } pub(crate) fn clear_openhome_playlist(&self) -> Result<(), ControlPointError> { @@ -201,11 +208,21 @@ impl OpenHomeRenderer { let playlist = self.playlist_client_for("add_track_openhome")?; let insert_after = match after_id { Some(id) => id, - None => playlist - .id_array()? - .last() - .copied() - .unwrap_or(OPENHOME_PLAYLIST_HEAD_ID), + None => { + // Use the queue's cached track_ids() instead of direct id_array() call + let queue = self.queue.lock().unwrap(); + if let MusicQueue::OpenHome(oh_queue) = &*queue { + oh_queue + .track_ids()? + .last() + .copied() + .unwrap_or(OPENHOME_PLAYLIST_HEAD_ID) + } else { + return Err(ControlPointError::QueueError( + "Not an OpenHome queue".to_string(), + )); + } + } }; let new_id = playlist.insert(insert_after, uri, metadata)?; @@ -376,16 +393,18 @@ impl PlaybackPosition for OpenHomeRenderer { let mut track_uri = None; let mut track_metadata_xml = None; - // Get track ID from playlist - if let Some(playlist_client) = &self.playlist { - match playlist_client.id() { - Ok(id) => track_id = Some(id), + // Get track ID from queue (uses cached data) + let queue_guard_for_id = self.queue.lock().unwrap(); + if let MusicQueue::OpenHome(oh_queue) = &*queue_guard_for_id { + match oh_queue.current_track() { + Ok(id_opt) => track_id = id_opt, Err(err) => debug!( error = %err, "Failed to read OpenHome track id" ), } } + drop(queue_guard_for_id); // Use queue API to get current item with cached metadata let mut queue_guard = self.queue.lock().unwrap(); diff --git a/pmocontrol/src/queue/openhome.rs b/pmocontrol/src/queue/openhome.rs index 5d2c4a7a..52dd7617 100644 --- a/pmocontrol/src/queue/openhome.rs +++ b/pmocontrol/src/queue/openhome.rs @@ -1,4 +1,6 @@ use std::collections::HashMap; +use std::sync::{Arc, Mutex}; +use std::time::SystemTime; use std::usize; use quick_xml::escape::escape; @@ -15,6 +17,55 @@ use crate::queue::{ }; use crate::{DeviceId, DeviceIdentity, RendererInfo}; +/// Cache for OpenHome track IDs to avoid redundant SOAP calls +#[derive(Debug)] +struct TrackIdsCache { + /// Cached track IDs + ids: Option>, + /// Timestamp of last cache update + last_update: Option, +} + +impl TrackIdsCache { + fn new() -> Self { + Self { + ids: None, + last_update: None, + } + } + + /// Check if cache is valid (not expired and has data) + fn is_valid(&self) -> bool { + if let (Some(_), Some(last_update)) = (&self.ids, self.last_update) { + if let Ok(elapsed) = SystemTime::now().duration_since(last_update) { + return elapsed.as_millis() < 1000; // TTL: 1 second + } + } + false + } + + /// Get cached IDs if valid + fn get(&self) -> Option> { + if self.is_valid() { + self.ids.clone() + } else { + None + } + } + + /// Update cache with new IDs + fn set(&mut self, ids: Vec) { + self.ids = Some(ids); + self.last_update = Some(SystemTime::now()); + } + + /// Invalidate cache (called on write operations) + fn invalidate(&mut self) { + self.ids = None; + self.last_update = None; + } +} + /// Local mirror of an OpenHome playlist for a single renderer. #[derive(Clone, Debug)] pub struct OpenHomeQueue { @@ -26,6 +77,8 @@ pub struct OpenHomeQueue { /// Permet de maintenir des métadonnées à jour même si le service OpenHome /// ne permet pas de les modifier directement. metadata_cache: HashMap>, + /// Cache for track IDs to avoid redundant IdArray SOAP calls + track_ids_cache: Arc>, } impl OpenHomeQueue { @@ -41,6 +94,7 @@ impl OpenHomeQueue { info_client, product_client, metadata_cache: HashMap::new(), + track_ids_cache: Arc::new(Mutex::new(TrackIdsCache::new())), } } @@ -167,6 +221,9 @@ impl OpenHomeQueue { "Gentle sync completed: preserved playing track as first item (not in new playlist)" ); + // Invalidate cache after playlist modifications + self.track_ids_cache.lock().unwrap().invalidate(); + Ok(()) } @@ -344,6 +401,9 @@ impl OpenHomeQueue { "Gentle sync completed: double-LCS with pivot (playing track preserved)" ); + // Invalidate cache after playlist modifications + self.track_ids_cache.lock().unwrap().invalidate(); + Ok(()) } @@ -442,6 +502,9 @@ impl OpenHomeQueue { ))); } + // Invalidate cache after playlist modifications + self.track_ids_cache.lock().unwrap().invalidate(); + Ok(()) } } @@ -562,7 +625,22 @@ impl QueueBackend for OpenHomeQueue { /// Return the list of OpenHome track IDs in order. fn track_ids(&self) -> Result, ControlPointError> { self.ensure_playlist_source_selected()?; - self.playlist_client.id_array() + + // Lock the cache for the entire operation to prevent race conditions + let mut cache = self.track_ids_cache.lock().unwrap(); + + // Check if cache is valid + if let Some(cached_ids) = cache.get() { + return Ok(cached_ids); + } + + // Cache miss or expired - fetch from service (keep lock held to prevent concurrent calls) + let ids = self.playlist_client.id_array()?; + + // Update cache before releasing lock + cache.set(ids.clone()); + + Ok(ids) } fn id_to_position(&self, id: u32) -> Result { @@ -637,6 +715,8 @@ impl QueueBackend for OpenHomeQueue { self.ensure_playlist_source_selected()?; self.playlist_client.stop()?; } + // Invalidate cache (seek_id modifies playlist state) + self.track_ids_cache.lock().unwrap().invalidate(); Ok(()) } @@ -659,6 +739,9 @@ impl QueueBackend for OpenHomeQueue { self.playlist_client.delete_all()?; self.metadata_cache.clear(); + // Invalidate cache after delete_all + self.track_ids_cache.lock().unwrap().invalidate(); + if items.is_empty() { return Ok(()); } @@ -677,6 +760,9 @@ impl QueueBackend for OpenHomeQueue { previous_id = new_id; } + // Invalidate cache after insertions + self.track_ids_cache.lock().unwrap().invalidate(); + Ok(()) } @@ -685,6 +771,8 @@ impl QueueBackend for OpenHomeQueue { if items.is_empty() { self.playlist_client.delete_all()?; self.metadata_cache.clear(); + // Invalidate cache after delete_all + self.track_ids_cache.lock().unwrap().invalidate(); return Ok(()); } @@ -819,6 +907,9 @@ impl QueueBackend for OpenHomeQueue { self.playlist_client.seek_id(new_id)?; } + // Invalidate cache after playlist modifications + self.track_ids_cache.lock().unwrap().invalidate(); + Ok(()) } @@ -857,8 +948,13 @@ impl QueueBackend for OpenHomeQueue { EnqueueMode::ReplaceAll => { // Replace the entire playlist self.replace_queue(items, None)?; + return Ok(()); } } + + // Invalidate cache after playlist modifications (except ReplaceAll which already does it) + self.track_ids_cache.lock().unwrap().invalidate(); + Ok(()) } @@ -867,7 +963,10 @@ impl QueueBackend for OpenHomeQueue { /// Optimized clear_queue: use delete_all() directly instead of replace_queue. fn clear_queue(&mut self) -> Result<(), ControlPointError> { self.ensure_playlist_source_selected()?; - self.playlist_client.delete_all() + self.playlist_client.delete_all()?; + // Invalidate cache after clearing playlist + self.track_ids_cache.lock().unwrap().invalidate(); + Ok(()) } /// Optimized is_empty: only fetch track IDs, not the full playlist. diff --git a/pmocontrol/src/upnp_clients/openhome_client.rs b/pmocontrol/src/upnp_clients/openhome_client.rs index 3e6e3b8d..b45812e1 100644 --- a/pmocontrol/src/upnp_clients/openhome_client.rs +++ b/pmocontrol/src/upnp_clients/openhome_client.rs @@ -8,6 +8,8 @@ use crate::soap_client::{ }; use anyhow::{Result, anyhow}; use pmodidl::DIDLLite; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, SystemTime}; use tracing::{debug, info, trace, warn}; use xmltree::{Element, XMLNode}; @@ -728,10 +730,87 @@ impl OhRadioClient { } } +#[derive(Debug)] +struct SourceXmlCache { + sources: Option>, + last_update: Option, +} + +impl SourceXmlCache { + fn new() -> Self { + Self { + sources: None, + last_update: None, + } + } + + fn is_valid(&self) -> bool { + if let (Some(_), Some(last_update)) = (&self.sources, self.last_update) { + if let Ok(elapsed) = SystemTime::now().duration_since(last_update) { + return elapsed < Duration::from_secs(600); // 10 minutes TTL + } + } + false + } + + fn get(&self) -> Option> { + if self.is_valid() { + self.sources.clone() + } else { + None + } + } + + fn set(&mut self, sources: Vec) { + self.sources = Some(sources); + self.last_update = Some(SystemTime::now()); + } +} + +#[derive(Debug)] +struct SourceIndexCache { + index: Option, + last_update: Option, +} + +impl SourceIndexCache { + fn new() -> Self { + Self { + index: None, + last_update: None, + } + } + + fn is_valid(&self) -> bool { + if let (Some(_), Some(last_update)) = (self.index, self.last_update) { + if let Ok(elapsed) = SystemTime::now().duration_since(last_update) { + return elapsed < Duration::from_secs(1); // 1 second TTL + } + } + false + } + + fn get(&self) -> Option { + if self.is_valid() { self.index } else { None } + } + + fn set(&mut self, index: u32) { + self.index = Some(index); + self.last_update = Some(SystemTime::now()); + } + + fn invalidate(&mut self) { + self.index = None; + self.last_update = None; + } +} + #[derive(Debug, Clone)] pub struct OhProductClient { pub control_url: String, pub service_type: String, + source_xml_cache: Arc>, + source_index_cache: Arc>, } impl OhProductClient { @@ -739,6 +818,8 @@ impl OhProductClient { Self { control_url, service_type, + source_xml_cache: Arc::new(Mutex::new(SourceXmlCache::new())), + source_index_cache: Arc::new(Mutex::new(SourceIndexCache::new())), } } @@ -754,16 +835,39 @@ 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(); + + // Check if cache is valid + if let Some(cached_sources) = cache.get() { + return Ok(cached_sources); + } + + // Cache miss or expired - fetch from service (keep lock held to prevent concurrent calls) let call_result = invoke_upnp_action(&self.control_url, &self.service_type, "SourceXml", &[])?; let envelope = ensure_success("SourceXml", &call_result)?; let response = find_child_with_suffix(&envelope.body.content, "SourceXmlResponse") .ok_or_else(|| anyhow!("Missing SourceXmlResponse element in SOAP body"))?; let xml = extract_child_text_any(response, &["SourceXml", "Xml", "Value"])?; - parse_product_source_list(&xml) + let sources = parse_product_source_list(&xml)?; + + // Update cache before releasing lock + cache.set(sources.clone()); + + Ok(sources) } 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(); + + // Check if cache is valid + if let Some(cached_index) = cache.get() { + return Ok(cached_index); + } + + // Cache miss or expired - fetch from service (keep lock held to prevent concurrent calls) let call_result = invoke_upnp_action(&self.control_url, &self.service_type, "SourceIndex", &[])?; let envelope = ensure_success("SourceIndex", &call_result)?; @@ -773,9 +877,14 @@ impl OhProductClient { })?; let value = extract_child_text_any(response, &["Index", "Value"])?; - value + let index = value .parse::() - .map_err(|_| ControlPointError::UpnpBadReturnValue("volume".to_string(), value)) + .map_err(|_| ControlPointError::UpnpBadReturnValue("volume".to_string(), value))?; + + // Update cache before releasing lock + cache.set(index); + + Ok(index) } pub fn set_source_index(&self, index: u32) -> Result<(), ControlPointError> { @@ -787,7 +896,15 @@ impl OhProductClient { "SetSourceIndex", &args, )?; - handle_action_response("SetSourceIndex", &call_result) + let result = handle_action_response("SetSourceIndex", &call_result); + + // Invalidate cache after write operation + if result.is_ok() { + let mut cache = self.source_index_cache.lock().unwrap(); + cache.invalidate(); + } + + result } pub fn ensure_playlist_source_selected(&self) -> Result<(), ControlPointError> {