Skip to content

Fix Hybrid SACD importing - #5149

Merged
snejus merged 1 commit into
beetbox:masterfrom
InvisibleFunction:fix-sacd-import
Nov 23, 2024
Merged

Fix Hybrid SACD importing#5149
snejus merged 1 commit into
beetbox:masterfrom
InvisibleFunction:fix-sacd-import

Conversation

@InvisibleFunction

@InvisibleFunction InvisibleFunction commented Mar 17, 2024

Copy link
Copy Markdown
Contributor

Description

Fixes #5148.

When importing, the code that matches tracks does not consider the medium number. This causes problems on Hybrid SACDs (and other releases) where the artists, track numbers, titles, and lengths are the same on both layers.

I added a distance penalty for mismatching medium numbers.

Before:

$ beet imp .

/Volumes/Music/ti/Red Garland/1958 - All Mornin' Long - 1 (6 items)

  Match (95.4%):
  The Red Garland Quintet - All Mornin' Long
  ≠ media, year
  MusicBrainz, 2xHybrid SACD (CD layer), 2013, US, Analogue Productions, CPRJ 7130 SA, mono
  https://musicbrainz.org/release/6a584522-58ea-470b-81fb-e60e5cd7b21e
  * Artist: The Red Garland Quintet
  * Album: All Mornin' Long
  * Hybrid SACD (CD layer) 1
     ≠ (#2-1) All Mornin' Long (20:21) -> (#1-1) All Mornin' Long (20:21)
     ≠ (#2-2) They Can't Take That Away From Me (10:24) -> (#1-2) They Can't Take That Away From Me (10:27)
     ≠ (#2-3) Our Delight (6:23) -> (#1-3) Our Delight (6:23)
  * Hybrid SACD (CD layer) 2
     ≠ (#1-1) All mornin' long (20:21) -> (#2-1) All Mornin' Long (20:21)
     ≠ (#1-2) They can't take that away from me (10:27) -> (#2-2) They Can't Take That Away From Me (10:25)
     ≠ (#1-3) Our delight (6:23) -> (#2-3) Our Delight (6:23)
➜ [A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates?

Note that all tracks tagged with disc 1 get moved to disc 2 and vice versa.

After:

$ beet-test imp .

/Volumes/Music/ti/Red Garland/1958 - All Mornin' Long - 1 (6 items)

  Match (95.4%):
  The Red Garland Quintet - All Mornin' Long
  ≠ media, year
  MusicBrainz, 2xMedia, 2013, US, Analogue Productions, CPRJ 7130 SA, mono
  https://musicbrainz.org/release/6a584522-58ea-470b-81fb-e60e5cd7b21e
  * Artist: The Red Garland Quintet
  * Album: All Mornin' Long
  * Hybrid SACD (CD layer) 1
     ≠ (#1-1) All mornin' long (20:21) -> (#1-1) All Mornin' Long (20:21)
     ≠ (#1-2) They can't take that away from me (10:27) -> (#1-2) They Can't Take That Away From Me (10:27)
     ≠ (#1-3) Our delight (6:23) -> (#1-3) Our Delight (6:23)
  * Hybrid SACD (SACD layer) 2
     * (#2-1) All Mornin' Long (20:21)
     * (#2-2) They Can't Take That Away From Me (10:24)
     * (#2-3) Our Delight (6:23)
➜ [A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates?

Yay!

To Do

  • [] Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.) (I don't really know how to do this effectively and would appreciate pointers.)

@InvisibleFunction

Copy link
Copy Markdown
Contributor Author

Can I get any feedback on this PR?

@InvisibleFunction

Copy link
Copy Markdown
Contributor Author

uhoh, I tried to get fancy with git :( let me fix that

@InvisibleFunction

Copy link
Copy Markdown
Contributor Author

Okay, I think I successfully got git fancy. Would love some feedback on this PR!

@InvisibleFunction InvisibleFunction changed the title Fix sacd import Fix Hybrid SACD importing May 18, 2024
@InvisibleFunction

Copy link
Copy Markdown
Contributor Author

Hey, before this slips off the front page of PRs, what do we have to do to implement this fix?

@emiham

emiham commented Aug 21, 2024

Copy link
Copy Markdown
Contributor

Looks good to me. I'd love to see this merged, as I have this exact problem. It also seems reasonable in general to consider the medium number even for other media.

@InvisibleFunction

Copy link
Copy Markdown
Contributor Author

It has come in handy for me when I want to import just one disk of a release that has both cds and bluerays or something.

@emiham

emiham commented Aug 29, 2024

Copy link
Copy Markdown
Contributor

Here is an example of a non-SACD release which can't be properly imported right now: https://musicbrainz.org/release/ea92a194-2d60-35c7-9d56-0e1dba20cd45

Comment thread beets/autotag/match.py Outdated
Comment thread beets/config_default.yaml Outdated
Comment thread docs/changelog.rst Outdated
`requests` timeout.
* Fix cover art resizing logic to support multiple steps of resizing
:bug:`5151`
* Fix bug where matcher doesn't consider medium number when importing making

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing - you probably want to rebase off the most recent master version and move your changelog note to the top, under the Unreleased section!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, rebased and squashed. Changelog note moved to the top unreleased section.

@snejus snejus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! This looks good, thanks!

@snejus
snejus merged commit 0e6ba45 into beetbox:master Nov 23, 2024
@InvisibleFunction
InvisibleFunction deleted the fix-sacd-import branch November 23, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Beets wants to change disc order on hybrid SACDs

3 participants