Check whether tracks are contiguous with respect to work/grouping/performance

[darrell-k]

For clients that can display subheadings in album track listings (it's only Material for now) the listing can be messed up if the tracks are not contiguous with respect to the grouping elements (work/grouping/performance). For example, this can happen with various compilation-type albums as well as recital type discs where the artist may play the movements of a work interspersed with other related music.

I have worked with a couple of users on the forum to develop and test a solution to identify such discs and it is working well with Material Skin. In order test the logic I provided them with a version of Queries.pm which having identified such albums, simply removes the elements involved with grouping from the result loop.

But we cannot do this "in real life" because it would mess up Default Skin and hardware client displays, which don't create subheadings but where the user can chose to include Work/Grouping/Performance in the Title Format.

So we have a couple of options:

1. Simply return a flag so that Material would know to delete or ignore works.id, works.title, tracks.grouping and tracks.performance (this would need to apply when adding tracks to the playlist and I think the status command as well as when loading a track listing from the titles_loop).
2. Introduce a new tag which would instruct LMS to remove those elements before returning the results. This would be implemented in _getTagDataForTracks which is used by the titles_loop and the status query. This would seem simpler, because it deals with the issue at source, but it would be another tag, and one specific to Material Skin (for now).

What do people think? mentioning @michaelherger and @CDrummond for discussion.

[michaelherger]

Would you have a reference link where I could see this in action? I lack imagination of the problem 😄.

My first thought was: let the Material plugin handle it on the server side. I believe there are/were some CLI handlers to tweak responses to the skin's needs. But I might obviously be wrong.

[CDrummond]

This sounds like you want to remove grouping info from the tracks command? If you did then the "multi group" flag would also need to be removed from the albums command - otherwise material will still show the "multi group" indicator, leading to inconsistencies.

But to be honest, I'm not sure I see a real issue. Either all tracks in an album should be in a group or not. If a user has a mixed grouped and not grouped album then it's up to then to fix their tags. And material will create "other" groups as a workaround.

[darrell-k]

@CDrummond This can't be fixed in the tagging: Here's an example. On this album, the 4 movements of "Death and the Maiden" are interspersed with other tracks (this is how the artist intended it, it's not messed up track numbering).

Obviously subheadings in this album track listing make no sense and need to be suppressed. But removing the "Death and the Maiden" work tag from those 4 tracks would mean that the work would no longer be found in a "Works" listing, so that's not the solution.

I take your point about the group_count at album level. Maybe the whole solution to this is to identify this situation at the album level and override the group_count?

Hopefully this also serves as the example @michaelherger was asking for.

[CDrummond]

@darrell-k I'd like to have the JSONRPC RESP that produced that list - as there should only be 2 "Other" groups. 2,3 and 4 should be a single group. And how has the "Franz Schubert" state it has 4 tracks, but only shows 1???

[darrell-k]

@CDrummond here you go (attached). I suppose what Material is doing makes sense from a logic point of view, there are 4 tracks in the "Franz Schubert" group, it's just that on the disc they're interspersed with tracks which are not in the group, which is why I say showing any group subheadings for this particular album doesn't make sense.
Death_and_the_Maiden.json

[CDrummond]

@darrell-k Thanks. I can see now - that's messed up. Yes, grouping should be disabled here - and at the album level. Material can disable the grouping here, as it could check if all tracks are in a group, and the groups are in single blocks. But it'd need the 'is grouped' flag to be disabled at album level - which would be up to LMS.

[darrell-k]

@CDrummond Thanks, I'll work on an SQL function that can spot this at the album level and override the returned group_count. What would you want, in such cases, LMS to return in group_count? 1? -1? 0?

[CDrummond]

@darrell-k Well seeing as no grouping will be done, then 0 makes more sense to me. But if multi-disc then that should still be returned - so tracks will not be split on group but on disc.

[Edit] And to be clear, all tracks need to be in groups and each group needs to be a single continuous block?

[darrell-k]

@CDrummond

each group needs to be a single continuous block

For actual groups (ie at least one of work, performance, grouping is not null), yes.

all tracks need to be in groups

No, there may be a group of tracks where work, performance and grouping are all null (lets call this the "null group"): in this case, subheadings ("Other (n)") are valid for each contiguous set of such tracks, as long as all non-null groups are contiguous.

But with LMS doing the work, couldn't Material just check the album level group_count, and if zero, bypass the grouping (but not the multi-disc) code?

Also, to confirm, I won't be changing the disc count for multi-disc releases.

[CDrummond]

@darrell-k The issue then is if there is are 4 blocks ["Other (1)", "Actual Group", "Other (2)", "Other Group"] then what will group_count be? I have such an example and LMS returns group_count of 3 - as I assume the first null group is ignored? But then if we have ["Other", "Actual Group"] - LMS would return group_count of 1, which will cause Material to not show the multi-group icon and not prompt which group to play. This is why I think all tracks need to be in groups.

[darrell-k]

@CDrummond in the first example, LMS is returning 3 because "Other (1)" and "Other (2)" are the same group (ie the null group). I think Material is already behaving correctly in that case.

In the second case, LMS would return a group count of 2 (1 null group and 1 non-null group, it doesn't distinguish). I am proposing that it would continue to do this as long as the non-null group was contiguous. But if the non-null group (any non-null group in the case of multiple non-null groups) was not contiguous, it would return a group count of zero.

My challenge is achieving this in pure SQL (avoiding having to drop into Perl manipulation of a result set, for efficiency/performance) given that SQLite doesn't support stored procedures. It's interesting!

[michaelherger]

My challenge is achieving this in pure SQL (avoiding having to drop into Perl manipulation of a result set, for efficiency/performance) given that SQLite doesn't support stored procedures. It's interesting!

Isn't solving this at the query level building in presentation level logic in the wrong place? If the presentation layer is requesting that information, we should not decide on the backend whether the frontend wanted to have the information or not. It's the frontend which should do this. Otherwise we're baking Material logic in to the core server, which I don't like, TBH.

[CDrummond]

@michaelherger I agree Material specific code should not be in LMS. Is there a way to allow a plugin to hook into a JSONRPC request? If not, then perhaps the LMS change could be to provide an interface for plugins to do this, and then Material's perl code code be updated to intercept the "albums" query and perform the extra processing required for this?

I guess the main question is whether this change would be useful for the Default skin too?

[michaelherger]

Didn't you write your own command wrappers in the past to "massage" the data before sending it to the frontend? Eg. the frontend would request "albums 0 100" any more, but "materialskinalbums 0 100". You'd then run the same old query internally, run through the data and remove what you don't want before sending it back to the browser?

The Default skin would probably require a huge effort to allow for these kinds of sub-titles. But I haven't looked into things like that in a long time.

[darrell-k]

Isn't solving this at the query level building in presentation level logic in the wrong place?

I don't think so, because all we are doing is describing something about the data (are the returned groups contiguous or not?). It's entirely up to the front end what it does (or doesn't do) with that information.

Having said that I'm leaning towards a new query which a front end can call only if it needs the information.

Just saw @CDrummond 's reply as well: yes, that would be even better: but rather than have Material do this for every album, when it's only ones with potential for grouping that need to be checked, perhaps a new command in the Material plugin Perl code which can be called only for albums where albums query has returned group_count>1?

If that makes sense, I'd be happy to submit a Material PR for the Perl code.

Finally (just saw @michaelherger 's latest reply), yes, I've already realised that that subtitles in default skin lists would be a massive change (and we'd have to ensure that it didn't apply for hardware device displays, and a lot of the code is shared), which is why i haven't attempted it.

[CDrummond]

@darrell-k No, I don't want to call "albums", get a list of 1000 albums, 500 of which have "group_count>1" and then have to make another call (or worse, 500 calls). But perhaps an "albums" query can be added internal to material - so, e.g. ["material-skin", "albums", start, end, "tags:"]. This could then call LMS's "albums" command, and then process each "group_count>1", all in the perl side? Still, I worry how much extra over-head this will add...

[darrell-k]

@CDrummond yes, performance is a worry. Doing it in LMS at the point the titles_loop is constructed is effectively cost free (just a few if statements, no extra database access), but that leaves the problem of the overlaid icon on the album image.

Really, the proper way to do this would be an extra column in the albums table, populated at scan time. We considered this for the group_count, but we did not want to be adding columns for a 9.0.point release of LMS. As it turned out, it wasn't expensive at all to do that in albumsQuery, but this is a different matter.

Maybe we should push this back to LMS 9.1 and do it with a new column? It's somewhat an edge case, after all. (But it could also be argued that the icon overlay appearing for these albums is not a big problem either, again because it's an edge case). The question is what's worse, a messed up track listing or an errant icon?

But before a final decision, I'll try what you just suggested, and get some performance metrics running against my big test library.

[michaelherger]

@darrell-k No, I don't want to call "albums", get a list of 1000 albums, 500 of which have "group_count>1" and then have to make another call (or worse, 500 calls). But perhaps an "albums" query can be added internal to material - so, e.g. ["material-skin", "albums", start, end, "tags:"]. This could then call LMS's "albums" command, and then process each "group_count>1", all in the perl side? Still, I worry how much extra over-head this will add...

I might have missed the actual issue. I thought it was about removing things from the current list (those "others" items). Which wouldn't require an additional query, would it? What am I missing? See initial posting:

Simply return a flag so that Material would know to delete or ignore works.id, works.title, tracks.grouping and tracks.performance

[darrell-k]

Providing this flag in the albumsQuery response requires means that query needs to analyse the structure of the album's tracks.

My idea was to provide a separate query to obtain this flag, so that this dive into the tracks table would not be done for every album, but only called for albums that we needed the extra info for.

But @CDrummond is correct: this is a bad idea: for large album sets it would likely worsen performance.

Anyway, I think I'm getting near a solution that doesn't hit performance to a material extent.

[darrell-k]

OK, I'm proposing a solution (implemented at https://github.com/darrell-k/slimserver/tree/noncontiguous-groups ) which keeps things simple: an extra flag returned by albumsQuery, driven by the existing tag 2 which is the one which already triggers return of the group_count.

This has to be more efficient that solutions which involve post-processing of the albums_loop.

The new flag is called contiguous_groups and will be set to 1 if the track groupings on the album are contiguous. The group_count returned will continue to contain the number of groups (including the null group), contiguous or not.

@CDrummond, is this everything you need?

I tested this with my large (>25000 albums) test library, and there was no appreciable difference in processing time, possibly because the 2 tag already results in a subselect over tracks, and the database engine is clever enough to optimise things when it comes across a second subselect over the same table.

Unless there are any massive objections, I'll shortly create a PR for LMS 9.0.

[CDrummond]

@darrell-k That sounds fine to me. I assume contiguous_groups is set to 0 if not contiguous. Therefore I can assume if this is not present, or set to 1, that groupings are contiguous.

[darrell-k]

@CDrummond I think it comes back as "" for non-contiguous so will fail a truthy test. It should always be present if you've submitted tag 2.

[CDrummond]

@darrell-k Why an empty string? I think it'd be more consistent to be a 1 or 0.

[darrell-k]

@CDrummond changed to return 1 or 0. I was being very lazy.

[darrell-k]

PR #1272 raised.