progress fix + last recent

[philippe44]

So... following discussion here https://forums.slimdevices.com/showthread.php?112541-Podcasts&p=981239&viewfull=1#post981239, I tried to provide a fix for memorizing progress.

The issue with the current version is that it memorizes redirected URL and it does that only once. Unfortunately, some feeds have changing redirections for the same podcast episodes and so tracking does not work. We can't either re-scan the feed every time as it would be very onerous. The current version already scans once the complete feed, even if it's not in a playlist and that is not great for example for long mp4 podcasts.

I've also added a "most recently played" item to try to make that PR more appealing... as unfortunately these are a lot of changes

[michaelherger]

I'm not sure how this should fix the progress tracking. It rather looks as if you removed it for podcasts which redirect to some other URL. Can you enlighten me, please?

Also I don't like the %recentlyPlayed cloning the recent pref. It uses more memory and need manual updating the pref etc. what for? Automatic sorting? Automatic purging? Is it worth it? I don't like when prefs get out of sync because they're cloned and updated outside the prefs handler. I prefer to have such handlers encapsulated, so external users would at least not reach out to a global variable.

[michaelherger]

This would always set the value to 0, as the value is only set if $position was falsy.

[philippe44]

indeed :-)

[michaelherger]

Do you really need this? It's a copy of the pref, but which requires more memory and manual handling to update the pref?...

[philippe44]

I've moved it to a single file so that it stays a local. The reason for using LRU is really to have recently played episodes sorted automatically. I agree I could use the pref array, find the current item, splice it and re-insert but it seemed to me that a LRU would do the job more easily. I thought that Tie::Cache:LRU was just keeping a hash of scalars/references, but had no copies of actual data, so it was low on memory requirements.

[philippe44]

I'm not sure how this should fix the progress tracking. It rather looks as if you removed it for podcasts which redirect to some other URL. Can you enlighten me, please?

Absolutely. What I saw from other users is that not only certain podcast redirect from their "feed" url (that was already taken care of in the plugin) but they also redirect to different urls. The way the plugin works today is that when a "feed" url does not exists in the cache, it does a scanUrl and then stores the redirected url as well.

This is done with a TTL of one month and then, when playing, to track the elapsed time, it looks for the feed url or the redirected url. Problem is that it will not follow an url that has been redirected to a different url than the one that was returned by the "monthly" scan. In addition, that scan is a bit nasty as it will enumerate all the episodes in a feed, even when there are 200. I know it only happens once a month, but that creates a lot of queries and potentially a lot of header caches (see mp4 discussion). It makes sense for that to happen when user makes a large playlist, but I'm not sure it's great when he is just listing episodes of a feed.

So what I'm trying to do here is to only follow the feed url which is the streamUrl of the $song object (I know it's not always entirely true...). The actual $track->url might change and follows the redirected url, but the streamUrl stays (mostly). At the same time, it allows to get rid of the scanUrl for every feed we browse.

I've tried to build things differently rather than using a timer to follow the progress, but I've not been able to find a solution. A protocol handler would easily track start and stop position (this is what I do for the YT plugin) but it seems difficult to have a podcast protocol handler that simply passes the hand to the actual protocol handler while still having its onStop method called. I don't think we can inherit from Player::HTTP as I assume rss could contain non-http urls and even if we could, I can't figure out how to have the "native" http protocol handler managing the object while keeping our protocol handler "in the loop" (especially when redirection happens, that's the issue I had with the "reliable" idea).

The updated version I've pushed only relies on detecting the track characteristics when a [newsong] event happens. It was useless to track it from the opml feed user selection, as it would not work obviously for a playlist when finishing track and going to N+1

I know there are still a case where the $song->streamUrl itself points to a redirected url and in that case we miss progress tracking. I'm not entirely sure when this happens, it seems rare. At least, this version follow much more tracks and offers recently played.

[michaelherger]

Ok, my approach might have been "brutal". But the scanning did fix an issue for some users. Pulling it without any alternative will cause those users to come back and report the issue again. Is it worth breaking functionality for some users for the sake of less resource consumption nobody worried about so far? Maybe I don't fully understand the extent of the waste of resources. Are you saying that this code caused the plugin to download all episodes of a podcast in order to do the scan?

[philippe44]

Ok, my approach might have been "brutal". But the scanning did fix an issue for some users. Pulling it without any alternative will cause those users to come back and report the issue again. Is it worth breaking functionality for some users for the sake of less resource consumption nobody worried about so far? Maybe I don't fully understand the extent of the waste of resources. Are you saying that this code caused the plugin to download all episodes of a podcast in order to do the scan?

It would cause all episodes to be scanned, which can mean, on long mp4, ~600kB of dowload for headers per episode.

But I don't understand, I'm not pulling any feature out. All episodes of all podcast I've tried played, including some "difficult ones". What do you think I have removed? Episodes with redirection are redirected and play. I would never intentionally remove a feature unless we agreed.

[michaelherger]

The tracking of progress for podcasts with re-directs would no longer work, would it?

[philippe44]

No it does, why would'nt it? I tested that it still does and it was certainly the intention. That's why I need access to the "feed" url

[michaelherger]

Well, that tracking of redirect URLs was there for a reason. And you removed it. So whatever use case triggered the addition of that tracking five years ago now is broken again.

I don't remember the exact issue. But I think if you wanted to continue playback of an already partly played podcast after a server restart it wouldn't pick up your previous position, as LMS at this point didn't have the reference to the original URL any more.

3f9860f#diff-1860524a31642d8e2cc94c3079d2d82f

[philippe44]

IMHO, it was done this way because the "timer to track" was tracking the $track->url which is the redirected url. Now, if this timer, has access to playingSong/streamigSong feed url, you don't have that problem anymore and $song->streamUrl gives you access to that. The problem is when the redirected url changes with time, nothing was tracked at all, hence the need to track the invariant, the feed url

[michaelherger]

Minor cleanup work only.

I'll be happy to merge this if you can confirm that storing the values of that Tie::Cache::LRU have a well defined order.

[michaelherger]

Is the order, in which Tie::Cache::LRU would return values defined?

[philippe44]

the idea is that we load them in reverse order in init() to neutralize changes made here

[michaelherger]

Huh? If values returned the values in random order (I'm questioning this, not saying that's the case), then reverse wouldn't be of any help at all, right? It would only, well, reverse, whatever random order we got. As Tie::Cache::LRU seems to be based on an array, I could imagine the order is well defined. But then we're going through a hash implementation...

If you tested this, then that's good. If not, it might be worth some testing.

[michaelherger]

Wrote this little test script:

#!/usr/bin/perl

use Tie::Cache::LRU

tie my %lruList, 'Tie::Cache::LRU', 256;
%lruList = ();

foreach (qw(one two three four five six seven eight nine ten)) {
	$lruList{$_} = $_;
}

print join(', ', values %lruList, "\n");

print $lruList{one} . "\n";

print join(', ', values %lruList, "\n");

$lruList{one} = 1;

print join(', ', values %lruList, "\n");

1;

Which shows that not only would the list of returned values not be sorted by their LRU state, but also the items are returned in random order on every run:

$ ./lru.pl 
one, three, two, four, eight, five, six, seven, nine, ten, 
one
one, three, two, four, eight, five, six, seven, nine, ten, 
1, three, two, four, eight, five, six, seven, nine, ten, 

$ ./lru.pl 
nine, six, seven, two, three, ten, one, four, five, eight, 
one
nine, six, seven, two, three, ten, one, four, five, eight, 
nine, six, seven, two, three, ten, 1, four, five, eight, 

$ ./lru.pl 
three, eight, nine, six, four, five, ten, one, two, seven, 
one
three, eight, nine, six, four, five, ten, one, two, seven, 
three, eight, nine, six, four, five, ten, 1, two, seven, 

$ ./lru.pl 
eight, nine, six, seven, four, three, ten, five, two, one, 
one
eight, nine, six, seven, four, three, ten, five, two, one, 
eight, nine, six, seven, four, three, ten, five, two, 1, 

$ ./lru.pl 
one, four, six, eight, five, nine, ten, seven, two, three, 
one
one, four, six, eight, five, nine, ten, seven, two, three, 
1, four, six, eight, five, nine, ten, seven, two, three, 

Therefore I believe you'll have to handle the sorting order of that list yourself if you want to keep a consistent order.

[philippe44]

I will verify again, but in all the tests so far it works and this is also the same thing being used in YT plugin and 2 others

[philippe44]

oh, the declaration must be before the tie, otherwise it's just a hash

[michaelherger]

I will verify again, but in all the tests so far it works and this is also the same thing being used in YT plugin and 2 others

Ok, that's reassuring :-). What am I missing?

oh, the declaration must be before the tie, otherwise it's just a hash

Heh!... lesson learned! And I took my code straight out of LMS...

[philippe44]

What I can do is give again some though later this evening to see if there is any chance to get a protocolhandler working. At the end of the day, all we need is an "onStop" event callback and we're good for tracking ...

[philippe44]

BTW, I've been trying to use a protocol handler this evening and so far I failed miserably. I have it working without a timer and a hook to [newsong] but not when starting from an offset which of course is useless. In addition, it requires to insert dynamically an 'onStop" method in the class of the episode, which I'm not sure is a nice thing to do. I'm running out of ideas for this one

[philippe44]

Per my post on the forum, tracking progress does not work when using direct streaming. I've tried to turn every stone to find a solution but can't think of any. Problem revolves around how to keep track of the original url in case of redirection. In proxied mode, the Song::streamUrl field remains the original, non redirected url, but it is overwritten by the redirected url in direct streaming.

• a dedicated ProtocoHandler would have been convenient to memorize the original url and would have avoided the timer used to follow progress as the "onStop" method tracks stop position. But it does not work as it can't inherit from any parent class (we don't know what rss feed might really contain) so a lot of methods are missing and they must be set before we know what the track will exactly be.

• without that, we need (as it does today) to add a notification to 'newsong' to detect when a track starts. But at this point, we have lost track of the original url in case of proxied streaming as $song->streamUrl might have been overridden and $song->track->url points to the redirected url.

• another option was to try to store that url when the podcast XML menu is created (with podcast enclosure) and when an item is used to play, then transfer that url in a pluginData field that would be retried in the 'newsong' notification. But that works when using the Web version of XMLBrowser which honors 'play' menu field but other XMLBrowser (material/iPeng) don't. So, no luck there either.

The only option left if to add a field in Player::Song that is set at Player::Song::new and store the original url. It seems a bit funny to do that just for the podcast plugin. Unless we agree that this can be a general option that can be used in the future for other reasons

Running out of ideas ...

[michaelherger]

Is this a limitation that was there before? Or is this a regression?

I assume you did look into ways to force proxied streaming for podcasts only?

[philippe44]

No I did not try to force proxy to podcast only but I found a solution here #389