From 38bd063109c1abd44620fc9b82b157ced3ab23a6 Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Sat, 31 Jan 2026 17:21:18 +0100 Subject: [PATCH] Optimize OpenHome queue operations with caching This commit introduces caching mechanisms for track IDs in OpenHome queues to reduce redundant SOAP calls. It adds a new `CurrentTrackIdCache` struct and integrates it into `OpenHomeQueue` to cache current track information with a 250ms TTL. The `queue_snapshot` method now uses cached track IDs instead of calling `read_all_tracks()` directly, and batched metadata reads are implemented. Cache invalidation is properly handled on write operations like `seek_id`, `stop`, `delete_all`, and `clear_queue`. The `read_all_tracks` function was removed from `openhome_client.rs` as it's now superseded by the caching logic. Additionally, the `Clone` derive was removed from `InternalQueue` and `MusicQueue` structs, and `trace` was added to the tracing imports in `openhome.rs`. --- pmocontrol/src/queue/interne.rs | 2 +- pmocontrol/src/queue/music_queue.rs | 2 +- pmocontrol/src/queue/openhome.rs | 119 ++++++++++++++++-- .../src/upnp_clients/openhome_client.rs | 48 ------- 4 files changed, 113 insertions(+), 58 deletions(-) diff --git a/pmocontrol/src/queue/interne.rs b/pmocontrol/src/queue/interne.rs index 8e95904d..e5b44930 100644 --- a/pmocontrol/src/queue/interne.rs +++ b/pmocontrol/src/queue/interne.rs @@ -29,7 +29,7 @@ use crate::{ /// /// It does not talk to any remote service. All operations are pure /// structural mutations on in-memory data. -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct InternalQueue { renderer_id: DeviceId, items: Vec, diff --git a/pmocontrol/src/queue/music_queue.rs b/pmocontrol/src/queue/music_queue.rs index 9388d834..44b3d4aa 100644 --- a/pmocontrol/src/queue/music_queue.rs +++ b/pmocontrol/src/queue/music_queue.rs @@ -4,7 +4,7 @@ use crate::queue::{ }; use crate::{PlaybackItem, QueueSnapshot, RendererInfo}; -#[derive(Debug, Clone)] +#[derive(Debug)] pub enum MusicQueue { Internal(InternalQueue), OpenHome(OpenHomeQueue), diff --git a/pmocontrol/src/queue/openhome.rs b/pmocontrol/src/queue/openhome.rs index 52dd7617..f8f8c9dc 100644 --- a/pmocontrol/src/queue/openhome.rs +++ b/pmocontrol/src/queue/openhome.rs @@ -4,7 +4,7 @@ use std::time::SystemTime; use std::usize; use quick_xml::escape::escape; -use tracing::{debug, warn}; +use tracing::{debug, trace, warn}; use crate::errors::ControlPointError; use crate::upnp_clients::{ @@ -66,8 +66,57 @@ impl TrackIdsCache { } } +/// Cache for current track ID to avoid redundant Id SOAP calls +#[derive(Debug)] +struct CurrentTrackIdCache { + /// Cached current track ID (None means no track playing, id=0) + current_id: Option>, + /// Timestamp of last cache update + last_update: Option, +} + +impl CurrentTrackIdCache { + fn new() -> Self { + Self { + current_id: 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.current_id, self.last_update) { + if let Ok(elapsed) = SystemTime::now().duration_since(last_update) { + return elapsed.as_millis() < 250; // TTL: 250ms + } + } + false + } + + /// Get cached current track ID if valid + fn get(&self) -> Option> { + if self.is_valid() { + self.current_id + } else { + None + } + } + + /// Update cache with new current track ID + fn set(&mut self, id: Option) { + self.current_id = Some(id); + self.last_update = Some(SystemTime::now()); + } + + /// Invalidate cache (called on write operations) + fn invalidate(&mut self) { + self.current_id = None; + self.last_update = None; + } +} + /// Local mirror of an OpenHome playlist for a single renderer. -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct OpenHomeQueue { renderer_id: DeviceId, playlist_client: OhPlaylistClient, @@ -79,6 +128,8 @@ pub struct OpenHomeQueue { metadata_cache: HashMap>, /// Cache for track IDs to avoid redundant IdArray SOAP calls track_ids_cache: Arc>, + /// Cache for current track ID to avoid redundant Id SOAP calls + current_track_id_cache: Arc>, } impl OpenHomeQueue { @@ -95,6 +146,7 @@ impl OpenHomeQueue { product_client, metadata_cache: HashMap::new(), track_ids_cache: Arc::new(Mutex::new(TrackIdsCache::new())), + current_track_id_cache: Arc::new(Mutex::new(CurrentTrackIdCache::new())), } } @@ -667,9 +719,23 @@ 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(); + + // Return cached value if valid + if let Some(cached_id) = cache.get() { + return Ok(cached_id); + } + + // Cache miss - fetch from backend let id = self.playlist_client.id()?; // OpenHome returns 0 when no track is selected/playing - if id == 0 { Ok(None) } else { Ok(Some(id)) } + let result = if id == 0 { None } else { Some(id) }; + + // Update cache + cache.set(result); + + Ok(result) } fn current_index(&self) -> Result, ControlPointError> { @@ -682,7 +748,40 @@ impl QueueBackend for OpenHomeQueue { fn queue_snapshot(&self) -> Result { self.ensure_playlist_source_selected()?; - let entries = self.playlist_client.read_all_tracks()?; + + // Use cached track_ids() instead of calling read_all_tracks() which bypasses cache + let ids = self.track_ids()?; + + if ids.is_empty() { + return Ok(QueueSnapshot { + items: Vec::new(), + current_index: None, + playlist_id: None, + }); + } + + // Read metadata for all tracks (batched) + const MAX_BATCH: usize = 64; + let mut entries = Vec::with_capacity(ids.len()); + for chunk in ids.chunks(MAX_BATCH) { + match self.playlist_client.read_list(chunk) { + Ok(mut batch) => entries.append(&mut batch), + Err(err) => { + // If batch fails, try one by one + if chunk.len() > 1 { + for id in chunk { + match self.playlist_client.read_list(&[*id]) { + Ok(mut single) => entries.append(&mut single), + Err(inner_err) => return Err(inner_err), + } + } + } else { + return Err(err); + } + } + } + } + let mut items = Vec::with_capacity(entries.len()); for entry in &entries { @@ -715,8 +814,9 @@ impl QueueBackend for OpenHomeQueue { self.ensure_playlist_source_selected()?; self.playlist_client.stop()?; } - // Invalidate cache (seek_id modifies playlist state) + // Invalidate caches (seek_id/stop modifies playlist state and current track) self.track_ids_cache.lock().unwrap().invalidate(); + self.current_track_id_cache.lock().unwrap().invalidate(); Ok(()) } @@ -739,8 +839,9 @@ impl QueueBackend for OpenHomeQueue { self.playlist_client.delete_all()?; self.metadata_cache.clear(); - // Invalidate cache after delete_all + // Invalidate caches after delete_all (clears queue and current track) self.track_ids_cache.lock().unwrap().invalidate(); + self.current_track_id_cache.lock().unwrap().invalidate(); if items.is_empty() { return Ok(()); @@ -771,8 +872,9 @@ impl QueueBackend for OpenHomeQueue { if items.is_empty() { self.playlist_client.delete_all()?; self.metadata_cache.clear(); - // Invalidate cache after delete_all + // Invalidate caches after delete_all (clears queue and current track) self.track_ids_cache.lock().unwrap().invalidate(); + self.current_track_id_cache.lock().unwrap().invalidate(); return Ok(()); } @@ -964,8 +1066,9 @@ impl QueueBackend for OpenHomeQueue { fn clear_queue(&mut self) -> Result<(), ControlPointError> { self.ensure_playlist_source_selected()?; self.playlist_client.delete_all()?; - // Invalidate cache after clearing playlist + // Invalidate caches after clearing playlist (clears queue and current track) self.track_ids_cache.lock().unwrap().invalidate(); + self.current_track_id_cache.lock().unwrap().invalidate(); Ok(()) } diff --git a/pmocontrol/src/upnp_clients/openhome_client.rs b/pmocontrol/src/upnp_clients/openhome_client.rs index b45812e1..ca76951b 100644 --- a/pmocontrol/src/upnp_clients/openhome_client.rs +++ b/pmocontrol/src/upnp_clients/openhome_client.rs @@ -436,54 +436,6 @@ impl OhPlaylistClient { } Ok(ids) } - - pub fn read_all_tracks(&self) -> Result, ControlPointError> { - let ids = self.id_array()?; - debug!( - control_url = self.control_url.as_str(), - id_count = ids.len(), - "OpenHome Playlist IdArray returned" - ); - - if ids.is_empty() { - info!( - control_url = self.control_url.as_str(), - "OpenHome Playlist is empty (no track IDs)" - ); - return Ok(Vec::new()); - } - - const MAX_BATCH: usize = 64; - let mut entries = Vec::with_capacity(ids.len()); - for chunk in ids.chunks(MAX_BATCH) { - match self.read_list(chunk) { - Ok(mut batch) => entries.append(&mut batch), - Err(err) if chunk.len() > 1 && is_invalid_entry_id_error(&err) => { - debug!( - control_url = self.control_url.as_str(), - requested = chunk.len(), - "ReadList chunk failed with invalid entry ids, falling back to per-id requests" - ); - for id in chunk { - match self.read_list(&[*id]) { - Ok(mut single) => entries.append(&mut single), - Err(inner_err) => return Err(inner_err), - } - } - } - Err(err) => return Err(err), - } - } - - debug!( - control_url = self.control_url.as_str(), - track_count = entries.len(), - expected_count = ids.len(), - "OpenHome Playlist tracks read" - ); - - Ok(entries) - } } impl OhInfoClient {