1084456 - Enable MSE for MP4 on Jelly Bean+

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

With bug 1014614 landed, I am able to play MP4 with MSE if I have media.mediasource.enabled set and media.mediasource.ignore_codecs. Should we enable medaisource and just set MP4 as supported on Android when we have Jelly Bean?

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

We should start by enabling the MP4Reader on Android for regular MP4 video. Once we're satisfied the MP4 playback through MSE works sufficiently we will enable it on all platforms that use MP4Reader.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #1)
> We should start by enabling the MP4Reader on Android for regular MP4 video.
> Once we're satisfied the MP4 playback through MSE works sufficiently we will
> enable it on all platforms that use MP4Reader.

Bug 1014614 enables that very thing, which is why I want to look into this next. Bug 1014614 landed Friday but bounced, hope to be able to reland today.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> Bug 1014614 enables that very thing, which is why I want to look into this
> next. Bug 1014614 landed Friday but bounced, hope to be able to reland today.

MP4Reader gets enabled as we get support on each platform. MP4 support for MSE will just get turned on on all platforms when it is ready. There will be no staging of MP4 support per se.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Anthony, do we want to do something here for 39 maybe?

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Yes. We should pref on MSE for Android in 39 for sure.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Anthony, are we ready for this? Which pref do we need to toggle? Just media.mediasource.enabled?

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #6)
> Anthony, are we ready for this? Which pref do we need to toggle? Just
> media.mediasource.enabled?

That is the correct pref. You can use http://youtube.com/html5 to check. The only part that isn't straightforward (to me at least) is pref'ing it on only when we have the Java PlatformDecoderModule available.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I'm not aware of anything that would block us from enabling MSE on Android so we should pref it on and see if anything comes up.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Enable MSE on Android

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Anthony what's the status with MSE for WebM? Should that work too?

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #10)
> Anthony what's the status with MSE for WebM? Should that work too?

The status is broken, so no.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Enable MSE on Android

We need to modify a DOM mochitest to enable this only for Android

Comment 13

[(no longer active)]

Comment on attachment 8605349 [details] [diff] [review]
Enable MSE on Android

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

::: dom/tests/mochitest/general/test_interfaces.html
@@ +1393,2 @@
>    var isAndroid = navigator.userAgent.includes("Android");
> +  var isLinux = /Linux/.test(navigator.oscpu) && !isAndroid;

Why do you need to do this?  This probably changes the meaning of the other tests in this file.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #13)
> Comment on attachment 8605349 [details] [diff] [review]
> Enable MSE on Android
> 
> Review of attachment 8605349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/tests/mochitest/general/test_interfaces.html
> @@ +1393,2 @@
> >    var isAndroid = navigator.userAgent.includes("Android");
> > +  var isLinux = /Linux/.test(navigator.oscpu) && !isAndroid;
> 
> Why do you need to do this?  This probably changes the meaning of the other
> tests in this file.

Without this, "linux" and "android" are both true, since navigator.oscpu is something like "Linux armv7l" on Android. AFAICT, we are really talking about desktop linux here.

Comment 15

[(no longer active)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #14)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #13)
> > Comment on attachment 8605349 [details] [diff] [review]
> > Enable MSE on Android
> > 
> > Review of attachment 8605349 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/tests/mochitest/general/test_interfaces.html
> > @@ +1393,2 @@
> > >    var isAndroid = navigator.userAgent.includes("Android");
> > > +  var isLinux = /Linux/.test(navigator.oscpu) && !isAndroid;
> > 
> > Why do you need to do this?  This probably changes the meaning of the other
> > tests in this file.
> 
> Without this, "linux" and "android" are both true, since navigator.oscpu is
> something like "Linux armv7l" on Android. AFAICT, we are really talking
> about desktop linux here.

Makes sense.  Can you please flag me for review again?  I'll look into this more closely tomorrow (it's probably fine, but I want to double check before r+ing :-)

Comment 16

[(no longer active)]

Comment on attachment 8605349 [details] [diff] [review]
Enable MSE on Android

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

Sorry for the turn-around.  :-)

::: dom/tests/mochitest/general/test_interfaces.html
@@ +1393,2 @@
>    var isAndroid = navigator.userAgent.includes("Android");
> +  var isLinux = /Linux/.test(navigator.oscpu) && !isAndroid;

OK, so I double checked, and currently all entries which have linux: false also have android: false, which is what your patch is trying to change.  So everything is fine!

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/560058cbd355

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/560058cbd355

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

So this made b2g mochitest-15 fail with https://treeherder.mozilla.org/logviewer.html#?job_id=9861906&repo=mozilla-inbound (which didn't start failing until after I had already merged it around everywhere).

As far as I can tell, it had the required DOM peer review, but maybe it was confused by the r=ajones,ehsan part? 

I'm not sure what needs to be done to fix this, just back it out and reland it with just r=ehsan?

Comment 20

[Pulsebot]

https://hg.mozilla.org/mozilla-central/rev/25c5525a1000

Comment 21

[(no longer active)]

Hmm, there is nothing related to who reviewed this in the test failure.  I think it just happened to change how we treated B2G, which is surprising.  Perhaps isLinux is evaluated to true there...

Comment 22

[(no longer active)]

(In reply to Pulsebot from comment #20)
> https://hg.mozilla.org/mozilla-central/rev/25c5525a1000

Anyways, I think we need to reopen this bug because of this: ^

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #21)
> Hmm, there is nothing related to who reviewed this in the test failure.  I
> think it just happened to change how we treated B2G, which is surprising. 
> Perhaps isLinux is evaluated to true there...

The only way we should've been able to break B2G is if isAndroid is true there. That doesn't make much sense, though, since the isB2G test is:

  var isB2G = !isDesktop && !navigator.userAgent.includes("Android");

And the isAndroid check is basically the opposite of that (without the isDesktop check). This is so fragile and broken, I'm inclined to just leave the current fix as-is. It looks like the result of that is we don't make any assertions about the presence of MediaSource stuff on Android.

Comment 24

[(no longer active)]

Well, I think we need to figure out what we actually want to do here and do that, rather than leaving things in the current state.

What are the exact conditions based on which you expect these APIs to be visible on each platform?

Comment 25

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #24)
> Well, I think we need to figure out what we actually want to do here and do
> that, rather than leaving things in the current state.
> 
> What are the exact conditions based on which you expect these APIs to be
> visible on each platform?

I don't really know the status on all the platforms, but it looks like it's supposed to be enabled on B2G (since it's hard to believe I accidentally enabled it), and I broke the test. I can't really see how that's possible by looking at the code. It seems the only way my change could affect B2G is if 'isAndroid' is true there, which would require 'Android' in the useragent, which should not be the case. Best way is to figure this out is to just print all of this junk out, but I can't push to Try right now because of hg issues.

Comment 26

[(no longer active)]

OK, that's fair, but do you mind investigating this later please?

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah, I'm looking now. Try run going here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b87e07329d8e

Comment 28

[(no longer active)]

Thanks a lot.  :-)

Comment 29

[James Willcox (:snorp) ([e-mail]) (he/him)]

Alright. The problem is that the MediaSource (and friends) now has 'android: true', and on b2g isAndroid is (and always has been) false. The 'entry.android === !isAndroid' conditional is now true, so we bail. Looks like you're really only ever supposed to use 'false' in the entries, so the fix that got pushed seems correct to me.

Comment 30

[(no longer active)]

Makes sense.  Thanks!

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

Renoming. This should just ride the train on 41 unless we have some reason to uplift that I don't know about.

Comment 32

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Riding the trains is the right approach because uplifting to 40 will be much more painful due to significant changes that have landed in 41.