Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Music playback
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by learman - 2006-05-01

FS#5268 - Improved MP4 metadata parser

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)
Closed by  learman
2006-10-11 19:05
Reason for closing:  Accepted

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.

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.

Oops, forgot all about ALAC. Sorry. Rescinded then.

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

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.

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.

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.

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.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing