-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fixes #1210: Skip non-audio tracks from MusicBrainz #2776
Conversation
This ignores non-audio tracks during import: - Data tracks, based on their title `[data track]` (which seems to be the MusicBrainz convention, as there's no specific flag to indicate that a track is a data one), - Video tracks, based on the `video=true` attribute. It's similar to the Picard changes mentioned in beetbox#1210, except it doesn't deal with `[silence]` tracks: These ones will probably require a setting to let the user control if they should be imported or not.
Users may want to keep tracking video tracks, for example if they rip the audio part of the video tracks. Added a setting to allow this.
Hm, I added a setting to continue including video tracks following @pprkut comment on #2688 , but I realize that we may also need a setting to continue including non-audio media too. Is that correct @pprkut we would need both? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome; thank you! I have only one comment from a code review.
The configuration option seems like a good idea. I'm OK with merging this as-is, and we can then sort out how the configuration options should work. Specifically, I would be interested to hear from @pprkut whether one configuration option that controls both video tracks and video media would be the right thing. If so, we might consider simplifying the name to ignore_video
.
beets/autotag/mb.py
Outdated
|
||
if ('video' in track['recording'] and | ||
track['recording']['video'] == 'true' and | ||
config['match']['ignore_video_tracks'].get(bool) is True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for is True
—calling .get(bool)
is guaranteed to return a bool.
In fact, it's actually possible to drop that part too—Confit automatically gets the truthiness of a view when it's used as a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, I changed it
Thanks! Regarding the settings I thought it would be good to split them regardless, because then we could have a list of formats to ignore, rather than hard-coding them in the code (Data CD, DVD, DVD-Video, etc). That way users could choose exactly what format they want to ignore or not. But let's see what @pprkut thinks. |
Great! I'll merge this initial version for now, and we can keep the discussion going about how to set up the config options. |
Hey! Sorry for the late reply! I would indeed also like to be able to import non-audio media, but wouldn't that work with the same setting? Ideally, video media would have |
I think how it will work with that change + a couple of other that were merged:
|
I guess I read @pprkut’s question as asking whether checking for |
Ah I see. Well, I have no idea 😉 We'd have to find a MusicBrainz example of a video track that doesn't have |
One could argue that a video track not marked as |
Got it. If that's the case then, @nguillaumin, perhaps we can remove the medium-level version of the setting? Just filtering on |
Hm, I'm keen to keep it:
|
Up to you of course, I won't object if you want to remove it 😉 |
The |
I don't think that's a safe assumption to make. Those are data disks and could potentially hold audio data. I, for example, do have a release that has a bunch of MP3 files on a CD-ROM (there's no audio tracks next to the data track) |
Fair enough, if that's the case. Do you have any doc explaining this? I was trying to lookup docs about the video attribute and how non-audio media is supposed to be entered in MusicBrainz but didn't find anything... |
@Freso might be able to answer that better than me, but I'll try :) Anything else you'd be interested in in particular? |
I guess I was after something that would confirm that
I'm not sure I understand this? Either it's a DVD-Audio, but if it's a DVD-Video then it's a video track (even if it does only play audio when you play it on your TV, it needs to be played on a DVD-Video player). Perhaps it's more a matter of interpretation, i.e. is it |
That's a quote from here. Nothing is marked as video by default. There's an open feature request for musicbrainz to mark tracks automatically as video if they are on a video medium, but for now everything has to be done explicitely.
Yes, there will be 2 recordings. One for the audio-only version, and one for the video.
It's video if the audio is linked to moving pictures. A static background picture (like mentioned for youtube tracks above) doesn't make it a video. I also have a DVD though where when ripping the track that plays the audio you'd really get audio-only, not even a single-frame background picture attached to it. |
Thanks, that clarifies things a bit. I still have a gut feeling there's value in having a setting to choose which media to include / exclude when importing, but I can't give a clear reason as to why, so if you want to take it away that's fine. |
Hi! Just checking back in on this: as it currently stands, we have two settings, right? It seems OK to keep them both, but maybe it would be useful to reexamine the defaults. For example, do most users really need to skip both |
Hehe, I think that's a tough question to answer. I would tend to think that most users would skip non audio tracks and media, because that's what I do. Others will think differently... If there's a channel through which you can survey users, that would be ideal, but of not then I suspect all we can do is guess and take a chance. |
Well, I suppose my question boils down to: are there examples of situations where the two settings differ? That is, are there tracks that will be caught by one setting but not the other? I get the sense that the answer is "ideally, no," but looking for real examples one way or the other might help. |
Hm... I'm not exactly sure what you're after 😉 but basically, as per the previous discussion, MusicBrainz makes the distinction of "Video media format" and "Video recordings", like we would do with these two settings. So you can have a media (e.g. Digital Media) with a single video recording (that may or may not get ignored depending on ..Or you can have a Video media (DVD) where the tracks were not flagged individually as being video: https://musicbrainz.org/ws/2/release/c9ca3d33-881c-41ad-8ad0-a6edd1aec185?inc=recordings (See the 2nd media). That may well be a MusicBrainz issue, but it exists. Does that help? |
I think I see! Here's another way of putting my question (with possible answers): what are some actual the differences in experience of users with these three settings?
It seems like the answer is that there may be some tracks that don't get filtered out in 3, but that's probably a MusicBrainz data problem that should be fixed—or a legitimate case of audio tracks appearing on typically-video media. But I don't think we've seen an example of anything meaningful getting missed in case 2. I think I lean toward making option 3 the default. We may miss some video tracks that way, but that seems like it's OK. It's a good semantic match for what most users probably actually want. And leaving the option to ignore entire media, for people who want it, will help address the remaining tracks. |
Hm, I'm not sure. For me the default that makes sense is 1. (which is why I implemented it 😉). I don't rip the audio track of video media (either a full media like DVD or a single video track), I only rip audio CDs. Beets currently report video tracks (media or single track) as "missing" when I import my media. With the new default, these tracks would not be reported missing. I would be inclined to think that's what most users want: it has been requested in #1210 and #2688, which is where all of this is coming from. See this comment on #2688 :
Other users are ripping video tracks, in that case the tracks weren't marked as missing by the current version of Beets. With the new default set to 1. these tracks will be marked "unmatched". If set to 3. , some (but not all) will be marked "unmatched". You would be inclined to think that's what most users want.
Going back to my initial point, unless you have survey data, I think that's hard to claim this. #1210 and #2688 seem to show that some users want to ignore video tracks and media. Is that just some, or most? To sum up, I don't think there's a good default unless we have more insight. Neither of us is "wrong", it's just that we use the software differently. We have to make a choice though, so I think I would go either with ignore all, or ignore none. It's likely that users either import audio only (like me), or import everything (video media and individual tracks like @pprkut ). Using 2. or 3. as a default may actually cover the smallest portion of users, the ones who import only individual video tracks but not full media, or the ones importing full media but no video tracks. Though again, it's a guess 😅 |
Certainly we can’t know what people want without a survey, but I can anticipate what will be the least confusing. :) That user above, for example:
is talking about tracks, not media, which suggests they expect option number 3. That’s what I mean by “semantic match”: the tracks thing is the easiest to explain, even if it might make some “mistakes” in some circumstances. So I think I’ll make that the default. Fortunately, your options now make it easy to get exactly the behavior that anyone might want! |
Also! If you're interested, probably the best way to get feedback would be a quick post on our forum: https://discourse.beets.io |
I agree, option 3 sounds like a sensible default approach, if only because it doesn't hide musicbrainz data problems/inconsistencies/etc and might trigger people to improve the data there instead of hacking around it in beets. Just for completeness though, a case that might be missed in scenario 2 is videos in data tracks. Beets doesn't handle data tracks yet, but once it does, those would qualify as video recordings not on video media. They would be covered by scenario 3 though, if those tracks are properly marked as video in musicbrainz. A different question though. What would be the behavior if you ignore video tracks (and or video media) and you want to import one that is only that, for example a live DVD or some such. I would imagine that beets would try to match a release with 0 tracks on it? Or would it abort that process somehow? This could hit people new to beets who don't know that video tracks are ignored by default, or maybe don't even know that the tracks they want to import were originally video tracks. Could be nice if they'd get some feedback then in the form of "Hey, you're trying to import a video release, but beets is currently configured to ignore those. Read more about it here" or some such. |
That's a great point. It would, the way we've currently implemented, just say the release has zero tracks, so all the ones you're trying to import are "extra." One easy (but incomplete) way to address this would be to add some debug logging, at least, that enumerates all the tracks that are skipped because they're video tracks. Then people at least have a hope of figuring out by looking at the verbose log. |
I don't think that's the case. If you read the full issue it's originally about a CD+DVD, so it's a media issue:
I'm reading that as "all non audio files should be skipped", probably because they don't know the subtleties of MB regarding media and recordings. Perhaps we can them @zuzzurro . As you say the options are flexible enough to do whatever users may want, so I'm glad we kept both in the end 😉 3. is fine, as long as I can reconfigure it. 😄 |
We determined on the PR thread that ignoring video tracks is enough, and ignoring typically-video media has more pitfalls.
This ignores non-audio tracks during import:
[data track]
(which seems to bethe MusicBrainz convention, as there's no specific flag to indicate
that a track is a data one),
video=true
attribute.It's similar to the Picard changes mentioned in #1210, except it doesn't
deal with
[silence]
tracks: These ones will probably require a settingto let the user control if they should be imported or not.