Enable MSE for MP4 on Jelly Bean+

RESOLVED FIXED in Firefox 41

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 41
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, fennec41+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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?
Flags: needinfo?(ajones)
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.
Flags: needinfo?(ajones)
(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.
Depends on: 1014614
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Anthony, do we want to do something here for 39 maybe?
Flags: needinfo?(ajones)
Yes. We should pref on MSE for Android in 39 for sure.
Flags: needinfo?(ajones)

Updated

3 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → 39+
Assignee: nobody → snorp
Anthony, are we ready for this? Which pref do we need to toggle? Just media.mediasource.enabled?
Flags: needinfo?(ajones)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Flags: needinfo?(ajones)
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.
Created attachment 8602905 [details] [diff] [review]
Enable MSE on Android
Attachment #8602905 - Flags: review?(ajones)
Anthony what's the status with MSE for WebM? Should that work too?
Flags: needinfo?(ajones)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10)
> Anthony what's the status with MSE for WebM? Should that work too?

The status is broken, so no.
Flags: needinfo?(ajones)
Attachment #8602905 - Flags: review?(ajones) → review+
Created attachment 8605349 [details] [diff] [review]
Enable MSE on Android

We need to modify a DOM mochitest to enable this only for Android
Attachment #8605349 - Flags: review?(ehsan)
Attachment #8602905 - Attachment is obsolete: true
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.
Attachment #8605349 - Flags: review?(ehsan) → review-
(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.
Flags: needinfo?(ehsan)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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 :-)
Flags: needinfo?(ehsan)
Attachment #8605349 - Flags: review- → review?(ehsan)
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!
Attachment #8605349 - Flags: review?(ehsan) → review+
Blocks: 778617

Comment 17

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/560058cbd355
https://hg.mozilla.org/mozilla-central/rev/560058cbd355
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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?
Flags: needinfo?(snorp)
Flags: needinfo?(ehsan)

Comment 20

2 years ago
https://hg.mozilla.org/mozilla-central/rev/25c5525a1000
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...
Flags: needinfo?(ehsan)
(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: ^
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Flags: needinfo?(snorp)
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?
Flags: needinfo?(snorp)
(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.
Flags: needinfo?(snorp)
OK, that's fair, but do you mind investigating this later please?
Yeah, I'm looking now. Try run going here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b87e07329d8e
Thanks a lot.  :-)
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.
Makes sense.  Thanks!
Renoming. This should just ride the train on 41 unless we have some reason to uplift that I don't know about.
tracking-fennec: 39+ → ?
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
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.
tracking-fennec: ? → 41+
You need to log in before you can comment on or make changes to this bug.