Created attachment 2583 [details]
patch to fix

I believe this is the correct fix for this bug.

Andy/Triode: can you review for 7.0?

The trouble with this approach is that it requires a complete library rescan to reflect the option when it's changed.  Prefs with this type of requirement should be avoided whenever possible.  It should be possible to construct database queries that treat these roles as being equivalent.  That logic may be in more than one place in the code, so it may not be as easy a fix, but would more correct.

Jim, I agree in principle, however, because of Adrian/Triode's fix for bug 3255 in r11105, which added the contributor column to albums, that would be pretty difficult without coming up with another fix for that bug as well.  Currently the album contributor column would need to be computed differently depending on the setting of that preference.  Also the code just below my addition which was added for bugs 2317 & 2638 looks at whether there is an ALBUMARTIST and so would need to depend on the value of this preference, so a different solution would be needed for that which defers the decision from scan time to database query time.

There is actually another bug in ALBUMARTIST handling that I have found, and since my fix touches the same code as my patch for this bug I'm going to start by describing it here even though it's a separate issue.  In the code added to fix bugs 2317 & 2638 just below the code I added to fix this bug, the ARTIST role is transformed to TRACKARTIST when there is an ALBUMARTIST.  Any ARTISTSORT tags that go along with the ARTIST tags are lost here.  Adding a line to transform ARTISTSORT to TRACKARTISTSORT fixes that.  Arguably the analogous thing should be done in my patch: transform BANDSORT to ALBUMARTISTSORT when transforming BAND to ALBUMARTIST, but I'm not sure BANDSORT tags will ever actually exist so it may be moot.

Created attachment 2590 [details]
revised patch

Here's a revised patch fixing the second bug just described.

Andy/Triode:  Can you review the patch?

I have split out the second bug I described in comment #5 to bug 6507.

Created attachment 2596 [details]
new version of patch

here is a new version of the patch fixing only the primary bug reported here, with the fix for bug 6507 removed.

To clarify the conclusion I intended by my comment #4, I believe Jim's goal of having a change to this preference not require a complete rescan is very worthy.  But I think it is a significant undertaking because of a possibly long series of bandaid fixess that have been applied for other scanner/database problems and would have to be unwound, and this is likely to be destabilizing to the codebase and involve schema changes.

Since the useBandAsAlbumArtist setting does not work at all now I think the less-than-ideal fix should be applied for 7.0 and a more thorough review and fix put off for the following major release.  It would be good to combine this with reviewing the database layout and queries in the hopes of improving UI performance.  The web UI is rather sluggish on my dual 2.8 GHz zeon box with 4Gb ram running FC3.  But it's only a guess that the problem is with database queries, it could be the UI implementation.  I definitely prefer 200 items per page, which was fine in 6.5 but that was simply unbearable for the browse artists menu in 7.0 and I went back to 50.

I think it would be good to understand the specific problem before commiting this patch as there is some code to support useBandAsAlbumArtist at present and it would be good to know what it's not doing...

Its in Slim::Schema::Album for $album->artists and should mean you see the the band if you do not have an albumartist tag which should be your case?  I would have thought this means you see the band as the artist on the web page?  It means it is used for sorting/searching though - so is this the specific problem?

The idea of the current schema (as I understand it..) was to avoid needing to rescan when preferences change and hence more work is done at query time.  In my view this is why we are now slower than before - but that's another story.

Hi Triode,  I will try to explain the issue more clearly.  My expectation is that when I have set "useBandAsAlbumArtist", that the BAND(TPE2) tag in an MP3 will be treated the same as if it were an ALBUMARTIST tag (in a FLAC for example, since ID3 does not have an ALBUMARTIST tag).

It is very easy to see that this is not the case: the scanning code just below the lines my patch adds muck with the track's tags only when there is an ALBUMARTIST tag (but not when there is a BAND tag and "useBandAsAlbumArtist" is set).

Consider the Bob Dylan tribute album as described in bug 4882, comment #35: each track has ARTIST/TPE1 set to the actual performer, and BAND/TPE2="Bob Dylan".  When "useBandAsAlbumArtist" is set, I expect this album to appear in "Browse Artists" under "Bob Dylan", but it is found under "Various Artists".

Looking at the database after a full rescan, for the first track, the contributor_track table contains two contributors, "Eliza Gilkyson" with role=1(artist) and "Bob Dylan" with role=4(band).  In the contributor_album table, I see contributors with role=1(artist) for all the actual performers, and a contributor "Bob Dylan" with role=4(band).  In the albums table, the entry for this album has the contributor column incorrectly populated with "Various Artists".

If on the other hand I have the album tagged with ALBUMARTIST="Bob Dylan", the contributor_track table again contains the same two contributors, however with different roles: "Eliza Gilkyson" has role=6(trackartist) and "Bob Dylan" has role=5(albumartist).  In contributor_album table, I see contributors with role=6(trackartist) for all the actual performers, and a contributor "Bob Dylan" with role=5(albumartist). Finally, in the albums table, the entry for this album has the contributor correctly column populated with "Bob Dylan" and that is where the album appears in "Browse Artists".

Triode wrote:
> The idea of the current schema (as I understand it..) was to avoid needing to
> rescan when preferences change and hence more work is done at query time.  In
> my view this is why we are now slower than before - but that's another story.

Again, I agree this is a good property.  However, this particular option is not really one a user is expected to change much, if at all.  Either you have tagged your music with the assumption that BAND/TPE2 will be treated as ALBUMARTIST or not, and the setting needs to reflect that.

Triode wrote:
> Its in Slim::Schema::Album for $album->artists and should mean you see the the
> band if you do not have an albumartist tag which should be your case?

I don't really understand the call path to that code, and suspect that if it is in fact involved in producing the list of artists in "Browse Artists" that other changes have rendered this logic ineffectual.

Greg, can you confirm whether you have the preference bandInArtists set?

My reading of the current code is:
- useBandAsAlbumArtist is only used for displaying the artists associated with an album, so for e.g. on the web page under each album once you have browsed to the album
- xxxxxInArtist is used to define which contributor role are used for searches/decending by "artist", this is checked in Slim::Schema->artistOnlyRoles, so it includes the preference bandInArtists which should include searching by role BAND - can you confirm this is not working?

The current code was intended to avoid the need to rescan when you change preferences (although I believe there is a significant performance hit and it should be reevaluated) - I'd like to check whether we can get the desired results without needing force rescans at present.

Created attachment 2601 [details]
MySQL query log

I turned on MySQL query logging.  See the attached log snippet for some of the queries being generated when browsing Albums.

This is with useBandAsAlbumArtist enabled.  You'll see three queries generated for every album on the page, where SC is querying first for the role of albumartist, then band, then artist.

Can these three queries not be consolidated into a single query?  The number of queries on an album page would be reduced substantially.

I absolutely agree with this - this is one of the main reasons the latest SC is slower than before for some web pages as Slim::Schema::Album::artists does significant work per album.  If you look at the code you will see there is some caching to avoid doing it multiple times for the same album object, but its still expensive...

> Greg, can you confirm whether you have the preference bandInArtists set?

I do not; I will comment out my changes, rescan, and let you know what I see.

Hmm, looking in my server.prefs to confirm the setting, I see:

| bandInArtists: 
|   - 0
|   - 1

but the other two look like this:

| composerInArtists: 0
| conductorInArtists: 0

what's up with that?  shouldn't it be:

| bandInArtists: 1

No, "bandInArtists" does not do what I want.  There are two types of problems that I noticed quite quickly, which cause the results to be different than using ALBUMARTIST on FLACs.  I'll give an example of each:

1. I have the album "Shoot Out The Lights" with each track tagged as follows:

ARTIST=Richard and Linda Thompson
BAND=Richard Thompson;Linda Thompson  (";" is configured as a separator)

I do not want "Richard and Linda Thompson" to appear in the "Browse Artists" menu, only "Richard Thompson" and "Linda Thompson" (I have other albums by each separately) with this album listed under each.  If BAND were *really* treated as ALBUMARTIST is, it would work that way.

I suspect the reason the "TRACKARTIST" role exists is to support this behavior, and that this is why the code just below the code I proposed adding converts ARTIST to TRACKARTIST when ALBUMARTIST is present.

2. I have the album "You Ain't Talkin' To Me" mostly by "Charlie Poole" but with a smattering of tracks by other artists.  So tracks either have

ARTIST=Charlie Poole

or

ARTIST=Some other Artist

and all tracks have

BAND=Charlie Poole

When I go to the "Browse Artists" menu, "Charlie Poole" is not listed.  It seems to work in other similar cases, so I suspect the issue here is that this is the only Charlie Pool album I have.  Again this works for ALBUMARTIST.

Hum - looks to me that the code is doing what was intended, but it does not work for you...

Key question for me is whether BAND (TPE2) should be treated exactly the same as ALBUMARTIST.  They used to be (as per your patch), but then to support the run time preferences they are no long done this way.

Really needs a review on how we see this working going forwards and what all of the requirements are, I think we should hold off your patch though as it will render 2 preferences useless (useBandAsAlbumArtist and bandInArtists) and needs more thought.

If I turn on an option called "useBandAsAlbumArtist" and BAND tags are not treated the same as ALBUMARTIST tags, then how can you say the code is doing what is intended?  I guess I see my patch rendering that option useful, not useless.  I don't know about the "bandInArtists" option, I don't really understand the intended behavior of that.

Not a huge deal to me as I am running out of SVN and can just keep updating, though a friend who's on windows would also like to use a functioning version of that option.

What's the process for reviewing how this option should work?  Thank you for looking at this!

Will review post-7.0.  Do we have a recommendation for how this should work?

I've looked at the code and... It really looks like the only way this can be done is to do it at scan time, otherwise the queries involved in generating album lists to get the artist are way too expensive.  If you do it at scan time there's no reason that TPE2 can't be treated as being exactly the same as ALBUMARTIST.  The role set in contributor_album should be ALBUMARTIST when this pref is enabled, not BAND.  As with other prefs that require a rescan before taking effect, put an explanation in the settings description saying just that.

I'm not sure what the default value is for this pref, but I think it should be enabled, as it's common for other applications to treat TPE2 as the album artist.

I've been following various threads in the beta forum about this sort of issue.

I posted a reply in one thread, and Michael Herger suggested that I paste that summary into this bug report:



I think you've reminded me of the difference in usage that SqueezeCenter makes between TXXX ALBUMARTIST and TPE2 BAND tags.

I've found some instances where I have used BAND.  For example, Paul Simon and Art Garfunkel are widely renowned as "Simon & Garfunkel".  I tag their work with:

ARTIST=Paul Simon
ARTIST=Art Garfunkel
BAND=Simon & Garfunkel

All three appear in the album list.

I could also add ALBUMARTIST="Simon & Garfunkel", and therefore I would only see this name in the artists list.  (Actually, I would also see Paul Simon, as I have an album by that specific artist, and if I browsed to Paul Simon, I would see all "Simon & Garfunkel" albums under that artist too).

For me, I use BAND purely as another contributor, and would not want it to work as ALBUMARTIST, as that would then cause Paul Simon and Art Garfunkel to disappear from the browse artist list.  It would also be going against the ID3 tagging standards, which say that BAND is a contributor, not ALBUMARTIST.  Due to there being no standard tag for ALBUMARTIST, some software packages have borrowed BAND for this purpose, but that is non-standard.

The setting "List albums by band" I think is broken.  Having changed the option from "List albums by band" to "List albums by all artists for that album", there was no effect.  I rescanned and still there was no difference in how my music library had interpretted the tags.  I think the intention of this setting is "Treat band as album artist?", and thus should be a checkbox.

I actually want it to not treat BAND as album artist, as I want to use the flexibility already in place, where ALBUMARTIST and BAND are used for different purposes (I can add an ALBUMARTIST tag when I want songs reported only by album artist, and I can add BAND tags as aliases for the group of artists independently).


So, to sumarise:

ALBUMARTIST is a way of grouping songs on an album to appear under a specific artist name.  This can be used to avoid the album being considered a compilation/various artists album, instead it lists the album as being only by the specified ALBUMARTIST tag.

BAND is just another contributor on the song, treated exactly the same as COMPOSER and CONDUCTOR contributor tags.

I believe that if an ALBUMARTIST tag has been entered for songs on an album, this takes precidence over the setting of "List albums by all artists for that album/List albums by band" - the album would always be listed only under the ALBUMARTIST.


Suggested Changes:

SqueezeCenter is not treating the band tag as an album artist tag if "List albums by band" is selected.

The radio button "List albums by all artists for that album/List albums by band" should be changed to a checkbox option "Treat band as album artist".


I would make the default for "Treat band as album artist" as unchecked, as this then follows the ID3 standard, but users have the option to make it work out-of-standard and similar to other (inferior!) software packages (i.e. iTunes).

i'm not sure who is on top of all this, but please read this thread:

http://forums.slimdevices.com/showthread.php?t=45457

i can tell you that without a doubt, my mp3s TPE2 tags are NOT being used by SC7 to sort.

i believe it is because the scan fills SC7s internal BAND tag, but not its ALBUMARTIST tag.

for whatever reason, SC7 will not use the BAND tag to sort, only the ALBUMARTIST tag.

this seemingly contradicts the option in settings->music library-> "List albums by Band."

so, my TPE2 tag info doesn't populate SC7s internal ALBUMARTIST field

AND

SC7, at the very least, is unclear about what it uses to sort.  it certainly is NOT the BAND field, even though thats what the option seems to suggest.

BAND should remain separate from ALBUMARTIST (one should not populate the other), otherwise when changing configuration options, a full rescan would be required for the changes to be picked up.

Instead the scanner should continue to populate the band and albumartist content as distinct entities irrespective of settings.  The option would change how SqueezeCenter selects data from the database and presents it on screen.

Sorting should sort the album list by ARTISTSORT, so if there is no ARTISTSORT tag, the contributor (artist, band or albumartist) name would be used for sorting.  "list albums by band" (or "Treat band as album artist" as I think it should be called) should have no effect on sorting.

i am quoting philip meyer here with > symbols


>BAND should remain separate from ALBUMARTIST (one should not populate the
>other), otherwise when changing configuration options, a full rescan would be
>required for the changes to be picked up.


in case i was unclear, i was saying TPE2 should populate the internal tags SC7 has for BAND and ALBUMARTIST.  right now it seems to only do BAND, and as a consequence music is not sorted properly since it uses ALBUMARTIST to sort, (which btw seemingly contradicts the documentation for the option).

this is very confusing since the option on the music library page indicates otherwise, (and i assume it is a sorting option, not just a 'display album by' option)

i don't see why filling both fields is a problem, or that a full clear and rescan is a problem.  i hear that you are saying there is a drawback but i don't see what the drawback is?


>Instead the scanner should continue to populate the band and albumartist
>content as distinct entities irrespective of settings.  The option would change
>how SqueezeCenter selects data from the database and presents it on screen.


i am not suggesting that SC7 should not have distinct internal tags of BAND and ALBUMARTIST, just that in the case of someone with mp3s, it should fill them both with the same TPE2 info, (if nothing else is there to fill it).


>Sorting should sort the album list by ARTISTSORT, so if there is no ARTISTSORT
>tag, the contributor (artist, band or albumartist) name would be used for
>sorting.  "list albums by band" (or "Treat band as album artist" as I think it
>should be called) should have no effect on sorting.


i do not use "ARTISTSORT" i have only TPE2 tags, and they are ignored for sorting.

so my question is this...  if "List albums by Band" is NOT meant to sort my albums, then how in the world am i supposed to get SC7 to sort my albums by my TPE2 tags?

CLEARLY this is a desirable function for SC7 to have is it not?

can someone FROM SLIM with the final authority to say once and for all:

is the "List albums by Band" option supposed to sort albums or not?

and if not, then answer how one is supposed to do it with mp3 TPE2 tags?

and also explain why SC7 sorts by ALBUMARTIST internally but not BAND internally?

Comment on attachment 2590 [details]
revised patch

second patch is obsolete

greg,

i ask b/c i am curious:  did bug 8001 solve your issue, and if not, why not?

i would have thought bug 8001, which scans TPE2 to mean ALBUMARTIST in SC7.1 would have fixed your issue, (its been fantastic for me).  if it hasn't, i'm curious whats still wrong, or in what way the solution isn't fully satisfactory?

thx.

> i ask b/c i am curious:  did bug 8001 solve your issue, and if not, why not?

Sorry I missed your question from back in July, Mike..

Looking at the change, it should, though I am still running 7.0, and when I
upgrade, I plan to switch to using TXXX ALBUMARTIST tags.

I'll still need bug 4584 to be fixed for all this to be completely resolved.

where is the new_schema keyword?

Greg,

have you had a chance to try this yet?  also, i voted for your other bug.

to answer my own question above, you can display and sort now by TPE2 if using the option in bug 8001

however, i am beginning to question the slim methodology.  but thats for another thread...

resolve duplicate of bug 8001.

*** This bug has been marked as a duplicate of bug 8001 ***