Closed Bug 1295886 Opened 3 years ago Closed 2 years ago

Add flac decoder on Android

Categories

(Core :: Audio/Video: Playback, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
relnote-firefox --- 58+
firefox58 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(18 files, 1 obsolete file)

58 bytes, text/x-review-board-request
gerald
: review+
Details
149.71 KB, audio/aac
Details
165.02 KB, audio/aac
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
snorp
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
snorp
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
Details
I am told that android has a flac decoder by default.

Which should enable it via the Android PlatformDecoderModule as adding FFmpeg support would unnecessarily increase the binary size too much.
Flags: needinfo?(esawin)
Quick shot at it...

so android doesn't support 24 bits.. so we need a way to disable testing the flac-s24 sample in the mochitest.
Just update what I found for record (I used HTC M and Nexus5 L for testing).

As Blake mentioned on comment 1, document said it supported FLAC.

Gecko used [1] for creating the MediaCodec instance and we use "audio/flac" as mimetype parameter.

We got 

 09-01 17:51:10.881 11526 12867 E ACodec  : Unable to instantiate a decoder for type 'audio/flac' with err 0xfffffffe.
09-01 17:51:10.881 11526 12867 E ACodec  : signalError(omxError 0xfffffffe, internalError -2)
09-01 17:51:10.882 11526 12867 E MediaCodec: Codec reported err 0xfffffffe, actionCode 0, while in state 1
09-01 17:51:10.884 11526 12853 W MediaCodec-JNI: try to release MediaCodec from JMediaCodec::~JMediaCodec()...
09-01 17:51:10.884 11526 12853 W MediaCodec-JNI: done releasing MediaCodec from JMediaCodec::~JMediaCodec().
09-01 17:51:10.884 11526 12853 W System.err: java.lang.IllegalArgumentException: Failed to initialize audio/flac, error 0xfffffffe
09-01 17:51:10.886 11526 12853 W System.err: 	at android.media.MediaCodec.native_setup(Native Method)
09-01 17:51:10.886 11526 12853 W System.err: 	at android.media.MediaCodec.<init>(MediaCodec.java:1670)
09-01 17:51:10.886 11526 12853 W System.err: 	at android.media.MediaCodec.createDecoderByType(MediaCodec.java:1618)


As mentioned by document [2],
using findDecoderForFormat(MediaFormat) and createByCodecName(String) to get the decoder, I tried but findDecoderForFormat return null.

I also enumerate the codec by [3]

findDecoderCodecInfoForMimeType mimeType = video/avc codec name = OMX.qcom.video.decoder.avc
findDecoderCodecInfoForMimeType mimeType = video/mp4v-es codec name = OMX.qcom.video.decoder.mpeg4
findDecoderCodecInfoForMimeType mimeType = video/3gpp codec name = OMX.qcom.video.decoder.h263
findDecoderCodecInfoForMimeType mimeType = video/x-ms-wmv codec name = OMX.qcom.video.decoder.vc1
findDecoderCodecInfoForMimeType mimeType = video/x-vnd.on2.vp8 codec name = OMX.qcom.video.decoder.vp8
findDecoderCodecInfoForMimeType mimeType = audio/x-ms-wma codec name = OMX.ARICENT.AUDIO.DEC.WMAPRO
findDecoderCodecInfoForMimeType mimeType = video/x-ms-wmv codec name = OMX.ARICENT.VIDEO.DEC.WMV
findDecoderCodecInfoForMimeType mimeType = audio/mpeg codec name = OMX.google.mp3.decoder
findDecoderCodecInfoForMimeType mimeType = audio/3gpp codec name = OMX.google.amrnb.decoder
findDecoderCodecInfoForMimeType mimeType = audio/amr-wb codec name = OMX.google.amrwb.decoder
findDecoderCodecInfoForMimeType mimeType = audio/mp4a-latm codec name = OMX.google.aac.decoder
findDecoderCodecInfoForMimeType mimeType = audio/g711-alaw codec name = OMX.google.g711.alaw.decoder
findDecoderCodecInfoForMimeType mimeType = audio/g711-mlaw codec name = OMX.google.g711.mlaw.decoder
findDecoderCodecInfoForMimeType mimeType = audio/vorbis codec name = OMX.google.vorbis.decoder
findDecoderCodecInfoForMimeType mimeType = audio/opus codec name = OMX.google.opus.decoder
findDecoderCodecInfoForMimeType mimeType = audio/raw codec name = OMX.google.raw.decoder
findDecoderCodecInfoForMimeType mimeType = audio/gsm codec name = OMX.google.gsm.decoder
findDecoderCodecInfoForMimeType mimeType = video/mp4v-escodec name = OMX.google.mpeg4.decoder
findDecoderCodecInfoForMimeType mimeType = video/3gpp codec name = OMX.google.h263.decoder
findDecoderCodecInfoForMimeType mimeType = video/avc codec name = OMX.google.h264.decoder
findDecoderCodecInfoForMimeType mimeType = video/hevc codec name = OMX.google.hevc.decoder
findDecoderCodecInfoForMimeType mimeType = video/x-vnd.on2.vp8 codec name = OMX.google.vp8.decoder
findDecoderCodecInfoForMimeType mimeType = video/x-vnd.on2.vp9 codec name = OMX.google.vp9.decoder

I cannot see the audio/flac .

But!!!  I use Z3C L , I will get the decoder....

E/GeckoHardwareCodecCapabilityUtils(21333): findDecoderCodecInfoForMimeType mimeType = audio/flaccodec name = OMX.sony.flac.decoder

but get error later

I/OMXClient(21333): Using client-side OMX mux.
D/MediaCodec(21333): MediaCodec[kWhatConfigure]: video-output-protection: 00000000, audio-output-protection: 00000000
E/SoftFlacDecoder(  332): errorCallback status: FLAC__STREAM_DECODER_ERROR_STATUS_LOST_SYNC
E/SoftFlacDecoder(  332): FLAC decoder state: FLAC__STREAM_DECODER_SEARCH_FOR_METADATA
E/SoftFlacDecoder(  332): inQueue empty in readCallback
E/SoftFlacDecoder(  332): FLAC decoder state: FLAC__STREAM_DECODER_SEARCH_FOR_METADATA
E/SoftFlacDecoder(  332): FLAC decoder state: FLAC__STREAM_DECODER_ABORTED
W/ActivityManager(  858): getTasks: caller 10165 does not hold GET_TASKS; limiting output
W/ActivityManager(  858): getTasks: caller 10165 does not hold GET_TASKS; limiting output


In conclusion,
it seems it depends on OEM which should support flac instead of google native support.
But I still cannot play it on the device(Z3C) so I cannot verify if this patch is working or not on certain devices.


[1] https://dxr.mozilla.org/mozilla-central/rev/b7f7ae14590aced450bb0b0469dfb38edd2c0ace/dom/media/platforms/android/MediaCodecDataDecoder.cpp#45


[2] https://developer.android.com/reference/android/media/MediaCodec.html#createDecoderByType(java.lang.String)

[3] https://dxr.mozilla.org/mozilla-central/rev/b7f7ae14590aced450bb0b0469dfb38edd2c0ace/mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/HardwareCodecCapabilityUtils.java#46
James, 
I believe most Android devices should support FLAC unless phone makers disable it on purpose. Did you play flac files with the music app on the phone to get the logs in comment 4?
Comment on attachment 8786222 [details]
Bug 1295886 - P2. Extract FFmpeg BSF config options.

updated by me, so marked it obsolete
Attachment #8786222 - Attachment is obsolete: true
Hi jya,

I update your patch and update something I investigated and have some questions needs your help.

I found Android native app can play the flac file you provied.

I put some log and build a AOSP image to see what did the magic happen.

I saw native app used the mime type "audio/raw" to create the audio decoder.

Q1: Does it make sense?

So I use audio/raw to get the decoder. It can be played with the broken sound.

You can hear the sound by my attachment.

Q2: Could you find something wrong with my patch and the symptom of the broken sound?

I have no idea how to deal with this situation since everything looks fine except the mimetype.

Q3: If you think this bug is worth investigating more or trying, could I take this bug?

Thank you!
Flags: needinfo?(jyavenard)
audio/raw Is uncompressed audio. My guess is that they decompress the audio somehow and output that as raw to another decoder for playback.

Flac is lossless audio; just compressed. So it's entirely possible that you hear something that sounds a bit like the original. 

That would be confirmed if you could capture the data they send as raw and verify that it's different. 

I'm not sure if you should spend much time on this. Not using the ACodec decoder may be a simpler approach. 

But feel free to take the bug. I only had a shot as I hoped it would be straightforward.
Flags: needinfo?(jyavenard)
Thank you, 

I would like to stop here.

I can see some log which is related to software encoder before the log for getting the decoder.

So I think your guess is right, they decompress to pcm data at the beginning.

Thank you again and please cc me if you open another bugs to handle flac by ffmpeg.
Flags: needinfo?(esawin)
Duplicate of this bug: 1335208
ni John to put this on his plate. :-)
Flags: needinfo?(jolin)
FWICT the magic happens in FLACExtractor [1]. Looks like it uses (static-linked) libFLAC to parse/decode audio frames and sends PCM (audio/raw) to the MediaCodec. I don't think we can build a PDM with it.
As James found, some devices, such as Sony Z3C, have standalone decoder installed. But in order to support as many devices as possible, having FLAC decoder in Gecko seems a better choice.

[1] https://android.googlesource.com/platform/frameworks/av/+/master/media/libstagefright/FLACExtractor.cpp
Flags: needinfo?(jolin)
On completely vanilla AOSP, both Opera and Chrome manage to play FLAC (just punch it into the src
of an <audio> element).  Even the elderly Tint browser manages to leverage Android's default FLAC
support and support the format.
See Also: → 1364374
(In reply to Andrew Valencia from comment #16)
> of an <audio> element).  Even the elderly Tint browser manages to leverage
> Android's default FLAC
> support and support the format.

Chrome on Android uses ffmpeg to decode flac.

There's no usable flac decoder on Android (the flac decoder can't be used independently from the flac demuxer)
Assignee: nobody → jyavenard
Do we care about arm64 or can we assume at this stage that when compiled on Android, it's always arm 32 bits target?

thanks
Flags: needinfo?(snorp)
We do care about arm64, but we aren't shipping it yet. It's not entirely clear when that will change.
Flags: needinfo?(snorp)
Oh, I see a "Android 5.0 AArch64" option on try....

maybe for now I limit the flac decoder to be C only, with no arm/neon optimisations ...
Attachment #8787529 - Attachment is obsolete: true
Comment on attachment 8786222 [details]
Bug 1295886 - P2. Extract FFmpeg BSF config options.

https://reviewboard.mozilla.org/r/75220/#review200824
Attachment #8786222 - Flags: review?(gsquelart) → review+
Comment on attachment 8924382 [details]
Bug 1295886 - P3. Remove darwin 32 bits support.

https://reviewboard.mozilla.org/r/195662/#review200826
Attachment #8924382 - Flags: review?(gsquelart) → review+
Comment on attachment 8924383 [details]
Bug 1295886 - P4. Ensure HAVE_STDATOMIC_H is properly defined.

https://reviewboard.mozilla.org/r/195664/#review200828
Attachment #8924383 - Flags: review?(gsquelart) → review+
Comment on attachment 8924384 [details]
Bug 1295886 - P5. Add flac support on Android.

https://reviewboard.mozilla.org/r/195666/#review200830
Attachment #8924384 - Flags: review?(gsquelart) → review+
Comment on attachment 8924386 [details]
Bug 1295886 - P7. Add support for S16 decoded output.

https://reviewboard.mozilla.org/r/195670/#review200836
Attachment #8924386 - Flags: review?(gsquelart) → review+
Comment on attachment 8924387 [details]
Bug 1295886 - P8. Never use AndroidDecoderModule to decode FLAC.

https://reviewboard.mozilla.org/r/195672/#review200838
Attachment #8924387 - Flags: review?(gsquelart) → review+
Comment on attachment 8924389 [details]
Bug 1295886 - P10. Add a FLAC only option to ffvpx.

https://reviewboard.mozilla.org/r/195676/#review200842
Attachment #8924389 - Flags: review?(gsquelart) → review+
Comment on attachment 8924390 [details]
Bug 1295886 - P11. Enable FFmpeg small mode.

https://reviewboard.mozilla.org/r/195678/#review200844

::: media/ffvpx/config_android.h:1
(Diff revision 1)
>  /* Automatically generated by configure - do not modify! */

In commit message: "remove" -> "removes", "algorithm" -> "algorithms", "with" -> "which"
Attachment #8924390 - Flags: review?(gsquelart) → review+
Comment on attachment 8924391 [details]
Bug 1295886 - P12. Have a generic C-only FLAC decoder.

https://reviewboard.mozilla.org/r/195680/#review200846
Attachment #8924391 - Flags: review?(gsquelart) → review+
Comment on attachment 8924392 [details]
Bug 1295886 - P13. Disable FAST_CLZ on Windows.

https://reviewboard.mozilla.org/r/195682/#review200848
Attachment #8924392 - Flags: review?(gsquelart) → review+
Comment on attachment 8924394 [details]
Bug 1295886 - P15. Enable FLAC mochitest on Android.

https://reviewboard.mozilla.org/r/195686/#review200852
Attachment #8924394 - Flags: review?(gsquelart) → review+
Attachment #8924474 - Flags: review?(cpearce) → review?(gsquelart)
Comment on attachment 8924390 [details]
Bug 1295886 - P11. Enable FFmpeg small mode.

https://reviewboard.mozilla.org/r/195678/#review200844

> In commit message: "remove" -> "removes", "algorithm" -> "algorithms", "with" -> "which"

trying to make comment disappear
Comment on attachment 8924474 [details]
Bug 1295886 - P1. Remove unused flac parser.

https://reviewboard.mozilla.org/r/195756/#review200930
Attachment #8924474 - Flags: review?(gsquelart) → review+
Comment on attachment 8924385 [details]
Bug 1295886 - P6. Add ffmpeg libs to package manifest.

https://reviewboard.mozilla.org/r/195668/#review200960
Attachment #8924385 - Flags: review?(snorp) → review+
Comment on attachment 8924388 [details]
Bug 1295886 - P9. Fix linkage of libmozav* libs on Android.

https://reviewboard.mozilla.org/r/195674/#review200962
Attachment #8924388 - Flags: review?(snorp) → review+
Extra size added to a release build of the apk is 154,448 bytes for the flac decoder itself (which is neon or vfp accelerated) or 123,436 bytes on aarch64, the decoder is in C only, didn't bother with neon there as I didn't find a way to locally test compilation).

Increase size on the package is 133kiB, that is 0.03% bigger than today's nightly size.

While we've had flac support since Firefox 51, we only had it on Desktop.

Chrome 62 added flac support on android too, and has been available for a couple of weeks.
Flags: needinfo?(jcheng)
Flags: needinfo?(abovens)
> Chrome 62 added flac support on android too, and has been available for a couple of weeks.

Note, from my own recollection, and born out by articles like:

  phoronix.com/scan.php?page=news_item&px=Google-Chrome-56

Chrome has had flac support for quite a bit more than a couple weeks.

Which does not at all take away from my delight on seeing this work progressing!
Thanks much.
(In reply to Andrew Valencia from comment #99)
> > Chrome 62 added flac support on android too, and has been available for a couple of weeks.
> 
> Note, from my own recollection, and born out by articles like:
> 
>   phoronix.com/scan.php?page=news_item&px=Google-Chrome-56
> 
> Chrome has had flac support for quite a bit more than a couple weeks.
> 
> Which does not at all take away from my delight on seeing this work
> progressing!
> Thanks much.

https://developers.google.com/web/updates/2017/09/chrome-62-media-updates

Actually, that was for desktop. yes, flac on android has been available for a while (though it doesn't appear to work with all devices).

probably for the same reasons we had the failure commented about a year ago.
Comment on attachment 8924483 [details]
Bug 1295886 - P16. Enable FLAC C decoder on aarch64 machines.

https://reviewboard.mozilla.org/r/195762/#review201134
Attachment #8924483 - Flags: review+
Attachment #8924483 - Flags: review?(core-build-config-reviews)
Comment on attachment 8924393 [details]
Bug 1295886 - P14. Add arm neon and vfp optimized methods to ffmpeg.

https://reviewboard.mozilla.org/r/195684/#review201136

Given the amount of code being added, you'll also want to get additional review from your module.

::: media/ffvpx/ffvpxcommon.mozbuild:36
(Diff revision 4)
> +        USE_YASM = True
>          # Default to unix, similar to how ASFLAGS setup works in configure.in
>          ASFLAGS += ['-Pconfig_unix64.asm']
> +    if CONFIG['CPU_ARCH'] == 'x86' or CONFIG['CPU_ARCH'] == 'x86_64':
> -    # default disabled components
> +        # default disabled components
> -    ASFLAGS += ['-Pdefaults_disabled.asm']
> +        ASFLAGS += ['-Pdefaults_disabled.asm']

If I'm reading man pages correctly, I believe the -P flag is only valid if USE_YASM is set. Is there a reason you moved USE_YASM=True to only be set in certain cases? What happens if we don't hit one of those but we end up adding -Pdefaults_disabled.asm to the command-line?
Attachment #8924393 - Flags: review?(core-build-config-reviews)
Comment on attachment 8924393 [details]
Bug 1295886 - P14. Add arm neon and vfp optimized methods to ffmpeg.

https://reviewboard.mozilla.org/r/195684/#review201136

> If I'm reading man pages correctly, I believe the -P flag is only valid if USE_YASM is set. Is there a reason you moved USE_YASM=True to only be set in certain cases? What happens if we don't hit one of those but we end up adding -Pdefaults_disabled.asm to the command-line?

you can only get there if FFVPX_FLAGS is set, and its only set for platforms for which we have accelerated code.
that is arm32, x86 and x86_64

yasm can only compile x86 or x86_64 code, it cant compile arm intrinsecs, clang/gcc is used for those

so USE_YASM was moved so that it is set on x86 family only.

otherwise the various .S wouldn't compile
Comment on attachment 8924393 [details]
Bug 1295886 - P14. Add arm neon and vfp optimized methods to ffmpeg.

https://reviewboard.mozilla.org/r/195684/#review201136

> you can only get there if FFVPX_FLAGS is set, and its only set for platforms for which we have accelerated code.
> that is arm32, x86 and x86_64
> 
> yasm can only compile x86 or x86_64 code, it cant compile arm intrinsecs, clang/gcc is used for those
> 
> so USE_YASM was moved so that it is set on x86 family only.
> 
> otherwise the various .S wouldn't compile

there would be an issue for Darwin platform if we ever supported Darwin on arm. but for now we dont. and probsbly never will...
(In reply to Jean-Yves Avenard [:jya] from comment #103)
> > If I'm reading man pages correctly, I believe the -P flag is only valid if USE_YASM is set. Is there a reason you moved USE_YASM=True to only be set in certain cases? What happens if we don't hit one of those but we end up adding -Pdefaults_disabled.asm to the command-line?
> 
> you can only get there if FFVPX_FLAGS is set, and its only set for platforms
> for which we have accelerated code.
> that is arm32, x86 and x86_64

Would it be appropriate to do:

    if USE_YASM:
        # default disabled components
        ASFLAGS += ['-Pdefaults_disabled.asm']

Instead of the arch checking then?
Comment on attachment 8924393 [details]
Bug 1295886 - P14. Add arm neon and vfp optimized methods to ffmpeg.

https://reviewboard.mozilla.org/r/195684/#review201172
Attachment #8924393 - Flags: review+
(In reply to Michael Shal [:mshal] from comment #105)
> Would it be appropriate to do:
> 
>     if USE_YASM:
>         # default disabled components
>         ASFLAGS += ['-Pdefaults_disabled.asm']
> 
> Instead of the arch checking then?

It would be. I've made the appropriate change.
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f682de302ea1
P1. Remove unused flac parser. r=gerald
https://hg.mozilla.org/integration/autoland/rev/6ba63e774fa9
P2. Extract FFmpeg BSF config options. r=gerald
https://hg.mozilla.org/integration/autoland/rev/13389b0adb69
P3. Remove darwin 32 bits support. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ee67bb4e6b7a
P4. Ensure HAVE_STDATOMIC_H is properly defined. r=gerald
https://hg.mozilla.org/integration/autoland/rev/c5db9b984777
P5. Add flac support on Android. r=gerald
https://hg.mozilla.org/integration/autoland/rev/a7674738651f
P6. Add ffmpeg libs to package manifest. r=snorp
https://hg.mozilla.org/integration/autoland/rev/e6126e000672
P7. Add support for S16 decoded output. r=gerald
https://hg.mozilla.org/integration/autoland/rev/003bffedc86e
P8. Never use AndroidDecoderModule to decode FLAC. r=gerald
https://hg.mozilla.org/integration/autoland/rev/f34a22b5d9ff
P9. Fix linkage of libmozav* libs on Android. r=snorp
https://hg.mozilla.org/integration/autoland/rev/fbcfbb6be12f
P10. Add a FLAC only option to ffvpx. r=gerald
https://hg.mozilla.org/integration/autoland/rev/8ed7a7c6472f
P11. Enable FFmpeg small mode. r=gerald
https://hg.mozilla.org/integration/autoland/rev/2a9ed25ca816
P12. Have a generic C-only FLAC decoder. r=gerald
https://hg.mozilla.org/integration/autoland/rev/e9649d83f2be
P13. Disable FAST_CLZ on Windows. r=gerald
https://hg.mozilla.org/integration/autoland/rev/cd2b7c34307e
P14. Add arm neon and vfp optimized methods to ffmpeg. r=mshal
https://hg.mozilla.org/integration/autoland/rev/c387badaa88b
P15. Enable FLAC mochitest on Android. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ae52929709bd
P16. Enable FLAC C decoder on aarch64 machines. r=mshal
Attachment #8924483 - Flags: review?(core-build-config-reviews)
Attachment #8924393 - Flags: review?(core-build-config-reviews)
As I was CCed on the 133kiB file size increase - noted. It's unavoidable, I suppose. Out of curiosity, do we know of any major sites using Flac (on mobile)?
Flags: needinfo?(abovens)
Flags: needinfo?(jcheng)
BBC is currently experimenting a trial that will stream using flac in MP4 for their music channels.
The target for now was on Firefox desktop. Chrome only added support for those streams in Chrome 62, and now we have a solution on Android too.

There was zero support for FLAC in desktop browser until we implemented it in 51.

Hopefully the use will increase now that the two major browsers support it on all platforms.
Not sure of the risk associated here. Is there a testing plan to monitor stability? Should we initiate something?
Flags: needinfo?(bwu)
Flags: needinfo?(ajones)
(In reply to Thomas Elin [:relaas] from comment #115)
> Not sure of the risk associated here. Is there a testing plan to monitor
> stability? Should we initiate something?

The risk here is really low. We are just enabling a path that is already used elsewhere. There aren't very many sites using FLAC, which reduces the risk even further.
Flags: needinfo?(ajones)
For the number of patches laneded and lines of codes changed, there might be some risks. But as Anthony pointed out, not many websites use FLAC. Besides this is not a user facing/interacting feature or a big refactoring. The overal risk is low. Also before release we have time to monitor and check if there is any problems.
Flags: needinfo?(bwu)
Blocks: 1424555
Added to Fx58 release notes.
You need to log in before you can comment on or make changes to this bug.