Closed Bug 1131392 Opened 5 years ago Closed 5 years ago

Restrict EME to MSE

Categories

(Core :: Audio/Video, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- wontfix
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpeterson, Assigned: eflores)

References

Details

Attachments

(3 files)

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.
Hack to return an error here until we're *actually* unable to play encrypted plain MP4.
Attachment #8568266 - 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.
Do we want this uplifted?
Flags: needinfo?(cpearce)
Yes, we need this fix uplifted to Aurora 38 and Beta 37.
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?
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+
You need to log in before you can comment on or make changes to this bug.