Rockbox

Tasklist

FS#9371 - create database application

Attached to Project: Rockbox
Opened by Yoshihisa Uchida (Uchida) - Saturday, 06 September 2008, 13:53 GMT
Last edited by Björn Stenberg (zagor) - Thursday, 04 December 2008, 22:01 GMT
Task Type Patches
Category Utils
Status Unconfirmed   Reopened
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

Create database application.


1) At first FS9349 patch file (http://www.rockbox.org/tracker/task/9349?getfile=17401) apply.

2) After this task's patch apply.
This task depends upon

Comment by Yoshihisa Uchida (Uchida) - Tuesday, 14 October 2008, 07:43 GMT
Because appropriating the patch to the source file of the latest version (svn. 18806) failed,
the patch file updates.
Comment by Yoshihisa Uchida (Uchida) - Wednesday, 15 October 2008, 10:27 GMT
For the latest version (svn.18814), songdb_2.patch cannot applied, the patch file modified.
Comment by Yoshihisa Uchida (Uchida) - Thursday, 16 October 2008, 08:50 GMT
songdb was able to be made by using MinGW.

It is necessary to install MSYS.

Comment by Yoshihisa Uchida (Uchida) - Thursday, 06 November 2008, 13:18 GMT
sync for svn. 19026
Comment by Yoshihisa Uchida (Uchida) - Monday, 08 December 2008, 08:46 GMT
update patch file.

changes:
- sync svn.19360.
- add -d (--rockbox-directory) option.
set Rockbox directory option.
- add -V (--version).
show version information option.
- -m (--mode) option add "append".
append non registered files only (not checked deleted file) mode.

Because the patch of FS9349 was committed, the patch of FS9349 need not be appld.

P.S. Should I add to the patch the addition of my name to docs/CREDITS ?
Comment by Björn Stenberg (zagor) - Tuesday, 16 December 2008, 10:34 GMT
Generally I think this is the right approach. utils/songdb is a better place than tools/database, and your songdb.c is more evolved than the database.c skeleton.

But there are a number of lines in this patch that I don't see why you have changed. Could you please try stripping the patch to only include the changes which are needed?

Also, feel free to copy parts of tools/database/Makefile so it includes support for all codecs.

As for docs/CREDITS, normally patches don't include this since it is a frequent cause of patch conflict. The committer adds it when he commits the patch.
Comment by Yoshihisa Uchida (Uchida) - Wednesday, 17 December 2008, 12:54 GMT
Thank you for the comment.

> Also, feel free to copy parts of tools/database/Makefile so it includes support for all codecs.

utils/songdb/Makefile has contained all supported codecs. (see line 74)
Even if new codec increases, it is automatically added.
Whenever codec increases when it is a method of A, it is necessary to correct Makefile.
Therefore, I did not do the correction Makefile.

> As for docs/CREDITS, normally patches don't include this since it is a frequent cause of patch conflict. The committer adds it when he commits the patch.
I understood.

changes:
 - sync svn. 19465
 - The correction to erase warning when making songdb was excluded.

About patch file
 Correction for songdb to make it compile on PC
  - apps/metadata/asf.c
  - apps/apps/settings.h
  - apps/metadata.c
  - apps/tagcache.c
  - apps/misc.c
  - firmware/export/logf.h
  - uisimulator/common/io.c

 Correction to make it compile even if "SIMULATOR" is not defined
  - firmware/export/system.h
  - firmware/export/thread.h
  - firmware/include/file.h

 Correction to use language such as SJIS etc.
  - firmware/include/rbunicode.h
  - firmware/common/unicode.c

 Correction to have to change "Rockbox directory" dynamically
  - apps/main.c
  - apps/tree.c
  - apps/tagcache.h
  - apps/tagcache.c

 In MinGW or Cygwin with -mno-cygwin option, if ch is signed char and ch < 0 (ch = 0x80 - 0xff) , isspace(ch) is doesn't operate normally.
  - apps/metadata/mp3.c
  - apps/metadata/metadata_common.c
  - apps/replaygain.c


Please comment when there is a correction that seems to be unnecessary it.
Comment by Yoshihisa Uchida (Uchida) - Monday, 12 January 2009, 04:02 GMT
sync svn. 19752.
copyright year change: 2008 -> 2009.
Comment by Björn Stenberg (zagor) - Monday, 12 January 2009, 19:21 GMT
Yoshihisa, are you building this for win32 in some other environment than cygwin? That would explain some of the changes I find "unnecessary".
Comment by Yoshihisa Uchida (Uchida) - Saturday, 17 January 2009, 02:33 GMT
I am using Cygwin and MinGW (+msys) for Win32.
Comment by Yoshihisa Uchida (Uchida) - Saturday, 07 March 2009, 10:04 GMT
sync svn. 20227.
Comment by Yoshihisa Uchida (Uchida) - Tuesday, 10 March 2009, 11:06 GMT
sync r20281
Comment by Thomas Martitz (kugel.) - Thursday, 02 April 2009, 20:56 GMT
I admit I'd like to see this patch in, but it has quite some necessary and unrelated changes, it seems to me.

For example, the chanes to uisimulator/common/io.c, may you can explain why this file is needed at all for the database tool? The renaming of the DATABASE_FILE etc things make the patch bigger and harder to read, without serving a purpose (they still point to the database *file*, right? so why call it _PATH_ all the way?).

I don't quite understand the "#ifdef WIN32" to "#ifdef _WIN32" too, but I haven't really looked into this generally.
Comment by Yoshihisa Uchida (Uchida) - Sunday, 05 April 2009, 07:38 GMT
I checked needlessness corrections.

I left corrections that changed "WIN32" into "_WIN32" (in io.c).
It is because build warnings increases though can not changed to "_WIN32" by deleting "-std=c99" from "GCCOPTS" (in utils/songdb/Makefile).
(It is necessary to correct to "_WIN32" because "WIN32" is not defined in MinGW's gcc when "-std=c99" is in "GCCOPTS".)

changes:
- sync r20619
- The correction to uisimulator/common/io.c was reduced.
- For apps/tagcache.c, TAGCACHE_FILE_* returned to the state before it was corrected. (do not use TAGCACHE_PATH_*)
- readme.txt: The description of options update.


Comment by Thomas Martitz (kugel.) - Sunday, 05 April 2009, 14:31 GMT
Sorry, but I don't understand why uisimulator/ files are even compiled in. The same goes for firmware/ files.
Comment by Thomas Martitz (kugel.) - Sunday, 05 April 2009, 21:50 GMT
The attached patch implements your songdb tool using the existing external database tool (using the old Makefile), vastly reducing the number of changes needed.

Only tested on linux so far.

PS: This tool is apparently unable to build the database for an actual player, since the pathes don't match the Rockbox' ones on the player. I didn't understand all the directory parameters though, maybe I'm doing anything wrong?
PS2: What's "-m commit" used for? It appears to be unused entirely.
Comment by Yoshihisa Uchida (Uchida) - Thursday, 09 April 2009, 11:20 GMT
The patch file was reviewed further.

changes:
- apps/metadata.c, apps/metadata/metadata_common.c do not change.
- uisimulator/common/io.c use WIN32. (does not use _WIN32) and corrections not necessary to build the songdb was deleted.
- For utils/songdb/Makfile, the options of GCCOPTS was made gradual and songdb.d does not create.
- The songdb was able to be built by gcc 4 on Cygwin.
- -m commit deletes.


About corrected files:
1) Correction necessary to build the songdb.
- apps/misc.c
- apps/tagcache.c
- apps/tagcache.h
- firmware/common/unicode.c
- firmware/include/rbunicode.h

2) Correction to build the songdb without defining SIMULATOR.
- firmware/export/system.h
- firmware/export/thread.h
- firmware/include/dir_uncached.h
- firmware/include/file.h

3) Correction to build the songdb without using SDL libraries.
- uisimulator/common/io.c

4) Include files that exists in Rockbox and Linux (Cygwin, MinGW) uses files that exists in Linux (Cygwin, MinGW).
(Because neither the function nor the function are enough by files that exist in Rockbox.)
Include files written by <...> uses the one that exists in Linux (Cygwin, MinGW).

Files corrected by the above-mentioned reason is as follows.
- apps/metadata/asf.c
- apps/replaygain.c
- apps/settings.h
- firmware/export/logf.h

5) Correction for the songdb build by MinGW to operate normally
- apps/metadata/mp3.c
Comment by Thomas Martitz (kugel.) - Thursday, 09 April 2009, 12:23 GMT
You seem to have ignored my patch and my questions...
Comment by Yoshihisa Uchida (Uchida) - Friday, 10 April 2009, 13:25 GMT
I corrected your patch.

changes:
- I corrected that the database tool was able to be build on Cygwin and MinGW.
- When optional -c >ISO-8859-8 | ISO-8859-11 | CP1256 | SJIS | GB-2312 | KSX-1001 | BIG5> was specified, the database tool works well.
- When optional -d was specified, the database tool works well.
- The option "-m create" deletes.
- mutex_init(), mutex_lock(), mutex_unlock(), thread_sdl_thread_lock(), thread_sdl_thread_unlock() deletes form database.c
=> I corrected that these functions do not use in io.c.
- fprintf() -> printf()
=> I do not change stdio.c. Because stdin, stdout, and stderr do not defined on Cygwin and MinGW. (It is because of use for which stdio.h exists in the Rockbox. )
- I corrected that SDL include files was not used.


The following corrections were not done.
Please let me hear your opinion.

1) I think that Makefile should not define SIMULATOR.
Because the database tool is not a simulator.

2) Because functions is insufficient only in the library that exists in Rockbox, the system library uses the one that exists in Linux, Cygwin, and MinGW.

When these are corrected, I will do the same correction as my patch file to your patch.
Comment by Thomas Martitz (kugel.) - Friday, 10 April 2009, 13:34 GMT
Nice.

I agree that SIMULATOR should not be defined.

I don't quite understand your second question. But yes, we should be using the system's header for standard functions, not the Rockbox' ones (because our target plattform is the PC, not Rockbox).
Comment by Yoshihisa Uchida (Uchida) - Saturday, 11 April 2009, 07:37 GMT
My patch was corrected further.

changes:
- The method of building the songdb changed.
(When the sondb.c compiles, include files of the build environment is used. And other source files uses the Rockbox's include files.)
- Change _ANSIDECL_H_ -> _ANSI_DECL_H_ in _ansi.h.
(For Cygwin with gcc 4, correction to prevent compilation of io.c from failing.)
- I corrected that the dependence file of source files when the songdb is built.
(Because the songdb is not updated when only header files was corrected.)

Moreover, rockbox-songdb_2.diff was corrected.
changes:
- I did the same correction as the correction to songdb_13.patch.
- I correctes that SIMULATOR does not define.

There are some questions.
1) How do you think though I think that the database tool should store on "utils" directory ?
-- Because "tools" directory is the directory that stores tools necessary to build the Rockbox firmware, codecs and, plugins.

2) How shall I do the name of the database tool ? database or songdb. or change other name ?
-- I think that the name of "database" is too general. So, I think that I should change the name of the database tool to another name. For example, songdb, musicdb, dbcreater etc.
Comment by Thomas Martitz (kugel.) - Saturday, 11 April 2009, 12:42 GMT
I don't really care in which folder it is or how we're naming it. I just thought basing it off the existing database tool was a good idea to implement your patch with least changes needed.
Comment by Thomas Martitz (kugel.) - Saturday, 11 April 2009, 12:50 GMT
Hmm, this looks pretty OK to me now.

Edit: What's the difference between the two versions now?
Comment by Yoshihisa Uchida (Uchida) - Saturday, 11 April 2009, 13:19 GMT
The difference between songdb_13.patch and rockbox-songdb_3.diff is as follows.
1) Directory that stored the database tool.
songdb_13.patch: utils/songdb
rockbox-songdb_3.diff: tools/database

2) database tool name
songdb_13.patch: songdb
rockbox-songdb_3.diff: database

Only the above-mentioned two points are different parts.
Comment by Thomas Martitz (kugel.) - Saturday, 11 April 2009, 14:08 GMT
Can it build a database on the target?

Last time I tried it couldn't. Rockbox' paths are different. But also endianess needs to be taken into account.
Comment by Thomas Martitz (kugel.) - Sunday, 12 April 2009, 18:23 GMT
Once it can create the database on the target (possibly through a special mode which will use rockbox's paths), and once the GSOC database project is cleared up, it probably going to commit this.
Comment by Thomas Martitz (kugel.) - Monday, 13 April 2009, 16:30 GMT
Fine, I compiled this on windows using mingw32-make and powershell (no need for msys/sh/cygwin).

I could also create a database, however, there seemed to issues with creating a database off a song dir which is on another partition.
Comment by Yoshihisa Uchida (Uchida) - Tuesday, 14 April 2009, 13:32 GMT
It corrected the database tool based on your comment as follows.

1) I added the option that specified the endian of the database.
-b (--big-endian): for big endian.
-l (--little-endian): for little endian.

2) Makefile was corrected like defining "ROCKBOX_BIG_ENDIAN" or "ROCKBOX_LITTLE_ENDIAN" according to an actual build environment.

But the database can not save in the place different from the Rockbox directory.

I confirmed whether the songdb was able to be made without installing msys (only MinGW + PowerShell).
Because grep, sed, date do not exist on MinGW(These are installed by msys packages.), I cannot build the songdb.
Comment by Thomas Martitz (kugel.) - Tuesday, 14 April 2009, 13:55 GMT
Ah ok, I do have msys installed, but I typed make in the powershell, so I probably didn't notice it used those tools.

Nice job about the endianess. So the remaining issue is the database for the actual target?

EDIT: How do you apply your patches on Windows? I couldn't do it. I only made it by actually applying your patch on a linux system, creating a svn diff. The svn diff can be applied using tortoise. Patch (which comes with the svn cli client) for windows failed due to the line endings.

EDIT2: What are the changes you made to tagcache.c? Please do not do further changes that are unrelated to the songdb application. Keep the changes to a minimum and put unrelated changes to a separate task.
Comment by Thomas Martitz (kugel.) - Wednesday, 15 April 2009, 21:21 GMT
Uhm, sorry if I was unclear, but I actually meant to create the database in little or big endian, not to tell the tagcache which endianess was used (it will notice himself and correct it). I think we just ignore the endianess for now.

I'm still somewhat unconfortable with the changes to io.c. The old tool doesn't need them. If you can explain why these are needed, I maybe feel better.

The other thing: database.h is a HACK. We want to include the tagcache as a (static) library. And thus we include tagcache.h. I know (played around with it myself) that it is troublesome, but database.h is basically just doing our own prototyping. I really don't like that.
Comment by Yoshihisa Uchida (Uchida) - Thursday, 16 April 2009, 11:22 GMT
A lot of corrections to my patch.
I think that changes in the functions becomes it at the end.

changes:
- sync r20712. (Please use the source since r20710.)
- the songdb include codepage table data. (the songdb does not use codepages/*.cp.)
- -i (--input-path) option add.
- -o (--output-path) option add.
- -d (--rockbox-directory) option deletes. (This option became unnecessary because of the above mentioned change. )
- I changed describing the definition of the songdb from Makefile to config-pctool.h.
- config.h includes config-pctool.h
- firmware/export/thread.h does not change.

The correction of rockbox-songdb_vc_5.diff is made as soon as the confirmed of the songdb_15.patch is completed.
Comment by Yoshihisa Uchida (Uchida) - Thursday, 16 April 2009, 13:13 GMT
More comment add.

About tagcache.c/h
For songdb_13 -> songdb_14, tagcache.c was corrected as follows.
1) tagcache_init(void) -> tagcache_init(int endian) when __PCTOOL__ is defined.
Should I newly make tagcache_set_endian() without changing tagcache_init(), and specify the endian of the database?

2) I corrected that whether database_idx.tcd was saved by the same endian as the specified endian was checked when it opened.

For songdb_14 -> songdb_15.
1) I change the functions that database files open or delete. (open() -> db_open(), remove() -> db_remove().)
It is a correction because it can dynamically change the directory saving of the database.
db_open() and db_remove() are implemented by songdb/dbfile.c.

2) I corrected that an unnecessary header files was not included in tagcache.c.

I confirm the tagcache.c again though I do not correct the above mentioned.

About unicode.c.
Until the songdb_14.patch, to convert the character code, codepage files that existed in the codepages directory had been used.
For the songdb_15.patch, I corrected that codepages/*.cp do not used to convert the character code.
The Rockbox directory need not exist when the songdb is executed by this correction.

About config.h/config-pctool.h
I corrected that definitions necessary to make the songdb was described in the config-pctool.h and the config.h read it.
This modification is mimicked to read the config-*.h when the Rockbox firmware or simulator is maked .

The problem might still remain though I confirmed operation on Cygwin, MinGW and Linux.

Comment by Thomas Martitz (kugel.) - Thursday, 16 April 2009, 14:02 GMT
a) What's logf needed for?
b) Remove the endianess thing please, the database will handle it without us. Unless we can change the endianess before creating, just telling the database which endianess the database is in doesn't make sense.
c) Please get including tagcache.h to work. This file gives us the prototypes. Please don't use hacks like "extern void tagcache_init()".
Comment by Yoshihisa Uchida (Uchida) - Friday, 17 April 2009, 11:15 GMT
a) I am using it as an output for debugging now. when the songdb will release, I don't use logf.c because it is unnecessary.

b) Because the database is normally read even if the endian of the database is different, endian change options (-b/-l) will delete.

c) tagcache_init() will return to original state.

When the above mentioned correction ends, I will upload the correction version patch file.
Comment by Yoshihisa Uchida (Uchida) - Friday, 17 April 2009, 13:29 GMT
Patch files updates.

changes:
- logf.c does not used.
- -b/-l options delete.
- tagacache_init() returns to original state.
- stdio.h does not change.
Comment by Yoshihisa Uchida (Uchida) - Friday, 17 April 2009, 13:48 GMT
sorry, rockbox-songdb_5.diff is wrong.

changes:
add database.h
delete stdio.h, thread.h
Comment by Yoshihisa Uchida (Uchida) - Friday, 17 April 2009, 14:14 GMT
sorry, rockbox-songdb.vc_6.diff is very wrong. I upload further more.
Comment by Thomas Martitz (kugel.) - Friday, 17 April 2009, 14:28 GMT
Why is Rockbox code pages not used anymore? Doesn't that create possible incompabilities? What's bad about the rockbox' codepages.
Also, you re-create some file functions, what's the need for io.c then?
The #defines in config-pctool look a bit weird imo.

Please, also note that this database tool isn't the only __PCTOOL__, e.g. checkwps and the wps editor are __PCTOOL__ too.
Comment by Yoshihisa Uchida (Uchida) - Saturday, 18 April 2009, 07:54 GMT
I think that io.c is necessary.
io.c is used to solve the following problems.
1) convert path. (pc path <-> Rockbox path)
2) convert character code used path. (For MinGW environment)
3) absorption of difference of build environment.

The file that takes out only a necessary function of io.c .
However, I think that I would rather use io.c than make such a file.
If this is done to io.c, I should take out only the part where tagcache.c (and other sources) is necessary.

I think that it is time-consuming to make the database tool without using the functions of Rockbox if this is done.

changes:
- bug fix: I correct the codepage.c.
- does not use config-pctool.h.(#define write in Makefile.)



Comment by Yoshihisa Uchida (Uchida) - Saturday, 18 April 2009, 07:56 GMT
sorry, because an unrelated file had mixed, it uploads again.
Comment by Yoshihisa Uchida (Uchida) - Sunday, 19 April 2009, 07:43 GMT
I made the version that did not use uisimuator/common/io.c.
There is a possibility that an unnecessary part remains still.

And it doesn't test enough.
Comment by Yoshihisa Uchida (Uchida) - Monday, 20 April 2009, 10:34 GMT
My patch file corrects.

changes:
- bug fix: I corrected that the songdb can be made on Linux.

I will make the correction version of rockbox-songdb_vc_8.diff if there is no problem.
Comment by Thomas Martitz (kugel.) - Monday, 20 April 2009, 10:39 GMT
a) You don't need to put #ifdef's around cpu_boost(), it's a macro on __PCTOOL__ anyway.
b) You didn't tell me why you use external codepages.
Comment by Yoshihisa Uchida (Uchida) - Monday, 20 April 2009, 12:39 GMT
comment thanks.
a) I corrected that I don't put #ifdef's around cpu_boost().

b) Becase it becomes unnecessary codepage files (*.cp) file.
And it is possible to make the database by this even in the environment in which Rockbox is not installed.
Comment by Thomas Martitz (kugel.) - Monday, 20 April 2009, 12:42 GMT
"And it is possible to make the database by this even in the environment in which Rockbox is not installed." - I don't understand this. You'll always need the rockbox source, don't you?

I just fear, that using different codepages, the database created by songdb might get incompatible to the Rockbox' one.
I think it should be possible for the songdb to compile the codepages in, and if not, .cp files aren't too bad imo.
Comment by Yoshihisa Uchida (Uchida) - Wednesday, 22 April 2009, 11:16 GMT
"And it is possible to make the database by this even in the environment in which Rockbox is not installed.":
It means any file included in .rockbox directory though the songdb operates is not needed.
(Of course, Rockbox sources is necessary to make the songdb.)

The Rockbox and the songdb are the same codepage table data.
codepage/iso.cp is created by using iso_decode() in tools/codepages.c.
And in the songdb, iso codepages (iso8859-*, WIN*) data are created by using iso2ucs() in utils/songdb/codepage.c.
But because iso_decode() and iso2ucs() are functions that do the same processing, codepage data is not different at all.

The reason to make iso2ucs() newly is that iso_decode() function exists in tools/codepages.c and firmware/common/unicode.c.

Because iso2ucs() becomes unnecessary if the function name of iso_decode() (in tools/codepages.c) is changed, it is not happened that the codepage data is different.
Is it good in such a correction?
Comment by Yoshihisa Uchida (Uchida) - Saturday, 25 April 2009, 11:09 GMT
My patch updates.

changes:
-- deletes iso2ucs() from utils/songdb/codepage.c
-- name change iso_decode() -> iso2ucs() from tools/codepgaes.c.
-- use codepages.c

Any codepage data is not different in Rockbox and the songdb.
Comment by Yoshihisa Uchida (Uchida) - Sunday, 21 June 2009, 05:01 GMT
sync. r21437
Comment by Sascha Wolf (Horscht) - Sunday, 15 May 2011, 16:25 GMT
Hi,

i just recently noticed that the tool now corrupts the database on the rockbox device (songdb.exe compiled in june 2009). Trying to apply this patch to the current rockbox code in order to compile a new .exe fails with a bunch of "HUNK Failed" messages. Any chance of seeing an updated version.

Loading...