Skip to content
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

Take the audio tracks in a data track into account. #1638

Merged
merged 8 commits into from Apr 29, 2018
Merged

Conversation

@jdetrey
Copy link
Contributor

jdetrey commented Oct 7, 2015

Hi,

On some releases, the data track of the medium (CD, usually) may contain audio/video files which MusicBrainz takes into account as actual tracks in the release. See for instance this release.

Commit 6ec04e6 of jdetrey/python-musicbrainzngs adds support for the corresponding data-track-list XML node (as per the MusicBrainz schema). This is pull request #180 of alastair/python-musicbrainzngs.

This patch merges the two track lists when importing a release from MusicBrainz.

Comments welcome :)

Cheers,
Jérémie.

Depends on commit 6ec04e6 of `python-musicbrainzngs'.
@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Oct 7, 2015

Looks great! Would you mind adding a note to the changelog? Briefly mentioning that this isn't in the library yet but may be soon would do nicely.

@jdetrey

This comment has been minimized.

Copy link
Contributor Author

jdetrey commented Oct 7, 2015

Hi,

Thanks for the prompt feedback! I've just pushed a tentative changelog update.

Cheers,
Jérémie.

@@ -217,6 +217,10 @@ def album_info(release):
format = medium.get('format')

all_tracks = medium['track-list']
if 'data-track-list' in medium:
all_tracks += medium['data-track-list']
total = len(all_tracks)

This comment has been minimized.

Copy link
@Freso

Freso Oct 7, 2015

Member

IMHO, total is not a very good name (total… what?). I would probably just remove that line and…

This comment has been minimized.

Copy link
@jdetrey

jdetrey Oct 9, 2015

Author Contributor

I had elected to name this variable total because the track index in the same function is named index. Likewise, the corresponding parameter of track_info is medium_total. Sounded kind of consistent to me. But I don't mind changing its name to anything else which you'd think is more explicit :)

Cheers,
Jérémie.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Apr 29, 2018

Member

Perhaps something like track_count would be more descriptive?

@@ -228,7 +232,7 @@ def album_info(release):
index,
int(medium['position']),
int(track['position']),
len(medium['track-list']),
total,

This comment has been minimized.

Copy link
@Freso

Freso Oct 7, 2015

Member

… use len(all_tracks) here instead. all_tracks is a good variable name (it contains all the tracks), and len() is a known function (or should be, anyway).

This comment has been minimized.

Copy link
@sampsyo

sampsyo Oct 7, 2015

Member

Alas, that would inadvertently count the pregap track.

@Freso

This comment has been minimized.

Copy link
Member

Freso commented Oct 7, 2015

Just got a minor nit-pick, otherwise it looks good. ;)

listed in MusicBrainz: the corresponding audio tracks are now merged into the
main track list. (This feature depends on a new version of the
``musicbrainzngs`` library which is not yet released, but will start working
when it is available.) Thanks to :user:`jdetrey`. :bug:`1638`

This comment has been minimized.

Copy link
@sampsyo

sampsyo Oct 7, 2015

Member

Sounds great!

@jdetrey

This comment has been minimized.

Copy link
Contributor Author

jdetrey commented Oct 8, 2015

Mmh, just realised that some albums might store music videos in the data track, and that these would then be also listed in the data-track-list XML node. See, e.g., this release.

What's beets' actual policy regarding video tracks? I guess some people might not want to have the video files included when importing an album into their music library. I suppose that a possibility would be to add a configuration flag to the importer, specifying whether or not the user wants video files imported as well. What do you think?

Cheers,
Jérémie.

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Oct 8, 2015

Ah, thanks for investigating.

The current de facto policy is that beets does not do video files. (For most video formats, we're not anywhere close to being able to read and write metadata, so we can't import them.)

Can we, for now, make this exclude video tracks? Then, when we add support for actually interacting with video files, we can enable them.

@pprkut

This comment has been minimized.

Copy link
Collaborator

pprkut commented Oct 8, 2015

What about when you have audio only versions of those video tracks? I tend to always rip out the audio part of the videos and track them together with beets (e.g. on CD+DVD combo releases). Now, usually the videos available in the data track are way below DVD quality, but audio is usually at least up to par with an MP3 download. I could see people wanting to track those as well, especially when a release contains videos for tracks otherwise not already available as audio.

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Oct 8, 2015

Sure, sounds reasonable! Let's make it a config option then, under the MusicBrainz section.

@jdetrey

This comment has been minimized.

Copy link
Contributor Author

jdetrey commented Oct 9, 2015

Sounds good to me as well, thanks!

But should it be a MusicBrainz-specific option, or a global importer option? Even though only MusicBrainz might currently support indexing of video tracks, it might very well be the case that Discogs or other databases will eventually do so as well.

What do you think?

Cheers,
Jérémie.

@jdetrey jdetrey closed this Apr 29, 2018
@jdetrey jdetrey force-pushed the jdetrey:master branch from fda740a to 4288e11 Apr 29, 2018
@jdetrey

This comment has been minimized.

Copy link
Contributor Author

jdetrey commented Apr 29, 2018

Whoops, sorry, I messed up and closed the PR by mistake. The commit it referenced was jdetrey@edd6e7f.

By the way, since #1210 is now fixed, maybe this commit can now be merged?

@jdetrey jdetrey reopened this Apr 29, 2018
@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Apr 29, 2018

Hi! Perhaps you can refresh my memory about the changes, in the context of our work to ignore non-audio tracks by default. For example, do you expect that most users will not notice this change in the default configuration (because we’re now including data tracks, but those data tracks will be ignored because they’re usually marked as non-audio)? If so, can you perhaps run an experiment or two and report on the results? (For example, pasting some importer output from before and after the change would help illustrate why we want this.)

Also, it looks like we’ll want an updated changelog entry. 👍

@jdetrey

This comment has been minimized.

Copy link
Contributor Author

jdetrey commented Apr 29, 2018

Hi!

The idea is to allow for releases with audio data tracks, such as 9ce41d09-40e4-4d33-af0c-7fed1e558dba.

Without this patch, it is impossible to import such releases: one gets the error No matching release found for 198 tracks., as beet overlooks audio data tracks, and cannot therefore compute the actual number of tracks. With the patch, this import runs fine.

Also, importing releases with video data tracks, such as 2db71b51-1389-3ecf-bdfc-934173ce28b2, seems to be working correctly before and after the patch (with ignore_video_tracks set to yes), as only non-video tracks are imported (be they regular tracks or data tracks).

The following output was obtained while importing a mock-up of 2db71b51-1389-3ecf-bdfc-934173ce28b2 (that is, 11 MP3 files in a single directory) before the change:

/tmp/beets-test/flykkiller (11 items)
Sending event: before_choose_candidate
No matching release found for 11 tracks.
For help, see: http://beets.readthedocs.org/en/latest/faq.html#nomatch
[S]kip, Use as-is, as Tracks, Group albums, Enter search, enter Id, aBort? i
Enter release ID: 2db71b51-1389-3ecf-bdfc-934173ce28b2
Tagging  - 
Searching for album ID: 2db71b51-1389-3ecf-bdfc-934173ce28b2
Requesting MusicBrainz release 2db71b51-1389-3ecf-bdfc-934173ce28b2
primary MB release type: album
Sending event: albuminfo_received
Candidate: FlyKKiller - Experiments in Violent Light (2db71b51-1389-3ecf-bdfc-934173ce28b2)
Computing track assignment...
...done.
Success. Distance: 1.00
Evaluating 1 candidates.
Sending event: before_choose_candidate
Correcting tags from:
    (unknown album)
To:
    FlyKKiller - Experiments in Violent Light
URL:
    https://musicbrainz.org/release/2db71b51-1389-3ecf-bdfc-934173ce28b2
(Similarity: 0.0%) (tracks, album, artist) (Enhanced CD, 2008, PL, EMI Music Poland)
 * 01.mp3 (#0) (1:13) -> FlyKKiller (#1) (3:39) (title, length)
 * 02.mp3 (#0) (1:13) -> Peroxide (#2) (3:16) (title, length)
 * 03.mp3 (#0) (1:13) -> Fear (#3) (4:18) (title, length)
 * 04.mp3 (#0) (1:13) -> Sell My Pulse (#4) (4:48) (title, length)
 * 05.mp3 (#0) (1:13) -> Shine Out (#5) (2:58) (title, length)
 * 06.mp3 (#0) (1:13) -> Get All Pulled Out (#6) (2:59) (title, length)
 * 07.mp3 (#0) (1:13) -> Controlled Environment (#7) (4:03) (title, length)
 * 08.mp3 (#0) (1:13) -> B. Murphy (#8) (3:54) (title, length)
 * 09.mp3 (#0) (1:13) -> Music for 12 Sounds (#9) (4:28) (title, length)
 * 10.mp3 (#0) (1:13) -> Czy nie szkoda ci (#10) (4:59) (title, length)
 * 11.mp3 (#0) (1:13) -> FlyKKiller (David Holmes remix) (#11) (6:05) (title, length)
Apply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort? a

After the change, the output is rigourously the same.

Thus, from what I could test, this should not impact users using a default configuration (that is, ignore_video_tracks: yes).

Cheers!

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Apr 29, 2018

Perfect!! Thank you for the explanation—that made it very clear. Nice work, and thanks for your patience!

@sampsyo sampsyo merged commit 9656408 into beetbox:master Apr 29, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jdetrey

This comment has been minimized.

Copy link
Contributor Author

jdetrey commented Apr 29, 2018

Great! Thank you very much for the feedback and the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.