convert: New feature "Write m3u playlist to destination folder"

[JOJ0]

Description

• Enables the functionality proposed in https://github.com/beetbox/beets/discussions/4373
• The convert plugin is suitable to be used as a media file exporter already.
• Thus it makes sense to add a feature to save a playlist file into the export folder containing all exported media files in the order they were exported/converted.
• Since with the convert plugin all sorts of "beets queries" could be exported/converted, the feature uses a more general approach than described in the initial proposal (above linked discussion). To achieve exactly what's described in the discussion, additionally the beets playlist feature can be used.
• Relative paths should be used to make the playlist file portable, e.g used on another computer.
• The classic m3u format does not do well with special characters in media file names. Since the beets "path" variables could be used to write all sorts of characters, the m3u8 format which requires unicode for all media file entries, is required.

To Do

• [x] Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
• [x] Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
• [x] Tests.
  • [x] convert plugin tests.
  • [x] M3UFile class tests (macOS/Unix/General).
  • [x] M3UFile class tests (Windows).
• [x] Currently there is an issue with VLC media player not reading the generated files correctly. It seems to still have a problem with special characters (e.g square brackets: []). -> Fixed, it was a bug in VLC 3.0.16, it works with 3.0.17.3, see https://forum.videolan.org/viewtopic.php?f=2&t=158920&p=529284#p529284
• [x] It seems that there is another bug (?) in the current VLC version 3.0.17.3 that files containing the # character are not opening, also see same post: https://forum.videolan.org/viewtopic.php?f=2&t=158920&p=529284#p529284 I'm not considering this a problem with my implementation here. Let's see what happens on that forum post, but for now I check this task as done.
• [x] Test correct reading of the files in other software. So far working fine (all tested on macOS 10.15.3 Catalina):
  • [x] iTunes (latest on Catalina)
  • [x] Traktor Scratch Pro 2 (latest, 2.11.3.17)
  • [x] VLC (3.0.17.3)
• [x] ~Ensure cross-platform compatibility~Ensure compatibility of playlists generated on an OS for an OS. For example, lists generated on Windows should work on Windows.

[JOJ0]

Usage examples

Simple example

Using a beets query to save a playlist of all files of an artist sorted chronologically:

plugins:
    - convert

convert:
    dest: /Users/jojo/Music/Techno/0-export
    never_convert_lossy_files: yes
    no_convert: format:AAC, format:WMA, format:ALAC, format:FLAC
    threads: 1
    paths:
        default: "$artist - $album - $title [$catalognum] ($year)"
        singleton: "$artist - $album - $title [$catalognum] ($year)"
        comp: "$artist - $album - $title [$catalognum] ($year)"

beet convert -y "artist:Amon Tobin" year+ --dest ~/Downloads/tmp --playlist 0-amon_tobin_chronologically.m3u

More advanced example

An existing playlist (eg. generated with the "smartplaylist" plugin) should be exported to the export folder:

Required config:

plugins:
    - convert
    - playlist

playlist:
    playlist_dir: ~/Music/Techno/0-playlists
convert:
    dest: /Users/jojo/Music/Techno/0-export
    never_convert_lossy_files: yes
    no_convert: format:AAC, format:WMA, format:ALAC, format:FLAC
    threads: 1
    paths:
        default: "$artist - $album - $title [$catalognum] ($year)"
        singleton: "$artist - $album - $title [$catalognum] ($year)"
        comp: "$artist - $album - $title [$catalognum] ($year)"

beet convert -y playlist:/Users/jojo/Music/Techno/0-playlists/110..130_TechnoJungleDub_Lo.m3u --dest ~/Downloads/tmp --playlist 0-play.m3u8

Certainly any playlist file could be exported, no matter if it was created with a beets plugin or manually, as long as it's using media files existing in the beets db already.

It's intended that the playlist file is saved alongside the media files, in this case the path provided by the --dest option. If --dest is not provided with the command it's taken from the config file (as the convert plugin handles it already).

[sampsyo]

Seems like a neat feature! I have a couple of high-level questions about the way the playlist file is generated here:

• Are races possible (when the plugin is running in parallel)? I notice that the playlist is not written out in a single "batch"; rather, the file is re-opened to write every line (to record every file). When the files are being converted in parallel, in separate threads, could this cause a problem where different threads are trying to write to the same file at the same time?
• Is there any way that we could centralize logic with the playlist and/or smartplaylist plugins, which also write .m3u files? Having a single implementation of this could really help ease a maintenance burden, especially if we ever change how we want to write these files to include the extension comment, for example (see #4356).

[JOJ0]

yes there will most probably be an issue with threading I guess. Actually I wanted to ask you for advice on that topic :-) Thanks for the early review I'll think about your ideas and will hopefully get back to this next week and probably have more questions.

[sampsyo]

Sounds good! I wonder if there's a way to "batch up" all the new paths and then write them out all at once…

[JOJ0]

This is stalling.

There is summer/vacations/other projects need finishing but most of all because I don't get the unittest to work.

Anyway, I have 5 minutes and ask for help. I'm currently trying to write a unittest that checks whether the playlist.m3u8 file was written and is existing. I'm feeling bad already for asking Adrian all the time, he probably has a lot on the plate with this project already. @arsaboo I'm not sure if you're officially a maintainer of the project but you seem highly motivated lately and since you have quite some experience with your Spotify and Plex stuff going on you might be able to help: I'm trying to check if a file was created, but the file doesn't seem to be there: https://github.com/beetbox/beets/pull/4399/commits/147e01885bae522da613f63f5d54c6c1cee88213 This is what I get:

(beets) ➜  beets git:(convert_playlist) python -m unittest test.test_convert.ConvertCliTest.test_playlist
Sending event: database_change
Sending event: database_change
Sending event: item_copied
Sending event: database_change
Sending event: database_change
Sending event: database_change
Sending event: database_change
Sending event: database_change
no user configuration found at /var/folders/3t/l575q0f96fx91pp61zb158z00000gn/T/tmpettdj4j2/config.yaml
data directory: /var/folders/3t/l575q0f96fx91pp61zb158z00000gn/T/tmpettdj4j2
plugin paths:
Sending event: pluginload
the artist - älbum - tïtle 0
Convert? (Y/n) convert: Creating playlist file: /var/folders/3t/l575q0f96fx91pp61zb158z00000gn/T/tmpettdj4j2/convert_dest/playlist.m3u8
E
======================================================================
ERROR: test_playlist (test.test_convert.ConvertCliTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jojo/git/beets/test/test_convert.py", line 254, in test_playlist
    self.run_convert('--playlist', 'playlist.m3u8')
  File "/Users/jojo/git/beets/test/test_convert.py", line 156, in run_convert
    return self.run_convert_path(self.item.path, *args)
  File "/Users/jojo/git/beets/test/test_convert.py", line 152, in run_convert_path
    return self.run_command('convert', *args)
  File "/Users/jojo/git/beets/test/helper.py", line 451, in run_command
    beets.ui._raw_main(_convert_args(list(args)), lib)
  File "/Users/jojo/git/beets/beets/ui/__init__.py", line 1291, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/Users/jojo/git/beets/beetsplug/convert.py", line 491, in convert_func
    m3ufile.write()
  File "/Users/jojo/git/beets/beets/util/__init__.py", line 184, in write
    with open(self.path, "w") as playlist_file:
FileNotFoundError: [Errno 2] No such file or directory: b'/var/folders/3t/l575q0f96fx91pp61zb158z00000gn/T/tmpettdj4j2/convert_dest/playlist.m3u8'

----------------------------------------------------------------------
Ran 1 test in 0.068s

FAILED (errors=1)
(beets) ➜  beets git:(convert_playlist)
(beets) ➜  beets git:(convert_playlist) ls -l /var/folders/3t/l575q0f96fx91pp61zb158z00000gn/T/tmpettdj4j2/
total 72
drwxr-xr-x  3 jojo  staff     96 Aug 10 07:24 libdir
-rw-r--r--  1 jojo  staff  36864 Aug 10 07:24 library.db
(beets) ➜  beets git:(convert_playlist)

I think I misunderstand something pretty basic here. Maybe you have an idea to further debug this. Thanks so much in advance!

[sampsyo]

Hmm… my best guess here is that the code currently tries to write the playlist file (i.e., does m3ufile.write()) before the actual conversion happens (self._parallel_convert()). That latter call has the responsibility to create the destination folder—in the test harness, that convert_dest directory. So maybe this would just work if the playlist file were created after that?

[JOJ0]

Hi, thanks so much @sampsyo, it's working now, some progress is to note and some new questions arise:

• I'm not sure how I would ensure the compatibility with playlists created on Windows for Windows. Some pointers to helper functions would be great.
• Would it be ok for this PR to go through if the M3UFile class "only" provides a good basis for further implementation of features required by other m3u playlist oriented plugins?
  • What would be required to achieve that? Currently I'm testing, if I haven't forgotten anything, all the features the M3UFile class is capable of, even if not all is required by the convert+playlist feature (e.g load())
  • Reason is: I'm a little afraid that trying to port all of the existing m3u file things in playlist, smartplaylist and play plugins is a lot of errorprone and delicate work. I'm not sure if I'm willing to do that and as said want to provide a good and rock solid basis for further advancing the M3UFile class. What do you think?

PS: Also added some checkboxes to the inital post for some things I consider essential.

[sampsyo]

Well, awesome! I'm glad this seems to be working.

It's a good question about Windows. I'm not a Windows expert either… I think the best route here would probably be to just assume that everything's fine on that platform (as long as we're correctly using syspath as we do elsewhere in beets) and then let the bug reports come in. :smiley: Having concrete evidence of how things go wrong can really help point in the direction of the right fix, in my experience.

And sure! Starting with a special-purpose M3U-writing utility, and expanding its functionality on demand, seems like a perfectly good plan to me.

[arsaboo]

@JOJ0 sorry....was out for a conference and could not respond. I am not a maintainer....just an active user of beets and trying to add features that I care about. In any case, looks like the issue was resolved. I hope to see this feature in beets soon. I would like to use parts of it to covert playlists.

[JOJ0]

It's currently untested when this feature is used together with the --keep-new option. The dest option seems to be mandatory, so supposedly there shouldn't be a problem with writing the playlist file to this path. I'm a little afraid to test this right now, anyone?

[JOJ0]

Interesting, now I'm getting exactly the error I was kind of hoping for. Something is different on Windows but atm I don't know how to proceed: Testfile was created on macOS Catalina, which I guess uses unicode. Windows can't read the path correctly from the file, even though I use syspath() for reading the entry. https://github.com/beetbox/beets/runs/8009869557?check_suite_focus=true#step:7:115

       self.assertEqual(m3ufile.media_list[0],
                         '/This/is/�/path/to_a_file.mp3\n')
E       AssertionError: '/This/is/å/path/to_a_file.mp3\n' != '/This/is/�/path/to_a_file.mp3\n'
E       - /This/is/å/path/to_a_file.mp3
E       ?          ^^

[JOJ0]

Hi @sampsyo , I think this PR finally is ready for the final review. I tried to make sure that unicode files reading and writing is working on Windows by adding some Windows specific unittests. I'm still not 100% sure if I understood syspath() entirely (I don't understand why adding \\?\ makes sense and what it's purpose is). I disabled the prefix when adding file paths to the playlist. I'm not sure if syspath() would be required at all when reading in media file paths from an m3u playlist file but I did it anyway (https://github.com/beetbox/beets/pull/4399/files#diff-f20a2ab41ac3a3c83575cac2b871653999b0e4d28c27336a1f3897a004f5d6daR49) Hopefuilly if someone experiences issues with that approach they will report it and I'll be able to fix it. Please ping me if an issue like that would come in. Thanks!

Please specifically review the Windows tests (and the corresponding fixture files) and tell me whether you think they make sense like that: https://github.com/beetbox/beets/pull/4399/files#diff-277edfc80947a6330096d584f8c3a0e418445dbc69976834a2ea474ac856c681R65-R89

https://github.com/beetbox/beets/pull/4399/files#diff-277edfc80947a6330096d584f8c3a0e418445dbc69976834a2ea474ac856c681R108-R118

[JOJ0]

Okay, okay, I did some homework and finally read the msdn article that's linked within the syspath() function (https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247.aspx) and some stackoverflow answers. I think I get it now. It enables Windows to handle paths longer than the maximum of 260 characters. Using syspath() always adds the prefix of \\?\ to paths, because it can't hurt, even when paths are shorter. Still I'm unsure if it's correct to apply it when loading an m3u playlist's entry (https://github.com/beetbox/beets/pull/4399/files#diff-f20a2ab41ac3a3c83575cac2b871653999b0e4d28c27336a1f3897a004f5d6daR49) and when adding one (https://github.com/beetbox/beets/pull/4399/files#diff-ac0584bc04d691dc3bb362557c68b83e7e71f491b7e4e0b42013e3a73cf30dd3R488).

Isn't adding this prefix the responsibility of an application that wants to read an m3u file's entry? Thus I should not use syspath() (with prefix=True) when adding to the list? (https://github.com/beetbox/beets/pull/4399/files#diff-ac0584bc04d691dc3bb362557c68b83e7e71f491b7e4e0b42013e3a73cf30dd3R488)

[JOJ0]

I don't want to leave all the clutter of the last 2 messages standing as the final messages here. All this is probably esoteric at the moment and as always there might be room for improvement. A final review and merge could be a sensible next step. Thanks a lot in advance.

[JOJ0]

Hi @sampsyo and @wisp3rwind, how can I help getting this feature merged? It might just be a matter of time and resources on your end. Please tell me if there's anything I can do to ease up the final review for you!

[JOJ0]

There where some changes in master in test_convert.py and changelog.rst, rebased them in, fixed conflicts, did some test runs (tox -e lint, python -m unittest discover -p 'test_convert*' and python -m unittest discover -p 'test_m3u*', which ran fine.

gh actions checks seem to be fine again as well.

[JOJ0]

Ready for a final review.

[arsaboo]

@JOJ0 I am trying to add a feature to import playlists (e.g., from Spotify). While I want to import the playlist in Plex eventually, I thought it may be a good idea to use m3u as an intermediate step. So, we can import any playlist (once we have the parsers ready) and create an m3u playlist. I know this PR does not search for tracks (based on album, title, etc.), but I am thinking that we can add that feature to make it more generic. What are your thoughts on that?

[JOJ0]

@arsaboo I like the idea of importing playlists from Spotify, though I can't imagine how this would work if a track initially wasn't imported into beets by using the Spotify source plugin but any other source plugin or MusicBrainz. Would you then try to match by searching for artist, album and title? That could work in some cases, but probably not safe enough.

As for the M3UFile class in this PR: It's purpose should be to become a general way of reading and writing such files from anywhere in beets. Currently it has limited features:

A list of files needs to be compiled beforehand and then passed to the class: https://github.com/beetbox/beets/pull/4399/files#diff-f20a2ab41ac3a3c83575cac2b871653999b0e4d28c27336a1f3897a004f5d6daR53-R65 Then it can be written to disk using the write method.

For loading an m3u file from disk there is the load method.

That's it currently, but could be a foundation for future refactoring of all the existing plugins that do stuff with m3u files. If you want to be the first "user" of the class, I'd be happy about it. I didn't implement any more "magic" of querying into it on purpose because when I looked into what all the other plugins do with playlists and how they do it it felt like a neverending story to integrate them all to use this class. So I tried to keep it as minimal as possible.

[arsaboo]

I don't want you to change anything with the current PR....let us get this merged first (it has been pending for a while anyways :p)

My plan was to first search using the Spotify id. If not found, match using album, title, and artist (some sort of fuzzy match). Note that Spotify is only of the sources, there are other sources that this approach could be applied to - JioSaavn, Gaana (or any other source that can provide structured lists).

We can use any pre-defined source to input the playlist, identify the matching tracks, and log the tracks that were not found. Once we have a list of matched tracks, we can create an m3u file or directly import it into something like Plex (which is where I want them to be).

[JOJ0]

Hi @Serene-Arc sorry for bothering with a rather huge PR but I think you are the right person to have a look at this. I'm wondering if my tests make sense. Could you have a look at:

• whether the tests that check the M3UFile class cover the functionality as good as possible or what could be added (or even what is redundant)= https://github.com/beetbox/beets/pull/4399/files#diff-277edfc80947a6330096d584f8c3a0e418445dbc69976834a2ea474ac856c681
• and if the tests for the actual feature could be better, I only have 2 tests implemented here and no idea if there could be done more: https://github.com/beetbox/beets/pull/4399/files#diff-bbbb8de7fcf63a7338c349f85b860817966c3400530c0ffeac2518e5cf2c1fab

Thanks for any hints on how to make sure this is tested well. I'd like to get it merge-ready finally :-)

[Serene-Arc]

@JOJ0 I'll start going through it!

[JOJ0]

Any chance you found time looking through some of the tests @Serene-Arc? :-)

[JOJ0]

Your review was instrumental @wisp3rwind, I went through all the things in this PR again and realised that I didn't follow the "rule" in beets to handle stuff internally as bytes at all.

Without talking too much, I think the commit messages and changes in these commits speak for themselves even without having much context:

The writing part:

https://github.com/beetbox/beets/pull/4399/commits/8b1e226397c873c8acb80f3725b855aec5647b5e

The reading part:

https://github.com/beetbox/beets/pull/4399/commits/c859367d3fc962cab489cecf9cfedfa26cd0a974

Both of you @Serene-Arc and @wisp3rwind, if you want to take a quick look that's appreciated.

Some background info: When I started to work on this I had the goal of writing m3u8, i.e. unicode encoded paths in playlist files. I took the route of let Python handle the files in text mode and encode unicode that way. Now I changed everything to do the "beets way" and keep everything as bytes internally, writing and reading in binary mode and making sure encoding is handled via the bytestring_path function. I think / I hope I'm on the right track now :-)

[JOJ0]

I think this is finally ready to go. There is error handling, unittests for Posix and Windows, util.syspath and util.bytestring_path wrappers are used and I use it often personally for exporting from my collection. There have been reviews by experienced beeters and I tried to incorporate there suggestions. The one thing that is lacking is testing thoroughly on Windows (besides unittests). We'll see if it holds and I'll be here for fixing.

The util.m3u.M3UFile() class that's included in this PR is ready to be used in other parts of beets. Its minimal functionality is intended but I think by adding just a few features a next step could be to use it in the smartplaylist plugin, which is the one place in beets where I stole some file handling practices from ;-)

[JOJ0]

Whoops, reminder to myself: A markdown hyperlink is not a rst hyperlink. Quickfix this in master ;-)