Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dealer: Rework context retrieval #1414

Open
wants to merge 32 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2f24606
connect: simplify `handle_command` for SpircCommand
photovoltex Dec 10, 2024
e0f11f1
connect: simplify `handle_player_event`
photovoltex Dec 10, 2024
c8131be
connect: `handle_player_event` update log entries
photovoltex Dec 10, 2024
29cd9b3
connect: set `playback_speed` according to player state
photovoltex Dec 10, 2024
d8969da
connect: reduce/group state updates by delaying them slightly
photovoltex Dec 10, 2024
28588c4
connect: load entire context at once
photovoltex Dec 10, 2024
c73871c
connect: use is_playing from connect_state
photovoltex Dec 10, 2024
d2f1dee
connect: move `ResolveContext` in own file
photovoltex Dec 11, 2024
673ded3
connect: handle metadata correct
photovoltex Dec 11, 2024
d3ae5c0
connect: resolve context rework
photovoltex Dec 11, 2024
11040d9
connect: rework aftermath
photovoltex Dec 11, 2024
cd9f822
general logging and comment fixups
photovoltex Dec 11, 2024
b13ab3a
connect: fix incorrect stopping
photovoltex Dec 12, 2024
932c6fa
connect: always handle player seek event
photovoltex Dec 12, 2024
0752b8d
connect: adjust behavior
photovoltex Dec 14, 2024
08624dd
fix typo and `find_next`
photovoltex Dec 15, 2024
710e2fc
connect: fixes for context and transfer
photovoltex Dec 16, 2024
e6fe2ac
revert removal of state setup
photovoltex Dec 16, 2024
f59b43b
`clear_next_tracks` should never clear queued items
photovoltex Dec 17, 2024
dfbac73
connect: make `playing_track` optional and handle it correctly
photovoltex Dec 17, 2024
0dfced8
connect: adjust finish of context resolving
photovoltex Dec 17, 2024
1873e2c
Merge branch 'dev' into improve-dealer
photovoltex Dec 17, 2024
fbe1657
connect: set track position when shuffling
photovoltex Dec 19, 2024
67ceecd
example: adjust to model change
photovoltex Dec 19, 2024
7d7e786
connect: remove duplicate track
photovoltex Dec 19, 2024
309ca8d
connect: provide all recently played tracks to autoplay request
photovoltex Dec 19, 2024
3cf4197
connect: apply review suggestions
photovoltex Dec 23, 2024
eb14e46
connect: impl trait for player context
photovoltex Dec 23, 2024
acc2b39
connect: fix incorrect playing and paused
photovoltex Dec 22, 2024
771f9e9
connect: apply options as official clients
photovoltex Dec 23, 2024
0749aa6
Merge branch 'dev' into improve-dealer
photovoltex Dec 24, 2024
ba7fed3
protocol: move trait impls into impl_trait mod
photovoltex Dec 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
368 changes: 368 additions & 0 deletions connect/src/context_resolver.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,368 @@
use crate::state::context::ContextType;
use crate::{
core::{Error, Session},
protocol::{
autoplay_context_request::AutoplayContextRequest,
player::{Context, TransferState},
},
state::{context::UpdateContext, ConnectState},
};
use std::cmp::PartialEq;
use std::{
collections::{HashMap, VecDeque},
fmt::{Display, Formatter},
hash::{Hash, Hasher},
time::Duration,
};
use thiserror::Error as ThisError;
use tokio::time::Instant;

#[derive(Debug, Clone)]
enum Resolve {
Uri(String),
Context(Context),
}

#[derive(Debug, Clone)]
pub(super) enum ContextAction {
Append,
Replace,
}

#[derive(Debug, Clone)]
pub(super) struct ResolveContext {
resolve: Resolve,
fallback: Option<String>,
update: UpdateContext,
action: ContextAction,
}

impl ResolveContext {
fn append_context(uri: impl Into<String>) -> Self {
Self {
resolve: Resolve::Uri(uri.into()),
fallback: None,
update: UpdateContext::Default,
action: ContextAction::Append,
}
}

pub fn from_uri(
uri: impl Into<String>,
fallback: impl Into<String>,
update: UpdateContext,
action: ContextAction,
) -> Self {
let fallback_uri = fallback.into();
Self {
resolve: Resolve::Uri(uri.into()),
fallback: (!fallback_uri.is_empty()).then_some(fallback_uri),
update,
action,
}
}

pub fn from_context(context: Context, update: UpdateContext, action: ContextAction) -> Self {
Self {
resolve: Resolve::Context(context),
fallback: None,
update,
action,
}
}

/// the uri which should be used to resolve the context, might not be the context uri
fn resolve_uri(&self) -> Option<&str> {
// it's important to call this always, or at least for every ResolveContext
// otherwise we might not even check if we need to fallback and just use the fallback uri
match self.resolve {
Resolve::Uri(ref uri) => ConnectState::valid_resolve_uri(uri),
Resolve::Context(ref ctx) => ConnectState::get_context_uri_from_context(ctx),
}
.or(self.fallback.as_deref())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work correctly? I believe or is eagerly evaluated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it doesn't evaluated any values it shouldn't matter if it's eagerly or not. Unless there isn't any benefit to it, I would just keep it as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking that this will probably always evaluate to None because of the eager evaluation at compile time, not sure if that's the behavior you intend?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why would it always evaluate to None, I think eagerly evaluated just means always/early evaluated? As self.fallback is an Option<String> it should only provide the value in place. I think the only difference would be the evaluation of as_deref(). But this is only used here to convert Option<String> into Option<&str>.

Just want to understand where you are coming from, as from my perspective it shouldn't make a difference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right.

}

/// the actual context uri
fn context_uri(&self) -> &str {
match self.resolve {
Resolve::Uri(ref uri) => uri,
Resolve::Context(ref ctx) => &ctx.uri,
}
}
}

impl Display for ResolveContext {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
write!(
f,
"resolve_uri: <{:?}>, context_uri: <{}>, update: <{:?}>",
self.resolve_uri(),
self.context_uri(),
self.update,
)
}
}

impl PartialEq for ResolveContext {
fn eq(&self, other: &Self) -> bool {
let eq_context = self.context_uri() == other.context_uri();
let eq_resolve = self.resolve_uri() == other.resolve_uri();
let eq_autoplay = self.update == other.update;

eq_context && eq_resolve && eq_autoplay
}
}

impl Eq for ResolveContext {}

impl Hash for ResolveContext {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess deriving Hash on the structs doesn't work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quit easily as Context doesn't implement Hash. I could implement Hash and Eq in the protocol crate, then I could just derive the trait. Both ways seems fine to me. Just tell me what you would prefer :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. Certainly if you need to patch up protobuf generated code, I'd say don't bother.

fn hash<H: Hasher>(&self, state: &mut H) {
self.context_uri().hash(state);
self.resolve_uri().hash(state);
self.update.hash(state);
}
}

#[derive(Debug, ThisError)]
enum ContextResolverError {
#[error("no next context to resolve")]
NoNext,
#[error("tried appending context with {0} pages")]
UnexpectedPagesSize(usize),
#[error("tried resolving not allowed context: {0:?}")]
NotAllowedContext(String),
}

impl From<ContextResolverError> for Error {
fn from(value: ContextResolverError) -> Self {
Error::failed_precondition(value)
}
}

pub struct ContextResolver {
session: Session,
queue: VecDeque<ResolveContext>,
unavailable_contexts: HashMap<ResolveContext, Instant>,
}

// time after which an unavailable context is retried
const RETRY_UNAVAILABLE: Duration = Duration::from_secs(3600);

impl ContextResolver {
pub fn new(session: Session) -> Self {
Self {
session,
queue: VecDeque::new(),
unavailable_contexts: HashMap::new(),
}
}

pub fn add(&mut self, resolve: ResolveContext) {
let last_try = self
.unavailable_contexts
.get(&resolve)
.map(|i| i.duration_since(Instant::now()));

let last_try = if matches!(last_try, Some(last_try) if last_try > RETRY_UNAVAILABLE) {
let _ = self.unavailable_contexts.remove(&resolve);
debug!(
"context was requested {}s ago, trying again to resolve the requested context",
last_try.expect("checked by condition").as_secs()
);
None
} else {
last_try
};

if last_try.is_some() {
debug!("tried loading unavailable context: {resolve}");
return;
} else if self.queue.contains(&resolve) {
debug!("update for {resolve} is already added");
return;
} else {
trace!(
"added {} to resolver queue",
resolve.resolve_uri().unwrap_or(resolve.context_uri())
)
}

self.queue.push_back(resolve)
}

pub fn add_list(&mut self, resolve: Vec<ResolveContext>) {
for resolve in resolve {
self.add(resolve)
}
}

pub fn remove_used_and_invalid(&mut self) {
if let Some((_, _, remove)) = self.find_next() {
for _ in 0..remove {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let _ = self.queue.pop_front();
}
}
self.queue.pop_front();
photovoltex marked this conversation as resolved.
Show resolved Hide resolved
}

pub fn clear(&mut self) {
self.queue = VecDeque::new()
}

fn find_next(&self) -> Option<(&ResolveContext, &str, usize)> {
let mut idx = 0;
loop {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not change into a for loop? You can still break out of that.

let next = self.queue.get(idx)?;
match next.resolve_uri() {
None if idx < self.queue.len() => {
warn!("skipped {idx} because of no valid resolve_uri: {next}");
idx += 1;
continue;
}
value => break value.map(|uri| (next, uri, idx)),
}
}
}

pub fn has_next(&self) -> bool {
self.find_next().is_some()
}

pub async fn get_next_context(
&self,
recent_track_uri: impl Fn() -> Vec<String>,
) -> Result<Context, Error> {
let (next, resolve_uri, _) = self.find_next().ok_or(ContextResolverError::NoNext)?;

match next.update {
UpdateContext::Default => {
let mut ctx = self.session.spclient().get_context(resolve_uri).await;
if let Ok(ctx) = ctx.as_mut() {
ctx.uri = next.context_uri().to_string();
ctx.url = format!("context://{}", ctx.uri);
}

ctx
}
UpdateContext::Autoplay => {
if resolve_uri.contains("spotify:show:") || resolve_uri.contains("spotify:episode:")
{
// autoplay is not supported for podcasts
Err(ContextResolverError::NotAllowedContext(
resolve_uri.to_string(),
))?
}

let request = AutoplayContextRequest {
context_uri: Some(resolve_uri.to_string()),
recent_track_uri: recent_track_uri(),
..Default::default()
};
self.session.spclient().get_autoplay_context(&request).await
}
}
}

pub fn mark_next_unavailable(&mut self) {
if let Some((next, _, _)) = self.find_next() {
self.unavailable_contexts
.insert(next.clone(), Instant::now());
}
}

pub fn apply_next_context(
&self,
state: &mut ConnectState,
mut context: Context,
) -> Result<Option<Vec<ResolveContext>>, Error> {
let (next, _, _) = self.find_next().ok_or(ContextResolverError::NoNext)?;

let remaining = match next.action {
ContextAction::Append if context.pages.len() == 1 => state
.fill_context_from_page(context.pages.remove(0))
.map(|_| None),
ContextAction::Replace => {
let remaining = state.update_context(context, next.update);
if let Resolve::Context(ref ctx) = next.resolve {
state.merge_context(Some(ctx.clone()));
}

remaining
}
ContextAction::Append => {
warn!("unexpected page size: {context:#?}");
Err(ContextResolverError::UnexpectedPagesSize(context.pages.len()).into())
}
}?;

Ok(remaining.map(|remaining| {
remaining
.into_iter()
.map(ResolveContext::append_context)
.collect::<Vec<_>>()
}))
}

pub fn try_finish(
&self,
state: &mut ConnectState,
transfer_state: &mut Option<TransferState>,
) -> bool {
let (next, _, _) = match self.find_next() {
None => return false,
Some(next) => next,
};

// when there is only one update type, we are the last of our kind, so we should update the state
if self
.queue
.iter()
.filter(|resolve| resolve.update == next.update)
.count()
!= 1
{
return false;
}

match (next.update, state.active_context) {
(UpdateContext::Default, ContextType::Default) | (UpdateContext::Autoplay, _) => {
debug!(
"last item of type <{:?}>, finishing state setup",
next.update
);
}
(UpdateContext::Default, _) => {
debug!("skipped finishing default, because it isn't the active context");
return false;
}
}

let active_ctx = state.get_context(state.active_context);
let res = if let Some(transfer_state) = transfer_state.take() {
state.finish_transfer(transfer_state)
} else if state.shuffling_context() {
state.shuffle()
} else if matches!(active_ctx, Ok(ctx) if ctx.index.track == 0) {
// has context, and context is not touched
// when the index is not zero, the next index was already evaluated elsewhere
let ctx = active_ctx.expect("checked by precondition");
let idx = ConnectState::find_index_in_context(ctx, |t| {
state.current_track(|c| t.uri == c.uri)
})
.ok();

state.reset_playback_to_position(idx)
} else {
state.fill_up_next_tracks()
};

if let Err(why) = res {
error!("setup of state failed: {why}, last used resolve {next:#?}")
}

state.update_restrictions();
state.update_queue_revision();

true
}
}
1 change: 1 addition & 0 deletions connect/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use librespot_core as core;
use librespot_playback as playback;
use librespot_protocol as protocol;

mod context_resolver;
mod model;
pub mod spirc;
pub mod state;
Loading
Loading