Closed
Bug 1112761
Opened 9 years ago
Closed 9 years ago
Restrict MSE support to youtube.com domain for Firefox 36
Categories
(Core :: Audio/Video, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: ajones, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
5.74 KB,
patch
|
kinetik
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
13.37 KB,
patch
|
kinetik
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Restrict MediaSource API to *.youtube.com temporarily in order to make adaptive streaming available to more users sooner.
Reporter | ||
Comment 1•9 years ago
|
||
Peter - is this something that is up your alley? We don't need it right away.
Flags: needinfo?(peterv)
Reporter | ||
Updated•9 years ago
|
Priority: -- → P1
Comment 2•9 years ago
|
||
Should also restrict it to Windows for Firefox 36. Minimize our exposure there.
Comment 3•9 years ago
|
||
(In reply to Ralph Giles (:rillian) from comment #2) > Should also restrict it to Windows for Firefox 36. Minimize our exposure > there. Why? I'd be happy to organize some more Linux testing. We need to prepare sooner than most for a Flash free web..
Reporter | ||
Comment 4•9 years ago
|
||
We don't have the right MP4 support available for Linux at this point.
Reporter | ||
Comment 5•9 years ago
|
||
JST - given that this doesn't require media expertise, can you find someone to work on this?
Flags: needinfo?(jst)
Assignee | ||
Comment 6•9 years ago
|
||
Anthony, I'm happy to hook this up. Just wanted to check three things about the desired behavior: 1) We want to expose all the things that are currently conditioned on the "media.mediasource.enabled", right? 2) Which interaction do you want with the existing preference? Do you want it to be "true" in 36 but have the additional restriction to only youtube (such that setting to "false" overrides and disables it everywhere but there is no way to enable everywhere unless we add a separate pref for that) or do you want it to be "false" in 36 but have youtube whitelisted (such that you can toggle to "true" and enable everywhere, but can't disable on youtube unless we add a separate pref for that)? In either case, do we want the separate pref to allow the thing that would no longer be possible with the "media.mediasource.enabled" pref? 3) Do we want to enable for both http://*.youtube.com and https://*.youtube.com, or just https:? The latter would be somewhat preferable from a security standpoint, obviously, and at first glance youtube is https-only, so it seems to me like it should be sufficient...
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(ajones)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bzbarsky
Flags: needinfo?(peterv)
Flags: needinfo?(jst)
Reporter | ||
Comment 7•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #6) > Anthony, I'm happy to hook this up. Just wanted to check three things about > the desired behavior: > > 1) We want to expose all the things that are currently conditioned on the > "media.mediasource.enabled", right? Yes. > 2) Which interaction do you want with the existing preference? Do you want > it to be "true" in 36 but have the additional restriction to only youtube > (such that setting to "false" overrides and disables it everywhere but there > is no way to enable everywhere unless we add a separate pref for that) or do > you want it to be "false" in 36 but have youtube whitelisted (such that you > can toggle to "true" and enable everywhere, but can't disable on youtube > unless we add a separate pref for that)? In either case, do we want the > separate pref to allow the thing that would no longer be possible with the > "media.mediasource.enabled" pref? Lets use: media.mediasource.whitelist=https://*.youtube.com media.mediasource.enabled=true If it is blank then it would default to * and setting media.mediasource.enabled=false would disable MSE everywhere. > 3) Do we want to enable for both http://*.youtube.com and > https://*.youtube.com, or just https:? The latter would be somewhat > preferable from a security standpoint, obviously, and at first glance > youtube is https-only, so it seems to me like it should be sufficient... Lets go with https only.
Flags: needinfo?(ajones)
Reporter | ||
Comment 8•9 years ago
|
||
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #7) > media.mediasource.whitelist=https://*.youtube.com Don't put any extra effort into this whitelist matching. We can just have "youtube" or "*" as the only supported options if it saves you effort.
Assignee | ||
Comment 9•9 years ago
|
||
OK. So my plan is to have the following two prefs: media.mediasource.enabled: false means no, true means check the other pref. media.mediasource.youtubeonly: false or unset means enabled everywhere, set to true means only https://*.youtube.com.
Assignee | ||
Comment 10•9 years ago
|
||
Actually, one more policy question. Do we youtubeonly to be true on Aurora 36, or to be false on Aurora 36 but become true when 36 goes to beta? That is, do we want to be testing RELEASE_BUILD or NIGHTLY_BUILD here?
Flags: needinfo?(ajones)
Reporter | ||
Comment 11•9 years ago
|
||
We should restrict it to YouTube for beta and release, not for nightly and aurora
Flags: needinfo?(ajones)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8544992 -
Flags: review?(kinetik)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8544993 -
Flags: review?(kinetik)
Assignee | ||
Comment 14•9 years ago
|
||
I'm assuming we'll land this stuff on m-c too; the patches apply to both m-c and aurora.
Updated•9 years ago
|
Attachment #8544992 -
Flags: review?(kinetik) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8544993 [details] [diff] [review] part 2. Enable MediaSource based on a whitelist, not in general Review of attachment 8544993 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/mediasource/MediaSource.cpp @@ +315,5 @@ > { > + // Don't use aGlobal across Preferences stuff, which the static > + // analysis thinks can GC. > + JS::Rooted<JSObject*> global(cx, aGlobal); > + Whitespace. @@ +335,5 @@ > + if (NS_FAILED(principal->GetURI(getter_AddRefs(uri))) || !uri) { > + return false; > + } > + > + bool isHttps = false;; Double semicolon at EOL.
Attachment #8544993 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 16•9 years ago
|
||
Matthew, thanks for the review! https://hg.mozilla.org/integration/mozilla-inbound/rev/4a03e62bd2d2 https://hg.mozilla.org/integration/mozilla-inbound/rev/37bd1af93c54
Target Milestone: --- → mozilla37
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8544992 [details] [diff] [review] part 1. Replace Pref="media.mediasource.enabled" annotations in IDL with calls to a function which will end up doing something a bit more interesting Approval Request Comment [Feature/regressing bug #]: Ability to enable MSE by default for YouTube on Windows release builds. [User impact if declined]: The Flash crashes will continue until morale improves. [Describe test coverage new/current, TBPL]: We have some MSE tests. I've performed some manual testing that the site whitelist work. More manual testing of YouTube itself will likely be needed... [Risks and why]: The main risks are fourfold: 1) The patch could work incorrectly and enable MSE on sites other than YouTube. This would probably not be so great, since I'm not sure how ready our MSE imeplementation is for that. Depending on how incorrect the behavior is, and how late we discover it, it may be possible to fix the behavior, disable across the board with a pref flip, or a backout may be required. I've done some manual testing that suggests this part should work. 2) The patch could work incorrectly and not actually enable MSE on YouTube. Then we're not really worse off than we are right now, I guess. Again, I've done manual testing that suggests this part should work. 3) The patch could work incorrectly and enable MSE on some parts of YouTube but not others, leading to broken video playback. This is a bigger worry than #1 and #2, because I've done no in-depth testing of YouTube with the patch on a beta or release build (or otherwise with the youtubeonly pref flipped to restrict to YouTube). Testing here would be much appreciated. 4) The patch could work correctly but we could run into problems with either our MSE code or YouTube's code that uses MSE that break playback. This is somewhat mitigated by the fact that we've had MSE enabled on nightly and Aurora for a bit now, so presumably got bug reports about any issues that arose as a result. [String/UUID change made/needed]: None.
Attachment #8544992 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8544993 -
Flags: approval-mozilla-aurora?
Comment 18•9 years ago
|
||
>We don't have the right MP4 support available for Linux at this point.
In quick testing with nightly on Windows XP, Youtube.com/html5 doesn't show H.264 support. MSE will work fine if it just has VP8/VP9 AFAICT, and if it needs H264 it would fall back to either Flash or H264 without MSE (as most Linux users have gstreamer provided H264).
If I'm missing something on this MP4 support issue, is there a bug on it?
Comment 19•9 years ago
|
||
We want to enable this for Windows 7 and later, since we don't have mp4 playback on XP (and Vista is buggy). We could fall back to WebM, but MSE+WebM isn't ready. So we want MSE only on YouTube, only on Windows 7+, and optionally MacOS X. So if you have normal HTML5 video (WebM on XP) on XP (or flash) that's as expected. If you get DASH=yes, codecs=vp9 in Youtube's "Stats for nerds" with a default profile, that's not what want. Linux users should get normal HTML5 video (WebM or mp4) with DASH=no.
Assignee | ||
Comment 20•9 years ago
|
||
OK. It sounds like we need a followup bug to do the "Windows 7+" thing, but only when the youtubeonly pref is on?
Flags: needinfo?(giles)
Comment 21•9 years ago
|
||
Bor(In reply to Boris Zbarsky [:bz] from comment #20) > OK. It sounds like we need a followup bug to do the "Windows 7+" thing, but > only when the youtubeonly pref is on? Yes. I believe we need some kind of runtime version detection. I don't know how to do that on window. However, if canPlayType fails for mp4 YouTube should fall back to Flash, which may minimize the problem. Anthony, how important is excluding Vista?
Flags: needinfo?(giles) → needinfo?(ajones)
Comment 22•9 years ago
|
||
:cpearce believes the fix on bug 1107889 resolves the issues with Vista, so we're ok there.
Flags: needinfo?(ajones)
Comment 23•9 years ago
|
||
Landing on Aurora with a=mse pre-approval. https://hg.mozilla.org/releases/mozilla-aurora/rev/038ee1a68a54 https://hg.mozilla.org/releases/mozilla-aurora/rev/f989781e9a23
status-firefox36:
--- → fixed
status-firefox37:
--- → affected
https://hg.mozilla.org/mozilla-central/rev/4a03e62bd2d2 https://hg.mozilla.org/mozilla-central/rev/37bd1af93c54
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Reporter | ||
Comment 25•9 years ago
|
||
Boris - thanks for sorting that bit out for us. Much appreciated.
Assignee | ||
Comment 26•9 years ago
|
||
Not a problem. Comparative advantage for the win. ;)
Updated•9 years ago
|
Attachment #8544993 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8544992 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•