Closed Bug 1266792 Opened 4 years ago Closed 3 years ago

Enable rust mp4parse on android

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox48 --- affected
firefox52 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1220307 added rust support for android. I believe we need to explicitly enable our code by setting MOZ_RUST_MP4PARSE in mobile/android/confvars.sh
Enable the rust mp4 parser code like we do on desktop.

Review commit: https://reviewboard.mozilla.org/r/48515/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48515/
Attachment #8744398 - Flags: review?(snorp)
The size difference reported in bug 1220307 may be so small because the pilot code wasn't actually enabled. Android builds the gtests, but doesn't run them, so it's hard to tell if support is hooked up. I pushed this to try to check if there's an effect on the size. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8c205fc2777
The stlport we use on android doesn't support std::vector::data()
from C++11. Fall back to the older front() method.

Review commit: https://reviewboard.mozilla.org/r/48525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48525/
Attachment #8744398 - Attachment description: MozReview Request: Bug 1266792 - Enable MOZ_RUST_MP4PARSE on fennec. r=snorp → MozReview Request: Bug 1266792 - Enable MOZ_RUST_MP4PARSE on fennec. r?snorp
Attachment #8744405 - Flags: review?(kinetik)
Comment on attachment 8744398 [details]
Bug 1266792 - Enable MOZ_RUST_MP4PARSE on fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48515/diff/1-2/
Android stlport doesn't support C++11, which confirms the code wasn't being included. New push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=31e15aaf6579
Looks like we need an explicit -lm on Android. The desktop builds rely on everything rust-std depends on already being on the link-line for XUL.so.

librul.a(std-4fda350b.0.o):std.0.rs:function f32::f32::log2::h90aa6ba577989944Mxa: error: undefined reference to 'log2f'
[...]
10:36:49 INFO - /android/ndk-arm-18/sysroot/usr/include/unistd.h:177: error: undefined reference to '__page_size'

FWIW, I have code in the upstream mp4parse repo to pull this out of the rustc output, and a partial patch to have rustc output it in an easier to parse form.

https://github.com/mozilla/mp4parse-rust/blob/master/examples/Makefile
https://github.com/rust-lang/rust/pull/31875
Actually, I suspect this is due to the android target mismatch. Rust targets android-18, which provides log2, while mozilla builds against android-9 which does not. This is an llvm intrinsic, so hopefully just building libstd against the earlier android version will resolve the issue.
Depends on: 1266877
(In reply to Ralph Giles (:rillian) from comment #8)
> Actually, I suspect this is due to the android target mismatch. Rust targets
> android-18, which provides log2, while mozilla builds against android-9
> which does not. This is an llvm intrinsic, so hopefully just building libstd
> against the earlier android version will resolve the issue.

Nope.  I think the sequence goes like this ($SYMBOL is something interesting like ftruncate64 or log2):

1. rustc builds against android-18.
2. rustc links libstd.so against platform libs, finds $SYMBOL in libc.so or libm.so.
3. $SYMBOL is therefore marked as an UNDEFINED symbol in the symbol table for the dynamic linker to handle.
4. rustc links libstd.rlib, but doesn't check whether $SYMBOL would be found, as that would come later.

That handles the rustc build side of things.  Now comes our usage:

5. mozilla builds against android-9.
6. mozilla builds rlibs for its Rust code, and then a staticlib at the end.
7. Said static lib pulls in rustc libstd.rlib from step 4.  $SYMBOL still undefined.
8. Said static libc is linked into libxul, so the linker needs to resolve symbols.
9. Linker looks for $SYMBOL in android-9 libc.so or libm.so, but doesn't find it.
10. Linker errors.

Note that building rustc against android-9 would just cause this to fail earlier, cf https://github.com/rust-lang/rust-buildbot/pull/99#issuecomment-213801208

I don't know what's up with the __page_size error, though: that symbol's been around for a long time.
I understand the __page_size thing, sigh.  Google went through the stub libraries they distribute with the NDK and removed a bunch of "POSIX doesn't support this and we don't either" symbols in r11* NDKs.  Rust compiles against a r10e NDK, which winds up embedding one of said symbols (__page_size) in the Rust rlibs and .so's they distribute.  But you can't use those rlibs or .so's when compiling against *later* versions of the NDK, since the symbols you depend on don't exist in the libraries from the later NDKs.

I think this is not the smartest move, but Google's position on this seems to be that you can recompile.  So that's what we'll have to ask Rust to do.
Just so I understand...They retroactively changed the ABI for older android releases in newer NDK releases? Wow.

But, as you say, we can all upgrade.
Blocks: 1161350
(In reply to Ralph Giles (:rillian) from comment #12)
> Just so I understand...They retroactively changed the ABI for older android
> releases in newer NDK releases? Wow.

Binary-only artifacts that linked against previous NDKs may or may not link against current NDKs, depending on what symbols they use.  Not sure if libraries on newer devices won't have those symbols as well, which would definitely be a breaking change.  I think I'd call it an ABI break; it certainly looks like one from the outside.  I appreciate the desire for cleanliness, but it is a huge, huge pain.
Mass change P2 -> P3
Priority: P2 → P3
So, as far as I recall, I tried these patches, but it looked like the rust mp4parse code wasn't actually being called, then I got distracted by other things.
Rebased earlier MOZ_RUST_MP4PARSE patch, which no longer applied. The MP4Metadata patch is no longer necessary. With these changes, I still can't play https://people.mozilla.org/~rgiles/2016/sample.mp4 (which requires the rust demuxer). Logs say:

W/VideoCapabilities( 9115): Unsupported mime video/mp4v-esdp
I/VideoCapabilities( 9115): Unsupported profile 4 for video/mp4v-es
W/AudioCapabilities( 9115): Unsupported mime audio/x-ape
W/AudioCapabilities( 9115): Unsupported mime audio/ffmpeg
W/AudioCapabilities( 9115): Unsupported mime audio/vnd.dts
W/AudioCapabilities( 9115): Unsupported mime audio/mpeg-L2
W/AudioCapabilities( 9115): Unsupported mime audio/vnd.rn-realaudio
W/AudioCapabilities( 9115): Unsupported mime audio/x-ms-wma
W/VideoCapabilities( 9115): Unsupported mime video/divx
W/VideoCapabilities( 9115): Unsupported mime video/x-flv
W/VideoCapabilities( 9115): Unsupported mime video/mpeg2
W/VideoCapabilities( 9115): Unsupported mime video/vnd.rn-realvideo
W/VideoCapabilities( 9115): Unsupported mime video/vc1
W/VideoCapabilities( 9115): Unsupported mime video/ffmpeg
W/VideoCapabilities( 9115): Unsupported mime video/x-ms-wmv
I/OMXClient( 9115): Using client-side OMX mux.
E/OMXMaster( 9115): A component of name 'OMX.ffmpeg.wmv.decoder' already exists, ignoring this one.
E/ACodec  ( 9115): [OMX.google.opus.decoder] ERROR(0x80001001)
E/ACodec  ( 9115): signalError(omxError 0x80001001, internalError -2147483648)
E/MediaCodec( 9115): Codec reported err 0x80001001, actionCode 0, while in state 6
W/System.err( 9115): java.lang.IllegalStateException
W/System.err( 9115): 	at android.media.MediaCodec.native_dequeueInputBuffer(Native Method)
W/System.err( 9115): 	at android.media.MediaCodec.dequeueInputBuffer(MediaCodec.java:986)
W/System.err( 9115): java.lang.IllegalStateException
W/System.err( 9115): 	at android.media.MediaCodec.native_dequeueOutputBuffer(Native Method)
W/System.err( 9115): 	at android.media.MediaCodec.dequeueOutputBuffer(MediaCodec.java:1036)
I/Gecko   ( 9115): [9115] WARNING: Exiting decoder loop due to exception: file /home/giles/firefox/dom/media/platforms/android/MediaCodecDataDecoder.cpp, line 533
I/Gecko   ( 9115): [9115] WARNING: 0x806e0005: void mozilla::MediaCodecDataDecoder::DecoderLoop(): file /home/giles/firefox/dom/media/MediaFormatReader.cpp, line 717

So it certainly looks like it's using the native stagefright demuxer.
I get the same result as comment 18 (though I'm not totally sure how to get Gecko logs on my android device); logcat, at least, is giving me java.lang.IllegalStateExceptions in android.media.MediaCodec.

The peculiar thing is that the Rust code *is* being compiled in.  I can disassemble the relevant bits under media/libstagefright/ and I see that calls to Rust things are being made.  I can look at libxul and verify that there are the Rust mp4 functions in it, and everything.

So I don't know what's going on here.
OK, logging indicates that all the Rust code is being called, and that Rust is being preferred for playing the track, but nothing shows up on the phone (I have a Nexus 5X).  Maybe our mobile frontend doesn't have the UI for playing music files directly, like our desktop frontend does, or something?
Flags: needinfo?(snorp)
According to comment #18 we are making it to the decoder, which actually suggests that the demux is successful. Maybe we're using the wrong decoder? It looks like this is Opus, which I would expect to be played via our in-tree stuff. Maybe something going south there?
Flags: needinfo?(snorp)
Yes, the test file is opus-in-mp4 which should only demux successfully in the rust code. If you say we're demuxing successfully, I guess I don't know how to get logging from a android run either. I was using `adb shell [...] --es env0 MOZ_LOG=MP4Metadata:5,Media:Decoder:5` and looking at the logcat output.
(In reply to Ralph Giles (:rillian) needinfo me from comment #22)
> Yes, the test file is opus-in-mp4 which should only demux successfully in
> the rust code. If you say we're demuxing successfully, I guess I don't know
> how to get logging from a android run either. I was using `adb shell [...]
> --es env0 MOZ_LOG=MP4Metadata:5,Media:Decoder:5` and looking at the logcat
> output.

People work on fennec in Taipei using following command to get log:

adb shell am start -a android.activity.MAIN -n org.mozilla.fennec_$USER/.App --es env0 NSPR_LOG_MODULES=GMP:5,MediaFormatReader:5,AndroidDecoderModule:5
Thanks, Alfredo. With this I get:

I/Gecko   (13377): [MediaPlayback #2]: D/AndroidDecoderModule AndroidDecoderModule(95d8e4e8)::virtual already_AddRefed<mozilla::MediaDataDecoder> mozilla::AndroidDecoderModule::CreateAudioDecoder(const mozilla::CreateDecoderParams&): CreateAudioFormat with mimeType=audio/opus, mRate=48000, channels=2
I/OMXClient(13377): Using client-side OMX mux.
E/OMXMaster(13377): A component of name 'OMX.ffmpeg.wmv.decoder' already exists, ignoring this one.
E/ACodec  (13377): [OMX.google.opus.decoder] ERROR(0x80001001)
E/ACodec  (13377): signalError(omxError 0x80001001, internalError -2147483648)
E/MediaCodec(13377): Codec reported err 0x80001001, actionCode 0, while in state 6

So we're handing the decoded samples to the OMX platform decoder instead of the generic one.
Comment on attachment 8744405 [details]
Bug 1266792 - Target armv7 android.

https://reviewboard.mozilla.org/r/48525/#review88876

Good catch
Attachment #8744405 - Flags: review?(snorp) → review+
Thanks.

Updated remaining issues:

- We parse opus fine. On my handset (Sony Z3C running cyanogen 12.1 nightly/android 5.1.1) AndroidDecoderModule says it can play 'audio/opus' but then crashes when we feed it data. If I make it reject this mimetype we play fine with the agnostic decoder.

- We parse flac, but fail with a bad bit-depth assertion in AndroidDecoder, even though the input file ins 16 bit. Debugger says mBitDepth is zero, so something is getting mangled. On desktop we use ffvpx to decode flac, but this isn't faster for vp9 decode on android so we don't build it, and the AndroidDecoderModule is the only option.

  F/MOZ_Assert(20869): Assertion failure: config.mBitDepth == 16 (We only handle 16-bit audio!), at /home/giles/firefox/dom/media/platforms/android/AndroidDecoderModule.cpp:187
Comment on attachment 8806130 [details]
Bug 1266792 - Don't play opus with AndroidDecoderModule.

https://reviewboard.mozilla.org/r/89630/#review89134

::: dom/media/platforms/android/AndroidDecoderModule.cpp:154
(Diff revision 2)
>        (VPXDecoder::IsVPX(aMimeType, VPXDecoder::VP9) &&
>         !GetFeatureStatus(nsIGfxInfo::FEATURE_VP9_HW_DECODE))) {
>      return false;
>    }
>  
> +  // Prefer the gecko decoder for opus; stagefright crashes

I think it would be preferable to only call FindDecodercodecInfoForMimeType for type we do want:
e.g: h264, aac, mp3, vp9, vp8

and that's it...
Attachment #8806130 - Flags: review?(jyavenard) → review+
Comment on attachment 8806130 [details]
Bug 1266792 - Don't play opus with AndroidDecoderModule.

https://reviewboard.mozilla.org/r/89630/#review89240

::: dom/media/platforms/android/AndroidDecoderModule.cpp:156
(Diff revision 2)
>      return false;
>    }
>  
> +  // Prefer the gecko decoder for opus; stagefright crashes
> +  // on content demuxed from mp4.
> +  if (aMimeType.EqualsLiteral("audio/opus")) {

I was going to suggest we only do this for busted phones, in order to take advantage of any platform decoding features (hardware), but I don't think opus hardware decoding exists anywhere.
(In reply to Jean-Yves Avenard [:jya] from comment #33)

> I think it would be preferable to only call FindDecodercodecInfoForMimeType
> for type we do want:
> e.g: h264, aac, mp3, vp9, vp8

Ok, thanks. That was also the consensus at yesterday's standup. I'll do that in a follow-up bug.(In reply to James Willcox 


(:snorp) (jwillcox@mozilla.com) from comment #34)

> I was going to suggest we only do this for busted phones, in order to take
> advantage of any platform decoding features (hardware), but I don't think
> opus hardware decoding exists anywhere.

Right. We might prefer a platform implementation for licensing (patented mpeg codecs), performance (hardware-accelerated video) or distribution size (we should try to use android flac if we can rather than shipping an ffmpeg build) but otherwise it's better to prefer to use a software implementation for consistency and ease of maintenance.
Android 4.0 arm apk size is 31433935 on try vs 31440079 for a similarly-times m-c automation build, so it looks like lto is taking care of the libstd bloat we saw earlier.
Blocks: 1314794
Comment on attachment 8806130 [details]
Bug 1266792 - Don't play opus with AndroidDecoderModule.

https://reviewboard.mozilla.org/r/89630/#review89850

::: dom/media/platforms/android/AndroidDecoderModule.cpp:154
(Diff revision 2)
>        (VPXDecoder::IsVPX(aMimeType, VPXDecoder::VP9) &&
>         !GetFeatureStatus(nsIGfxInfo::FEATURE_VP9_HW_DECODE))) {
>      return false;
>    }
>  
> +  // Prefer the gecko decoder for opus; stagefright crashes

I filled bug 1314794 to implement this suggestion. I agree it's better, but think it should be a separate issue because (a) the goal here is to get the rust demuxer enabled, and (b) that's an unrelated behaviour change.
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96412ec21495
Target armv7 android. r=kinetik,snorp
https://hg.mozilla.org/integration/autoland/rev/5041f62a5416
Enable MOZ_RUST_MP4PARSE on fennec. r=snorp
https://hg.mozilla.org/integration/autoland/rev/c8fd2298f0fc
Don't play opus with AndroidDecoderModule. r=jya
You need to log in before you can comment on or make changes to this bug.