Rockbox

Tasklist

FS#7287 - Support metadata sort tags in the Database

Attached to Project: Rockbox
Opened by Dan Everton (safetydan) - Monday, 11 June 2007, 07:50 GMT
Last edited by Marc Guay (Marc_Guay) - Tuesday, 13 May 2008, 12:19 GMT
Task Type Patches
Category ID3 / meta data
Status New
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

Attached is a patch that starts support for the sort tags specified in ID3 v2.4. It adds three new tags to the tagcache system, sortartist, sortalbum, and sorttitle which are drawn from the TSOA/TSOP/TSOT tags.

This is only the beginning and is very, very lightly tested (i.e. it compiles and doesn't seem to break anything).
This task depends upon

View Dependency Graph

This task blocks these from closing
 FS#6153 - Support for ARTISTSORT/TSOP in tagcache 
Comment by Dan Everton (safetydan) - Monday, 11 June 2007, 08:19 GMT
Now supports parsing the soal/sora/sonm tags from MP4 files and the ALBUMSORT/ARTISTSORT/TITLESORT comments from Vorbis and APE files.
Comment by Dave Chapman (linuxstb) - Monday, 11 June 2007, 08:21 GMT
As mentioned in a forum post, this page describes the equivalents to the id3v2 sorting tags for other tag formats:

http://musicbrainz.org/doc/PicardQt/TagMapping

Another page describing the same vorbis comments mapping:

http://wiki.slimdevices.com/index.cgi?SlimServerSupportedTags

Comment by Dieter (dip) - Monday, 11 June 2007, 12:03 GMT
Thanks a lot this sounds great!

Is it possible to include ALBUMARTISTSORT in your patch? I use most of the time ALBUMARTIST instead of ARTIST when browsing so the patch wouldn't help without support for ALBUMARTISTSORT. Since in the ID3 v2.4 definition this tag is not separately defined I assume that for mp3 files TXXX:ALBUMARTISTSORT must be used instead as indicated in the above TagMapping link. For the other formats (Vorbis...) ALBUMARTISTSORT can directly be used.
Comment by Dieter (dip) - Tuesday, 12 June 2007, 19:29 GMT
I applied this patch to the current svn version and compiled it for the iPod Simulator. When running the simulator and choosing "Database" a message appears "Database is not ready". When I then press SELECT to initialize the database the message "Building databse..." appears and does never stop (it says "0 found" and does not change the counter). When I try to initialize the database through the General Settings menu it says "updating in background" but the database is in fact not generated.

I then reversed the patch and compiled again and then the simulator creates the database without problems. So it seems to be a problem with the patch to initialize the database.
Comment by Dan Everton (safetydan) - Tuesday, 12 June 2007, 22:30 GMT
Yeah the changes to the database don't actually seem to work. I'm also chasing another issue with parsing ID3v2.4 tags. Consider this a very much "work in progress" patch.
Comment by Dan Everton (safetydan) - Wednesday, 13 June 2007, 10:48 GMT
Well this version actually builds a database. Hopefully it's correct. Now I just need to figure out how to configure the tagnavi file so the sort tags are used.
Comment by Dan Everton (safetydan) - Wednesday, 13 June 2007, 11:37 GMT
Oh, and the above patch also contains a "fix" for parsing of id3v2.4 tags. I don't know if it's correct but it works for me.
Comment by Dieter (dip) - Wednesday, 13 June 2007, 11:41 GMT
Does this patch only enable you to use e.g. the ARTISTSORT tag instead of the ARTIST tag in the tagnavi file which means that the ARTISTSORT tag is used for sorting AND for displaying the artist? As mentioned by Llorean in the forum http://forums.rockbox.org/index.php?topic=10689.0 the correct implementation would be to use the ARTISTSORT tag for sorting but the ARTIST tag for displaying. If you tag e.g. your songs from the Beatles with ARTIST="The Beatles" and ARTISTSORT="Beatles, The" when browsing artists the Beatles should be sorted inbetween e.g. "Barbra Streisand" and "Bryan Adams" but shown as "The Beatles" like

...
Barbra Streisand
The Beatles
Bryan Adams
...

If I have to use ARTISTSORT instead of ARTIST in the tagnavi file does that mean that ALL my songs have to be tagged with ARTISTSORT since all songs without this tag are only shown in the <untagged> category?
Comment by Dan Everton (safetydan) - Wednesday, 13 June 2007, 20:40 GMT
You are correct. I've gone about this the wrong way. Next patch will just use the sort tags for sorting (if they're there) and not require any changes to tagnavi.
Comment by Dieter (dip) - Thursday, 14 June 2007, 06:25 GMT
As mentioned above, could you please also include ALBUMARTISTSORT in the new patch?
Comment by Dieter (dip) - Thursday, 21 June 2007, 23:41 GMT
@Dan
I know it's a little bit off topic but I just tried to create a new patch supporting a new tag for GROUPING (or CONTENT GROUP) and now I have the same problem as I had with version 2 of your patch. After installing the patched version Rockbox fails to initialize the database. What did you have to change from version 2 to 3 that it worked (maybe I have a similar problem)?
Comment by Phil Brownlee (ell1ps1s) - Saturday, 11 August 2007, 23:13 GMT
I expect there's a general feeling against non-standard tags; however, since one of the advantages of recognising TSOA etc. tags is that artists can be sorted by surname (so, for instance, Charles Mingus is listed under 'M'), it would be nice to do the same thing with 'Composer' tags (ie. sort 'Claude Debussy' as 'Debussy, Claude'). The id3v2.4 standard doesn't seem to include a 'Sort Composer' tag, but would it be unreasonable to extrapolate something like 'TSOC'?

This is no sort of recommendation, but I notice that recent versions of iTunes, which have implemented sorting tags for more fields than the 3 defined by id3v2.4, writes a TSOC tag if you add a 'Sort Composer' to a file.'
Comment by Travis Tooke (tdtooke) - Thursday, 15 November 2007, 11:48 GMT
I'm familiar with the workings of tagnavi.config at all, but would replacing:
"A" -> artist ? artist ^ "A" -> yearalbum -> title = "fmt_title"
in the artist submenu with:
"A" -> sortartist ? sortartist ^ "A" -> yearalbum -> title = "fmt_title"
implement sortartist?
Comment by Phil Brownlee (ell1ps1s) - Friday, 16 November 2007, 20:34 GMT
@Travis
As I understand it, tagnavi can't display tags that are not in the database in the first place. The sort tags need to be collected when the database is built; then the tagnavi syntax can be used to display them. So it involves a rewriting of the database-building code, as well as the interpretation and display of the database.
Comment by Travis Tooke (tdtooke) - Saturday, 17 November 2007, 01:28 GMT
But would that work after you rebuilt the database? I was under the impression the patch would build a database with those tags as it is now, it just didn't have it setup in tagnavi.config. If what I mentioned does actually work we probably need to define sortartist as artist for files that don't have that tag also. All this is what I'm thinking anyway.. I'm no expert, that's for sure! :)
Comment by Dan Everton (safetydan) - Sunday, 09 December 2007, 11:14 GMT
Resync to SVN 15899.
Comment by Dieter (dip) - Sunday, 09 December 2007, 23:18 GMT
Does this patch now use the sorting tags for sorting and the "normal" tags (like ARTIST) for display and use in the the tagnavi.config file?
Comment by Dan Everton (safetydan) - Sunday, 09 December 2007, 23:27 GMT
Right now it doesn't do much of anything apart from add some extra fields to the database. The intention is that the sort tags will be used for sorting vales and that the non-sort will be use for everything else.
Comment by Travis Tooke (tdtooke) - Tuesday, 13 May 2008, 08:05 GMT
Here is a synced version of what was here before if anybody wants to look into it. I'm attempting to myself with not much luck at the moment.
Comment by Thomas Martitz (kugel.) - Wednesday, 28 May 2008, 23:04 GMT
Hmm, there's something weird with this patch. I've applied it and compiled the sim. The test music seems to be sorted completely random (see picture), as it does clearly not sort alphabetically.

The screen dump shows the database -> album artist view. Note that only "Die Toten Hosen" has tagged the TSOP filed (TSOP = "Toten Hosen"), the others not. Without the patch the view is sorted correctly (with "Die Toten Hosen" under D of course).

Another note: Browsing database -> A to Z .. -> Artists -> D shows "Die Toten Hosen", proving the implementation isn't working yet (I assume it's intended that it should appear under T in the A to Z view as well).
Comment by Travis Tooke (tdtooke) - Thursday, 29 May 2008, 02:46 GMT
I have noticed in my tests results like what you posted. I haven't looked at it in 2 weeks or so but I believe I have the random sorting fixed, I just haven't posted anything because I have yet to make a patch that actually works. I think the mp4 metadata section needs to be redone if this is going to support things tagged by itunes. I used itunes to add it's sortartist tags to something and the actual tag name of what it added was: PERFORMERSORTORDER. I'm playing around with something that is a cross between versions 3 and 5 with some changes of my own.
Comment by Dan Everton (safetydan) - Friday, 30 May 2008, 10:28 GMT
Yeah I never did get the sorting working. The attempt that's there in the v5 patch didn't really seem to do the right thing.

As for the MP4 metadata I based that on the documentation here http://musicbrainz.org/doc/PicardQt/TagMapping which says iTunes uses 'soaa' for the artist sort order.
Comment by Thomas Martitz (kugel.) - Friday, 30 May 2008, 10:47 GMT
You didn't only get it not working, you even messed it up :D

Have you allready looked into tagtree.[ch]? There's most of the actual database stuff done (tagcache != tagtree). I didn't.
Comment by Dan Everton (safetydan) - Friday, 30 May 2008, 10:52 GMT
Interesting, no I didn't look at tagtree. I'm guessing that's where the presentation side of things happens and tagcache is more about the data persistence. Since sorting is a display issue I guess it should be handled in tagtree.

Comment by Thomas Martitz (kugel.) - Friday, 30 May 2008, 10:54 GMT
Correct.
Comment by Dan Everton (safetydan) - Friday, 30 May 2008, 10:57 GMT
Hah! Yes, it looks like you could combine the database changes I made to tagcache.c so that the sort tags are stored with some changes to the compare() function in tagtree.c and get this working.
Comment by Thomas Martitz (kugel.) - Friday, 30 May 2008, 11:04 GMT
And where's the updated patch? :>
Comment by Dan Everton (safetydan) - Friday, 30 May 2008, 11:12 GMT
Well actually now I'm not so sure that's all that's needed. There might be slightly more extensive changes to tagtree required to ensure that the sort name doesn't get displayed to the user. You could always just display the sort name I guess, but I'm assuming people want to sort by, e.g. Beatles but still display "The Beatles". Though I could be wrong on that.
Comment by Thomas Martitz (kugel.) - Friday, 30 May 2008, 11:28 GMT
I don't think you're wrong, I'd personally prefer to show the "The", or what ever is the artist is tagged like. Maybe I'll take a quick look later on. I'm unfortunately not a tagcache/tree master.

One question: What are your thought about the album/albumartist issue (for ID3)? Should performersortorder (TSOP) replace both, or only artist or album artist? Some people prefer browsing artist, others (like me) prefer albumartist. In both cases replacing artist and albumartist respectively with performersortorder in the list seems appropriate to me.

But on the other side: This example raises something weird. I have some compilations with like 20 or more artists. I've chosen albumartist "Various Artist" for them. If I chose performersortorder to be "Various Artists" (for whatever reason) they'll show up under V even in the artist browsing, where each track should have it's own entry, since I've tagged the artist tag properly (with each artist, and not "Various Artists"). This sounds weird.

A workaround would be of course to not tag performersortorder for compilations for ID3, but it looks more like a hack.

AAC and others shouldn't have this problem, since they support different albumartist sort and artist sort fields.
Comment by Dieter (dip) - Friday, 30 May 2008, 11:48 GMT
In my opinion there should be different sorting tags for artist and album artist. Since for mp3 there is no standard sorting tag for album artist I would go with the mostly used TXXX:ALBUMARTISTSORT as described in http://musicbrainz.org/doc/PicardQt/TagMapping
Comment by Thomas Martitz (kugel.) - Friday, 30 May 2008, 12:02 GMT
I personally wouldn't go with non-standard stuff. While we can talk about reading such unofficial tags optionally, requiring these is a bad option imo.

A user might intentionally using the TSOP tag for albumartist, and wonder why Rockbox doesn't read it.
Comment by Dieter (dip) - Friday, 30 May 2008, 12:36 GMT
But if there is no standard you cannot follow one. And, as far as I know, even using the TPE2 tag for album artist is not really standard (since according to the official definition of the id3 standard is was supposed to be the tag for band/orchestra/accompaniment) but everybody uses it for album artist.

I have defined an "album artist" browse menu and an "artist" browse menu in tagnavi_custom.config. I usually use the first for browsing since normally I want to browse only those artist for which I have at least one complete album. In the first browse menu the number of listed artists is thus much smaller than in the second browse menu. I use the second browse menu only when I want to browse to ALL artists, even listing artists for which I have only one song on a compilation album. Of course browsing through the second list takes much more time and the list is much more clutter with the huge number of listed artists.

How should the sorting tags be used in the tagnavi_custom.config files? If for defining a browse menu in the tagnavi_custom.config file still the strings "artist" and "albumartist" and for filtering the listed results the corresponding tags were used and only for the sorting of the resulting list the TSOP tag was used then it could be sufficient to use only one sorting tag for both tags. As far as I understood, the "album artist" browse list would then still be much short than the "artist" browse list. Of course, the user hat then to define for compilations e.g. TPE1="The Beatles", TPE2="Various Artists" but TSOP="Beatles, The".

However, if the sorting tag should be used in tagnavi_custom.config to FILTER the browse list (e.g. defining a new syntax element "artistsort" which must be used INSTEAD of "artist" for defining a browse structure in tagnavi_custom.config) then there must be two different sorting tags to be able to distinguish between artists and album artist.

Comment by Thomas Martitz (kugel.) - Friday, 30 May 2008, 12:54 GMT
I'd rather allways use TSOP etc if it's there (and otherwise fall back to albumartist/artist) and leave the tagnavi like it is. If one uses the sort tags, it should work out of the box. Why should we make it optional/configurable?
These tags are only for sorting. There's absolutely no need to have it disabled in sorted lists.

Sure, there's no standard. But your idea is just like using the command tag for that stuff. It's quite random. TPEx tags are specifically designed for several performer types, which includes album artist. Surely, it's not declared as standard, but it's a de facto standard. And it makes sense, when TPE1 is used for the artist, to use the next for album artist.

I really dislike your idea. We should decide if we either use TSOP for both, artist and albumartist, or only for one of those (which I prefer -> TSOP only for artist). Album artists ("Various Artists" e.g.) may have a lower chance to include an unwanted leading article or something.
Comment by Thomas Martitz (kugel.) - Friday, 30 May 2008, 14:57 GMT
Ok, I think we're not too far away.

The screen dump shows the sorting working. It's in the artist view. Notes: The album artist view is still messed up. I also have tagged the TSOP tag for all those files.
Note: It shows the "Die", which is nice (as before, TSOP for it is "Toten Hosen", artist "Die Toten Hosen").

So, TODO:
- Disable the sorting for albumartist view (or enable it the same way as the artist view, I prefer disabling though, as I allready mentioned).
- Implement a fall back to artist if TSOP is untagged.
Comment by Thomas Martitz (kugel.) - Friday, 30 May 2008, 15:01 GMT
Forgot the dump :)
   dump.png (12.9 KiB)
Comment by Dieter (dip) - Friday, 30 May 2008, 15:07 GMT
Does disabling sorting for albumartist view mean that for browsing by album artist sorting cannot be used? Then it's useless for all people who browse by album artist like me (I browse by album artist 95% of the time).
Comment by Thomas Martitz (kugel.) - Friday, 30 May 2008, 15:15 GMT
No. With disabling I mean, that in case of ID3 the sort tags won't be used for the album artist view, since ID3 doesn't have such a tag for album artist (only for artist). You can surely use the view as before.
Comment by Thomas Martitz (kugel.) - Saturday, 31 May 2008, 00:31 GMT
This patch goes a different way.

It adds the sort tags as normal tags, which is IMO definitely the cleaner, less hackish, solution.

I've managed to be able to use "artistsort" etc in the tagnavi.config. I've also implemented a fall-back to normal tags, if the sorting tag is untagged.

Play around a bit. I'm not entirely sure about the behavior with albumartistsort and mp3 (i.e. ID3-tags), I haven't really tested it.

The only remaining problem: With this patch, the sorting tag is shown in the database. E.g. in the database it reads "Toten Hosen" instead of "Die Toten Hosen". I don't really like that. I'm not sure at this point, where the proper place for this to fix is. From what I've seen, creating exceptions in tagtree isn't as easy as I thought.

Maybe Slasheri would be so kind and just jump in for this last step? :) If we've done this, we can consider completely replacing e.g. "artist" with "artistsort" (the tagnavi.config would still read "artist"), so that the sorting tags are allways taken into account.

I've also removed those #ifdef HAVE_TAGCACHE, it's pretty useless, since tagcache is only build when HAVE_TAGCACHE is defined.
Comment by Travis Tooke (tdtooke) - Tuesday, 24 June 2008, 11:01 GMT
I've been playing around with this and I'm very happy with it. Actually it might be a good idea to leave it as is. Scrolling through and seeing "Smiths, The" instead of "The Smiths" gives it a more consistent feel with the surrounding artists starting with the same letter in the surrounding menu to me. That's just me though. You're my new hero kugel, just you you know.
Comment by Chris Kagan (KindOfBlues71) - Friday, 27 June 2008, 13:54 GMT
As kugel states above, sorting isn't working yet. I tested the patch using tdtooke's r17790 custom build and replacing "artist" with "artistsort" in tagnavi strings only displays that tag. The patch should somehow collect the sorting tags if populated when initializing the database but no changes should be required in the tagnavi. The sorting should be done "behind the scenes" somehow, otherwise what is listed in the tagnavi strings is displayed when browsing the database.
Comment by Thomas Martitz (kugel.) - Friday, 27 June 2008, 14:15 GMT
"Isn't working" is wrong.

I said I added it as a normal tag. So it's working like a normal tag.

What we want is that it displays artist but sorts by artistsort, like you already said.

But, I fear that this will only be possible with a "dirty hack" (see first few patches).
Comment by Thomas Martitz (kugel.) - Saturday, 02 August 2008, 23:31 GMT
After some days of testing in my build (which also contains several other patches), I'm noticing heavy database instabilities (e.g. songs dissappear randomly) after I applied this patch. Since there're so many other patches in my build, I can't tell for sure that this patch is causing this by itself, but I at least beleave so.
Comment by Chris Kagan (KindOfBlues71) - Monday, 04 August 2008, 13:46 GMT
I have the same problem as kugel with a custom iPod build based on r18125 and an iPod simulator based on r17906, both compiled by tdtooke with other patches. Auto update was taking as long as initializing did, so I turned it off and that seems to have worked.

If anyone has compiled an iPod build with only v6 of the sort tags patch, I'd be more than happy to do some testing. All the tracks on my iPod have ID3v2.4 Artist Sort and Album Sort tags populated, and track names beginning with "The " have ID3v2.4 Track Sort tags populated.
Comment by Thomas Martitz (kugel.) - Wednesday, 03 September 2008, 00:37 GMT
Update:

Fix database corruption/weirdness
Edit tagnavi.config to use sort tags by default (which is fine, since we fall back to the non-sort versions if the tags empty)
Comment by Thomas Martitz (kugel.) - Wednesday, 03 September 2008, 00:38 GMT
And here's the patch
Comment by Travis Tooke (tdtooke) - Friday, 05 September 2008, 02:22 GMT
I'm certainly no expert on tagnavi.config, but I think you modified it a little too much. As is with this patch if you start from the main menu and select artist, then album, you can't listen to the tracks in the order of their track numbers. Instead of the music icons next to the tracks you have folder icons and if you select a track it doesn't do anything.
Comment by Thomas Martitz (kugel.) - Saturday, 06 September 2008, 15:18 GMT
Ahh well, it seems that titlesort isn't working as espected.

It seems, titlesort works for displaying, but the translation into the file name at the end of the database browsing is messed.
Comment by Thomas Martitz (kugel.) - Sunday, 07 September 2008, 16:07 GMT
Try this one.

It seems that using titlesort in the "# Basic format declarations" alone does the job. I haven't tested it though.
Comment by Travis Tooke (tdtooke) - Sunday, 07 September 2008, 18:00 GMT
I'm pretty sure it'll work, that sounds about right. I will test it though :-) Appreciate the effort on this patch. It might be nice if we could *volunteer* somebody to host one of those one-patch feature test builds. I'd do it but I barely can manage updating my own build nowadays.
Comment by Thomas Martitz (kugel.) - Monday, 29 September 2008, 00:06 GMT
More tagnavi.config fixes. Should be all now.

Note: You don't need to recompile, since tagnavi.config isn't compiled. Just copy the tagnavi.config in the apps/ dir to your player to update.
Comment by Chris Kagan (KindOfBlues71) - Monday, 29 September 2008, 15:16 GMT
@kugel.- I was looking through the changes you made to tagnavi.config in v9 of this patch and there seems to be a typo in the A to Z track menu:

-"W" -> title ? title ^ "W" -> title = "fmt_title"
+"W" -> title ? title ^ "Z" -> title = "fmt_title"
Comment by Thomas Martitz (kugel.) - Monday, 29 September 2008, 16:36 GMT
Uhm, yea :(
Comment by James Zimmerman (Toxikator) - Saturday, 04 October 2008, 00:41 GMT
I'm not sure this has been said, but...

I know that Sortartist will revert to artist if no TSOA tag is present. Likewise, Sortalbumartist will revert to albumartist if no TXXX:ALBUMARTIST tag is present.

However, there really needs to be two levels to this... sortalbumartist must first revert to albumartist if there is no tag... then, if there is no albumartist tag, it should check for sortartist... then if there is no sortartist, it should check for artist.

ATM if browsing by albumartist with the sort tags, a huge amount of <untagged> files show up because I only tag albumartist if the track artist is different for that track. If I browse by REGULAR albumartist, this doesn't happen... which is probably how it ought to be.

Just a thought... and thanks for picking up work on this, I assumed it was a dead patch.

Tox
Comment by Thomas Martitz (kugel.) - Saturday, 04 October 2008, 00:54 GMT
If you only tag album artist if it's different to artist, then you're tagging wrong. Album artist and artist are two entirely different things to me: the one person/group creates the album and another person/group made this particular song. And if they happen to be the same, then hence both tags should contain the same.
At least in my opinion. I wouldn't revert albumartist to artist even without sorting tags.

But thanks for the notice. Either I didn't realize or I just have forgotten that I removed this "feature".

Can you be more precise on "browsing albumartist with and without sort tags". Do you mean without the entire patch or with a regular tagnavi.custom?

Nice to get some feedback though.
Comment by Thomas Martitz (kugel.) - Saturday, 04 October 2008, 01:01 GMT
Hmm...weird.

It seems that I didn't remove this (now I know why I can't remember!).

if (has_albumartist)
{
offset += add_tag(offset, &entry, tag_albumartist, &id3.albumartist);
}
else
{
offset += add_tag(offset, &entry, tag_albumartist, &id3.artist);
}


^This is the code, which implements the fall back to artist if there was no album artist. I'm a bit confused now. I'll have a look as soon I have time.

Comment by James Zimmerman (Toxikator) - Thursday, 16 October 2008, 14:23 GMT
Let me preface by saying: I have not yet tagged any of my tracks with sorttags, and will likely not until this patch is fully working... But,with the patch applied, if I set tagnavi to sort by genre > albumartist > album > track, Albumartist will automatically be replaced with "artist" when no albumartist is available. However, if I set tagnavi to sort by genre > sortalbumartist > sortalbum > track, then sortalbumartist will find no SAA tag and grab the regular albumartist tag instead. HOWEVER, if there is no regular albumartist tag, it leaves it as <untagged> rather than pulling down the sortartist tag (and not finding it, pulling down the artist tag).

I guess what I'm saying is that one should be able to change the tagnavi from their current settings to the sort-tag equivalents and see no difference if the sort tags aren't populated.

PS I'm a tagging pragmatist, I don't put tags where I don't need them and I don't need them when there aren't any featured artists on an album.
Comment by Thomas Martitz (kugel.) - Thursday, 16 October 2008, 14:35 GMT
Well, I repeat. I didn't change it intentionally and I have no real idea why this isn't working anymore. Besides I haven't much time for Rockbox coding at this time (and if, I concentrate on the sansav2 port).

Feel free to fix this.

PS: Having both artist and albumartist tagged (even if they're the same) doesn't hurt at all. It rather gives you more compatibility. Every fall-back from albumartist to artist is a specific implementation of the media player and NOT standard-conform.
Comment by Thomas Martitz (kugel.) - Tuesday, 09 December 2008, 11:48 GMT
sync
Comment by Thomas Martitz (kugel.) - Tuesday, 09 December 2008, 22:09 GMT
This one actually works, and should fix James' issue.

I wonder if it has a commit chance?
Comment by Dan Everton (safetydan) - Tuesday, 09 December 2008, 22:18 GMT
I haven't been keeping up with the state of this patch. Does it still show the sort tag value in the database screen or the original value, e.g. Beatles instead of The Beatles?
Comment by Thomas Martitz (kugel.) - Tuesday, 09 December 2008, 22:22 GMT
It shows the sort tag value in the tagtree (i.e. database browser), but only if you tagged it properly.
Comment by Jonas Häggqvist (rasher) - Tuesday, 09 December 2008, 22:28 GMT
I very much agree with this comment: http://www.rockbox.org/tracker/task/7287#comment15750

The sort tags should _only_ be used internally by Rockbox when sorting the DB - never displayed (except perhaps on the metadata screen).
Comment by Thomas Martitz (kugel.) - Wednesday, 10 December 2008, 00:19 GMT
I tend to agree too.

But looking at the code, it'll be a pain to implement. Due to the way it works, the tagtree sorts and shows exactly that what he gets by reading the tagnavi file. So if he reads the tag artist in the format, he'll lookup for artist and use this for sorting and displaying.
Comment by Jonas Häggqvist (rasher) - Friday, 12 December 2008, 17:30 GMT
I don't think an implementation that displays the sortorder tags should be accepted. They're there for the exact reason that the displayed name doesn't match how it should be sorted, so displaying the sortorder tag would completely run counter to why it's there. You might as well just change the artist tag, then.
Comment by Thomas Martitz (kugel.) - Friday, 12 December 2008, 17:40 GMT
With the difference that the WPS still shows the artist etc tags.

Just for the reference: http://www.rockbox.org/irc/log-20081212#01:21:54 Here I explain the problems I see with implementing that.
Comment by Brian Sutherland (rmaniac) - Tuesday, 03 March 2009, 23:19 GMT
Ok, so back over to this patch since I seem to have my 32k+ song issue working pretty well.... I imported my library with itunes to see how the pod and rockbox handles the large number of songs.. Something interesting which might have to do with the sort patch fall through. If I go to "Artists" I get "Weird Al" Yankovic as my first artists... under "album artists" i get:
"We
"Wei
"Weird Al" Yankovic

each one carrying a diff selection of albums (some duplicate each other).
None of the "Al" has album artist in itunes. It was all build from flac using the same script. All of the artist names are identical. This could be something completely different. Just thought I would toss it out here.
Comment by Brian Sutherland (rmaniac) - Wednesday, 04 March 2009, 00:29 GMT
Sorry! Skip that last comment. It is not related to this patch but rockbox-3.1 I'll do further testing a report a bug if needed.
Comment by Brian Sutherland (rmaniac) - Friday, 06 March 2009, 15:51 GMT
Has there been any further discussion of a commit for this? I would love to see it in svn.
Comment by Thomas Martitz (kugel.) - Friday, 06 March 2009, 16:14 GMT
There has been a discussion.

The consensus though is to not use the sorting tags as seperate tags (and not showing those), but only sort by those in the database browser. Of course, that makes absolutely sense.

But IMO, it adds much complexity into code, which already nobody (except Slasheri) is really able to figure out. And it would add a good deal to RAM usage if we cached the sorting tags too besides the normal tags. Afterall, I'm not really convinced of doing it the consensus way, so I basically post-poned work on this one.
Comment by Brian Sutherland (rmaniac) - Tuesday, 24 March 2009, 23:45 GMT
Synced to 3.2.
Is there any chance we could get this patch worked on for summer of code?

Comment by John Millikin (jmillikin) - Monday, 20 April 2009, 02:45 GMT
This patch shows the original name in the database, but sorts based on the sortname.
Comment by Thomas Martitz (kugel.) - Monday, 20 April 2009, 10:37 GMT
Uhm...NICE!

Gonna take a look at it, for sure :)
Comment by Thomas Martitz (kugel.) - Monday, 20 April 2009, 12:45 GMT
Am I right that both the artist and sortartist .tcd files need to be in RAM if browsing artist? That's probably duplicated in most cases.

Would it be possible to detect at database initialisation that those tags are not used at all? Or only create the tcd entries if/when the sort tag is different from the artist? To save RAM, you know.

Other than that, good job!
Comment by Thomas Martitz (kugel.) - Monday, 20 April 2009, 13:54 GMT
Dataabort on my e200.
Comment by John Millikin (jmillikin) - Monday, 20 April 2009, 15:28 GMT
> Am I right that both the artist and sortartist .tcd files need to be in RAM if browsing artist? That's probably duplicated in most cases.

I don't think so -- the sort* tags are (should be) used only when generating the indexes for the primary fields, and are not otherwise stored.

The only device I've tested this on is an iPod video. I will build the e200 sim tonight, and see if I can reproduce your error.
Comment by James Zimmerman (Toxikator) - Monday, 20 April 2009, 15:28 GMT
:D

Big dumb grin on my end, I'm totally stoked that this is finally in the works for real! Exceptional, over-the-top props to jmillikin, way to go.
I'm dumb as hell, so I can't do it myself... but I'd be eternally grateful if someone could do a quicke patch of this to the current build for the iPod video 60/80GB so I can test it!

_jp
Comment by Thomas Martitz (kugel.) - Monday, 20 April 2009, 15:32 GMT
John, the sort* tags are stored in tagcache_X.tcd, where ix is 9...12. And loaded into RAM when you browse it.

ramcache tries to allocate 270K here, which happens to be ~2x the value of allocation without this patch. (that's the only info I can get using logf, before the data abort, which is in load_tagcache(), btw)

EDIT: You need Load to RAM enabled for the dataabort.
Comment by John Millikin (jmillikin) - Tuesday, 21 April 2009, 03:47 GMT
Here is an updated patch, which allows the sorting information to be used when updating the database.

> John, the sort* tags are stored in tagcache_X.tcd, where ix is 9...12. And loaded into RAM when you browse it.

Is there any way to avoid the RAM loading? With this new patch, it is necessary to keep the files around, but only for database updates. They don't need to be loaded during normal operation.

I still can't reproduce the e200 error you're seeing, in the sim. I've been using Rockbox for less than three days now, so I don't even know how to begin tracing a memory error.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 06:51 GMT
You probably cannot get the data abort in the sim. I can reproduce it reliably on my e200 when I have "Load to RAM" enabled (no data abort without this).
> "Here is an updated patch, which allows the sorting information to be used when updating the database."
?? Can you explain this please?

> "Is there any way to avoid the RAM loading? With this new patch, it is necessary to keep the files around, but only for database updates. They don't need to be loaded during normal operation."

No. It's needed to avoid spinning the disk every time your browse for music. That's why I'm asking whether it is possible to reduce the impact, e.g. by only collecting the sort data for tracks where it is different. Hmm, but on the other hand, saving 4 int's for each song for the IDs to get the data "dynamically" is probably worse.

Edit: It could be a setting too.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 11:40 GMT
>It's needed to avoid spinning the disk every time your browse for music. That's why I'm asking whether it is possible to reduce the impact,...
I don't think it matters much... though that may be player-dependent. Using older incarnations of this patch (before they actually SORTED, but when the tag was still read and the database was still populated with it) I still had all 12 database.tcd files created and in use, and I never noticed it running appreciably slower than now when it just runs 0-9 on the SVN.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 12:45 GMT
Earlier versions (and yours) too loaded 9...12 into RAM, which is why you didn't notice slowness. Hence the RAM usage is doubled, taking precious RAM for audio buffering away (which translates to reduced battery runtime).

I think loading 9...12 into RAM is ok, but we should provide a user setting. Users which know they don't use special sorting tags should not suffer from doubled RAM usage, imo.

I only have 1k songs, but RAM usage already raised from 140k to 270k. Imagine this with 10k songs, or 20k. The point is not to waste RAM if the tags aren't used, not being as fast as SVN.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 13:02 GMT
Ah, I see what you're driving at. Though let's be fair, rockbox supports a NUMBER of tags that I never use. Few people are likely using, for example, the comments field to do any kind of sorting in the dbase, should they be given an option to build w/o it to save RAM? I see your point that it's a major consumption factor, but... If you're saying RAM usage doubles using these tags (which seems weird to me; 3 new tags and you've doubled the size of the dbase files?) and your 1k songs take you to 270k RAM... then 20k songs is only 5.4MB of RAM... so an increase of 2.2MB. AFAIK the only player supported by Rockbox that's even CAPABLE of holding that many songs (at the same reasonable size I'm assuming you use) would be a modified iPod up to 120GB (maybe even the 240GB monster pod would be needed)... and that has 64MB of RAM. So you're talking about a reduction that, honestly, doesn't seem like much of a hit in the grand scheme of things, given that it leaves more than 60MB of RAM (and what is Rockbox loading into RAM at any given time besides the dbase? I don't know but I'd guess some text, a few .bmp images of very small size, and two songs (current, next).

I dunno, I'm not a coder, just a fan of the feature, so I'm talking out of my ass here, but I just don't see a small increase in dbase size warrants having to once AGAIN go into rewrite mode on this patch to save the non-TSOing users a bit of RAM usage.

_jp
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 13:25 GMT
Well, you with your 64MB ipod probably don't care. But what about the 16MB of iaudio x5? Don't ignore the other targets. Also, I claim that 2.2MB less RAM will also result in noticeably shorter runtime on the long run on your ipod too.
Rockbox isn't just ipod.

It doesn't double the entire DB size. But it doubles the amount of data loaded into RAM when you browse. The tagcache allocates memory as it needs at bootup, which is the maximum you can need at once (browsing by title). And the maximum is almost doubled (master index + track index w/o this patch, master index + track index + tracksort index with this patch).
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 13:38 GMT
>(master index + track index w/o this patch, master index + track index + tracksort index with this patch).
OOHHHH, I think I understand. It's essentially building the db twice... once for what it displays, and once for how it sorts?

>Well, you with your 64MB ipod probably don't care. But what about the 16MB of iaudio x5
Yes, but then while it has only 1/4th of the RAM, it's also got only 1/4th of the HD space, as well (relative to the 120GB HD iPod I talked about above). That 2.2MB increase for 20,000 songs doesn't apply when 20,000 songs aren't going to fit on the player. Now you're talking about what, 5,000 songs (assuming we're comparing your 6GB e200 to a 30GB iAudio X5... I don't know for sure which ones we're talking about)? So that's only like a 500k increase in RAM consumption... 500k to a 16MB system shouldn't be much more of a hit than 2.5MB to a 64MB system, should it?

>Also, I claim that 2.2MB less RAM will also result in noticeably shorter runtime on the long run on your ipod too
Perhaps, but then isn't that how features work? Sooner or later you're going to HAVE to increase the demands on the system if you make Rockbox do more things. I don't particularly care about viewports either, I was fine with my WPS without them, but I'm betting switching rockbox to viewport-based WPSs has increased RAM usage as well. Such is the price... (Rockbox makes up for it, IMO, by adding battery optimizations, a new sleep mode feature, etc. So while some new features decrease runtime, others increase it, and it's a push in the end.)
Comment by Brian Sutherland (rmaniac) - Tuesday, 21 April 2009, 13:43 GMT
> Yes, but then while it has only 1/4th of the RAM, it's also got only 1/4th of the HD space, as well (relative to the 120GB HD iPod I talked about above)

Don't make the mistake of thinking the iPod is the only player that can have its drive replaced. Comparing an upgraded iPod (no stock 120gb iPod works with rockbox) to a stock x5 is hardly fair.

I think what we need is for rockbox only to load two tags when the sort tag is actually different from the regular tag and have it default to the regular tag otherwise. Somehow...
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 13:54 GMT
>Don't make the mistake of thinking the iPod is the only player that can have its drive replaced. Comparing an upgraded iPod (no stock 120gb iPod works with rockbox) to a stock x5 is hardly fair.
Okay, I can accept that. But then, you're also asking me to believe that this implementation will greatly reduce battery life on an x5 but not on an iPod... but that WITHOUT this patch, the x5's 16MB of RAM will do it just fine with a 120GB HD compared to an iPod. I hardly think it's fair to expect a 16MB system to perform as well as a 64MB system - the reality of the situation is, if you put a 120GB HD in your x5, and fill it up, you've quadrupled the size of the dbase file already.

>I think what we need is for rockbox only to load two tags when the sort tag is actually different from the regular tag and have it default to the regular tag otherwise. Somehow...
as kugel said, though, in order to do this you'd probably do more damage than you repaired by virtue of the resources required to MARK each track as "uses Sort*" or "doesn't use Sort*"

I personally think the ideal way to do it would be to only build the dbase using tags that tagcache is told to look for; so if you don't tell it to USE Sort* in your dbase tree, it won't populate the dbase with those tags to begin with. Not sure if this is doable under the current architecture, though, as I believe earlier versions of this patch built all 12 .tcd files despite none of my tags having Sort* info on them, and the tagcache never being told to use it.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 14:08 GMT
>the ideal way to do it would be to only build the dbase using tags that tagcache is told to look for; so if you don't tell it to USE Sort* in your dbase tree, it won't populate the dbase with those tags to begin with
Don't know how to edit a comment but...

What I'm assuming, here is that the way this patch is set to work, when you build the dbase with the tagnavi tree saying:
artist -> album -> title = "fmt_title"
It's separately creating two indexes; one for display (which is built as above), and one for sorting, which is populated with the sortartist, sortalbum, and sorttitle variants of these tags...?

I think instead, the idea should be that if you enter
artist -> album -> title = "fmt_title"
the sort tags get left alone, whereas if you enter
sortartist -> sortalbum -> sorttitle = "fmt_title"
THEN the sort tags are used... though obviously, displaying the usual "artist -> album -> title" tree.

The question remains, though, whether the dbase is configured to work in such a way that it knows which tags it needs to load and which it doesn't, or whether it just loads every tag Rockbox spec's as "usable" for tagnavi. If it's the latter, then stuck. If it's the former, switching the usage of the tags to only apply when their "sort" counterparts are entered should nicely solve the problem..?
Comment by John Millikin (jmillikin) - Tuesday, 21 April 2009, 15:36 GMT
>> "Here is an updated patch, which allows the sorting information to be used when updating the database."
>?? Can you explain this please?

The previous patch would only work when the database was initially generated. If tracks were then added to the device, and the database updated, the tracks would not be properly sorted.

>> "Is there any way to avoid the RAM loading? With this new patch, it is necessary to keep the files around, but only for database updates. They don't need to be loaded during normal operation."
>No. It's needed to avoid spinning the disk every time your browse for music. That's why I'm asking whether it is possible to reduce the impact, e.g. by only collecting the sort data for tracks where it is
> different. Hmm, but on the other hand, saving 4 int's for each song for the IDs to get the data "dynamically" is probably worse.

The sorting tags, if I understand correctly, are not used when browsing for music. When the index files are generated, they seem to pre-sort all tags and then cache the sorting order. I modified this process to use different strings for sorting, but the sort tags themselves are not used after that.

Hence, my question about avoiding loading to RAM. The sort tags are only needed when generating the index. They don't have to be loaded all the time.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 15:49 GMT
well if the sorttags are only being used to generate the index, and aren't actually being called, then after the index is generated, can't database9...12.tcd just be deleted after the fact to prevent them from being loaded into RAM on startup..?
Comment by John Millikin (jmillikin) - Tuesday, 21 April 2009, 15:57 GMT
If you delete the cached sort tags, they can't be used for re-sorting on database updates.

For example, you start with the following sorted database:

Abba
The Beatles

and then add "Daft Punk". If you don't keep around sorting information for "The Beatles", your database will now be sorted as:

Abba
Daft Punk
The Beatles
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 16:35 GMT
That's because they're used to sort at browse time. The .tcd files are a cache.
Comment by John Millikin (jmillikin) - Tuesday, 21 April 2009, 16:45 GMT
Perhaps I'm misunderstanding how the tagcache works, but based on experimentation, the code goes something like this:

1. Extract tags to temporary index file
2. (reboot)
3. Generate tag caches from index file
4. delete index file

5. (add new tracks)
6. extract tags to temporary index file
7. (reboot)
8. Load previous tag caches
9. Generate tag caches from index file
10. Write new tag caches
11. delete index file

Specifically, when I add logging to the comparison function (which is the only place where sort/not sort matters), it's *only* called when updating the caches, which apparently happens on rebooting after updating the database. Furthermore, the cached sort tags need to be available when updating, because otherwise already-populated tags will be sorted incorrectly. So the sort-tag caches *have* to be present when updating, but are not used during normal browsing. If there's any way to prevent the sort-tag caches from being loaded into RAM -- or perhaps to unload them after updating -- I do not think it would break the sorting behavior.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 17:07 GMT
Well, you have a point there. But why is the RAM usage higher then (at least with Load to RAM)?
Comment by John Millikin (jmillikin) - Tuesday, 21 April 2009, 17:18 GMT
> Well, you have a point there. But why is the RAM usage higher then (at least with Load to RAM)?

I assume the sort-tags caches are being loaded, despite being unneeded. Hence my question, "is there any way to avoid the RAM loading?".
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 17:21 GMT
It seems you actually found some brilliant way to do it :) Right, we should investigate why it's affecting the RAM then. I assume they're loaded (hence I also assumed they're actually used at browsing-time) necessarily.

On my e200 (64bit) sim, the RAM usage is higher too, btw, but not by the doubled amount.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 17:22 GMT
Edit: We must not forget auto-update though, with this, the sorting tag files probably want to be in RAM too.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 17:29 GMT
Ok, I appoligize. The RAM issue is only given if "Load to RAM" is active. This will cause _all_ tag files to be loaded into RAM. Without this, only the one you're browsing is loaded (maybe even only partly too, not sure right now). It would still be nice to kick the sorting ones out if they're not actually used.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 17:35 GMT
This should do it, then. It skips the sorting tags in load_tagcache(). Not really tested.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 19:57 GMT
well if the sorttags are only being used to generate the index, and aren't actually being called, then after the index is generated, can't database9...12.tcd just be deleted after the fact to prevent them from being loaded into RAM on startup..?
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 19:58 GMT
oop, doublepost, sorry... browser refresh function, you know

Will test kugel's patch presently.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 19:59 GMT
The patch is worthless, since the static allocation didn't change. The tag files cannot be deleted since we need them for updating.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 20:24 GMT
This fixes the allocation too. The only added RAM usage is the entries in the master index (offset which points to the data in the sorting tag files), which - I guess - is needed for updating.

Please test it, I think about committing it.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 20:47 GMT
Am presently compiling a build w/ v16, will report results once I can test it...

Interesting conceptual problem, though, that I'd rather someone who understands the patch solve that I would attempt to by trial and error.

If I've got Artist: The Prodigy, and I add Sortartist: Prodigy, then The Prodigy should show up amongst the "Ps".
Now, suppose for one track by The Prodigy, it's Artist:The Prodigy feat. Pop Will Eat Itself. As a result, I've got that track tagged with AlbumArtist: The Prodigy.. so it all shows up A-OK, no problems.

But how do I sorttag that? Should I be using Sortartist for all of the tracks WITHOUT an albumartist tag, and Sortalbumartist for all of the tracks WITH an albumartist tag? Or, since tagnavi is being told to look at the AlbumArtist tag (and only displaying the Artist tag if the AlbumArtist tag isn't populated), should I just use the Sortalbumartist field for all in question? the question being: does the sort tag apply to whichever tag is specifically *displayed*, or does it apply to the tag that tagnavi is being told to *check*?

_jp

PS thx again for finally making this happen, SO stoked. :P
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 21:08 GMT
Depends if you like to browse artist or albumartist more. id3v2.4 has no support for albumartistsort (hell, it doesn't even officially have album artist), but we're falling back to artistsort. So, for your issue, you have to decide between artist and albumartist and use artistsort accordingly.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 21:14 GMT
..? so what you're saying is, regardless of whether the albumartist or artist tag is populated, it will sort by artistsort? That makes sense. Will test now.

PS the tag is "artistsort", not "sortartist"..? I can never remember whether it's artistsort, sortartist, or TSOA... all seem to be used at some point or another.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 21:16 GMT
sortartist or artistsort....whatever. That's just how we call it. Mp3tag says ARTISTSORTORDER, other people might say beefy. In fact, only TSOA counts (for id3, at least).
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 21:28 GMT
James, it seems the patch looks in custom tags for "ALBUMARTISTSORT", maybe you can figure it out (or someone else).
Comment by John Millikin (jmillikin) - Tuesday, 21 April 2009, 21:35 GMT
The Vorbis comment style is to append "sort" to whatever the normal field is. So artist -> artistsort, title -> titlesort, etc. Most custom Id3 taggers seem to follow this convention.

The fallback order is albumartistsort -> artistsort -> albumartist -> artist.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 21:36 GMT
>In fact, only TSOA counts (for id3, at least).
So the actual names of the fields that the value needs to exist in are TSOA, TSOP, and TSOT?

Also, I don't know if you're right about only TSOA counting, as apparently ARTISTSORT is the preferred name for vorbis...?
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 21:37 GMT
Read what I wrote "for id3, at least"...
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 21:39 GMT
quick Q as this is driving me mad: does tagnavi need to be told to look at "artistsort" to find these tags, or can I just leave "artist" and the patch takes care of it?
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 21:40 GMT
No, just leave tagnavi untouched. That's the whole point why my earlier patches weren't good enough.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 21:44 GMT
>Read what I wrote "for id3, at least"...

Ya, sry, by bad. though doesn't rockbox need to look at both ANYWAY, considering it doesn't just use id3 tags to populate the dbase..?
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 21:47 GMT
Our supported audio formats use different tags formats. Like FLAC and Vorbis use Vorbis comments (and no id3), AAC uses his own, and mp3 (this list wasn't complete). It's not only about id3.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 21:51 GMT
>No, just leave tagnavi untouched. That's the whole point why my earlier patches weren't good enough.

Okay, well I have a bug to report then: the patch doesn't seem to DO anything.
Now, tbf this was my first actual patch-and-compile of rockbox source, so it's possible the patch didn't take for some reason and I missed it, but I doubt it, as I'm seeing the artefacts of the patch itself (e.g. showing 13/9 committed b/c the extra 4 .tcd files are being written). No crashes, no errors... but nothing is getting sorted.

TSOA tags are populated, dbase reinitialzed, and tagnavi set to browse by both artist and albumartist, but still nothing. "The Prodigy" is still showing up amongst the "Ts".
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 21:52 GMT
*headsmack* damnation and craptastrophy, I'm retarded.

"TSOA" isnt' "Sort ARTST", it's "Sort ALBUM"! I want TSOP, as in "P"erformer!

d'oh! Excuse my stupidity, kind messrs.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 22:05 GMT
Okay, no, still a problem.

Now the TSOP tags are properly populated (rather than TSOA) but still no sorting taking place, when browsing by either Artist OR Albumartist.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 22:22 GMT
How do you mean that they're properly populated? They shouldn't be "populated" at all.

It works for me.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 22:26 GMT
What I mean by "populated" is that I've actually entered data into them, so rockbox will have something to see (and it's the right tag, this time).
TSOP = Prodigy, Artist = The Prodigy, and the dbase still puts The Prodigy in with the "T"s.
I've tried browsing by artist vs. albumartist, getting rid of conditionals (I have a ? genre != "Spoken Word' conditional in my tagnavi) but neither works. :(

Will continue to test, let you know if I figure out the issue here...
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 22:29 GMT
What file are you trying (format, id3 version maybe)? What tagger do you use? Did you try deleting the .tcd files in the .rockbox dir before re-initializing? Also, can you try without custom tagnavi (to get rid of posible error sources).

On my MP3s with MP3Tag, it works. I used ARTIST-/ALBUM- and TITLESORTORDER.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 22:35 GMT
>What file are you trying (format, id3 version maybe)?
It's id3v2, that's all the properties dialog says.

>What tagger do you use?
foobar2k

>Did you try deleting the .tcd files in the .rockbox dir before re-initializing?
I always do this when initializing (so yes)

>Also, can you try without custom tagnavi (to get rid of posible error sources)
Yes but it may take a minute, I don't have the original tagnavi on hand, will have to redl rockbox...
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 22:37 GMT
>ARTIST-/ALBUM- and TITLESORTORDER

can you verify that these are being written to the TSOP, TSOA, and TSOT fields?
Foobar doesn't default to include sortfields, you have to specify it... I've written the tags to "Sortartist", "artistsort", and "TSOP" (and "TSOA"), but so far it's yielded nothing. I may have the id3 name for the field wrong...
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 22:37 GMT
I don't know whether foobar supports id3v2.4. TSO* is only in this id3 version. Earlier (like the common v2.3) don't have this and need custom tags (XSO*). I don't know how to do it though.

Try mp3tag, it definitely supports v2.4
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 22:51 GMT
foobar SAYS it's writing id3 v2.4, and I'm inclined to believe it... also, using default tagnavi, no change in behaviour.
Have now tried with The Medic Droid (/Medic Droid) as well, to verify that it wasn't an albumartist disagreement (there are no albumartist tags in my The Medic Droid files), same result. Simply refuses to work on my end.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 23:03 GMT
What if foobar doesn't know about this tags? Why aren't you just trying another tagger?
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 23:10 GMT
I did try another tagger. Loading the files into Mp3tag shows that they indeed have id3v2.4 tags written to them. What it DOESN'T show is what you described, which is "ARTISTSORTORDER" tags. It shows a properly written "TSOP" tag (you assured me TSO* was the proper format for sort tags), but no ARTISTSORTORDER tag.

I'm attempting to rewrite the files with the (hopefully) appropriate field names, though frankly this patch ought to know to look for TSOP and ARTISTSORT in addition to the nonstandard ARTISTSORTORDER if that is indeed the discrepancy...
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 23:18 GMT
Different tagger use different names for the TSO* fields. mp3tag uses ARTISTSORTORDER. This is what you can enter in the advanced tagging window/dialog. This is what definitely works (I tagged my files like this, and it works).
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 23:26 GMT
>Different tagger use different names for the TSO* fields
I'm telling you, this is untrue.

If it WAS true, then my "TSOP" field should have shown up with the LABEL "ARTISTSORTORDER" rather than simply "TSOP". While you may think MP3tag is using the term "Artistsortorder" as a placeholder name for the TSOP field, it's actually writing a tag *called* ArtistSortOrder. Verified this by loading the mp3s into foobar, telling it to write the sort tag with the field name "ArtistSortOrder", and loaded the files back into Mp3tag... where they showed up with Mp3tag's "ArtistSortOrder" field already filled in.

Of course, it's a moot point as this STILL hasn't fixed the problem, the Mp3tag-verified ArtistSortOrder id3v2.4 files are STILL being sorted improperly.
Comment by Thomas Martitz (kugel.) - Tuesday, 21 April 2009, 23:29 GMT
Should I show you screen shots? This it what works for me. But true, I haven't verified that it's writing TSOP. Nevertheless, Rockbox reads it.
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 23:34 GMT
A breakthrough! Though The Prodigy was NOT being properly sorted (and that's the one I've been trying to fix), I noticed by accident that The Ting Tings, which I recently d/led, were showing up next to the "ti..."s, not the "th..."s. Loading these files into MP3tag shows their Artistsortorder tags populated properly as "Ting Tings, The"... but loading them into foobar shows nothing where the ArtistSortOrder tag was!

So what seems to have happened is when foobar wrote a tag called ArtistSortOrder, it wasn't in fact writing the correct tag... though when MP3tag LOADED the files foobar had written, it still showed an ARTISTSORTORDER tag being populated, simply because both had the same name.

This is particularly strange, but you were of course correct... foobar, though it does indeed verifiably write id3v2.4 tags, doesn't seem to understand the SORT tags. What's even STRANGER is that foobar can see all manner of nonstandard tags (if it doesn't recognize a tag it simply shows <FIELDNAME> for the tag) but for some reason these aren't showing up at all.

:S I need to go talk to HydrogenAudio, foobar is not keeping to spec like it should. Erstwhile, I believe we finally have liftoff

_jp
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 23:52 GMT
Okay, it would appear the issue was an out-of-date foobar installation, I updated and now the ArtistSortOrder tags are properly dealt with. Chalk that up to carelessness on my part.

So now that my own idiocy has been roundly compensated for, it's time to deal with the real issues:

1) the "fallback" system seems to fail, or is at least implemented wrong. ArtistSortOrder works properly if browsing by Artist, but when browsing by albumartist it does not.
I was under the impression that the sortorder system for Albumartist went: AlbumArtistSortOrder > ArtistSortOrder > AlbumArtist > Artist... apparently, if browsing by AlbumArtist the SortArtist tag isn't even checked.

If this is intentional, that's fine (I mean, if browsing by AlbumArtist, I suppose technically it makes SENSE to have to check AlbumArtistSort), but it seems like a less-than-ideal approach when compared to checking for the four in-order specified above... especially as jmillik has said this is SUPPOSED to be the fallback order (though it's clearly not implemented).
Comment by James Zimmerman (Toxikator) - Tuesday, 21 April 2009, 23:54 GMT
oh and 2) (though it's not really important), it seems odd that, upon initialization of the dbase, you get a count-up to 9/9... then keep going to 13/9. Minor thing, but...
Comment by Brian Sutherland (rmaniac) - Wednesday, 22 April 2009, 00:09 GMT
Hey kugel.,
I took a stock 3.2 source, added the v16 patch and compiled. I had it build the db up against all my oggs. All these oggs have been used with the v12 patch and worked fine in the album area showing the sort tags. With this new patch and the same files I no longer see "year album" (which is my albumsort) but instead see album and it is in alphabetical order. I am currently running this up against my mp3s with have id3v2.4 and have the sort tag set on them also for album that works fine in itunes. I will let you know how that goes. In the meantime could there be something wrong with the way it is handling oggs? or is it handling them different than v12 and previous. Also I get the db init issue that says 1/9 and ends with 13/9. That may be the result of your change to not load it into ram, it would not bother me except for the albums not sorting.
Comment by Brian Sutherland (rmaniac) - Wednesday, 22 April 2009, 00:09 GMT
Sorry I forgot to mention. I did a complete db rebuild once I updated rockbox. I wanted to make sure it was not just something silly like that.
Comment by Brian Sutherland (rmaniac) - Wednesday, 22 April 2009, 00:27 GMT
I just tried with Mp3s. I am getting the same issue. Not seeing them sorted. My Mp3s have for example:

(TSOA): 1995 Alice in Chains

or on the Oggs:

albumsort: 1995 Alice in Chains

So at least on album sort there seems to be something weird.



Comment by James Zimmerman (Toxikator) - Wednesday, 22 April 2009, 00:39 GMT
But you're having success with ArtistSort? What program are you using to tag (because, as I just found out, the Sort tags actually AREN'T a part of the id3 v2.4 spec, they're iTunes specific, and written to a different frame than the rest of the tags; as a result, not all apps can write them properly.
Comment by Thomas Martitz (kugel.) - Wednesday, 22 April 2009, 00:39 GMT
Can you please read what changed with the most recent patches? I'm getting tired....
Comment by Thomas Martitz (kugel.) - Wednesday, 22 April 2009, 00:41 GMT
James: TSO* ARE official! http://www.id3.org/id3v2.4.0-frames (starting at 4.2.5). Please don't propagate the untruth.
Comment by James Zimmerman (Toxikator) - Wednesday, 22 April 2009, 00:45 GMT
Okay, so not only does the patch not use ArtistSortOrder for sorting when browsing by AlbumArtist...
but it ALSO doesn't use BandSortOrder, even when written from Mp3Tag or iTunes.

I assume this patch only adds support for the main three; if that's the case, I think the patch needs to be updated to use ArtistSortOrder for sorting AlbumArtist entries in addition to Artist entries.
Comment by Brian Sutherland (rmaniac) - Wednesday, 22 April 2009, 00:53 GMT
James, you must be calm I think your turning this into a forum your posting to every couple seconds is getting a bit much. I downloaded the patch first thing but spent a few hours testing and playing with it before just filling this comment section full of back and forth. I don't mean to be rude but I think you might be making Thomas regret even trying to fix this.

Thomas, Sorry am I missing something? I have been reading everything but the signal to noise has been a little rough. I don't know if you are just asking me to reread all the comments, the source or explain something better.

Thanks
Comment by James Zimmerman (Toxikator) - Wednesday, 22 April 2009, 00:53 GMT
>TSO* ARE official! http://www.id3.org/id3v2.4.0-frames (starting at 4.2.5). Please don't propagate the untruth
Oh, I guess I have some folk over at the foobar2k support forums to tell off (they seem convinced they're not yet standard).

Anyway, reading the patch I see that ALBUMARTISTSORT is the nonstandard tag being checked for use as the sort order for albumartist..? Both iTunes and Mp3tag are wanting to write "Bandsortorder" instead. No difference to me, but I'm sure this will be an area of much confusion down the road if this gets sync'd.

_jp
Comment by John Millikin (jmillikin) - Wednesday, 22 April 2009, 00:55 GMT
Version 16 does not work properly for me -- artists added on a database update are not properly sorted.

I also suggest, to anybody interested in testing this patch, that you either use Ogg/Vorbis files, or a well-tested tagger like MusicBrainz Picard. Trying to track down every bizarre custom tag from broken taggers will mean this patch will never be accepted.
Comment by James Zimmerman (Toxikator) - Wednesday, 22 April 2009, 01:09 GMT
Alright, well I've been told to f*** off, so I will. :P
but I will leave this saying that neither BandSortOrder (the "normal" tag for this, it would seem) NOR AlbumArtistSort (what the patch seems to say should be used, given the absence of a proper standard) are recognized. That being the case I think the patch needs to be updated so that TSOP sorts when browsing by AlbumArtist as well as by Artist, otherwise there's no way to use this patch in conjunction with browsing by AlbumArtist at all.
Comment by John Millikin (jmillikin) - Thursday, 23 April 2009, 03:57 GMT
Updated to update correctly, and I believe it avoids loading sorting tags into RAM. I can't figure out how to see the current RAM use, so please tell me if it works or not.
Comment by Brian Sutherland (rmaniac) - Thursday, 23 April 2009, 05:50 GMT
I tried this up against the Oggs and Mp3s again on the Sim and the hardware. I applied the v17 patch to a stock 3.2 setup. Again I get the 13/9 thing and my albums do not sort. Does this possibly effect only artists? I don't really have any artist sort tags so its hard to tell. I tried setting one artist tag and did an update on the mp3s but nothing seemed to happen.
Is it possible this patch needs something from SVN and me sticking to the "stable" 3.2 is causing issues?

Also you can find the RAM usage under System->Debug (Keep Out)->View database info


Comment by Thomas Martitz (kugel.) - Thursday, 23 April 2009, 09:57 GMT
The latest patch seems to raise the allocated RAM again. The loading is fine, but it allocates too much (I have ~153k/230k, it should be ~153k/186k)

Brian, this patch displays the normal artist etc tags, but sorts by the sorting tags.
Comment by Brian Sutherland (rmaniac) - Thursday, 23 April 2009, 15:38 GMT
All of my album sort tags say "YEAR ALBUM" when I used the older patch that is what was displayed and they were in order by year. Now it only displays "ALBUM" and does it alphabetically. Those should be sorted by year with this patch, correct?
Comment by James Zimmerman (Toxikator) - Thursday, 23 April 2009, 15:57 GMT
>Now it only displays "ALBUM" and does it alphabetically. Those should be sorted by year with this patch, correct?
Yes, they should... what field have you written the sort tags to, and with what program? I went through this same thing with the artist sort tags, turns out they were being written wrong.

Also, you've changed your tagnavi so it is no longer being told to DISPLAY the sortalbum field (as with earlier versions of this patch), but is instead being to display the normal Album field..?
Comment by Brian Sutherland (rmaniac) - Thursday, 23 April 2009, 16:01 GMT
All listed in previous comments and has not changes:
(TSOA): 1995 Alice in Chains

or on the Oggs:

albumsort: 1995 Alice in Chains

I used mutagen in a python script to set it all up. The mp3 tags v 2.4. Also there is no older tagnavi because I applied the patch to a fresh copy of 3.2. If it was set to the old sort tags one it would show everything as blank since those tags are not loaded anymore. I get my albums. There is just no sort.
Comment by Brian Sutherland (rmaniac) - Friday, 24 April 2009, 01:27 GMT
I tried with the latest SVN, nothing changed. Again, all these files worked fine with the old sort patch when you could see the sort tag. They sorted fine. Also everything I am looking at is "year album" so could there be issues with album sorting or number sorting in this? Did something change in the patch that would invalidate the tags I was using that worked just find with the older patch? They were sorting great, then this new patch get tried and it all breaks. So if it is something with my tags it has to do with either a tag change in the new patch or an issue with album sorting because the files are all the same and used to work.
Comment by Thomas Martitz (kugel.) - Friday, 24 April 2009, 01:37 GMT
The tag parsing didn't change IIUC, i.e. is the same as in the older patches. Just the displaying part is supposed to be changed. Though, maybe John can tell you for sure.
Comment by John Millikin (jmillikin) - Friday, 24 April 2009, 01:41 GMT
I didn't change tag parsing at all. Have you tried it on a simulator, or a fresh database?
Comment by Brian Sutherland (rmaniac) - Friday, 24 April 2009, 01:54 GMT
Yeah I tried it on virgin 3.2 and SVN under the sim and the ipod hardware I used my Oggs which I know worked and tried my Mp3s which I am pretty sure used to work. I can't imagine what the deal would be since it seems to work for both of you unless there was something left over from the old patch that is working for you. I assume though that you have both tried it on clean sources though and since my tags used to work I find it kinda confusing what could have changed here.
Comment by James Zimmerman (Toxikator) - Friday, 24 April 2009, 03:03 GMT
I can verify that I patched a fresh build, yes. However, I only tested the TSOP tags, I never got far enough to do a proper test of TSOA (as I gave up once I realized TSO2 wasn't working). Does TSOP work on your end, or are ALL the sort tags nonfunctional?
Comment by Brian Sutherland (rmaniac) - Friday, 24 April 2009, 20:23 GMT
Ok Kiddos. I added some artistsort tags and those are working fine. It seems to just be the album sort that is failing. Again this used to work on the old patch. Here is what I think the deal is. When I just go to the great bit album list they are in order by year (or so it seems). When I look at the artists they are in order. BUT when I go to an artist and then list the albums under that artist the go alphabetical. It looks like the index is only being used to sort when looking at all albums not an artists albums. Maybe there is a sort command happening during that listing or it is skipping the index for some reason. Either way I think expected behavior would be for the albums to sort under that artist name as well.
Comment by James Zimmerman (Toxikator) - Friday, 24 April 2009, 21:01 GMT
Verifying Brian's problem on my end, and not TSOA specific. When browsing by genre first and then artist, sortartist breaks. It seems sort tags only like to be applied to the first level of any particular tagnavi browsing system...

Also, a note on albumartistsort:
It seems that, when browsing by albumartist, whichever value is DISPLAYED is the one that gets sorted by. So if there is no albumartist tag, and rockbox falls back to the artist tag, it will sort those tracks by Sortartist. If there IS an albumartist tag, it will sort by SortAlbumArtist... there is no sort fallback, though (if no sortalbumartist tag is present, it sorts by albumartist, rather than by sortartist... not sure if this is the intended behaviour).
Comment by James Zimmerman (Toxikator) - Friday, 24 April 2009, 21:16 GMT
in addition to tagnavi tree levels breaking sort tags, it seems a conditional at the first level (e.g. artist ? genre != spoken word) will ALSO break sorting...
Comment by John Millikin (jmillikin) - Sunday, 26 April 2009, 02:02 GMT
Here's another attempt to reduce the RAM cache size -- I think it's correct.

Also fixes the 13/9 problem.

-----------------

Sorting in the tagtree appears to be, for some reason, completely separate from sorting in the tagcache. I can't figure out how to re-use the tagcache indexes for tagtree sorting. I believe that with the current split sorting system, the only way to achieve correct sorting in user-specified sorting systems would require keeping sorting tags in RAM.
Comment by Thomas Martitz (kugel.) - Sunday, 26 April 2009, 02:04 GMT
>> I believe that with the current split sorting system, the only way to achieve correct sorting in user-specified sorting systems would require keeping sorting tags in RAM.

That is possibly correct. I'm not against doing this, however we might introduce a setting for the sort-tag support, then.
Comment by John Millikin (jmillikin) - Sunday, 26 April 2009, 02:51 GMT
> That is possibly correct. I'm not against doing this, however we might introduce a setting for the sort-tag support, then.

Most of the space is taken up by track titles -- a feature that I suspect would be almost entirely unused. It should be possible to allow custom artist and album sorting without much trouble.
Comment by Thomas Martitz (kugel.) - Sunday, 26 April 2009, 02:54 GMT
That surely depends on the user. I, for example, have lots of single files and lots of albums with various artists (so that each song is by a different artists), that makes my artist list pretty huge too.
Is a setting a problem for you?
Comment by Brian Sutherland (rmaniac) - Tuesday, 30 June 2009, 16:08 GMT
Anyway. So I a setting would work on this I imagine... also since the initial artist or album list is sorted right.. why is it no sorted on artists->albums? is it a matter of when it pulls that artists albums from the index allowing them to show up first come first serve? In theory they would come in order. Are we sorting them after we pull them and that is the issue? I am not real sure where to look for that info.. just a thought.
Comment by Thomas Martitz (kugel.) - Tuesday, 30 June 2009, 18:58 GMT
I think the first node is directly taken from the tagcache files, all subnotes are generated at runtime.
Comment by Brian Sutherland (rmaniac) - Wednesday, 01 July 2009, 14:00 GMT
by generated at runtime.. where is it generating them from? I am sure its not pulling from the ID3 or Vorbis tags again... where would it be pulling them from in the subnodes?

Comment by Thomas Martitz (kugel.) - Wednesday, 01 July 2009, 14:01 GMT
From the tagcache files, but filtering according to tagnavi.config.

How else would you be able to browse the albums made by a specific artist?
Comment by Alan Fitch (apfitch) - Wednesday, 25 August 2010, 21:29 GMT
Updated the patch to work against r27875. Tested it with .ogg albumsort field.
Comment by Thomas Gay (yagmot) - Friday, 14 January 2011, 01:55 GMT
Anyone want to take a crack at getting this to work with a current build? I'm not a programmer, but tried to modify Alan's most recent patch, and it compiles correctly, but hangs when it's building the database. Here it is in case someone wants to use it as a starting point.
Comment by Jean-Louis Biasini (JeanLouisBiasini) - Wednesday, 23 November 2011, 23:10 GMT
update of the last patch to the present tree, same issue

Loading...