Rockbox

Tasklist

FS#9977 - Tagcache Database Song limit of 32768

Attached to Project: Rockbox
Opened by Brian Sutherland (rmaniac) - Tuesday, 03 March 2009, 03:10 GMT
Last edited by Thomas Martitz (kugel.) - Friday, 06 March 2009, 15:44 GMT
Task Type Bugs
Category Database
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Version 3.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

When running an iPod 5G player with a 240GB drive in it, any song added to the database after 32,768 have been added will not play. It lists it's play time as -2:-2 and just skips past it when trying to play. It seems the tagcache uses and int somewhere that is cutting off rockbox's ability to to support large drives with large amount of files going forward. I have attempted to change some of the int vars to long to see if I can isolate where it is. Unfortunately I do not know rockbox or c well enough to find a real solution to this issue. Since only a few people at current will need this many files I guess it should be a compile time option. Is there anyone who can work a patch for this?
This task depends upon

Closed by  Thomas Martitz (kugel.)
Friday, 06 March 2009, 15:44 GMT
Reason for closing:  Fixed
Additional comments about closing:  Fixed in r20214. Thanks
Comment by Nils Wallménius (nls) - Tuesday, 03 March 2009, 07:54 GMT
I don't know the database at all but for all our current targets "long" is as large as "int" so you'll have to use "long long" or an explicit type like int64_t
Comment by Thomas Martitz (kugel.) - Tuesday, 03 March 2009, 15:48 GMT
I think you rather want to search for short, and change that to int/long,
Comment by Brian Sutherland (rmaniac) - Tuesday, 03 March 2009, 16:08 GMT
I have changed the shorts and some ints in apps/tag* is there somewhere else I should be looking?
Comment by Brian Sutherland (rmaniac) - Tuesday, 03 March 2009, 17:52 GMT
I had a misunderstanding of how the simulator works. Now that I am clear on that. I put the code back to normal and started over. I then changed what I thought
would have been the most obvious line:
tagcache.c
170: short idx_id; to an int.

After compiling and running it will build the database (I erased the database files) when it commits it seems to add only to _4 and _idx. When I try to view
the database it looks like it is blank. Any ideas?
Comment by Thomas Martitz (kugel.) - Tuesday, 03 March 2009, 17:59 GMT
Try to change tag_length to int too, other than that, I have no real idea. Slasheri is the man to ask that.
Comment by Thomas Martitz (kugel.) - Tuesday, 03 March 2009, 18:04 GMT
Err, wait, you'll need to change static const char *tagfile_entry_ec = "ss"; too. This is for endianess correction. The s is for short. You'll want to use "l" for int/long/int32_t.
Comment by Brian Sutherland (rmaniac) - Tuesday, 03 March 2009, 18:13 GMT
That's the ticket.. I would not have seen that. I knew you could find it. :) I am rebuilding the db now to 32768+ to see how it goes. If that works then I will be adding the (your?) sort tag patch and testing that.. if that is all good then I will run all this joy on my real device... which I sadly just converted back to mp3s for the moment so I could use the oemOS until I could work out my rockbox woes.
Comment by Thomas Martitz (kugel.) - Tuesday, 03 March 2009, 18:18 GMT
MP3 isn't too bad on an ipod, one of the best in terms of battery life time (it uses the 2nd core).
Comment by Steve Bavin (pondlife) - Wednesday, 04 March 2009, 07:39 GMT
Hi Brian,

Please could you attach a patch of your database changes (and only those) at some point?
Comment by Brian Sutherland (rmaniac) - Wednesday, 04 March 2009, 15:19 GMT
Shortest. Patch. Ever.
Here is a patch to up the number of files allowed in the db from 32k to more than you would ever want. This does not include anything to enable LBA48 in any targets or add a 4096 sector size like my hard drive needs. I mention this because the odds are if you need this patch you have a drive over 138GB in size.
Comment by Thomas Martitz (kugel.) - Wednesday, 04 March 2009, 15:21 GMT
And this works? You use "ll", but still short and long in the struct.
Comment by Brian Sutherland (rmaniac) - Wednesday, 04 March 2009, 15:30 GMT
Yes, seems to be fine. I have tested it in the simulator and on my device.
The only short in that struct is "short tag_length" which I believe has to do with the tags within any single file.
If that is correct then 32768 should be plenty enough.
Comment by Thomas Martitz (kugel.) - Wednesday, 04 March 2009, 15:33 GMT
Yea, but I image "sl" would be correct then.
Comment by Brian Sutherland (rmaniac) - Wednesday, 04 March 2009, 15:43 GMT
riiiiight. I will change and test and send a new patch.
Comment by Brian Sutherland (rmaniac) - Wednesday, 04 March 2009, 16:32 GMT
I put it to sl, rebuilt the database from scratch and its back to not displaying anything at all in the tag database. 'll' seems to be the ticket as to how that relates to the short/int/char in the struct is a little beyond me. I have updated the patch though. It seems I should have put "int idx_id" not "long idx_id" the long makes the db look empty just like "sl" does. If anyone has some comments or something else you want me to test on this I will be glad to.
Comment by Nils Wallménius (nls) - Wednesday, 04 March 2009, 16:53 GMT
Right, i was clearly not thinking right before, 32 bit should be Enough for Anyone (tm) and yes int
is better than long as long is 64 bits on amd64 linux so it would mess up the sim for some of us.

About this ec thing thoug, a struct member is aligned to it's size so there will be 2 bytes of padding
after the short now that the following member is an int so using "sl" will not work as then two bytes from
the int will be byteswapped with the padding bytes.
Changing the other short to int and using "ll" or placing the int before the short and using "ls" should work
in this respect, not sure if any other part of the database depends on member order or something like that though.
Comment by Thomas Martitz (kugel.) - Wednesday, 04 March 2009, 16:58 GMT
Ah right, due to alignment, the short will be the size of an int anyway (at least on most/some archs).
Comment by Brian Sutherland (rmaniac) - Wednesday, 04 March 2009, 18:07 GMT
I toyed with a bunch of different combinations.. sl, ls (reversing the struct) etc... It looks like the only one that will work on a 64bit simulator and the iPod is the one in the v2 patch. int and ll.


Comment by Thomas Martitz (kugel.) - Thursday, 05 March 2009, 14:21 GMT
I'd be interested in the impact, particularly with "Load to RAM" enabled. Given that the struct is now 12 bytes instead of 8, each tagcache entry (in each of the *.idx files) has added 4 bytes at least. You're basically the first person I heard of experiencing this limit.
Comment by Brian Sutherland (rmaniac) - Thursday, 05 March 2009, 14:37 GMT
I do like to be an over achiever... I do have my db loaded into ram. I have 38057 oggs on my player at the moment. I also have the sort patch applied. Every Ogg has
Artist/Track/Album/Albumartist/albumsort/title/date filled out at the minimum. With all that going on the ramcache is right around 6MB according to the debug info and so far everything
seems quite snappy. My player is a 5.5G that was an 80gb so it has 64mb of ram. One other thing I noticed is my "Disk Info" looks find but the Disk Info under debug does not seem happy with the
LBA48. I might see if I can find a fix for that.
Comment by Thomas Martitz (kugel.) - Thursday, 05 March 2009, 14:52 GMT
How big is the ram cache without this patch here?
Comment by Brian Sutherland (rmaniac) - Thursday, 05 March 2009, 16:24 GMT
Well... I tried to test that in the simulator... the database still seems to be around 6MB but when I try to enable ram cache on the simulator and reboot it just keeps seg faulting... which is a whole nother bag of worms I guess. I tried to build it on the sim and copy it to the ipod and it works fine until I turn on ramcache and then it wants to rebuild the db.. So I guess I would have rebuild it on the device in order to get an accurate reading. But let me make some speculation... The whole 38k gets built into the database*.tcd files with out and issue. It was only the reading of the later parts of the db that caused problems. I would assume the ramcache just puts the whole db in memory. That being said and that the db seems to be about 6MB either way.. there may not be much change. I guess if we assume 4 bytes for a library of 38k that would be about 148KB maybe times two on the high end for the index and all that. Since some players really need their ram and have no need for this number of files.. I think it would make sense for this to be a compile time option if it were to go into a release. I'll get exact numbers maybe tonight. I just don't feel like putting my pod out of commission right now to rebuild. Anyone else running Linux on AMD64 getting a sim crash with ramdb turned on?
Comment by Brian Sutherland (rmaniac) - Thursday, 05 March 2009, 16:26 GMT
Also of course.. anyone who did not need 38k files would have no where near the extra memory use.
Comment by Thomas Martitz (kugel.) - Thursday, 05 March 2009, 16:29 GMT
Hm, I don't think the sim is overly useful for this.
Comment by Thomas Martitz (kugel.) - Thursday, 05 March 2009, 16:31 GMT
oh, and PS: I don't think the 38k files properly get index'd.
Comment by Brian Sutherland (rmaniac) - Thursday, 05 March 2009, 16:34 GMT
On that point I guess the wise thing to do it take about 10k files on the device and index them with and without the patch. That way we don't have to worry about what is happening to anything over 32768 in either scenario.
Comment by Thomas Martitz (kugel.) - Thursday, 05 March 2009, 16:35 GMT
Well, you don't need to delete files. You can also put some database_ignore files around.
Comment by Brian Sutherland (rmaniac) - Thursday, 05 March 2009, 16:36 GMT
Oh don't worry that was the plan. :)
Comment by Steve Bavin (pondlife) - Thursday, 05 March 2009, 16:36 GMT
FWIW, the sim normally does work ok for database building, at least on a 32-bit system...
Comment by Thomas Martitz (kugel.) - Thursday, 05 March 2009, 16:37 GMT
Good :)

No matter of the outcome of the RAM usage, I think the limit should at least be raised for high mem and tagcache-as-pc-app.
Comment by Brian Sutherland (rmaniac) - Thursday, 05 March 2009, 16:40 GMT
I used the sim to build my current DB. I think what is messing me up is I was trying to be lazy and build it completely offline with the files stored on my computer and then copy it to the device (the device is rsync'd from those files). But my working one I actually just pointed the sim to the mounted ipod.
Comment by Thomas Martitz (kugel.) - Thursday, 05 March 2009, 16:41 GMT
If you're building the database this way, it would be interesting if you compiled tagcache as standalone and use this then to build the db.
Comment by Brian Sutherland (rmaniac) - Thursday, 05 March 2009, 17:27 GMT
Yeah.. not real sure I know how to do that. Don't see any obvious way to do that and I am not so much of a programmer.
Comment by Thomas Martitz (kugel.) - Thursday, 05 March 2009, 17:28 GMT
make in tools/database should build it.
Comment by Brian Sutherland (rmaniac) - Thursday, 05 March 2009, 17:29 GMT
Oh right that.. I have played with that before. It's pretty gimpy. heh
Comment by Brian Sutherland (rmaniac) - Thursday, 05 March 2009, 19:27 GMT
Ok here we go:
8713 Files

Pre-Patch:
1554060/1587296 Bytes
Post-Patch:
1667164/1700400 Bytes

Also remember I have the sort patch applied and all tracks were tagged with:
Artist/Track Number/Track Total/Album/Album Artist/Album Sort/Title/Date/Disc Number on multi disc sets.

So I guess it grew by 110KB or about 13 Bytes per song

Comment by Linus Nielsen Feltzing (linusnielsen) - Friday, 06 March 2009, 10:11 GMT
IMHO, the database size increase is irrelevant. We need to fix this bug, and if this patch works, it should be committed. My only worry is that it may fail to handle existing database files. Is there a way to handle this incompatibility?
Comment by Steve Bavin (pondlife) - Friday, 06 March 2009, 10:14 GMT
The database needs a version number, and the data should be treated as non-existant if out-of-date. I don't know if we have such a field at the moment, but if not, this would be a good chance to introduce it.
Comment by Thomas Martitz (kugel.) - Friday, 06 March 2009, 10:25 GMT
Yes, it has a version number in tagcache.h. Just increase it and existing tagcache will be invalid.

Let's see how irrelevant this increase really is :)
Comment by Steve Bavin (pondlife) - Friday, 06 March 2009, 10:31 GMT
Great!

I'd suggest someone checks that that 64-bit systems are still happy with the sim (and database tool), then this should go in.
Comment by Thomas Martitz (kugel.) - Friday, 06 March 2009, 10:42 GMT
Let me suggest something.

#if MEMORY_SIZE <= 2
33k files [or, if possible change to unsigned,
#else
2/4M files
#endif

I don't really want to restrict (and I think on low mem they're just as restricted too due to the ram usage increase, but probably less if we leave things unchanged for them) database user which depend on voice more, or something like that.
Comment by Brian Sutherland (rmaniac) - Friday, 06 March 2009, 13:56 GMT
It works fine on the 64bit sim. The version number of the tag cache can be upped and I think I rebuild of the tagcache is just gonna have to happen. I expect we do have some targets that every little bit counts... we could def try to help them out by making this a choice. Of course also the smaller the target the less this would effect them anyway.
Comment by Nils Wallménius (nls) - Friday, 06 March 2009, 13:59 GMT
targets with less than 8 megs of ram don't have ramcache at all so this would
only "waste" a couple kb's of diskpace, i vote for consistency across targets
(this also helps in case people want to generate the db externally)
Comment by Thomas Martitz (kugel.) - Friday, 06 March 2009, 14:01 GMT
Hm, yea, you're probably right. Let's keep it consistent.

BTW: The clip has the database and 2MB of RAM.

Loading...