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

m3u: Major refactoring #1386

Merged
merged 5 commits into from Jan 11, 2016

Conversation

4 participants
@tkem
Member

tkem commented Jan 4, 2016

This is a major refactoring of the m3u extension bundled with Mopidy core, motivated by #1370 . I would rather have made several smaller PRs, but in the end was unable to get this straight without tearing everything apart and starting from scratch again. I apologize.

Major changes included in this PR:

  • Remove mopidy.internal and mopidy.compat imports to make it easier to split this from Mopidy core. Note this means that m3u.translator now defines its own uri_to_path and path_to_uri methods since it can't use mopidy.internal.path any more, so I took the opportunity and tried to improve Python 3 compatibility in this area.
  • Remove m3u/library.py since this was only a stub, anyway, and its purpose was not entirely clear to me.
  • Do not scan playlist directory and parse playlists at startup or refresh; similar to the file extension, this now happens on request.
  • Use Refs for reading and writing playlists. This means that Track.length is no longer stored in extended M3U playlists and #EXTINF runtime is always set to -1.
  • Add new config value default_encoding for specifying the encoding of plain .m3u files. Motivated by #1364, so @Joolee can set this to utf-8-sig, for example. The Wikipedia entry for M3U, which serves as our primary specification, now states that "the text is encoded in the local system's default non-Unicode encoding", which is rather vague. UTF-8 encoding is still required fro .m3u8 files.
  • Add new config value default_extension to specify the extension for playlists created using the Playlists.create() API. Defaults to .m3u for backwards compatibility, but should probably be set to .m3u8, since few clients use that part of the API yet.
  • Clean up error handling, e.g. remove assertions, to bring this more in line with the API docs. Note that AFAICS the docs don't specify error handling for Playlist.create(), but returning None seems to be the expected behavior.
  • New m3u.translator unit tests using py.test. There's some room for improvement, especially regarding file system encodings.
  • Only minor changes to test_playlists.py.

Note that I did not update the changelog yet, since I guess this PR will invoke some discussion and may take several iterations ;-)

except EnvironmentError as e:
log_environment_error('Error reading playlist %s' % uri, e)
else:
return translator.playlist(path, items, mtime)
def refresh(self):

This comment has been minimized.

@adamcik

adamcik Jan 5, 2016

Member

This could in theory check the mtimes of the playlists to see if they have changed, and then emit playlist changed events. Not sure if this is needed though and if it would make sense for any clients (mostly thinking about MPD ones). And if we went down that path we would probably want to consider just watching for changes instead. So perhaps just file a bug if more people think this could make some sense?

This comment has been minimized.

@tkem

tkem Jan 5, 2016

Member

Well, I think getting rid of the self._playlists dict and following the file backend is probably the most radical change in this PR, so I'm quite curious how clients will react to that ;-)
However, triggering a playlist_changed event in lookup or as_list or whatever doesn't make that much sense, IMHO. The client triggering the lookup() or whatever will notice the change anyway, so there would be only a benefit if another client accidentaly triggers this, AFAICS.
If we were watching the playlist_dir using inotify, for example, triggering playlist_changed events would make sense. And when modifying playlists via the Mopidy API, playlist_changed events should be sent, of course.

def save(self, playlist):
path = translator.uri_to_path(playlist.uri)
name = translator.name_from_path(path)
try:

This comment has been minimized.

@adamcik

adamcik Jan 5, 2016

Member

We probably want to have a write-replace based on http://blog.gocept.com/2013/07/15/reliable-file-updates-with-python/ - My plan is currently to take the code in local, improve it and then expose in such a way that it can be used in local, for the state loading feature and here as well. Though I don't know yet if it would go into internal or not so maybe just copy the pattern from the link to have:

with tempfile.NamedTemporaryFile(
      'w', dir=os.path.dirname(filename), delete=False) as tf:
   tf.write(...)
   tempname = tf.name
os.rename(tempname, filename)
dirfd = os.open(os.path.dirname(filename), os.O_DIRECTORY)
os.fsync(dirfd)
os.close(dirfd)

This comment has been minimized.

@tkem

tkem Jan 5, 2016

Member

Thought about this myself, but then decided to postpone this; there's already the case of renaming and writing in Playlists.save(), and another os.rename() would have made my brain hurt ;-)

@adamcik

This comment has been minimized.

Member

adamcik commented Jan 5, 2016

Nothing major that stuck out after an initial pass from my point of view at least.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jan 5, 2016

I'm curious about the third one, what is the advantage of that? Is making it more like the File backend better than making it different to the major existing playlist providers (gmusic, spotify).

@tkem

This comment has been minimized.

Member

tkem commented Jan 5, 2016

@kingosticks: Basically, three things:

  • New/changed/deleted M3U files will be available instantly. Now you have to do a Playlists.refresh() to detect any changes. Think of it as local and its mopidy local scan command vs. the file backend.
  • Improved startup time, since the extension doesn't have to read every M3U playlist at startup. This could have been worked around in other ways, though.
  • I think it will be easier to maintain that way, and it kind of looks like that's going to be my job for the forseeable future ;-)
@tkem

This comment has been minimized.

Member

tkem commented Jan 5, 2016

BTW, I don't think of gmusic and spotify as "playlist providers", since they don't implement even half the playlists API ;-)

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jan 5, 2016

It's been a while since I dabbled with the m3u backend, but doesn't the current code update it's _playlist structure and make the changes available instantly? Don't the events allow you to detect changes?

It just seems odd to start an extension with a playlist provider and not actually have the playlists available yet. I agree that parsing the contents is no longer necessary with the new as_list/get_items API and that might be the cause of any bottleneck (I only remember one user ever complaining about the startup time though).

@tkem

This comment has been minimized.

Member

tkem commented Jan 5, 2016

@kingosticks: Sorry, I don't quite get what you mean. For the m3u backend to detect changes "instantly", it would have to watch the playlist directory using inotify or something like that.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jan 5, 2016

Ohhhhh sorry, you obviously mean M3U files that you create/changed/delete outside of Mopidy.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jan 5, 2016

I suppose that watching (with inotify/whatever) would be the equivalent to what pyspotify does when it notifies a change made in an official Spotify client. But even with this proposed solution you won't be able to automatically update the client with any background changes and they still need to do a refresh?

@tkem

This comment has been minimized.

Member

tkem commented Jan 5, 2016

If (and that's clearly outside the scope of this PR) something like inotify-watches get implemented, a playlist_changed event would get triggered on any file changes, so clients would get notified instantly. However, this does not affect the changes in this PR in any way, AFAICS.

@tkem

This comment has been minimized.

Member

tkem commented Jan 5, 2016

Just to make this clear: Any changes to M3U playlists done through the Mopidy API will already result in playlist_changed or playlist_deleted events, so other clients connected will be notified. This is mostly done in core.PlaylistsController. So the changes in this PR don't affect that, really.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jan 5, 2016

One could argue it falls under the umbrella of 'major re-factoring'! But more seriously, I'm convinced, sounds good.

@tkem

This comment has been minimized.

Member

tkem commented Jan 6, 2016

@kingosticks: Main issue here is that inotify is not in standard Python. There are several packages on PyPi which provide it (for Linux only, IIRC), and they would all have to be evaluated. Anyway, @jodal wouldn't want another dependency in Mopidy core, so this won't become an option until m3u becomes a separate extension.

@tkem

This comment has been minimized.

Member

tkem commented Jan 10, 2016

Added write-replace functionality as a context manager, to minimize changes to existing code. Note that I intentionally omitted syncing the directory, as in @adamcik's example, since

  • it's not portable: According to the docs, O_DIRECTORY is a GNU extension, and I don't know (and can't verify) if this is supported on Mac, for example
  • IMHO, it's flawed: I've seen this pattern multiple times, and it usually ignores the fact that a crash may as well happen after the rename but before the fsync of the directory descriptor. So this maybe raises the odds that the rename is persisted, but mission-critical applications would still have to deal with left-over temp files. I don't consider Mopidy to be "mission-critical" enough to make that effort, sorry ;-) In the rather unlikely event that a crash (or other incident) occurs before the directory gets sync'd to disk by the OS, the left-over temp files should do no harm, anyway.
@adamcik

This comment has been minimized.

Member

adamcik commented Jan 10, 2016

Nice, I'll probably followup stealing that helper over to internal and making use of it on local.

As for

BTW, I don't think of gmusic and spotify as "playlist providers", since they don't implement even half the playlists API ;-)

I hindsight we probably should have had a read-only and a write API for playlists split up. As we've had quite a few cases of backends only implementing the write part of it.

Can't think of anything else blocking this as the only other comments are things that are beyond the scope of this PR, so merging :-)

@adamcik

This comment has been minimized.

Member

adamcik commented Jan 10, 2016

Ah, wait a sec, add changelog entry and then I'll merge :-)

@tkem

This comment has been minimized.

Member

tkem commented Jan 10, 2016

Sure. Shall I also add to the playlist docs that create should return None on error? I don't like backends hiding errors from clients usually, but this is more in line with the rest of the playlist API, and I think core checks it when iterating over backend.

@adamcik

This comment has been minimized.

Member

adamcik commented Jan 10, 2016

Yeah, makes the most sense with the current state of things. https://docs.mopidy.com/en/latest/api/core/#mopidy.core.PlaylistsController.save is the closest thing I see to us properly specifying this. Ideally we should probably switch to having a backend exception for these cases as the upgrade path would be backwards compatible anyway.

@adamcik

This comment has been minimized.

Member

adamcik commented Jan 10, 2016

That is at least for the core -> backends part, as for changing the behavior core exposes to frontends...

@tkem

This comment has been minimized.

Member

tkem commented Jan 10, 2016

One more thing: I'd like to change default_extension to .m3u8, so all track names can be encoded properly. Shouldn't affect existing clients, AFAICS. Oh, and new config values also need docs, of course.

@tkem

This comment has been minimized.

Member

tkem commented Jan 10, 2016

I hindsight we probably should have had a read-only and a write API for playlists split up.

Alternatively, we could encourage to make read-only "playlists" only available via the browse interface, and to treat them more like directories.

@adamcik

This comment has been minimized.

Member

adamcik commented Jan 10, 2016

Mhm, I've also played with the idea of having browse(PLAYLIST_ROOT) as an API for that, but I'm getting of topic now :-)

@Joolee

This comment has been minimized.

Joolee commented Jan 10, 2016

While discussing the Playlist API. Is this a good time to suggest adding a "get first song" and "get random song" method to the playlist API? When choosing to play a large (10.000+ songs for example) playlist, some delay is inevitable while the track objects are passed through the software. A client, or maybe even Mopidy itself could transparently implement these methods to start playback immediately. This would greatly improve response time and overall user experience.

Now that playlist objects are in memory, on a Raspberry pi 2 B and a playlist of 7500 items, the delay is quite noticeable. If I have read this PR correctly, the delay will increase because playlists will be read on the fly now.

@tkem

This comment has been minimized.

Member

tkem commented Jan 10, 2016

@Joolee: That's clearly a feature request that doesn't belong in this PR; please open a separate issue for that, or simply post it on discuss.mopidy.com for discussion.

@tkem

This comment has been minimized.

Member

tkem commented Jan 10, 2016

@adamcik: I think that's it from my part; please have another look at the updated docs and changelog.

adamcik added a commit that referenced this pull request Jan 11, 2016

@adamcik adamcik merged commit aa9e806 into mopidy:develop Jan 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adamcik adamcik added this to the v1.2 - Gapless and GStreamer 1.x milestone Jan 11, 2016

@adamcik adamcik self-assigned this Jan 11, 2016

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jan 22, 2016

@tkem I'm sorry for this post-merge question here, but is playlist_dir the correct directory to be using at https://github.com/mopidy/mopidy/blob/develop/mopidy/m3u/playlists.py#L100 ? Isn't it media dir we want here so that relative paths work as they used to?

@tkem

This comment has been minimized.

Member

tkem commented Jan 22, 2016

@kingosticks: Good question. According to https://en.m.wikipedia.org/wiki/M3U, which kind of serves as our reference, "local pathname relative to the M3U file location". Plus, for the file extension, you have multiple media_dirs as potential roots.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jan 22, 2016

This is different to how mpd behaves, do we really want that? I think it's worth opening an issue to discuss this.

@tkem

This comment has been minimized.

Member

tkem commented Jan 23, 2016

Sure, go ahead. Still, IIRC there are still plans to move local out of Mopidy core, which means there may not even be a media_dir (singular) in the future.

@tkem

This comment has been minimized.

Member

tkem commented Jan 23, 2016

BTW, same thing has been discussed for m3u itself, and I'd like for (at least optional) extensions to be self-contained.

@tkem tkem deleted the tkem:m3u-refactoring branch Jan 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment