Closed Bug 882683 Opened 7 years ago Closed 7 years ago

[Audio] AAC audio format in .3gp container is not playing

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

VERIFIED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: mbarone976, Assigned: dkuo)

References

Details

(Whiteboard: [cr 488549], [TD-53529], [LeoVB+])

Attachments

(4 files)

STR

1. Open Music app and load an AAC audio format in 3gp container

ACTUAL RESULT
The media file is not played

EXPECTED RESULT
The media file should be played
Can we get a test file & test page here to demonstrate the bug? I believe this should work, but I need to see the file here under question.
Flags: needinfo?(mbarone976)
Attached video Multimedia File
Flags: needinfo?(mbarone976)
Works fine in the browser, so I'm guessing this is a bug in the music app not parsing a supported media type.

AAC I believe is supported media type for 1.01, but I'm putting needinfo Sandip to clarify if this multimedia file should be supported in the music app for 1.1.
blocking-b2g: --- → leo?
Flags: needinfo?(skamat)
Currently in V1.0.1, there is no constraint to prevent 3rd party apps  and web pages to play AAC in mp4 and AMR in 3gp but with Bug 847809 we have restricted these on V1.1. With v1.1, AMR support will be with .amr extension file name (raw AMR format) (Bug 847809).
Flags: needinfo?(skamat)
(In reply to Sandip Kamat from comment #4)
> Currently in V1.0.1, there is no constraint to prevent 3rd party apps  and
> web pages to play AAC in mp4 and AMR in 3gp but with Bug 847809 we have
> restricted these on V1.1. With v1.1, AMR support will be with .amr extension
> file name (raw AMR format) (Bug 847809).

Are you suggesting this is expected behavior? Doesn't seem right to lose AAC audio playback in the music app in support of MMS audio (sounds like a regression), but triage may be wrong.
(In reply to Alex Keybl [:akeybl] from comment #5)
> (In reply to Sandip Kamat from comment #4)
> > Currently in V1.0.1, there is no constraint to prevent 3rd party apps  and
> > web pages to play AAC in mp4 and AMR in 3gp but with Bug 847809 we have
> > restricted these on V1.1. With v1.1, AMR support will be with .amr extension
> > file name (raw AMR format) (Bug 847809).
> 
> Are you suggesting this is expected behavior? Doesn't seem right to lose AAC
> audio playback in the music app in support of MMS audio (sounds like a
> regression), but triage may be wrong.

To my understanding, bug 847809 & a followup bug in bug 876099 was intending to restrict playing of AMR media types. AAC I believe should still play in the music app, especially since we are allowing it to play on the open web right now per bug 876099.
Just noticed this is about AAC (and not AMR) in 3gp. 

Roc - Did the recent change impact AAC in 3gp as well?
Flags: needinfo?(robert)
Attached audio AAC Audio file
Music app doesn't display AAC file with .aac extension . Looks like a regression.

I used 1.1 latest gecko/gaia build .
(In reply to Tapas Kumar Kundu from comment #8)
> Created attachment 762374 [details]
> AAC Audio file
> 
> Music app doesn't display AAC file with .aac extension . Looks like a
> regression.
> 
> I used 1.1 latest gecko/gaia build .

Well, that just answered the question is this is a regression. Looks like we broken AAC somewhere.

Definitely a blocker then.
Keywords: regression
(In reply to Tapas Kumar Kundu from comment #8)
> Created attachment 762374 [details]
> AAC Audio file
> 
> Music app doesn't display AAC file with .aac extension . Looks like a
> regression.

This file doesn't appear to be AAC in an MP4 container. I don't know what container format it is, but we only advertise support for AAC in MP4 and 3GP containers.
Hi,

In V1.1 we didn't support raw aac file with Audio Data Transport Stream (ADTS) format.
We just support aac in 3gp or mp4 container, so extension name of aac didn't be a legal filter type in devicestorage.property.
Hi,

About another file of aac in 3gp, this issue is that music app can't recognize the major/compatible brand names in that test file.

The major/compatible brand names in that file are
    3gp5   3gp5/isom

We may need music app to modify the key words for filtering.
Flags: needinfo?(dkuo)
Since raw aac file is not supported and devicestorage.properties doesn't allow *.aac so this is not a regression. We do support aac in *.3gp or *.mp4 container so as roc and Marco mentioned in comment 10 and comment 11, we will only fix the part with *.3gp or *.mp4 container.
Flags: needinfo?(dkuo)
Keywords: regression
The mp4 ftyp is "3gp5" which the metadata parser of Music app does not recognize yet, we should be able to fix this by modifying metadata.js in Music app, adding "3gp5" to the MP4Types object.
(In reply to Dominic Kuo [:dkuo] from comment #14)
> Music app, adding "3gp5" to the MP4Types object.

Hi Dominic,

Please refer to link as below.
http://en.wikipedia.org/wiki/ISO_base_media_file_format

I would suggest that to add isom as the keyword in metadata parser for identifying mp4 or 3gp container. Because
  1. 3gp5 is a main brand field in ftyp box and there are many variations.
  2. both of mp4 and 3gp are all based on iso based media file format so they should all have isom in their compatible brand name in ftyp box.
If this isn't a regression, we probably can't block on this, right? Renom if someone feels differently here.
blocking-b2g: leo? → ---
Depends on: 882099
I want block on this as mentioned on comment #10, and comment #11.
blocking-b2g: --- → leo?
I want block on this as mentioned on comment #10, and comment #11.
Comment 10 suggests this is expected behavior. roc - please renom if we got that wrong.
blocking-b2g: leo? → -
Comments 8 was irrelevant to this bug and comments 9-11 were just elucidating that.

This bug is valid and should stay open, and possibly block.
blocking-b2g: - → leo?
Triage- Partners agree to block on this per comment 20 and the rest of the discussions.

Dominic, are you the right person to work on this?
blocking-b2g: leo? → leo+
Flags: needinfo?(dkuo)
Bug 882099 should also solve this issue and Jim is working on that, dependcency is also set.
Flags: needinfo?(dkuo)
Hi Dominic, 

After applying the patch from bug # 882099, iam still not able to scan this file(Content-01_AAC_Marvin_12.2kbps.3gp)
Leo, you probabaly need to "make reset-gaia" after you apply that patch, because your test file was already recorded as "unsupported" in MediaDB if you had launched Music before, or you can try to rename your test file and that should also trigger MediaDB to rescan it.
Hi Dominic,

Still issue reproduced after a gaia reset.

test file used : Content-01_AAC_Marvin_12.2kbps.3gp
Thanks Leo, I also reproduced this issue after apply the patch of bug 882099, and I am trying to figure out what's missing in that patch.
Assignee: nobody → dkuo
Priority: -- → P1
Target Milestone: --- → 1.1 QE3 (26jun)
Whiteboard: [cr 488549] → [cr 488549], [TD-53529]
Severity: normal → critical
Can anyone confirm me whether https://bugzilla.mozilla.org/show_bug.cgi?id=882099 fixes this problem ?
Flags: needinfo?(dkuo)
Hi Tapas,

Bugzilla ID :882099  fixes only mp4.

With this patch , the 3gp still doesn't work.
Target Milestone: 1.1 QE3 (26jun) → 1.1 QE4 (15jul)
Thanks Leo, there is another bug in the metadata parser of music app which caused this issue and I have to give one more patch for this bug.
Flags: needinfo?(dkuo)
Thanks Dominic. When do you expect to have that patch ready?
Flags: needinfo?(dkuo)
Blocks: 882692
(In reply to Dietrich Ayala (:dietrich) from comment #30)
> Thanks Dominic. When do you expect to have that patch ready?

This bug took me a couple days because I have to figure out the structures of MP4 and 3GP containers from the specifications in detail, so that the metadata parser can handle the common files in the real life. And fortunately I just finished it and I am attaching it.
Flags: needinfo?(dkuo)
Jim,

Since you are familiar with the metadata parser and David is PTO, I think this patch should go to you. The patch fixed the issues that some 3gp files cannot be recognized by the metadata parser, there are two major issues:

1. There is a logic error in shared/js/blobview.js that breaks the metadata parser, please see details in my comment.
2. It's possible that a 3gp file contains multiple tracks(odsm/sdsm/vide/soun/hint...), but some of the tracks are not sound track, we should only detect sound track for music app and it also contains the codec that we can identify is supported or not, so my patch added the part of checking Media Header Box for "vmhd" and "smhd", which indicates "Video Media Header Box" and "Sound Media Header Box".

I have also added some comments about the specifications in the patch, and if you have any question, feel free to let me know, thanks.
Attachment #773845 - Flags: review?(squibblyflabbetydoo)
Please email if/when it appears this won't make it for 7/15.
I should be able to review this tomorrow; I was hoping to get to it today, but then my AC unit broke and I had to deal with that instead.
Comment on attachment 773845 [details]
Check trak atom to filter iso base media files

This works for me with the AAC test files I have, and the code looks good too!

It doesn't parse the file from comment 8 (even if I rename the extension), but I'm not sure that we should be parsing it. If that's ok, then r=me.
Attachment #773845 - Flags: review?(squibblyflabbetydoo) → review+
From comment 10 and comment 11, we just support aac in 3gp or mp4 container for v1.1, and the test file in comment 8 is a raw aac with .aac extension, so we don't really support it. And since this patch got r+ I am landing it.
Landed on master: 49349122b9f2fb7f3e8b04d46fc221d0168d6f4b
Status: NEW → RESOLVED
Closed: 7 years ago
Hardware: x86 → ARM
Resolution: --- → FIXED
Uplifted commit 49349122b9f2fb7f3e8b04d46fc221d0168d6f4b as:
v1-train: d52ce2278dbc2b2916a7054a220b24fb41aefd6e
(In reply to Dominic Kuo [:dkuo] from comment #36)
> From comment 10 and comment 11, we just support aac in 3gp or mp4 container
> for v1.1, and the test file in comment 8 is a raw aac with .aac extension,
> so we don't really support it. And since this patch got r+ I am landing it.

Thanks for the confirmation! I just wanted to make sure we didn't miss something accidentally.
It is possible to play the content: Content-01_AAC_Marvin_12.2kbps from Music app. Also it is possible to attach and send it in a MMS successfully.

Verified with Unagi v1-train 07/15 build:
Gecko-2b310a1
Gaia-d52ce22
ref ril
Status: RESOLVED → VERIFIED
Whiteboard: [cr 488549], [TD-53529] → [cr 488549], [TD-53529], [LeoVB+]
v1.1.0hd: d52ce2278dbc2b2916a7054a220b24fb41aefd6e
You need to log in before you can comment on or make changes to this bug.