Got a link for the API of the new model?

It doesn't seem to be documented anywhere.  I implemented it based on the official last.fm player source.

Created attachment 105820 [details] [review]
mostly working

The main problem for now is that gnomevfssrc makes two requests when opening http urls, and last.fm only allows clients to play tracks once.  Playback only works some of the time because the server doesn't invalidate our playback urls immediately, so if the second request arrives soon enough it'll still give us the track.

souphttpsrc is probably going to become the default http src element soon, and it won't have this problem.

Other things:
- the playlist parsing code should obviously go in totem-pl-parser, as it's just xspf
- handling of error cases (login failure, last.fm not giving us a playlist because no one listens to the specified artist, etc.) is probably pretty bad
- playing the songs out of order doesn't really make much sense, so we should override the play order somehow

Found some docs:
http://code.google.com/p/thelastripper/wiki/LastFM12UnofficialDocumentation
http://code.google.com/p/lastfmmobile/wiki/HowSessionsVersionsAffectingNowPlayingPHP

Do you want those additional tags parsed by the totem-pl-parser? Or if you want to have control over the downloading, should I add API to allow parsing directly from data?

lastfm:trackauth
lastfm:albumId
lastfm:artistId

I've added more tags to totem-pl-parser, it looks like this with the example playlist in the docs above:

added URI 'http://kingpin4.last.fm/user/d87034ad6ab5d7ed388a6dcd2d2df506.mp3'
        title = 'Killer Queen (Live)'
        duration = '118000'
        image-url = 'http://images.amazon.com/images/P/B000000OAP.01._SCMZZZZZZZ_.jpg'
        author = 'Queen'
        moreinfo = 'http://www.last.fm/music/Queen/_/Killer+Queen+%28Live%29'
        album = 'Live Killers'

Note that the duration is in milli-seconds. So we should probably use a different tag for it. Ideas?

We should also handle the "freeTrackurl", if Rhythmbox can use it. Want it?

Finally, we should get Rhythmbox added to the accept client names.

+			case 4:		/* unavailable */
That should use a different error message. ("The station is not available for streaming.")

It'd be great to have API for parsing directly from data.  For now I'll save the playlist to a temporary file and parse that.

I'm not sure the millisecond granularity for the duration tag is useful.  Perhaps we could have 'duration-ms' in milliseconds in addition to the regular 'duration' tag if anyone wants it.

Apparently we'll need the lastfm:trackauth tag to submit songs from last.fm streams back to last.fm when we switch to version 1.2 of the submission protocol (http://www.audioscrobbler.net/development/protocol/).  We're still using version 1.1, so we don't need it yet.

The free track URL sounds useful.  I haven't seen what the URLs look like.. hopefully it's a direct mp3 link so we can just have a 'download to library' button on the toolbar.

(In reply to comment #8)
> It'd be great to have API for parsing directly from data.  For now I'll save
> the playlist to a temporary file and parse that.

Can't really do this easily right now, before the freezes and all. Could you file a bug for that purpose?

> I'm not sure the millisecond granularity for the duration tag is useful. 
> Perhaps we could have 'duration-ms' in milliseconds in addition to the regular
> 'duration' tag if anyone wants it.

We don't give out a parsed duration from the entry-parsed signal, and use totem_pl_parser_parse_duration later on to have it parsed. So we'd use duration-ms in this case.

Added as TOTEM_PL_PARSER_FIELD_DURATION_MS.

> Apparently we'll need the lastfm:trackauth tag to submit songs from last.fm
> streams back to last.fm when we switch to version 1.2 of the submission
> protocol (http://www.audioscrobbler.net/development/protocol/).  We're still
> using version 1.1, so we don't need it yet.

Added as TOTEM_PL_PARSER_FIELD_ID.

> The free track URL sounds useful.  I haven't seen what the URLs look like..
> hopefully it's a direct mp3 link so we can just have a 'download to library'
> button on the toolbar.

Added as TOTEM_PL_PARSER_FIELD_DOWNLOAD_URL.

2008-02-24  Bastien Nocera  <hadess@hadess.net>

        * plparse/totem-pl-parser-xspf.c (parse_xspf_track):
        * plparse/totem-pl-parser.c (totem_pl_parser_class_init):
        * plparse/totem-pl-parser.h: Add support for the some last.fm
        XSPF tags (lastfm:trackauth, duration in milliseconds, and
        download URLs as TOTEM_PL_PARSER_FIELD_ID,
        TOTEM_PL_PARSER_FIELD_DURATION_MS and
        TOTEM_PL_PARSER_FIELD_DOWNLOAD_URL respectively

Thanks for the totem-pl-parser updates.  I've reworked my patch to use it now, and everything's working nicely.  I've also got playback working properly (without souphttpsrc) - turns out I just needed to stop trying to parse the playback URLs as playlists.

Instead of a specific parse-from-data API, perhaps once the gio port is done, it would make sense to have something like this:

  totem_pl_parser_parse_from_stream (TotemPlParser *parser, GInputStream *stream, GFile *base, gboolean fallback)

gio includes an input stream that reads from an in-memory buffer, so this use case is provided for, and it might be useful for other things too.

Created attachment 105941 [details] [review]
working a bit better

this also includes the changes to rhythmdb.h that were missing from the previous patch.  oops.

(In reply to comment #10)
<snip>
> Instead of a specific parse-from-data API, perhaps once the gio port is done,
> it would make sense to have something like this:
> 
>   totem_pl_parser_parse_from_stream (TotemPlParser *parser, GInputStream
> *stream, GFile *base, gboolean fallback)
> 
> gio includes an input stream that reads from an in-memory buffer, so this use
> case is provided for, and it might be useful for other things too.

Sounds like a good idea, although we usually just read from the file completely, and go from there :) Could you file a bug about it?

About the patch. You could simplify the xspf_entry_parsed() code by having a table with rhythmdb<->pl-parser tags match, and handling just that few special-cases handled. Also, the totem_pl_parser_parse_duration() retval can be negative when it fails to parse (although I don't think this could happen with a properly formed XSPF file).

As for TOTEM_PL_PARSER_FIELD_IMAGE_URL, I can easily not pass back the metadata if it's not interesting (ie. equal to LASTFM_NO_COVER_IMAGE).

(In reply to comment #12)

> Sounds like a good idea, although we usually just read from the file
> completely, and go from there :) Could you file a bug about it?

Bug 518811 filed.
 
> About the patch. You could simplify the xspf_entry_parsed() code by having a
> table with rhythmdb<->pl-parser tags match, and handling just that few
> special-cases handled. Also, the totem_pl_parser_parse_duration() retval can be
> negative when it fails to parse (although I don't think this could happen with
> a properly formed XSPF file).

Thanks, I'll fix these in the next revision of the patch.

> As for TOTEM_PL_PARSER_FIELD_IMAGE_URL, I can easily not pass back the metadata
> if it's not interesting (ie. equal to LASTFM_NO_COVER_IMAGE).

We ignore the last.fm 'no cover image' image because we've already got one of our own.  A dedicated last.fm streaming app using totem-pl-parser might want to use last.fm's image, so I think it makes sense to leave this check to the app.

Created attachment 105983 [details] [review]
another update

some cleanups, error handling works a bit better, uri-encode lastfm:// uris when building change-station requests.

*** Bug 524482 has been marked as a duplicate of this bug. ***

Created attachment 108601 [details] [review]
play order mangling

This gives sources the ability to provide their own play order (rearranging the queue code a bit).  This will probably be useful for a few other things.  The last.fm source provides a play order that only goes forwards.

I haven't tested the play order changes much, but they at least work for last.fm, and the play queue seems OK too.

I plan to commit this soon unless someone gives me a reason not to.

Committed.

Just tried recompiling with SVN r5748 and got this:

ps/rb-lastfm-source.Tpo -c /home/alex/src/remote-cvs/gnome.org/rhythmbox/plugins/audioscrobbler/rb-lastfm-source.c  -fPIC -DPIC -o .libs/rb-lastfm-source.o
/home/alex/src/remote-cvs/gnome.org/rhythmbox/plugins/audioscrobbler/rb-lastfm-source.c: In function 'xspf_entry_parsed':
/home/alex/src/remote-cvs/gnome.org/rhythmbox/plugins/audioscrobbler/rb-lastfm-source.c:1856: error: 'TOTEM_PL_PARSER_FIELD_ALBUM' undeclared (first use in this function)
/home/alex/src/remote-cvs/gnome.org/rhythmbox/plugins/audioscrobbler/rb-lastfm-source.c:1856: error: (Each undeclared identifier is reported only once
/home/alex/src/remote-cvs/gnome.org/rhythmbox/plugins/audioscrobbler/rb-lastfm-source.c:1856: error: for each function it appears in.)
/home/alex/src/remote-cvs/gnome.org/rhythmbox/plugins/audioscrobbler/rb-lastfm-source.c:1885: error: 'TOTEM_PL_PARSER_FIELD_DURATION_MS' undeclared (first use in this function)
/home/alex/src/remote-cvs/gnome.org/rhythmbox/plugins/audioscrobbler/rb-lastfm-source.c:1904: error: 'TOTEM_PL_PARSER_FIELD_ID' undeclared (first use in this function)
/home/alex/src/remote-cvs/gnome.org/rhythmbox/plugins/audioscrobbler/rb-lastfm-source.c:1909: error: 'TOTEM_PL_PARSER_FIELD_DOWNLOAD_URL' undeclared (first use in this function)
make[4]: *** [rb-lastfm-source.lo] Error 1
make[4]: Leaving directory `/home/alex/build/rhythmbox/plugins/audioscrobbler'


Does the version of totem-pls-parser need to be higher than totem-pl-parser-2.21.5 (which is what I compiled from source on Fedora 8)?  If so, then the configure script needs to be tweaked to reflect the minimum required version.

It could probably be made to work with the existing dependency version (wrapping the code that uses those fields with #if TOTEM_PL_PARSER_CHECK_VERSION(2,22,0) .. #endif), but you wouldn't get artist or album names, track durations, cover art, etc.

That doesn't seem like a great idea to me, so I've bumped the required version in configure.ac.  Thanks for pointing that out.

OK, just tested it out for the first time.  It more or less "works" but there are several significant problems I found:

1. I selected one of my existing stations, it populated the pane below with a list of 5 tracks.  I played one or two, skipped to the next (this works), then let it play the rest of the playlist.  Unfortunately on the last track, it didn't magically fill up with new tracks, but looped around to start playing the beginning track.  I tried this several times (skipping tracks) and it kept doing this.

2. When clicking from station to station it feeds up with new tracks on every click, which means that if you click back to the station you are currently playing, it has a completely new set of tracks which don't match the current track, so that information is lost, which is related to... number 3:

3. If you happen to single click a new station, it doesn't seem to be possible to use the usual "Ctrl-J" to jump to the playing song because of the issue above.  If you Ctrl-J from another source (e.g. the Library) it simply gives you focus to which station was selected last.

4. No tracks appear to be submitted to last.fm, nor are they stored in the queue (Library tracks are being submitted fine, tracks being played via last.fm radio stations are not).

Another issue while I'm at it:

5. Double-clicking on the radio station name doesn't start playing tracks in the feed like the old source did.  You have to click on the track itself, which breaks the existing pattern and doesn't feel that intuitive.

(In reply to comment #22)
 
> 2. When clicking from station to station it feeds up with new tracks on every
> click, which means that if you click back to the station you are currently
> playing, it has a completely new set of tracks which don't match the current
> track, so that information is lost, which is related to... number 3:

When you change station, all existing tracks are invalidated on the server and cannot be played.  There's no getting around this.

(In reply to comment #24)
> (In reply to comment #22)
> 
> > 2. When clicking from station to station it feeds up with new tracks on every
> > click, which means that if you click back to the station you are currently
> > playing, it has a completely new set of tracks which don't match the current
> > track, so that information is lost, which is related to... number 3:
> 
> When you change station, all existing tracks are invalidated on the server and
> cannot be played.  There's no getting around this.

But I'm not "changing stations", I'm simply changing focus station name (or using up or down arrows to scroll through the names).  The way all other sources work, is that you need to double-click an entry for it to actually start playing.  

I don't expect simply changing focus in the list of stations to affect playback, and I'm sure this will surprise most users.  For example as I go through the genres in the Library, I use the mouse or arrows to scroll while I'm playing back another track and I don't expect it to change the current playlist or list of tracks out from underneath me.

For that, I assume that you need to at least click the entry.  You should be able to change the UI so it only activates or repopulates a station on a double-click (which was the old behaviour).

What are you expecting will happen when you click on a different station name?  Either nothing happens (not particularly good; the playlist for the station will be empty) or it changes to that station and fetches some playlist entries.

I would expect that nothing happens until I tell it to do so (i.e. by double-clicking the station name).   Simply changing focus on the station names breaks with the UI convention for every other source.  If this happened when you changed focus on podcast feeds as you were playing a podcast feed then that would be equally jarring.

Double-clicking on a station, yes, I expect that the playlist would fill and start playing, merely changing focus should not do so.

Perhaps in the playlist area, it could have a message saying: to start station please double-click on feed.  I know it's not optimal, but better than the current behaviour.

Even more seriously, do you know what is going on with the looping in my point 1 above.  Can you replicate this?

In general it seems that it easily breaks and gets into some weird state if you start clicking entries on the playlist at random and/or skipping tracks.  Perhaps entries that can't be played now (i.e. under the "top" entry) could be greyed-out so they weren't selected and tracks played would disappear as they were played (or perhaps the last one kept as grey) and new tracks would appear, so the length of the playlist remained constant.  That's more or less how I would expect it to work (somewhat like iTunes party mode).

(In reply to comment #27)
> I would expect that nothing happens until I tell it to do so (i.e. by
> double-clicking the station name).   Simply changing focus on the station names
> breaks with the UI convention for every other source.  If this happened when
> you changed focus on podcast feeds as you were playing a podcast feed then that
> would be equally jarring.
> 
> Double-clicking on a station, yes, I expect that the playlist would fill and
> start playing, merely changing focus should not do so.

So, what do you think should happen when you merely select another station?

(In reply to comment #28)

> > Double-clicking on a station, yes, I expect that the playlist would fill and
> > start playing, merely changing focus should not do so.
> 
> So, what do you think should happen when you merely select another station?

I should say "merely focused" another station.  As I suggested: "perhaps in the playlist area, it could have a message saying: to start station please double-click on feed.  I know it's not optimal, but better than the current behaviour."

When you focus another station even as you are playing the old station, there is now no way to (re-)focus the currently playing track because that playlist (and the track that was contained within it) disappears.  Again, no other source behaves like this, which further confuses the user.

I really dislike adding instructions to the user interface.  It seems like a blatant admission that the interface doesn't make sense, and it always annoys me as a user.

I'm in the process to reworking how the playlist updates work.  Rather than pretending there's a separate playlist for each station (which was how I was hoping it could work), there will be one playlist for everything.

I'm also going to change the way station changes work.  Instead of fetching a new playlist when the selection changes, the selection will just provide the station to use when the next playlist update occurs.  Activating a station will immediately change to that station, fetching a new playlist and starting to play the first entry from it.  This will also make it possible to delete a station without causing a playlist update.

That's mostly done and committed now.  This should fix items 2 and 3 on your original list, and probably item 1 too.  

Currently when I skip songs I get playback errors saying "failed to seek; the server does not accept the Range HTTP header", then the song display gets out of sync with what's actually playing.  Probably a crossfading bug.

Thanks for the feedback.

Actually, that's caused by the souphttpsrc gstreamer element interacting badly with last.fm's streaming servers.  I'm not really sure what to do about it.

*** Bug 545173 has been marked as a duplicate of this bug. ***

*** Bug 559508 has been marked as a duplicate of this bug. ***