Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11776 - Metadata read log

Attached to Project: Rockbox
Opened by Jonas Häggqvist (rasher) - Friday, 26 November 2010, 03:48 GMT+2
Last edited by MichaelGiacomelli (saratoga) - Saturday, 11 December 2010, 05:31 GMT+2
Task Type Patches
Category ID3 / meta data
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Rbutil SVN
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch adds a menu entry to the debug menu which will allow the user to toggle writing of a log of files being read for metadata in /metadata.log. The filename is written before Rockbox attempts to read metadata from the file, so if Rockbox crashes when scanning for the DB, the last filename in the log should be the file that caused the crash. This should greatly help debug metadata reader problems.
   metadatalog_v1.diff (1.7 KiB) 
 apps/metadata.c   |   11 +++++++++++
 apps/debug_menu.c |   11 +++++++++++
 2 files changed, 22 insertions(+)

This task depends upon

Closed by  MichaelGiacomelli (saratoga)
Saturday, 11 December 2010, 05:31 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  Committed in r28789.
Comment by Jonathan Gordon (jdgordon) - Friday, 26 November 2010, 04:08 GMT+2
mentioned in irc but incase it gets forgotten or someone else says the same thing...
1) fd == 0 is valid so that needs to be changed
2) the 2 write()'s should really be a snprintf() and a write() (or just fprintf() if we have that). putting a MAX_BUF array on the stack is no big deal here (get_metadata() shouldnt be being called recursively)
Comment by Jonas Häggqvist (rasher) - Friday, 26 November 2010, 04:10 GMT+2
Fix the fd check to >= 0 (0 is a valid fd)

Comments from IRC:
"I'd do snprintf() then write instead of two write()'s" - this way a) was simpler b) doesn't use more stack (probably not important)

"opening it every time isnt very nice either" - agreed, it's not nice, but it gets the job done and makes sure no potential buffering messes up if Rockbox were to close with the file open. Performance is unimportant - getting the right result is very important.

I'll try it out later with a target full of music to see how long it takes.
   metadatalog_v2.diff (1.7 KiB) 
 apps/metadata.c   |   11 +++++++++++
 apps/debug_menu.c |   11 +++++++++++
 2 files changed, 22 insertions(+)

Comment by sideral (sideral) - Friday, 26 November 2010, 13:23 GMT+2
This patch does not work right in the simulator on any filesystem that supports permissions, as no file mode is provided in the call to open. The new log file is created without write permission, which prevents reopening the file after the first logentry was written.

The applied patch fixes the bug.
   metadatalog_fix.diff (0.6 KiB) 
 b/apps/metadata.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comment by Jonas Häggqvist (rasher) - Friday, 26 November 2010, 15:27 GMT+2
Well this was not the case in the sim. I don't know where else it would be the case. Can't hurt though.
Comment by Jonas Häggqvist (rasher) - Sunday, 28 November 2010, 02:13 GMT+2
In my somewhat unscientific testing, building the DB goes from taking about 35 seconds without logging to about 55 seconds with logging (e200 with 1180 music files).

Not sure there's any reason not to commit this?
   metadatalog_v3.diff (1.7 KiB) 
 apps/metadata.c   |   14 ++++++++++++++
 apps/debug_menu.c |   11 +++++++++++
 2 files changed, 25 insertions(+)

Comment by MichaelGiacomelli (saratoga) - Thursday, 09 December 2010, 21:33 GMT+2
IMO this should already be in SVN.
Comment by Jonas Häggqvist (rasher) - Saturday, 11 December 2010, 04:47 GMT+2
Feel free to commit it. I probably won't in the near future.

Loading...