882099 - [Music] Supported Audio files with MP4 containers doesn't get scanned/displayed in Music player

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Music, defect, P1)

Description

[Leo]

1. Title : Music player doesn't display Audio AAC with MP4 container
2. Precondition : Use a test stream with Only Audio(AAC Main Profile) with *.mp4 container format 
3. Tester's Action : 1. Copy the test stream to the device 
                     2. Open Music application
                     3. Music player displays mp3 files, aac files, but not MP4 container(with AAC Audio)
					 
4. Detailed Symptom (ENG.) : Music app shows AAC audio file, but doesn't scan/display for MP4 container(which has only AAC Audio)
5. Expected : AAC decoder is available, but there is no support for MP4 containers with only AAC Audio.
6.Reproducibility: Y
	1)Frequency Rate : 100%
7.Gaia Revision: a25fb6bd1b0284ce3e197e88567d433c7815ee52

Comment 1

[Marco Chen [:mchen]]

Hi Leo,

Could upload the testing file?
Thanks.

Comment 2

[Dominic Kuo [:dkuo]]

Marco, Leo has sent me the testing file via email, I will update the status here after I tested it.

Comment 3

[Dominic Kuo [:dkuo]]

Attachment: I053_MP4_AAC Main_NoVideo(296kb).mp4

Comment 4

[Dominic Kuo [:dkuo]]

The mp4 type is "isom" which the metadata parser of Music app does not recognize yet, just as same as Marco mentioned in bug 877571#c1.

Comment 5

[Jim Porter (:squib)]

I can take this. I know my way around the metadata parser pretty well.

Comment 6

[Dominic Kuo [:dkuo]]

Jim,

Good to know you can work on this. The problem here is how the metadata parser in Music sniffs the mp4 is different from the one in gecko. I have talked to Marco yesterday and planned to fix this issue by synchronizing the sniffers in gecko and gaia.

For mp4, currently:
1. Gecko looks on the compatible_brands in the ftyp for "mp42", "mp41", "isom" or "iso2".
2. Gaia looks on the major_brand in ftyp for "M4A", "M4B" or "mp42".

So if we want to fix this by modifying Music app, the metadata parser should also look on the compatible_brands instead of looking on the major_brand for mp4.

You can see some other details in /toolkit/components/mediasniffer/nsMediaSniffer.cpp or bug 859711.

Comment 7

[David Flanagan [:djf]]

Dominic: thanks for looking at what Gecko does here. That never occured to me...

Jim: after fixing this, be sure to test with a bunch of movies on the device to make sure that we don't start seeing videos in the Music app again.  (I think there is other code in the metadata parser now to prevent that, but I'm not certain.)

Comment 8

[Jim Porter (:squib)]

Hmm, according to this page <http://www.ftyps.com/what.html>, the major_brand should never be "isom" (anymore), though the hex dump in the attachment does that. Mainly, this is just an observation.

Based on my reading of the docs I could find, Gecko's mp4 detection matches *any* mp4 file, including mp4 video and even weird stuff like motion JPEG, right? Assuming that's accurate, I don't think we actually want to *synchronize* the sniffers. We just want to check for "M4A " and "M4B " in either the major_brand or compatible_brand, right? I'm not sure about "mp42", since that seems like it might include a video stream as well.

Comment 9

[Jim Porter (:squib)]

If our goal is to be accurate here, shouldn't we check for the presence of audio and video tracks in the file? There doesn't seem to be a 100% accurate way to tell if an mp4 file is just audio by the ftyp atom...

Comment 10

[Marco Chen [:mchen]]

(In reply to Jim Porter (:squib) from comment #9)
If you want to check the tracks info after making sure that is really a mp4 container by ftyp box then you may need to find each track box -> media box -> handler reference box. That will indicate whether this track is video or audio or sound.

Comment 11

[Marco Chen [:mchen]]

s/video or audio or sound/video or audio or hint/

Comment 12

[Dominic Kuo [:dkuo]]

Jim, I think what Marco said in comment 10 is already happened in parseMoovAtom() of metadata.js, so the metadata parser is able to tell that, a mp4 file is an audio or video.

And why some gecko supported mp4 are not listed in Music app is because the metadata parser(gaia) and the mediasniffer(gecko) check different brands(comment 6), and before gaia checking it's an audio or video, the mp4 that have target compatible_brands but don't have target major_brand will be treated as unsupported.

So if gecko uses compatible_brands as criteria for supported mp4, then gaia should also does it, and plus checking a mp4 is an audio or video.

Comment 13

[Jim Porter (:squib)]

Ok, all that sounds good then, and I agree that replicating Gecko's sniffer is probably ok. Do we have tests (or just some sample files) to ensure that we get our sniffing right? I hardly ever use MP4, so I don't have a big supply of files locally to test with.

Comment 14

[Dominic Kuo [:dkuo]]

I will try to find some sample links and see if we can cover most of the mainstream mp4.

Comment 15

[Dominic Kuo [:dkuo]]

Jim, here are some sample links for testing the files with MP4 series containers.
(I guess these might be some testing resources from some project, I just used google to find it...)

3GP: http://download.wavetlan.com/SVV/Media/HTTP/http-3gp-audio.htm
MP4: http://download.wavetlan.com/SVV/Media/HTTP/http-aac.htm
(I think we can ignore *.aac, just test *.mp4 and *.m4a, please see bug 882683 comment 10 and 11)
AMR: http://download.wavetlan.com/SVV/Media/HTTP/http-amr.htm

Comment 16

[Jim Porter (:squib)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/10546

Comment 17

[Dominic Kuo [:dkuo]]

Update: after I pulled and tested this patch, there are some test files in the blocking bugs still not displayed, and I found there should be a wrong logic of checking compatible_brands, will update later.

Comment 18

[jongsoo.oh]

After patch, leo work correctly

Comment 19

[Dominic Kuo [:dkuo]]

Comment on attachment 766194 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/10546

Jim, overall the patch looks good, but there is a problem that, it only check the 1st compatible_brand, and the 2nd compatible_brand will not be checked, this might ignore some files that has target compatible_brand in the 2nd ftyp code, like "3gp5isom" is actually a target compatible_brands we want, but it will only check "3gp5" and skip "isom". I will try to fix the patch for you since you are away.

Comment 20

[Dominic Kuo [:dkuo]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/10696

David, I have modified Jim's patch(Attachment #766194 [details]) to fix the wrong offset on checking the compatible_brands, and there are two commits in that PR because it will be easy for you to know where I modified the patch, so probably you can only review the part I committed if you don't have much time, thanks.

Comment 21

[David Flanagan [:djf]]

Comment on attachment 768967 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/10696

Dominic,

Thanks for taking this bug and trying to finish it.

I'm giving r- because the patch seems too complicated, and I don't like the risk of introducing all that new code into parserMP4Metadata().  Could you write a simpler function that just checks the major and minor brands in the header?  See my comments on github.

Comment 22

[Dominic Kuo [:dkuo]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/10723

David,

This patch simply added a new function called checkMP4Type() as you suggested on github, and I have also commented how checkMP4Type() checks the major and compatible brands, would you please review it again? thanks.

Comment 23

[David Flanagan [:djf]]

Comment on attachment 769734 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/10723

See my notes on github.

Your checkMP4Type() method modifies the state (the internal pointer) of the BlobView object. parseMP4Type() depends on that pointer being 0, so I think the check function breaks the parse function.

You can fix this by using the getXXX functions instead of the readXXX functions and always specifying the byte offset that you're reading from.

Other than that, the patch looks great.  Thanks!

Comment 24

[Dominic Kuo [:dkuo]]

Comment on attachment 769734 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/10723

David, thanks for finding the issue of the internal pointer, it did break the parse function. I have revised my patch to use getXXX functions instead of the readXXX functions, also make sure parse function works normally, can you review this again? thanks.

Comment 25

[David Flanagan [:djf]]

Comment on attachment 769734 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/10723

The code looks good to me. I don't have the sample files to test it against, but assuming that you have tested, it, then r+

r=djf for github commit 404fd2d

Comment 26

[Dominic Kuo [:dkuo]]

Thanks David! I have tested the sample file from leo and several files in comment 15, they can be scanned and displayed in music app.

Landed on master: 0a8b1d7c70c5ec2b48e52c2cd7e463f47fa0b9cb

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 0a8b1d7c70c5ec2b48e52c2cd7e463f47fa0b9cb to:
v1-train: 5261396aae524fa53ec6fc78ea73b17afd79161d

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 0a8b1d7c70c5ec2b48e52c2cd7e463f47fa0b9cb to:
v1-train: 5261396aae524fa53ec6fc78ea73b17afd79161d

Comment 29

[Chris Kreinbring [:CKreinbring]]

Verified fixed on Leo 1.1 commercial RIL.

Build ID: 20130715070218
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/6062fdf2deb8
Gaia: 55ed5e08a2250ea2d3571fff860c39e66fabed14
Platform Version: 18.1
RIL Version: 01.01.00.019.158

Comment 30

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 5261396aae524fa53ec6fc78ea73b17afd79161d