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

discogs: Incorrect disc numbering on two-sided media such as vinyl #2887

Closed
dbogdanov opened this issue Apr 19, 2018 · 11 comments
Closed

discogs: Incorrect disc numbering on two-sided media such as vinyl #2887

dbogdanov opened this issue Apr 19, 2018 · 11 comments
Labels
bug bugs that are confirmed and actionable

Comments

@dbogdanov
Copy link
Contributor

dbogdanov commented Apr 19, 2018

Problem

There is an issue with annotating vinyl releases from Discogs that have multiple discs (sides A/B, C/D, etc.).

For example, take this release.

The playlist metadata on Discogs:

A - Damon Wild vs. Function - Friction
B1 - Damon Wild - Skyline
B2 - Norman - Fortnight
C - C D C Crew - Flightdance  
D1 - Chris Sattinger - Decoder Ring 
D2 - Function - CNTRL

Beet import (tracks for Vinyl 1 and Vinyl 2 are split incorrectly):

Tagging:
    Various Artists - SW33
URL:
    https://www.discogs.com/Various-SW33/release/32654
(Similarity: 76.5%) (id, source) (Discogs, 2xVinyl, 1997, US, Synewave)
Vinyl 1
 * Friction (#0)     -> Friction (#1-1)
 * Skyline (#0)      -> Skyline (#1-2)
 * Fortnight (#0)    -> Fortnight (#1-3)
 * Flightdance (#0)  -> Flightdance (#1-4)
Vinyl 2
 * Decoder Ring (#0) -> Decoder Ring (#2-1)
 * CNTRL (#0)        -> CNTRL (#2-2)
[A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, plaY?

The output of beet info after importing:

beet info SW33 | grep -E "disc:|track:|tracktotal:|title:|artist:"
     albumartist: Various Artists
          artist: Damon Wild vs. Function
            disc: 1
           title: Friction
           track: 1
      tracktotal: 2
     albumartist: Various Artists
          artist: Damon Wild
            disc: 1
           title: Skyline
           track: 2
      tracktotal: 2
     albumartist: Various Artists
          artist: Norman
            disc: 1
           title: Fortnight
           track: 3
      tracktotal: 2
     albumartist: Various Artists
          artist: C D C Crew
            disc: 1
           title: Flightdance
           track: 4
      tracktotal: 2
     albumartist: Various Artists
          artist: Chris Sattinger
            disc: 2
           title: Decoder Ring
           track: 1
      tracktotal: 2
     albumartist: Various Artists
          artist: Function
            disc: 2
           title: CNTRL
           track: 2
      tracktotal: 2

The output tracktotal is incorrect and it breaks the behavior of the missing plugin.
Also the split into disc 1 and disc 2 is also incorrect, therefore the disc and trackvalues are erroneous. Those are two vinyls, and by conventions, A and B sides are vinyl 1, C and D should be vinyl 2.

@sampsyo
Copy link
Member

sampsyo commented Apr 19, 2018

Interesting—so would you summarize the problem as:

  • The disc field is holding the side index, and the actual disc index doesn't show up anywhere.
  • The tracktotal field seems to be per-side.

Is that an accurate diagnosis? Does it appear this way for other albums too?

Also, do you have per_disc_numbering enabled? If so, what happens if you turn it off?

@sampsyo sampsyo added the needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." label Apr 19, 2018
@dbogdanov
Copy link
Contributor Author

Side index would mean there should be disc1, disc2, disc3, disc4 which is not the case in my example, so no. Also in this example track C is incorrectly assigned to disc 1 instead of disc 2.

@dbogdanov
Copy link
Contributor Author

Disabling per_disc_numbering makes tracktotal correct (the total number of tracks on the release). Still, the disc numbers are messed up. So in short:

  • discogs plugin identifies disc values incorrectly
  • tracktotal is incorrect in the case of per_disc_numbering: yes

@dbogdanov
Copy link
Contributor Author

Related to per_disc_numbering, the issue is here.

@sampsyo
Copy link
Member

sampsyo commented Apr 20, 2018

Side index would mean there should be disc1, disc2, disc3, disc4 which is not the case in my example, so no. Also in this example track C is incorrectly assigned to disc 1 instead of disc 2.

Right—I meant the disc-relative side index. So side A on any disc is “disc 1,” side B anywhere is “disc 2,” etc. Is that right?

Disabling per_disc_numbering makes tracktotal correct (the total number of tracks on the release). Still, the disc numbers are messed up.

Indeed; I think the discogs problem is the root cause—if that were fixed, I think per_disc_numbering would behave correctly.

So if anyone has the time, I think the thing to do would be to look carefully at the disc assignment code in discogs and run a few tests with examples you provide.

@sampsyo sampsyo changed the title Wrong behavior of the Missing and Discogs plugins when used with vinyl releases discogs: Incorrect disc numbering on two-sided media such as vinyl Apr 20, 2018
@sampsyo sampsyo added bug bugs that are confirmed and actionable and removed needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." labels Apr 20, 2018
@dbogdanov
Copy link
Contributor Author

There are two independent issues:

  1. incorrect tracktotal. The error is here:
item.tracktotal = track_info.medium_total or len(album_info.tracks)

This will result in the medium_total value instead of len(album_info.tracks). The order of operands should be changed.


  1. incorrect medium and medium index numbering, but only in some cases:
A - Damon Wild vs. Function - Friction
B1 - Damon Wild - Skyline
B2 - Norman - Fortnight
C - C D C Crew - Flightdance  
D1 - Chris Sattinger - Decoder Ring 
D2 - Function - CNTRL

Medium C does not have medium_index and is erroneously considered as the medium_is_index == True case. This is because medium_count is not increased after each side, but two sides (the medium is considered two-sided).

    medium_is_index = track.medium and not track.medium_index and (
                len(track.medium) != 1 or
                ord(track.medium) - 64 != medium_count + 1
    )

@sampsyo
Copy link
Member

sampsyo commented Apr 23, 2018

This will result in the medium_total value instead of len(album_info.tracks). The order of operands should be changed.

Hmm… this is the part I'm not sure I agree with. The idea behind the the medium_total is that it contains exactly what tracktotal wants: the number of tracks on the medium (e.g., disc or side) that the track appears on. See the MusicBrainz backend, for example, in beets.autotag.mb: that value comes from the length of the track listing on each medium.

On the other hand, I think I see what you mean about the problem in discogs. Could you open a pull request, preferably with a few tests, to demonstrate the proposed change?

@dbogdanov
Copy link
Contributor Author

Hmm… this is the part I'm not sure I agree with. The idea behind the the medium_total is that it contains exactly what tracktotal wants: the number of tracks on the medium (e.g., disc or side) that the track appears on.

I see. So then it's the missing plugin's fault to identify those vinyl releases as having missing files.

@sampsyo
Copy link
Member

sampsyo commented Apr 24, 2018

Yeah, good point. Is it the case that missing is confused on any release tagged with per_disc_numbering, perhaps?

@dbogdanov
Copy link
Contributor Author

dbogdanov commented Apr 26, 2018

It appears that in the case of discogs medium_total contains the number of mediums, not the number of tracks on each medium. The same happens in tests for discogs, so it seems to be done intentionally.

I'll create a pull request after you confirm that this definition of medium_total isn't the correct one.

@sampsyo
Copy link
Member

sampsyo commented Apr 26, 2018

Aha! Yes indeed; that seems like a mistake to me. Thank you!

dbogdanov added a commit to dbogdanov/beets that referenced this issue Apr 27, 2018
Fix incorrect split of a tracklist by medium for the case of
two-sided mediums (beetbox#2887).

Following the discussion in beetbox#2887, the 'medium_total' value should
contain the number of tracks on the medium to which each particular
track belongs, not the total number of different mediums present on
a release.

Fix unit tests accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs that are confirmed and actionable
Projects
None yet
Development

No branches or pull requests

2 participants