Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#5268 - Improved MP4 metadata parser

Attached to Project: Rockbox
Opened by Magnus Holmgren (learman) - Monday, 01 May 2006, 14:25 GMT+2
Task Type Patches
Category Music playback
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch is a rewrite of the MP4 metadata parser. The main reason
for writing it is that the existing one makes assumptions about the
order in which elements (so called atoms) appear in the file, making
it fail for files not created by certain programs. This is an attempt
to make a more flexible one, that is based on the specifications (at
least the ones I was able to find...).

So why isn't it committed yet? Mainly because...

* It isn't very well tested yet. I've run in through a couple of test
files (generated by a couple of different programs), and those worked
well (both in the simulator an on a Coldfire target), but as the MP4
decoder isn't really usable on Coldfire-based players, I haven't been
motivated to test more. :)

* Some more error checking should probably be added. (Mainly in the
form of checking for non-zero errno in a few more places.)

* Some debug code should be removed (DEBUGF macros mainly).
   mp4.diff (12.5 KiB) 
 apps/metadata.c |  425 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 423 insertions(+), 2 deletions(-)

This task depends upon

Closed by  Magnus Holmgren (learman)
Wednesday, 11 October 2006, 21:05 GMT+2
Reason for closing:  Accepted
Comment by Paul Louden (darkkone) - Monday, 01 May 2006, 16:46 GMT+2
I personally would suggest committing this. Most of the iPod users are not big on patching, and it would be hard to break playback of aac much more than the "It does not work at all" state it was in a week ago anyway. :)

Once they are using it, maybe some useful feedback can be had.
Comment by Dave Chapman (linuxstb) - Tuesday, 02 May 2006, 09:46 GMT+2
Don't forget that the MP4/M4A parser is also used by ALAC - which will be in 3.0 - and I don't think anyone is having problems with ALAC files in Rockbox.

This patch needs a little more testing before it's ready for CVS (it only touches metadata.c, it doesn't change the parser which is actually being used by the AAC codec itself), so it may not fix the problems with all files.

I'm planning to carry on the work with this patch, but that will probably be after 3.0.
Comment by Paul Louden (darkkone) - Tuesday, 02 May 2006, 17:19 GMT+2
Oops, forgot all about ALAC. Sorry. Rescinded then.
Comment by Magnus Holmgren (learman) - Monday, 08 May 2006, 20:25 GMT+2
Realized the patch was made without -u, so here's a fixed version,
making it easier to apply.

Also includes support for the ©wrt tag, where it has priority over the
"composer" field, if both are present (completely untested though).
   mp4.1.1.diff (13 KiB) 
 apps/metadata.c |  433 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 432 insertions(+), 1 deletion(-)

Comment by Paul Louden (darkkone) - Monday, 08 May 2006, 20:41 GMT+2
Shouldn't Composer have more priority if both are present? The only time both should be present is if someone is actually using Composer for Composer, and Writer for Writer.
Comment by Magnus Holmgren (learman) - Wednesday, 10 May 2006, 20:55 GMT+2
As I understood it from the forum or the mailing list (werever I saw it), the two fields are intended for the same information. The writer tag is in a "real" atom, whereas the composer is in a custom atom with name=value pairs. That's why I gave writer priority. Oh well, easy to change if needed.
Comment by Paul Louden (darkkone) - Wednesday, 10 May 2006, 20:59 GMT+2
Yeah. From my perspective though, software will either default to one or the other. The only instance in which a user would have both is if they needed to use both. And in that instance, Writer should be the writer, and Composer should be the composer, right? In any other instance, they'd only have one of the two. (It still seems preferable to me not to default to having Writer show up in the composer field on the assumption that they're a user of iTunes.)

The @wrt tag is actually left over from the Quicktime format that the container apparently evolved from, according to my research. iTunes uses @wrt to hold the Composer information, but several other programs don't seem to default to doing so. My personal concern is simply that there's a conflict between existing implementations already, and it seems more technically correct to go with what the tag is named. But I'm okay with defaulting to @wrt if Composer isn't present, but strongly against giving it priority over Composer since that really doesn't seem to make any sense. You only create an actual Composer atom at the point when you've already decided _not_ to use @wrt for it.
Comment by Will Robertson (aliask) - Friday, 28 July 2006, 15:22 GMT+2
I've updated the patch to compile on current cvs, it didn't actually work for my MP4 file, but I thought I'd share my work.
   mp4.1.2.patch (12.7 KiB) 
 apps/metadata.c |  425 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 424 insertions(+), 1 deletion(-)

Loading...