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)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: ajones, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Restrict MediaSource API to *.youtube.com temporarily in order to make adaptive streaming available to more users sooner.
Peter - is this something that is up your alley? We don't need it right away.
Flags: needinfo?(peterv)
Priority: -- → P1
Should also restrict it to Windows for Firefox 36. Minimize our exposure there.
(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..
We don't have the right MP4 support available for Linux at this point.
JST - given that this doesn't require media expertise, can you find someone to work on this?
Flags: needinfo?(jst)
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...
Flags: needinfo?(ajones)
Assignee: nobody → bzbarsky
Flags: needinfo?(peterv)
Flags: needinfo?(jst)
(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)
(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.
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.
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)
We should restrict it to YouTube for beta and release, not for nightly and aurora
Flags: needinfo?(ajones)
I'm assuming we'll land this stuff on m-c too; the patches apply to both m-c and aurora.
Attachment #8544992 - Flags: review?(kinetik) → review+
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+
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?
Attachment #8544993 - Flags: approval-mozilla-aurora?
>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?
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.
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)
Blocks: 1119463
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)
Blocks: 1119535
:cpearce believes the fix on bug 1107889 resolves the issues with Vista, so we're ok there.
Flags: needinfo?(ajones)
Boris - thanks for sorting that bit out for us. Much appreciated.
Not a problem.  Comparative advantage for the win.  ;)
Attachment #8544993 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8544992 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1125621
Blocks: 1125621
No longer depends on: 1125621
No longer blocks: 1125621
Depends on: 1125621
You need to log in before you can comment on or make changes to this bug.