Rockbox

Tasklist

FS#10160 - Clean up MP4 metadata parsing to handle files with tags after 'mdat' atom

Attached to Project: Rockbox
Opened by Alex Bennee (ajb) - Wednesday, 22 April 2009, 12:22 GMT
Last edited by Thomas Martitz (kugel.) - Sunday, 18 April 2010, 12:31 GMT
Task Type Patches
Category ID3 / meta data
Status Closed
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 100%
Votes 0
Private No

Details

From 240b4eb19c0e167030bdbfe498f15596ca09316f Mon Sep 17 00:00:00 2001
From: Alex Bennee <alex@bennee.com>
Date: Wed, 22 Apr 2009 13:17:00 +0100
Subject: [PATCH] Clean up the MP4 metadata parsing.

The parser used to finish as soon as it saw an "mdat" atom and has set
the filesize. There is nothing that says all you metadata will have
been seen at this point. I've tweaked the parser to ensure it goes all
the way through the file scanning for metadata.

If tested this with an m4a of iTunes which mp4info parsed OK:

09:44 alex@danny/x86_64 [simdisk] >mp4info 03\ -\ This\ House\ Is\ Not\ for\ Sale.m4a
mp4info version 1.5.0.1
03 - This House Is Not for Sale.m4a:
Track Type Info
1 audio MPEG-4 AAC LC, 233.732 secs, 128 kbps, 44100 Hz
Metadata Name: This House Is Not for Sale
Metadata Artist: Ryan Adams
Metadata Year: 2003
Metadata Album: Love Is Hell Part 1
Metadata track: 3 of 10
Metadata Disk: 1 of 1
Metadata Genre: Alt.Country
Metadata Cover Art pieces: 1

However the file still won't play on Rockbox, I assume because the
codec that does the actual playing cannot deal with the unusual
structure of the m4a. Interestingly it plays the other songs of the
album fine!
---
apps/metadata/mp4.c | 116 +++++++++++++++++++++++++++++++++++----------------
1 files changed, 80 insertions(+), 36 deletions(-)

This task depends upon

Closed by  Thomas Martitz (kugel.)
Sunday, 18 April 2010, 12:31 GMT
Reason for closing:  Out of Date
Additional comments about closing:  Closure requested by task creator.  FS#10832  supersedes this.
Comment by Magnus Holmgren (learman) - Saturday, 25 April 2009, 15:53 GMT
The stop at the mdat atom is deliberate, since the decoder currently can't handle files with metadata after the mdat. In order to play a file, the codec needs various information (decoding parameters, some tables describing the file layout). This metadata can be fairly large (e.g., around 40 kB for a 4 minute song), so it can't be stuffed into the mp3entry struct. Also, a decoder shouldn't start playback by seeking back and forth, potentially causing a rebuffer.

To really fix this, the metadata reader should first extract (and somehow buffer) the information needed, then let the playback code buffer the rest of the file.
Comment by Alex Bennee (ajb) - Saturday, 25 April 2009, 16:06 GMT
I saw the comment as I was tweaking this bit. I'd like to fix the codec as well as there are obviously well formed files rockbox can't currently play. I'm just getting my head round how the playback code works. Currently the playback thread is paused very soon after playback is resumed via a STOP message. Is this the codec detecting an mdat it can't handle?

How much information does the codec need from the metadata. I assume anything not already covered by mp3data (bitrate and length) can be covered by giving a list of mdat pointers and sizes describing the blocks of media data that make up the file?
Comment by Magnus Holmgren (learman) - Sunday, 26 April 2009, 10:38 GMT
That stop is likely due to the codec failing or returning immediately, yes.

What the codec needs is this:

1) The data to pass to NeAACDecInit2 (in apps/codecs/aac.c; read from the esds atom).

2) Seek information (from the atoms stts, stsz, stsc and stco). Keeping the size of each sample (or frame) isn't necessary, as far as I know, so it should be enough to keep a number of seek point, as you suggest. For each seek point, the number of frames is needed (between this point and the next), as well as a time offset (as a sound sample count, I think). That should be enough to be able to decode a file, and seek in it based on file offset or time.

When creating a seek table, remember that there can be gaps between chunks, so a seek point can't always "begin" in one chunk and "end" in the next.
Comment by Alex Bennee (ajb) - Wednesday, 06 May 2009, 06:58 GMT
This is the current WIP patch. It seems the problem with my MP4 file
is one of the mov data being after the mdat data. The second mdat atom
in my test track is a phantom (0 length) as can be seen from this log:

audio_current_track: no id3 data
res = 0
read_mp4_container(16, 0x991680, 3933035)
un-handled case: type=0x17809
un-handled case: type=0x38c
second MP4_mdat (0), this file may not play
read_mp4_container(16, 0x991680, 96294)
un-handled case: type=0x64
read_mp4_container(16, 0x991680, 56705)
un-handled case: type=0x54
un-handled case: type=0x1c
read_mp4_container(16, 0x991680, 56569)
un-handled case: type=0x18
read_mp4_container(16, 0x991680, 56471)
un-handled case: type=0x8
un-handled case: type=0x1c
read_mp4_container(16, 0x991680, 56411)
read_mp4_container(16, 0x991680, 983)
un-handled case: type=0x2c30
un-handled case: type=0x9d54
un-handled case: type=0xec0
read_mp4_container(16, 0x991680, 39423)
un-handled case: type=0x22
MP4 bitrate 127, frequency 44100 Hz, length 233732 ms
audio_finish_load_track
Looking for album art for /03 - This House Is Not for Sale.m4a
audio_finish_load_track: elap=0, offset=0, codectype=12
foo!
audio_finish_load_track done
aac:codec_main
qtmovie_read: Found a chunk ftyp, length=32
qtmovie_read: Found a chunk free, length=96273
qtmovie_read: Found a chunk free, length=916
qtmovie_read: Found a chunk mdat, length=3739504
read_chunk_mdat: off=97229, len=3739496
qtmovie_read: Found a chunk mdat, length=8
qtmovie_read: Found a chunk moov, length=96302
format: mp4a
audioType=64, maxBitrate=136288, avgBitrate=127993
curpos=3837248, j=3837254 - Skipping 6 bytes
entry_remaining=896
NeAACDecInit2
codec_main: entering main decoding loop
sdl_audio_callback: No Data.

This presents me with a number of possibilities. One is simply to make
the MP4 decoder handle such simple cases or find some more MP4s with
proper multiple mdat assignments.

commit 35bfaf95659727a5fd06d1c34b85851f04c0dbc3
Author: Alex Bennee <alex@bennee.com>
Date: Wed May 6 07:52:44 2009 +0100

Wind stream position back to first mdat atom before starting playback.

This also leaves a FIXME for dealing with seeking in libm4a which needs fixing

apps/codecs/aac.c | 6 +++++-
apps/codecs/libm4a/demux.c | 3 ++-
apps/codecs/libm4a/m4a.c | 5 +++--
3 files changed, 10 insertions(+), 4 deletions(-)

commit 17a54ad890d52d4ce523333e79aa184eb84d89ac
Author: Alex Bennee <alex@bennee.com>
Date: Wed May 6 07:51:17 2009 +0100

Commentry for playback.c

apps/playback.c | 29 +++++++++++++++++------------
1 files changed, 17 insertions(+), 12 deletions(-)

commit e67cdc0582b9c80c392596ac15183911b66f3b6d
Author: Alex Bennee <alex@bennee.com>
Date: Tue May 5 18:37:35 2009 +0100

Parse MP4 files for multiple mdat chunks

apps/codecs/libm4a/demux.c | 97 ++++++++++++++++++++++++++++++++-----------
apps/codecs/libm4a/m4a.h | 21 +++++++--
2 files changed, 88 insertions(+), 30 deletions(-)

commit b4686e71a5fdeac751cba61feb391d8e78bec87b
Author: Alex Bennee <alex@bennee.com>
Date: Tue May 5 18:35:29 2009 +0100

Bit more info in DEBUGF

apps/metadata/mp4.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

commit 11c4a69120d72a10c593b237f864e9a5ea26723f
Author: Alex Bennee <alex@bennee.com>
Date: Wed Apr 22 13:17:00 2009 +0100

Clean up the MP4 metadata parsing.

The parser used to finish as soon as it saw an "mdat" atom and has set
the filesize. There is nothing that says all you metadata will have
been seen at this point. I've tweaked the parser to ensure it goes all
the way through the file scanning for metadata.

If tested this with an m4a of iTunes which mp4info parsed OK:

09:44 alex@danny/x86_64 [simdisk] >mp4info 03\ -\ This\ House\ Is\ Not\ for\ Sale.m4a
mp4info version 1.5.0.1
03 - This House Is Not for Sale.m4a:
Track Type Info
1 audio MPEG-4 AAC LC, 233.732 secs, 128 kbps, 44100 Hz
Metadata Name: This House Is Not for Sale
Metadata Artist: Ryan Adams
Metadata Year: 2003
Metadata Album: Love Is Hell Part 1
Metadata track: 3 of 10
Metadata Disk: 1 of 1
Metadata Genre: Alt.Country
Metadata Cover Art pieces: 1

However the file still won't play on Rockbox, I assume because the
codec that does the actual playing cannot deal with the unusual
structure of the m4a. Interestingly it plays the other songs of the
album fine!

apps/metadata/mp4.c | 116 +++++++++++++++++++++++++++++++++++----------------
1 files changed, 80 insertions(+), 36 deletions(-)
Comment by Juliusz Chroboczek (jch) - Saturday, 05 December 2009, 19:05 GMT
Please consider  FS#10832 
Comment by Alex Bennee (ajb) - Tuesday, 05 January 2010, 11:06 GMT
This bug can now be closed as FS 10832 supersedes it.

Loading...