Closed
Bug 1084456
Opened 10 years ago
Closed 10 years ago
Enable MSE for MP4 on Jelly Bean+
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox41 fixed, fennec41+)
RESOLVED
FIXED
Firefox 41
People
(Reporter: snorp, Assigned: snorp)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(ajones)
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Depends on: mediacodec
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
Anthony, do we want to do something here for 39 maybe?
Flags: needinfo?(ajones)
Comment 5•10 years ago
|
||
Yes. We should pref on MSE for Android in 39 for sure.
Flags: needinfo?(ajones)
Updated•10 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Updated•10 years ago
|
tracking-fennec: ? → 39+
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → snorp
Assignee | ||
Comment 6•10 years ago
|
||
Anthony, are we ready for this? Which pref do we need to toggle? Just media.mediasource.enabled?
Flags: needinfo?(ajones)
Comment 7•10 years ago
|
||
(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)
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8602905 -
Flags: review?(ajones)
Assignee | ||
Comment 10•10 years ago
|
||
Anthony what's the status with MSE for WebM? Should that work too?
Flags: needinfo?(ajones)
Comment 11•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8602905 -
Flags: review?(ajones) → review+
Assignee | ||
Comment 12•10 years ago
|
||
We need to modify a DOM mochitest to enable this only for Android
Attachment #8605349 -
Flags: review?(ehsan)
Assignee | ||
Updated•10 years ago
|
Attachment #8602905 -
Attachment is obsolete: true
Comment 13•10 years ago
|
||
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-
Assignee | ||
Comment 14•10 years ago
|
||
(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)
Comment 15•10 years ago
|
||
(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)
Assignee | ||
Updated•10 years ago
|
Attachment #8605349 -
Flags: review- → review?(ehsan)
Comment 16•10 years ago
|
||
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+
Comment 17•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 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 21•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
(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 → ---
Assignee | ||
Comment 23•10 years ago
|
||
(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)
Comment 24•10 years ago
|
||
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)
Assignee | ||
Comment 25•10 years ago
|
||
(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)
Comment 26•10 years ago
|
||
OK, that's fair, but do you mind investigating this later please?
Assignee | ||
Comment 27•10 years ago
|
||
Yeah, I'm looking now. Try run going here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b87e07329d8e
Comment 28•10 years ago
|
||
Thanks a lot. :-)
Assignee | ||
Comment 29•10 years ago
|
||
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•10 years ago
|
||
Makes sense. Thanks!
Assignee | ||
Comment 31•10 years ago
|
||
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+ → ?
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 32•10 years ago
|
||
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.
Updated•10 years ago
|
tracking-fennec: ? → 41+
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•