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

Cue sheet support #4200

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

Cue sheet support #4200

wants to merge 20 commits into from

Conversation

zhangdoa
Copy link

@zhangdoa zhangdoa commented Oct 15, 2023

Database Migration

YES
Migration 078 added an "IsSingleFileRelease" column to "TrackFiles" table.

Description

Added the support to read from .cue files as optional metadata which is typically supplied with a ripped disc or any arbitrary sources. A new concept "single-file release" is introduced to cover the "one file contains all tracks" case. For the single-file release case, a new .cue file would be created in the same media folder. The multi-disc release support is also implemented (relying on the disc ID provided from the .cue file).

Screenshot (if UI related)

Single-file release:
Screenshot 2023-10-15 135257
Screenshot 2023-10-15 135345

Multi-track release with a .cue file supplied:
Screenshot 2023-10-15 140422

Todos

  • Tests: Should add some unit tests.
  • Translation Keys (./src/NzbDrone.Core/Localization/Core/en.json): New frontend properties are currently hardcoded.
  • Wiki Updates
  • Need to hide some added frontend properties if they are not valid or empty.
  • Different diacritics from .cue file would cause the track mapping to ignore certain tracks (and then the user needs an extra manual selection step to fix it).
  • Disc ID-based cue sheet file grouping won't work properly if the disc ID field is not set in the .cue files.

Issues Fixed or Closed by this PR

@github-actions github-actions bot added Area: UI Issue is related to UI, should also add the issue to the new UI project, if it isn't a bug. Area: Organizer Issue is related to the Organizer Area: API Issue is related to the API Area: Parser Issue is related to parsing infrastructure. Area: Db-migration Warning: DB Migration | Related to DB Migration labels Oct 15, 2023
@Qstick
Copy link
Contributor

Qstick commented Oct 15, 2023

Very nice, we'll have a peek when we get a chance

@hughesjs
Copy link

hughesjs commented Dec 7, 2023

Does this actually split the FLAC into multiple files post-download? I think it would be nice if it did since a lot of music software doesn't pay attention to CUEs. If it doesn't, I think this is a step in the right direction, but only a partial solution to #515

@zhangdoa
Copy link
Author

zhangdoa commented Dec 8, 2023

Does this actually split the FLAC into multiple files post-download? I think it would be nice if it did since a lot of music software doesn't pay attention to CUEs. If it doesn't, I think this is a step in the right direction, but only a partial solution to #515

No, the feature is built around the idea that Lidarr is more of a resource management tool than an audio processing tool, so whatever the original file structure it is, the imported files should keep the layout. This is from my personal case of course, since I don't have issues on the playback side for a single .flac file and .cue file, but only the import to Lidarr database step.

But still I want to state that I do not agree that Lidarr should support any audio file-level operations since there are existing tools for that (another point is I don't see other Servarr projects have such “repacking” features added). And if the playback side limitation needs to be addressed on the source file side, then it could happen before importing the files to the Lidarr database.

The implementation for splitting a single audio media file into multiple individual track files is straightforward though, it could happen at the very last importing file copy/move step since the cue sheet information is accessible there, currently only a new .cue file would be generated. I'd expect a “Split Single-release File” checkbox could be added to the frontend as well. But as I said above, since I don't agree on the overall idea, I am not going to implement this.

@hughesjs
Copy link

@zhangdoa While I do somewhat agree in principal, I'd personally put file-splitting in the same category as renaming and reorganising files rather than, as you put it, a full repack. It feels more organisational to me rather than transformative. I don't see why this couldn't be a config option perhaps?

That being said, your change is still an improvement, but given #515 says "Or better yet split albums into multiple tracks via cue file.", this only partially addresses it and I don't think #515 should be closed if this is merged.

@zhangdoa
Copy link
Author

@hughesjs It could be a configuration option, but someone else would need to work on that. Do you know if a PR should address all the expected behaviours from an issue on Lidarr? If there's an improvement label or tag, I could change the PR description. Also, I don't mind if either the issue could be closed or not by my PR since it was not intended to address #515 initially, but I needed to figure out a solution for my own usage case and I thought it could be upstreamed since related issues had been opened.

@hughesjs
Copy link

hughesjs commented Dec 14, 2023

I have no objection to this getting merged mate, it's good work, you've just got a closing keyword that will result in #515 being closed if you merge this. If you change "fixes" to "partially addresses" in your description, I think that would sort it.
image

@github-actions github-actions bot added Area: Notifications Issue is related to Notifications Area: Import Lists Issue is related to Importing from Lists Area: Download Clients Issue is related to download clients Area: Indexer Issue is related to indexers. labels Jan 6, 2024
@zhangdoa zhangdoa closed this Jan 6, 2024
@zhangdoa
Copy link
Author

zhangdoa commented Jan 6, 2024

Rebased all changes to the latest and bumped the DB version to 076.

@zhangdoa zhangdoa reopened this Jan 6, 2024
@github-actions github-actions bot removed Area: Notifications Issue is related to Notifications Area: Import Lists Issue is related to Importing from Lists Area: Download Clients Issue is related to download clients Area: Indexer Issue is related to indexers. labels Jan 6, 2024
Copy link
Contributor

@Qstick Qstick left a comment

Choose a reason for hiding this comment

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

Just a quick first look. Some comments

Add a cue sheet class.
Enable track selection for single file releases.

(cherry picked from commit 430807a)
Rename "cuesheet" to "cue sheet".

(cherry picked from commit c968668)
…and other useful information.

Add support to import releases with multiple cue sheets.
Add the cue sheet support to the disc scan service.
Use the track info from cue sheet files to map local tracks.
Use the disc ID to group cue sheet files and deduce the disc count.

(cherry picked from commit fac76b7)
…copy file operation.

Use the correct ID overrides for processing grouped cue sheet files.

(cherry picked from commit 6f06938)
…e the IMakeImportDecision API change.

Fix a crash when trying to import an album while the artist is not added yet.

(cherry picked from commit d439677)
Fix an incorrect early-out in the cue sheet line parsing function.
Fix a crash caused by the invalid artist and album on the local tracks.
Add support to remove duplicated cue sheet files.

(cherry picked from commit 336c62e)
Use ID overrides when manually import items from a cue sheet.

(cherry picked from commit 21a2314)
Implement the diacritics and punctuation marks sanitation feature for the cue sheet track mapping.

# Conflicts:
#	src/NzbDrone.Core/Lidarr.Core.csproj

(cherry picked from commit 27622ce)
(cherry picked from commit 32382ac)
(cherry picked from commit f3a5c7c)
(cherry picked from commit a3ccf87)
(cherry picked from commit e1ab639)
…s and the database table.

(cherry picked from commit b18cd45)
…to only import the files in the same directory.

(cherry picked from commit 07c9213)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: API Issue is related to the API Area: Db-migration Warning: DB Migration | Related to DB Migration Area: Organizer Issue is related to the Organizer Area: Parser Issue is related to parsing infrastructure. Area: UI Issue is related to UI, should also add the issue to the new UI project, if it isn't a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants