1039639 - Add support for Flac on Firefox OS

Status: RESOLVED FIXED

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

Description

[Zéfling]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140611125853

Steps to reproduce:

I passed my Micro SD card of my android phone on Flame. All my music on my android phone is in Flac and unplayable.


Actual results:

Currently, the Flac audio format isn't support on Firefox OS. 


Expected results:

Add the Flac audio format support on Firefox OS.

Comment 1

[avnerus]

I can confirm that FLAC is also unsupported on my HEAD build of Firefox OS on flame.
Apple lossless (ALAC) is also not supported.

Comment 2

[Rika Pi]

I can confirm as well on Flame (Firefox OS 2.1)

Comment 3

[Hubert Figuiere [:hub]]

Storage API needs to be updated for FLAC. See bug 1059903.

Comment 4

[Hubert Figuiere [:hub]]

Part of this, we'd need to add support for FLAC metadata (mostly like VorbisComment).

Comment 5

[randomenduser]

+1 let me know what I can do to help test this. I have some flac on my SD card now that's not showing up and I update my phone pretty regularly (rebuilding after a `./repo sync`); I'd be happy to work with someone on squashing this one.

Comment 6

[randomenduser]

So I tried renaming my .flac files to .flac.mp3 (per the comment here: https://github.com/nazar-pc/CleverStyle-Music/issues/3#issuecomment-53760443, I could've also just renamed them to .mp3). The result in the built in player was that they work (as in I can play them), but there's no metadata (so all flac shows up under 'Unknown artist'). I'm not sure if that means more work, or if the built-in music app will "just work" after https://bugzilla.mozilla.org/show_bug.cgi?id=1059903 is fixed.

Comment 7

[daniel.james]

As someone with a large library of music in FLAC format, I would welcome not having to transcode to a lossy format in order to use a Firefox phone as a music player with metadata. There are a couple of additional benefits which were not mentioned above...

1. Decoding FLAC is likely to use less CPU/battery than decoding any lossy format (see the graphs at the end of the page http://www.hydrogenaud.io/forums/index.php?showtopic=92235)

2. Many hi-fi companies are supporting FLAC as an open standard format (https://en.wikipedia.org/wiki/List_of_hardware_and_software_that_supports_FLAC#Others), so having FLAC metadata support on the phone would enable them to create Firefox OS apps which integrate with their systems.

Comment 8

[Jim Porter (:squib)]

(In reply to randomenduser from comment #6)
> So I tried renaming my .flac files to .flac.mp3 ... The result in the built in
> player was that they work (as in I can play them)...

This works for me too. What the hell is going on here? I guess we can just take advantage of that, although I really want to know why this works (notably, it doesn't work at all on Firefox desktop, which is where I was testing this).

(In reply to daniel.james from comment #7)
> 1. Decoding FLAC is likely to use less CPU/battery than decoding any lossy
> format (see the graphs at the end of the page
> http://www.hydrogenaud.io/forums/index.php?showtopic=92235)

That's just not true. The main difference is that MP3s use a hardware decoder which consumes quite a bit less battery.

Anyway, give me a couple of days to land bug 1089718 and then it should be a simple matter to fully-support FLAC (or at least, as much as we support Vorbis, anyway).

Comment 9

[Jim Porter (:squib)]

Attachment: Parse FLAC metadata

I still need to write tests, and we need to make deviceStorage recognize FLAC (bug 1059903), but if you're ok with farting around with file extensions, this pull request should parse FLAC metadata (but no album art yet).

Comment 10

[daniel.james]

> > So I tried renaming my .flac files to .flac.mp3 ... The result in the built in
> > player was that they work (as in I can play them)...
> 
> This works for me too. What the hell is going on here?

Some players will reject a file as unplayable based on the extension alone, without looking at the mimetype, before attempting to decode them. It saves throwing an error later. I'll take a wild guess that this is the cause here.

> > 1. Decoding FLAC is likely to use less CPU/battery than decoding any lossy
> > format (see the graphs at the end of the page
> > http://www.hydrogenaud.io/forums/index.php?showtopic=92235)
> 
> That's just not true. The main difference is that MP3s use a hardware
> decoder which consumes quite a bit less battery.

I was not aware that the Flame phone contained an MP3 hardware decoder. Thanks for your work on the pull request.

Comment 11

[randomenduser]

Thanks for the quick turnaround! I'll try to build and flash your branch this weekend and let you know how it goes.

Comment 12

[Jim Porter (:squib)]

(In reply to daniel.james from comment #10)
> > > So I tried renaming my .flac files to .flac.mp3 ... The result in the built in
> > > player was that they work (as in I can play them)...
> > 
> > This works for me too. What the hell is going on here?
> 
> Some players will reject a file as unplayable based on the extension alone,
> without looking at the mimetype, before attempting to decode them. It saves
> throwing an error later. I'll take a wild guess that this is the cause here.

The "what the hell" was because Gecko doesn't actually have any FLAC code at all, but after talking to some people, apparently there's a FLAC decoder in the Gonk layer.

Comment 13

[Blake Wu [:bwu][:blakewu]]

 (In reply to Jim Porter (:squib) from comment #12)
> (In reply to daniel.james from comment #10)
> > > > So I tried renaming my .flac files to .flac.mp3 ... The result in the built in
> > > > player was that they work (as in I can play them)...
> > > 
> > > This works for me too. What the hell is going on here?
> > 
> > Some players will reject a file as unplayable based on the extension alone,
> > without looking at the mimetype, before attempting to decode them. It saves
> > throwing an error later. I'll take a wild guess that this is the cause here.
> 
> The "what the hell" was because Gecko doesn't actually have any FLAC code at
> all, but after talking to some people, apparently there's a FLAC decoder in
> the Gonk layer.
Bug 992756 may be helpful to you. 
Yes. Currently FFOS leverages libstagefright's decoder via MediaOmxReader.

Comment 14

[Jim Porter (:squib)]

Comment on attachment 8518458 [details] [review]
Parse FLAC metadata

Here's the Gaia side to FLAC support. David, I think you'll find this much simpler to review than my last metadata patch. :)

Comment 15

[Jim Porter (:squib)]

Attachment: Support FLAC in Gecko

This is currently untested, but it's really simple. I'll try to test this out myself later today, or possibly Monday.

Comment 16

[David Flanagan [:djf]]

Comment on attachment 8527017 [details] [diff] [review]
Support FLAC in Gecko

Review of attachment 8527017 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/mime/nsMimeTypes.h
@@ +79,4 @@
>  #define AUDIO_MP3                           "audio/mpeg"
>  #define AUDIO_MP4                           "audio/mp4"
>  #define AUDIO_AMR                           "audio/amr"
> +#define AUDIO_FLAC                          "audio/flac"

This isn't official yet according to http://www.iana.org/assignments/media-types/media-types.xhtml#audio

Does it matter?  (Also, I note that amr is audio/AMR in uppercase)

Should audio/amr and audio/flac be ifdeffed like they are in the cpp file?

Comment 17

[David Flanagan [:djf]]

Comment on attachment 8518458 [details] [review]
Parse FLAC metadata

A few nits on github, but otherwise it looks good.

Too bad that you couldn't reuse the existing vorbis comment parsing code. But given how short it is, it doesn't seem worth the time to refactor it into a form where it could be shared.

Comment 18

[Dave Hylands [:dhylands]]

Comment on attachment 8527017 [details] [diff] [review]
Support FLAC in Gecko

Review of attachment 8527017 [details] [diff] [review]:
-----------------------------------------------------------------

Where does the flac codec live?

If it requires OMX to do the decoding then I think we'll need more changes.

See bug 986331

DecoderTraits.cpp is now in dom/media.

You should get cajbir to review

Comment 19

[Ralph Giles (:rillian)]

(In reply to David Flanagan [:djf] from comment #16)
> Comment on attachment 8527017 [details] [diff] [review]
> Support FLAC in Gecko
> 
> Review of attachment 8527017 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/mime/nsMimeTypes.h
> @@ +79,4 @@
> >  #define AUDIO_MP3                           "audio/mpeg"
> >  #define AUDIO_MP4                           "audio/mp4"
> >  #define AUDIO_AMR                           "audio/amr"
> > +#define AUDIO_FLAC                          "audio/flac"
> 
> This isn't official yet according to
> http://www.iana.org/assignments/media-types/media-types.xhtml#audio
> 
> Does it matter?

It doesn't matter. One is supposed to use audio/x-flac until audio/flac is registered, but that just means one has to support _two_ mime types. Flac has been deployed for 13 years. No other codec is going to appear with a better claim for the official registration.

Comment 20

[Jim Porter (:squib)]

Attachment: Support FLAC in Gecko (v2)

Ok, I think this is right. I'm not entirely sure I got the tests right; they pass, but I'm not 100% what's getting tested...

Comment 21

[Jim Porter (:squib)]

I'm out right now, but this can probably land; I just don't have time to watch the tree after landing myself...

Comment 22

[Hubert Figuiere [:hub]]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Comment 23

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/17f58f7754df1f9b572b12a89fefb72131b954fd

Comment 24

[Hubert Figuiere [:hub]]

We still need the Gecko patch.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0460160af653

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes.
https://hg.mozilla.org/integration/b2g-inbound/rev/09bf82baf421

https://treeherder.mozilla.org/logviewer.html#?job_id=976986&repo=b2g-inbound

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Looks like other tests were hitting similar issues. Make sure this gets a full green B2G mochitest run (opt and debug) before re-landing.

Comment 28

[Jim Porter (:squib)]

:cajbir, any ideas why this might crash? I know next to nothing about how Gecko talks to libstagefright.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

They were timeouts that were force-crashed to get a stack, in case that helps.

Comment 30

[cajbir (:cajbir)]

Buggy flac decoder in Stagefright maybe? The crash looks to be inside the actual stagefright flac decoder code.

Comment 31

[Jim Porter (:squib)]

What's the way forward then? Disable the tests?

Comment 32

[cajbir (:cajbir)]

The way forward is for someone to debug exactly what's causing the crash, and what devices it occurs on. Then a decision would need to be made if we need to disable it on crashy devices and enable it on working ones. It may be something we are doing wrong in calling stagefright that triggers it too.

Comment 33

[cajbir (:cajbir)]

Sotaro may be able to help with identification of the reason for the crash.

Comment 34

[Sotaro Ikeda [:sotaro]]

(In reply to cajbir (:cajbir) from comment #33)
> Sotaro may be able to help with identification of the reason for the crash.

I can help it, but it becomes tomorrow(17th). Is it OK?

Comment 35

[Sotaro Ikeda [:sotaro]]

re-check tryserver result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28191f14e618
https://tbpl.mozilla.org/?tree=Try&rev=28191f14e618

Comment 36

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #35)
> re-check tryserver result.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=28191f14e618
> https://tbpl.mozilla.org/?tree=Try&rev=28191f14e618

test failure happens only on debug build and there seems no problem on flame and hamachi devices. Therefore, FLACParser's algorithm seems not have a problem.

Comment 37

[Sotaro Ikeda [:sotaro]]

The following is disassemble of copyStereo16()

------------------------------------------------
Dump of assembler code for function copyStereo16:
406	{
   0x432f6aec <+0>:	push	{r4, lr}

407	    for (unsigned i = 0; i < nSamples; ++i) {
=> 0x432f6aee <+2>:	movs	r3, #0
   0x432f6af0 <+4>:	b.n	0x432f6b08 <copyStereo16+28>
   0x432f6b02 <+22>:	adds	r3, #1
   0x432f6b08 <+28>:	adds	r0, #4
   0x432f6b0a <+30>:	cmp	r3, r2
   0x432f6b0c <+32>:	bne.n	0x432f6af2 <copyStereo16+6>

408	        *dst++ = src[0][i];
   0x432f6af2 <+6>:	ldr	r4, [r1, #0]
   0x432f6af4 <+8>:	ldrh.w	r12, [r4, r3, lsl #2]
   0x432f6af8 <+12>:	strh.w	r12, [r0, #-4]

409	        *dst++ = src[1][i];
   0x432f6afc <+16>:	ldr	r4, [r1, #4]
   0x432f6afe <+18>:	ldrh.w	r4, [r4, r3, lsl #2]
   0x432f6b04 <+24>:	strh.w	r4, [r0, #-2]

410	    }
411	}
   0x432f6b0e <+34>:	pop	{r4, pc}

End of assembler dump.

Comment 38

[Sotaro Ikeda [:sotaro]]

Crash address seems always 0x50. It is r4's value. From the following code, r4 is source address.

----------------------------------------------

12:53:04     INFO -  Crash reason:  SIGSEGV
12:53:04     INFO -  Crash address: 0x50
12:53:04     INFO -  Thread 26 (crashed)
12:53:04     INFO -   0  libstagefright.so!android::copyStereo16 [FLACExtractor.cpp : 409 + 0x2]
12:53:04     INFO -       r4 = 0x00000050    r5 = 0x00000001    r6 = 0x00000000    r7 = 0x467f8000
12:53:04     INFO -       r8 = 0x00000004    r9 = 0x42643157   r10 = 0x42643170    fp = 0x002e2761
12:53:04     INFO -       sp = 0x46ea8b50    lr = 0x4313d621    pc = 0x4313d176

Comment 39

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #38)
> Crash address seems always 0x50. It is r4's value. From the following code,
> r4 is source address.

Source buffer is come from FLAC library.

Comment 40

[Sotaro Ikeda [:sotaro]]

I could re-generate the crash by using long flac music file and do seek in Music app.

Comment 41

[Sotaro Ikeda [:sotaro]]

Implementation of FLACParser seems to have a problem. It seems better to handle it in a new bug. I am going to create a bug for it.

Comment 42

[Hema Koka [:hema]]

AFAIK, FLAC support is not a blocking feature for 2.2. Removing from blocking nom

Comment 43

[Luna Jernberg]

i want FLAC support aswell for next version if you could do it, as most of my music is in FLAC, any list of supported audio or video formats

//Martin "bittin" Jernberg

Comment 44

[zzt]

+1 Please add FLAC support, since most torrents are encoded using FLAC nowadays (*irony*)

Comment 45

[Jim Porter (:squib)]

There's no need to add "me too" comments to this bug. It already has patches and is just waiting for some platform fixes to land before I check in the final bit that fully-enables FLAC. Please be patient.

Comment 46

[Jim Porter (:squib)]

Since bug 1113271 is fixed, I think we can try landing this again.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Can we get a Try run to confirm that first? :)

Comment 48

[Jim Porter (:squib)]

Finally got around to pushing to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76959dd0683c

Comment 49

[Jim Porter (:squib)]

I think the tests look ok. There are a couple of failures, but they seem to be unrelated. (The closest seems to be a couple of failures for video playback on Windows, but I don't think that's related to this.)

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f19216bba031

Comment 51

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f19216bba031