Skip to content

Commit

Permalink
songHandler should be currentTrackHandler
Browse files Browse the repository at this point in the history
This completes the previous work on handlers and trying to clarify the roles of the handlers, between the song/track and the streamUrl.
Previously, the songHandler was prioritized over the streamUrl handler, but it was still missing once case where the $track is a playlist and in fact it's not that $song (master track) handler that should be used but the _currentTrack (sub-track) handler. This is the case where a remote/http playlist is an opml list that contains tracks servced by protocol handlers (e.g. Deezer)
  • Loading branch information
philippe44 committed Jul 3, 2021
1 parent 100d43c commit 68d68e0
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 33 deletions.
22 changes: 12 additions & 10 deletions DEVELOPERS.txt
Original file line number Diff line number Diff line change
Expand Up @@ -205,33 +205,35 @@ have been skipped). On further open(), the next sub-track is set in _currentTrac
- currentTrack() is a method that returns the actual current track, i.e. _currentTrack || _track

>>> Handlers in Slim::Player::Song and Slim::Player::SongStreamingController
- $song->handler (RO) is set at the creation of the song. it depends on track->url at creation but can
be if the PH wants to pass-on to another PH (like thin PH do)
- $song->handler (RO) is set at the creation of the song. it is based on track->url at creation and is
immutable.
- $song->_currentTrackHandler (RW) is set when a single track happens to be a playlist, for each item.
It allows each individual sub-tracks to have their own PR when it relates to some of their management.
It means that it is empty most of the time.
It allows each individual sub-tracks to have their own PH when it relates to some of their management.
It means that it is empty most of the time but PH can update it when $track is changed after scanning
(for example when upgrding from HTTP to HTTPS)
- $song->currentTrackHandler() is a method that returns (_currentTrackHandler || handler) so it tells
what shall be used for all url related to the current (sub) track
- $songStreamingController->urlHandler is RO and set at the creation of the song streaming controller,
based on the streamUrl (in S::P::Song::open)
- $songStreamingController->streamHandler is RO and set at creation of the song streaming controller.
It is the class of the *socket* object created by $song->handler->new.
- $songStreamingController->songHandler is a method that returns the $song->handler, so the song main
handler, not the sub-track
- $songStreamingController->currentTrackHandler is a shortcut to $song->currentTrackhandler

>>> Protocol Handlers
These files contains a base class but whose methods are called in different contexts (the $self)
1- a $song context means they are called with a $song->currentHandler or $song->handler
2- a $sock context means they are called with a $socket (or the object) that was created by $song->handler->new
3- a simple $url context means they are called by $song->currentHandler, $song->Handler or a $songStreamingController->streamHandler
1- a $song context means they are called with a $song->currentTrackHandler or $song->handler
2- a $sock context means they are called with a $socket (or the object) that was created by
$song->handler->new
3- a simple $url context means they are called by $song->currentTrackHandler, $song->Handler or a
$songStreamingController->streamHandler

So it's a bit confusing that methods from the same package can be object-oriented called with totally
different type of ancestors/context. In fact, some methods can be called in both context is the
ancestor of the PH is capable of (e.g. HTTP).

- functions like sysread are only called with a $sock context
- functions like getMetadataFor can be called with a $song or an $url context
- functions like requestString cal be called in a $song or $sock context
- functions like requestString can be called in a $song or $sock context

>>> Example of requestString
That function is to create the HTTP headers to be sent with the GET to grab the actual audio. It
Expand Down
4 changes: 2 additions & 2 deletions Slim/Player/Protocols/HTTP.pm
Original file line number Diff line number Diff line change
Expand Up @@ -262,9 +262,9 @@ sub getFormatForURL {
return Slim::Music::Info::typeFromSuffix($url);
}

sub getSongHandler {
sub currentTrackHandler {
my ($class, $self, $track) = @_;

# re-evaluate as we might have been upgraded to HTTPS
return $class ne __PACKAGE__ ? $class : Slim::Player::ProtocolHandlers->handlerForURL($track->url);
}
Expand Down
2 changes: 1 addition & 1 deletion Slim/Player/Song.pm
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ sub getNextSong {
if ($self->_track() == $track) {
# Update of original track, by playlist or redirection
$self->_track($newTrack);
$self->init_accessor(handler => $handler->getSongHandler($self, $newTrack)) if $handler->can('getSongHandler');
$self->_currentTrackHandler($handler->currentTrackHandler($self, $newTrack)) if $handler->can('currentTrackHandler');

main::INFOLOG && $log->info("Track updated by scan: $url -> " . $newTrack->url);

Expand Down
4 changes: 2 additions & 2 deletions Slim/Player/SongStreamController.pm
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ sub close {
}
}

sub songHandler {
return shift->song->handler();
sub currentTrackHandler {
return shift->song->currentTrackHandler();
}

sub isDirect {
Expand Down
22 changes: 11 additions & 11 deletions Slim/Player/Squeezebox.pm
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ sub play {
my $params = shift;

my $controller = $params->{'controller'};
my $handler = $controller->songHandler();
my $handler = $controller->currentTrackHandler();

# Calculate the correct buffer threshold for remote URLs
if ( $handler->isRemote() ) {
Expand Down Expand Up @@ -537,7 +537,7 @@ sub stream_s {
my $url = $controller->streamUrl();
my $track = $controller->track();
my $handler = $controller->streamUrlHandler();
my $songHandler = $controller->songHandler();
my $currentTrackHandler = $controller->currentTrackHandler();
my $isDirect = $controller->isDirect();
my $master = $client->master();

Expand Down Expand Up @@ -565,7 +565,7 @@ sub stream_s {
# use getFormatForURL only if the format is not already given
# This method is bad because it only looks at the URL suffix and can cause
# (for example) Ogg HTTP streams to be played using the mp3 decoder!
my $methodHandler = $songHandler->can('getFormatForURL') ? $songHandler : $handler;
my $methodHandler = $currentTrackHandler->can('getFormatForURL') ? $currentTrackHandler : $handler;
$format = $methodHandler->getFormatForURL($url) if !$format && $methodHandler;
}

Expand Down Expand Up @@ -752,8 +752,8 @@ sub stream_s {
$outputThreshold = 1;

# Handler may override pcmsamplesize (Rhapsody)
if ( $songHandler && $songHandler->can('pcmsamplesize') ) {
$pcmsamplesize = $songHandler->pcmsamplesize( $client, $params );
if ( $currentTrackHandler && $currentTrackHandler->can('pcmsamplesize') ) {
$pcmsamplesize = $currentTrackHandler->pcmsamplesize( $client, $params );
}

# XXX: The use of mp3 as default has been known to cause the mp3 decoder to be used for
Expand All @@ -774,7 +774,7 @@ sub stream_s {

main::INFOLOG && logger('player.streaming.direct')->info("SqueezePlay direct stream: $url");

my $methodHandler = $songHandler->can('requestString') ? $songHandler : $handler;
my $methodHandler = $currentTrackHandler->can('requestString') ? $currentTrackHandler : $handler;
$request_string = $methodHandler->getRequestString($client, $url, undef, $params->{'seekdata'} || $controller->song->seekdata);
$autostart += 2; # will be 2 for direct streaming with no autostart, or 3 for direct with autostart

Expand Down Expand Up @@ -818,8 +818,8 @@ sub stream_s {
}
$server_port = $port;

# prioritize song's protocol handler at even in direct mode it might change requestString
my $methodHandler = $songHandler->can('requestString') ? $songHandler : $handler;
# prioritize current track's protocol handler at even in direct mode it might change requestString
my $methodHandler = $currentTrackHandler->can('requestString') ? $currentTrackHandler : $handler;
$request_string = $methodHandler->requestString($client, $url, undef, $params->{'seekdata'} || $controller->song->seekdata);
$autostart += 2; # will be 2 for direct streaming with no autostart, or 3 for direct with autostart

Expand Down Expand Up @@ -939,10 +939,10 @@ sub stream_s {
}

# Bug 10567, allow plugins to override transition setting
if ( $songHandler && $songHandler->can('transitionType') ) {
my $override = $songHandler->transitionType( $master, $controller->song(), $transitionType );
if ( $currentTrackHandler && $currentTrackHandler->can('transitionType') ) {
my $override = $currentTrackHandler->transitionType( $master, $controller->song(), $transitionType );
if ( defined $override ) {
main::INFOLOG && $log->is_info && $log->info("$songHandler changed transition type to $override");
main::INFOLOG && $log->is_info && $log->info("$currentTrackHandler changed transition type to $override");
$transitionType = $override;
}
}
Expand Down
14 changes: 7 additions & 7 deletions Slim/Player/Squeezebox2.pm
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ sub directHeaders {
return unless $controller && $controller->isDirect();

my $url = $controller->streamUrl();
my $songHandler = $controller->songHandler();
my $currentTrackHandler = $controller->currentTrackHandler();

# We involve the protocol handler in the header parsing process.
# The current iteration of the firmware only knows about HTTP
Expand Down Expand Up @@ -529,12 +529,12 @@ sub directHeaders {

$directlog->warn("Invalid response code ($response) from remote stream $url");

if ($songHandler && $songHandler->can("handleDirectError")) {
if ($currentTrackHandler && $currentTrackHandler->can("handleDirectError")) {

# bug 10407 - make sure ready to stream again
$client->readyToStream(1);

$songHandler->handleDirectError($client, $url, $response, $status_line);
$currentTrackHandler->handleDirectError($client, $url, $response, $status_line);
}
elsif ($track->can('redir') && $track->redir && $track->redir ne $url) {

Expand Down Expand Up @@ -569,13 +569,13 @@ sub directHeaders {
$directlog->info("Processing " . scalar(@headers) . " headers");
}

# prioritize song's protocol handler over streamUrl handler
my $methodHandler = $songHandler->can('parseDirectHeaders') ? $songHandler : $handler;
# prioritize current track's protocol handler over streamUrl handler
my $methodHandler = $currentTrackHandler->can('parseDirectHeaders') ? $currentTrackHandler : $handler;

main::INFOLOG && $directlog->info("Calling $methodHandler :: parseDirectHeader");
# songHandler relates to the current (sub)track while streamUrl handler just use streamUrl
# currentTrackHandler relates to the current (sub)track while streamUrl handler just use streamUrl
($title, $bitrate, $metaint, $redir, $contentType, $length, $body) =
$methodHandler->parseDirectHeaders($client, $methodHandler == $songHandler ? $controller->song()->currentTrack() : $url, @headers);
$methodHandler->parseDirectHeaders($client, $methodHandler == $currentTrackHandler ? $controller->song()->currentTrack() : $url, @headers);

$controller->song()->isLive($length ? 0 : 1) if !$redir;
# XXX maybe should (also) check $song->scanData()->{$url}->{metadata}->{info}->{broadcast}
Expand Down

0 comments on commit 68d68e0

Please sign in to comment.