Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Bugs
  • Category Database
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Version 3.1
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Brian Sutherland - 2009-03-03
Last edited by Thomas Martitz - 2009-03-06

FS#9977 - Tagcache Database Song limit of 32768

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?

Closed by  Thomas Martitz
2009-03-06 15:44
Reason for closing:  Fixed
Additional comments about closing:  

Fixed in r20214. Thanks

Nils Wallménius commented on 2009-03-03 07:54

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

Thomas Martitz commented on 2009-03-03 15:48

I think you rather want to search for short, and change that to int/long,

Brian Sutherland commented on 2009-03-03 16:08

I have changed the shorts and some ints in apps/tag* is there somewhere else I should be looking?

Brian Sutherland commented on 2009-03-03 17:52

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?

Thomas Martitz commented on 2009-03-03 17:59

Try to change tag_length to int too, other than that, I have no real idea. Slasheri is the man to ask that.

Thomas Martitz commented on 2009-03-03 18:04

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.

Brian Sutherland commented on 2009-03-03 18:13

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.

Thomas Martitz commented on 2009-03-03 18:18

MP3 isn’t too bad on an ipod, one of the best in terms of battery life time (it uses the 2nd core).

Steve Bavin commented on 2009-03-04 07:39

Hi Brian,

Please could you attach a patch of your database changes (and only those) at some point?

Brian Sutherland commented on 2009-03-04 15:19

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.

Thomas Martitz commented on 2009-03-04 15:21

And this works? You use “ll”, but still short and long in the struct.

Brian Sutherland commented on 2009-03-04 15:30

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.

Thomas Martitz commented on 2009-03-04 15:33

Yea, but I image “sl” would be correct then.

Brian Sutherland commented on 2009-03-04 15:43

riiiiight. I will change and test and send a new patch.

Brian Sutherland commented on 2009-03-04 16:32

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.

Nils Wallménius commented on 2009-03-04 16:53

Right, i was clearly not thinking right before, 32 bit should be Enough for Anyone ™ 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.

Thomas Martitz commented on 2009-03-04 16:58

Ah right, due to alignment, the short will be the size of an int anyway (at least on most/some archs).

Brian Sutherland commented on 2009-03-04 18:07

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.

Thomas Martitz commented on 2009-03-05 14:21

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.

Brian Sutherland commented on 2009-03-05 14:37

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.

Thomas Martitz commented on 2009-03-05 14:52

How big is the ram cache without this patch here?

Brian Sutherland commented on 2009-03-05 16:24

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?

Brian Sutherland commented on 2009-03-05 16:26

Also of course.. anyone who did not need 38k files would have no where near the extra memory use.

Thomas Martitz commented on 2009-03-05 16:29

Hm, I don’t think the sim is overly useful for this.

Thomas Martitz commented on 2009-03-05 16:31

oh, and PS: I don’t think the 38k files properly get index’d.

Brian Sutherland commented on 2009-03-05 16:34

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.

Thomas Martitz commented on 2009-03-05 16:35

Well, you don’t need to delete files. You can also put some database_ignore files around.

Brian Sutherland commented on 2009-03-05 16:36

Oh don’t worry that was the plan. :)

Steve Bavin commented on 2009-03-05 16:36

FWIW, the sim normally does work ok for database building, at least on a 32-bit system…

Thomas Martitz commented on 2009-03-05 16:37

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.

Brian Sutherland commented on 2009-03-05 16:40

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.

Thomas Martitz commented on 2009-03-05 16:41

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.

Brian Sutherland commented on 2009-03-05 17:27

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.

Thomas Martitz commented on 2009-03-05 17:28

make in tools/database should build it.

Brian Sutherland commented on 2009-03-05 17:29

Oh right that.. I have played with that before. It’s pretty gimpy. heh

Brian Sutherland commented on 2009-03-05 19:27

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

Admin
Linus Nielsen Feltzing commented on 2009-03-06 10:11

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?

Steve Bavin commented on 2009-03-06 10:14

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.

Thomas Martitz commented on 2009-03-06 10:25

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 :)

Steve Bavin commented on 2009-03-06 10:31

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.

Thomas Martitz commented on 2009-03-06 10:42

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.

Brian Sutherland commented on 2009-03-06 13:56

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.

Nils Wallménius commented on 2009-03-06 13:59

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)

Thomas Martitz commented on 2009-03-06 14:01

Hm, yea, you’re probably right. Let’s keep it consistent.

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

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing