kingosticks / mopidy-tunein

Mopidy extension for playing music from tunein
Apache License 2.0
65 stars 14 forks source link

Dirty unwrap playlists. #21

Closed keinstein closed 8 years ago

keinstein commented 9 years ago

Some radio tuneIn URIs point to playlists. In such cases the playback fails as gstreamer doesn't support playlists so far.

The patch implements a working solution as a quick hack. For examples see: tunein:station:s56109 which is currently available e.g. via the path Europe->Germany->Dresden->Deutschlandradio Kultur 101.3 (Culture)

The patch is based on the solution in https://github.com/mopidy/mopidy/issues/1250 but the code is slightly changed as I first expected the error in the stream backend.

Maybe it would be much better to base the tunein backend on the stream backend rather than the bare backend api.

kingosticks commented 9 years ago

Thanks for the PR. I've not kept up with Mopidy's recent change regarding nested playlists, but I was under the vague impression it was going to handle all this stuff for us. I need to check what's going on with that when I'm back in a couple of days. On 23 Sep 2015 21:29, "keinstein" notifications@github.com wrote:

Some radio tuneIn URIs point to playlists. In such cases the playback fails as gstreamer doesn't support playlists so far.

The patch implements a working solution as a quick hack. For examples see: tunein[image: :station:]s56109 which is currently available e.g. via the path Europe->Germany->Dresden->Deutschlandradio Kultur 101.3 (Culture)

The patch is based on the solution in mopidy/mopidy#1250 https://github.com/mopidy/mopidy/issues/1250 but the code is slightly changed as I first expected the error in the stream backend.

Maybe it would be much better to base the tunein backend on the stream

backend rather than the bare backend api.

You can view, comment on, or merge this pull request online at:

https://github.com/kingosticks/mopidy-tunein/pull/21 Commit Summary

  • Dirty unwrap playlists.

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/kingosticks/mopidy-tunein/pull/21.

ZenithDK commented 9 years ago

I reworked the patch to pass the flake8 tests and also it now works for me - there were issues with the stream I tested with (http://live-icy.gss.dr.dk/A/A29H.mp3.m3u). New patch here: https://github.com/ZenithDK/mopidy-tunein/commit/452722a3ec2b7f0919072f1336f4e02315a2bca3 - didn't want to do a new pull request, but not sure how to handle it actually.

digrix commented 8 years ago

I tried to base TuneInPlayback on StreamPlaybackProvider as ZenithDK proposed https://github.com/mopidy/mopidy/issues/1250#issuecomment-146356612 and it works. What do you think about it? https://github.com/digrix/mopidy-tunein/commit/b2a0d173d262699ae0f7855736e27fe4a94298f4

keinstein commented 8 years ago

@digrix it looks nice regarding this particular problem. In general I think also the other classes in this file should be based on the corresponding Stream classes as suggested above. But I don't have the time to test it at the moment. Nevertheless, even at this state I think it is worth a separate concurring pull request to the current one.

digrix commented 8 years ago

Ok, I sent the PR.

Another issue for me is, that it now plays the first station from the m3u playlist. I don't know how is it with other radios, but for example one of my favourite statins returns the following playlist:

http://icecast5.play.cz/cro3-128.mp3
http://icecast4.play.cz/cro3-256.mp3
http://icecast3.play.cz/cro3-64.mp3
http://icecast5.play.cz/cro3-32.mp3

It currently selects the first one, so I don't get the best available quality :(. What I think about is either to add the whole playlist to queue and user can select or to sort the playlist by filename in numeric descending order as it could be a frequent case.

kingosticks commented 8 years ago

In my experience there is no consistent naming scheme and no easy way to reliably automatically pick the best stream from those discovered in a nested playlist. Ideally the stations would just list their streams from top to bottom in order of quality but they clearly don't all do this as your example demonstrates. Maybe they've found that 256k wasn't supported by some clients, or maybe not worth the extra bandwidth for some content types - who knows.

I've thought previously about modelling things slightly differently and thinking of a station as an album or an artist. An artist might make the most sense as each station (artist) would have a number of shows (albums) which are composed of a number of streams (tracks). I think it works for browsing but falls apart for searching. It'd also be quite different from other backends and, depending on the client, might be a very confusing user experience. You could easily end up with many tracks for each station cluttering your search results and making it much harder to find what you want. Some webclients may ignore the album and artist fields of search results; I can't remember what mpd clients handle here. Perhaps it's a bad idea.

Modelling as a playlist doesn't really work for the same reasons, plus you cannot return playlists from search results.

keinstein commented 8 years ago

As far as I know mpd does not have a tunein plugin (thats the reason why I'm using mopidy).

Probably modeling the playlists as folders would be OK in a first step. Unfortunately, this would break my current application. As my raspi radio allows to store the entire list of stations of a directiory as radio playlist in order to avoid the long path through the menu on a 2x16 character LCD. When you model the playlists as directories this would add many stations in many flavours (subfolders are flattened before adding them), thus, sorting the playlist gets more difficult, while still possible.

At least some playlists store information about the bandwidth of the compressed audio. This could be used. Probably an automatic bandwidth selection would be nice. If no bandwidth information is available from the playlist, it may be obtained from the streams. This should be implemented in the stream backend, not in the tunein backend.

kingosticks commented 8 years ago

In what format is this stored? Do you have an example? Getting bandwidth from the stream itself means first connecting to the stream and that'll be slow if there are lots of streams and even slower of some of them are broken and you must wait to timeout before proceeding.

Stream playlists as directories unfortunately won't work for searc results which only return tracks, albums or artists. On 22 Oct 2015 10:12, "keinstein" notifications@github.com wrote:

As far as I know mpd does not have a tunein plugin (thats the reason why I'm using mopidy).

Probably modeling the playlists as folders would be OK in a first step. Unfortunately, this would break my current application. As my raspi radio allows to store the entire list of stations of a directiory as radio playlist in order to avoid the long path through the menu on a 2x16 character LCD. When you model the playlists as directories this would add many stations in many flavours (subfolders are flattened before adding them), thus, sorting the playlist gets more difficult, while still possible.

At least some playlists store information about the bandwidth of the compressed audio. This could be used. Probably an automatic bandwidth selection would be nice. If no bandwidth information is available from the playlist, it may be obtained from the streams. This should be implemented in the stream backend, not in the tunein backend.

— Reply to this email directly or view it on GitHub https://github.com/kingosticks/mopidy-tunein/pull/21#issuecomment-150157135 .

kingosticks commented 8 years ago

So I finally went ahead and merged https://github.com/kingosticks/mopidy-tunein/pull/22. Apologies for taking so long and thanks to everyone who contributed.

jcass77 commented 8 years ago

I think this fixes a substantial enough problem that it warrants another release of Mopidy-TuneIn.

There really is a large number of TuneIn URI's that cannot be played without this PR, which the average user installing from PyPI won't have access to.