#1430 Add support for separate album and track artwork - #1627
#1430 Add support for separate album and track artwork#1627michaelherger wants to merge 3 commits into
Conversation
After long discussions, but still without a complete understanding of the challenge, I came up with the following opinionated approach: * no difference between box sets, digital playlists with per-track artwork, or sub titles: if an album has tracks with different artwork, consider it a "box set" (lack of better terminology) * add option to search for common artwork for box sets * search for box set level artwork is an additional scan step, rather than integrated into one of the others: we'd only have the final `coverid` for all tracks once the pre-cache stage has finished (which actually doesn't pre-cache if that option is turned off...) * try to find a common parent folder (only walk up three levels - I've had test cases where they only met at the file system's root) * if parent folder fails, pick `boxset.jpg` or `cover.jpg` etc. from track level. * I tried to re-use a maximum of code, but separate out this change as much as possible. It's pretty much self-contained, but leveraging all of the existing artwork lookup code. In my test setup with about 22k tracks (of which 20k on a rather slow NAS) only 97 candidates were found. Processing them took less than 4s. Potential issues I've identified so far: * false positives: I have albums with embedded artwork, some of which would differ from the others for whatever reason * three levels up is a deliberate choice - I'm sure someone will need more than that. Let's see. * the code would create track objects to keep the original `cover` value for folder level album artwork. This helps us keeping album level original artwork references. All is handled magically. But I don't know whether using the (already existing) `dir` content type could have unexpected side-effects. * I only tested digital playlists (per-track artwork, plus album) in one single folder, box sets with per disc sub-folders and the album artwork in the parent; but I have no classical music with works/sub titles and whatever. Please test. * my test collection is about 99% MP3... no cue sheets and other excentricities 😆 Feedback will be very welcome. Signed-off-by: Michael Herger <michael@herger.net>
|
OK, given this PR, I'll concentrate on testing it and reporting results, rather than packaging up my test data for others. On the 3 levels thing:
|
|
Issue D1 It's not picking up arbitrary image names from the album parent. When I rename to cover.jpg it is picked up. |
D1?
Working fine here. Please note that only the first common folder is being used. It's not walking up and checking every level. |
I think there's an issue where performance would suffer a lot: the search for "anything in that folder" does not only look at that folder, but at anything with partially matches! I'll have to review that. I think that's what you did that tricky query for... |
|
My structure is: |
|
BTW, "D1" is "Darrell Issue 1" so we can keep track! |
|
Oops... sorry: I edited your comment instead of quoting it 😞. What I wanted to ask: |
Signed-off-by: Michael Herger <michael@herger.net>
Yes, all one album. |
|
Re D1, I don't know if I was doing something wrong or the recent commit fixed something, but it's OK now. |
|
Issue D2
I'm guessing that this is because of the "tracks must have differing artwork" rule, so would equally apply even if the Full scan found the image in step 1. I know I keep going on about it, but I don't think there is any alternative to the grouping change as in #1536 in the Artwork.pm SQLs, so that we can process groups of tracks split by folder and/or discnumber. At the moment, grouping only by album and artwork in this situation can only process all tracks together. In my version, the cache is also made specific to a particular discnumber, and we check for disc-specific artwork by name, to allow a multi-disc album with multiple artwork images to reside in a single directory. For example a 2 disc album directory could contain:
(there is a subroutine which returns for various candidate disc-specific names with the discnumber appended.) |
|
Re: D2 - see edit *** above, important info, the test doesn't make sense without that!!!! |
For users with Box Sets in a single folder without embedding their artwork, will there be support for artwork filenames like:
I am okay with having to embed artwork for this to work (as previously stated) but several others in the forum took issue with that which is what drove the discussion to support for parent folder structure like you have described above and this idea of using artwork filenames that match tag value for DISC#, DISCSUBTITLE, SETSUBTITLE, or GROUPING tags as a way to align a single artwork file to a subset of tracks in the Box Set. I see no issue with limiting to 3 levels up as long as we create a guide (happy to help with that once the rules are locked) for users with options for supporting "varying artwork by track or groups of tracks with a unique overall cover for the album view or listing" (my definition of a Box Set for the purposes of this PR). |
This is implemented in #1536 .
Not yet implemented, but should be simple enough for DISCSUBTITLE once we have the basic logic and performance sorted out. I hadn't thought about GROUPING - it would have to be an either/or with DISC/DISCSUBTITLE otherwise it could clash and get messy. I'd recommend leaving this out for first release. I might be getting forgetful in my old age, but what's SETSUBTITLE? There was also an original requirement to use directory name heuristics to identify artwork (see #1513), but my view is that this is more trouble than it's worth. |
|
I found a bug in #1536 which was slowing it down considerably. Performance is now much improved. More tomorrow (well, later today now!!!) |
|
This discussion shows how even we developers are sometimes overwhelmed with LMS' feature set. And how we tend to keep adding complexity just to cover even the most exotic use case, because we don't understand what already is possible. I believe all that @mikeysas is looking for is already there. But it requires you to stick with some naming pattern:
Tracks will use the pattern matching, the boxset will use the static list of names. No matter what we come up with, the user will have to decide how to organise their files to fit the requirements, and stick with that plan. I am not willing to add infinite complexity to provide almost as infinite flexibility. Let's try to make the limited flexibility a feature, rather than introduce more edge cases and bugs. I'll look into D2 ASAP. |
Confirmed - both behaviour and your guessing. The current implementation does not walk up the tree in this situation.
I'd agree that walking up the tree to the parent (album level) does make sense if we otherwise end up without artwork. I've actually wanted that in the past a lot. I found it stupid to have to stick the same All the selection is done using
That's where I cringe. Can't this be solved using the metadata pattern matching? You mentioned you hadn't looked into that yet. Do it, really - if only to confirm it's still broken. Then we have to fix that. Because I believe it's the answer to most of your complexity. What you're suggesting IMHO is just another method to achieve the same goal. And that's what I don't like about your approach. |
… folder. This should solve typical situation where we have a multi-disc set and don't want to put the image file one copy per disc folder. Signed-off-by: Michael Herger <michael@herger.net>
|
Oops... the fix I just committed was for D2, not D1. It would probably be better if you created an issue here on GitHub. This would allow us to discuss individual issues without mixing things up. |
Issue D2 is not fixed. The initial scan now sets the album artwork from the parent folder, but the tracks get no artwork. And then the N&C scan after adding the specific artwork back replaces the album image with the image from the first bottom level folder, which is also used for all tracks.
On the pattern matching, I was going to test it, but you told me you'd already merged the fixes. I really, really think we are painfully re-inventing the wheel here. #1536 is almost there functionally, though it needs some tidy-up. I would love you to add the more elegant image table and general refactoring from #1621 . For n&c scan image change detection, I still don't think there is any solution other than the more fine-grained SQL grouping in On using only pattern matching to select specific artwork, yes it solves the cache change because it doesn't use the cache. But really, the use of some template names for specific artwork is very similar to the existing use of album/cover/etc.jpg for artwork. In many ways it's the simplest part of #1536. I would suggest a compromise, we could restrict the standard names to the same cover/album/etc names we already use, with the disc number appended. And direct the user to the pattern matching if they need anything more sophisticated. Sorry if i sound a little frustrated, but that is because I am. A few months ago you seemed quite positive about #1536 , and you made good suggestions like removing the instantiation of track objects and replacing In fact, if you haven't had the chance already, please read through the entire #1536 discussion (and perhaps also #1513 ). I think we're all at fault for leaving this to one side for so long. It's certainly taken me a few days to get back up to speed. So please, please can we change course? |
The need to support GROUPING is for the compilation releases that contain multiple albums / EP’s on single Disc. I have numerous Jazz reissues like this with more than one original album per disc with some of the albums also crossing over two discs. I use the GROUPING tag in this case and have Material option to prefer GROUPING. I think it is best to include this and would not worry about what happens if the user has disc1.jpg, discsubtitle.jpg and grouping.jpg. I think we can just pick an order of precedence just like currently cover.jpg is used over folder.jpg if both exist. My suggestion would be as grouping should have the highest precedence because if you find that match the user made an explicit choice to add both the tag and the matching filename.
SETSUBTITLE is an alternate tag for DISCSUBTITLE. I would have to test it again, but as I recall SETSUBTITLE takes precedence if both exist. SETSUBTITLE is what Picard populates. |
I feel your pain. Seriously. Yesterday I even talked about this change request to my wife in bed. Trust me: not many features have reached pillow talk level before! We've reached a point where in real life I'd try to sit down with you and catch up over a tea/coffee/whatever. We both have our idea, but the other fails to understand. I've spent hours (no kidding!) trying to understand your PR. I even wrote some feedback about re-using existing functionality etc. But it's too much. I can't get my head around it. I have to get my hands dirty in order to understand. That's how I ended up with this PR. Maybe you can reach me by email? The only darrell I found in my address book is our former CEO 😁. |
|
@michaelherger hopefully you have an email from me. The @mikeysas post I said I would reply to is here, just above, not in #1430 . |
I'm going to use this "grouping" question to hopefully clarify what is at the root of the current discussion I'm having with @michaelherger . Currently the scan for new/changed/deleted artwork in a n&c scan works like this:
For the current #1536 changes:
To add also grouping:
Of course, nothing is impossible, but I would prefer to get the folder and discnumber functionality out the wider 9.2-running community, for functional and performance validation, before we go further. I hope this makes sense, but feel free to disagree or ask for further clarification. |
|
@michaelherger Just to add to the misery, I've realised there's a real problem using So it's really only album level attributes (or data that can be derived from the album), plus columns present in the GROUP BY that are going to work properly. In current LMS and this PR that's only tracks.cover. In #1536 it would also include tracks.disc, but that's all. |
I don't fully understand the why here or the explanation of what you did in #1536 and how including GROUPING makes that more complex, but not sure that I need to either. I am certainly on board with getting this out in 9.2 sooner for others to help validate is better. Your explanation does make me wonder if there is not a overall simpler or more efficient way to approach the way to process new and changed, but I am not close to all the details as you are. |
3 participants
After long discussions, but still without a complete understanding of the challenge, I came up with the following opinionated approach:
coveridfor all tracks once the pre-cache stage has finished (which actually doesn't pre-cache if that option is turned off...)boxset.jpgorcover.jpgetc. from track level.In my test setup with about 22k tracks (of which 20k on a rather slow NAS) only 97 candidates were found. Processing them took less than 4s.
Potential issues I've identified so far:
covervalue for folder level album artwork. This helps us keeping album level original artwork references. All is handled magically. But I don't know whether using the (already existing)dircontent type could have unexpected side-effects.Feedback will be very welcome.