Restrict EME to MSE

RESOLVED FIXED in Firefox 37

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: eflores)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
x86
Windows Vista
Points:
---

Firefox Tracking Flags

(firefox36 wontfix, firefox37 fixed, firefox38 fixed, firefox39 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
cpearce says IE restricts EME to MSE. We should, too.
JW: would you be able to take this bug?

I'm not sure the best way to do this, you're welcome to figure that out, or ask me to. :)
Flags: needinfo?(jwwang)
We'll also not test the non-MSE path, so we can remove our non-MSE test cases too.
sure.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Flags: needinfo?(jwwang)
So what will IE do to an encrypted file in non-MSE case? Raising an error event?

I think it can be easily done by sending an 'error' event instead of 'encryped' event when non-MSE source is detected.
No longer blocks: 778617
Created attachment 8568266 [details] [diff] [review]
1131392-restrict-eme-mse.patch

Hack to return an error here until we're *actually* unable to play encrypted plain MP4.
Attachment #8568266 - Flags: review?(cpearce)
Created attachment 8568267 [details] [diff] [review]
1131392-fix-tests.patch
Created attachment 8568268 [details] [diff] [review]
1131392-kill-test-code.patch

A bit of cleanup
Attachment #8568267 - Flags: review?(cpearce)
Attachment #8568268 - Flags: review?(cpearce)
Assignee: jwwang → edwin
Comment on attachment 8568266 [details] [diff] [review]
1131392-restrict-eme-mse.patch

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

I think we should log to the webconsole when we fail due to non-MSE EME being unsupported.

But please add another patch to do this, as it'll require adding strings which may be not get uplift approval. So we may be able to uplift the rest of the patches, but not that one.

You can use nsContentUtils::ReportToConsole() to do that, for example:
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?#11418
Attachment #8568266 - Flags: review?(cpearce) → review+
Attachment #8568267 - Flags: review?(cpearce) → review+
Comment on attachment 8568268 [details] [diff] [review]
1131392-kill-test-code.patch

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

::: dom/media/test/eme.js
@@ +185,1 @@
>  // its play() function called; when it's loaded MSE fragments, or once the load

Remove "or once the load has started for non-MSE video".

Can you even just remove all callers of LoadTest(), can call PlayFragmented() directly?
Attachment #8568268 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #8)
> Comment on attachment 8568266 [details] [diff] [review]
> 1131392-restrict-eme-mse.patch
> 
> Review of attachment 8568266 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we should log to the webconsole when we fail due to non-MSE EME
> being unsupported.
> 
> But please add another patch to do this, as it'll require adding strings
> which may be not get uplift approval.

Or do this in another bug even.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3732d9a9abe
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f1c4d0e605
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1bfc458851
https://hg.mozilla.org/mozilla-central/rev/c3732d9a9abe
https://hg.mozilla.org/mozilla-central/rev/c7f1c4d0e605
https://hg.mozilla.org/mozilla-central/rev/ae1bfc458851
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Do we want this uplifted?
Flags: needinfo?(cpearce)
(Reporter)

Comment 14

3 years ago
Yes, we need this fix uplifted to Aurora 38 and Beta 37.
status-firefox36: --- → wontfix
status-firefox37: --- → affected
status-firefox38: --- → affected
Comment on attachment 8568266 [details] [diff] [review]
1131392-restrict-eme-mse.patch

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: EME will be usable in non-MSE videos. We're trying to reduce scope of EME, so we want to restrict EME to MSE only. Other browsers that implement EME do this.
[Describe test coverage new/current, TreeHerder]: We have plenty of tests on MSE+EME video.
[Risks and why]: Low; we're turning stuff off.
[String/UUID change made/needed]: None
Flags: needinfo?(cpearce)
Attachment #8568266 - Flags: approval-mozilla-beta?
Attachment #8568266 - Flags: approval-mozilla-aurora?
Comment on attachment 8568267 [details] [diff] [review]
1131392-fix-tests.patch

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: EME will be usable in non-MSE videos. We're trying to reduce scope of EME, so we want to restrict EME to MSE only. Other browsers that implement EME do this.
[Describe test coverage new/current, TreeHerder]: We have plenty of tests on MSE+EME video.
[Risks and why]: Low; we're turning stuff off.
[String/UUID change made/needed]: None
Attachment #8568267 - Flags: approval-mozilla-beta?
Attachment #8568267 - Flags: approval-mozilla-aurora?
Comment on attachment 8568268 [details] [diff] [review]
1131392-kill-test-code.patch

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: EME will be usable in non-MSE videos. We're trying to reduce scope of EME, so we want to restrict EME to MSE only. Other browsers that implement EME do this.
[Describe test coverage new/current, TreeHerder]: We have plenty of tests on MSE+EME video.
[Risks and why]: Low; we're turning stuff off.
[String/UUID change made/needed]: None
Attachment #8568268 - Flags: approval-mozilla-beta?
Attachment #8568268 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/releases/mozilla-aurora/rev/0c96d6164ccb
https://hg.mozilla.org/releases/mozilla-aurora/rev/b9ff759f56f8
https://hg.mozilla.org/releases/mozilla-aurora/rev/36bbf44de597
status-firefox38: affected → fixed
Comment on attachment 8568266 [details] [diff] [review]
1131392-restrict-eme-mse.patch

Looks pretty safe and comes with a test fix. Beta+ Aurora+
Attachment #8568266 - Flags: approval-mozilla-beta?
Attachment #8568266 - Flags: approval-mozilla-beta+
Attachment #8568266 - Flags: approval-mozilla-aurora?
Attachment #8568266 - Flags: approval-mozilla-aurora+
Attachment #8568267 - Flags: approval-mozilla-beta?
Attachment #8568267 - Flags: approval-mozilla-beta+
Attachment #8568267 - Flags: approval-mozilla-aurora?
Attachment #8568267 - Flags: approval-mozilla-aurora+
Attachment #8568268 - Flags: approval-mozilla-beta?
Attachment #8568268 - Flags: approval-mozilla-beta+
Attachment #8568268 - Flags: approval-mozilla-aurora?
Attachment #8568268 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-beta/rev/a8aa24cf19a5
https://hg.mozilla.org/releases/mozilla-beta/rev/dd9bfd410f7e
https://hg.mozilla.org/releases/mozilla-beta/rev/1a9d0193519a
status-firefox37: affected → fixed
You need to log in before you can comment on or make changes to this bug.